[PATCH] convert: clarify line ending conversion warning

[PATCH] convert: clarify line ending conversion warning

[Alex Henrie]

The warning about converting line endings is extremely confusing. Its
two sentences each use the word "will" without specifying a timeframe,
which makes it sound like both sentences are referring to the same
timeframe. On top of that, it uses the term "original line endings"
without saying whether "original" means LF or CRLF.

Rephrase the warning to be clear that line endings have not been changed
in the working directory but will be changed on the next checkout, and
explicitly say which line endings the file currently has in the working
directory.

Example commands to trigger the warning on Linux:

git config core.autocrlf true
echo 'Hello world!' > hello.txt
git add hello.txt
git commit -m "Add hello.txt"

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 convert.c            | 14 ++++++++------
 t/t0027-auto-crlf.sh |  2 +-
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/convert.c b/convert.c
index 8e39731efb..b024d74222 100644
--- a/convert.c
+++ b/convert.c
@@ -195,9 +195,10 @@ static void check_global_conv_flags_eol(const char *path,
 		if (conv_flags & CONV_EOL_RNDTRP_DIE)
 			die(_("CRLF would be replaced by LF in %s"), path);
 		else if (conv_flags & CONV_EOL_RNDTRP_WARN)
-			warning(_("CRLF will be replaced by LF in %s.\n"
-				  "The file will have its original line"
-				  " endings in your working directory"), path);
+			warning(_("CRLF will be replaced by LF in %s the next"
+				  " time you check it out.\n"
+				  "For now, the file still has CRLF line"
+				  " endings in your working directory."), path);
 	} else if (old_stats->lonelf && !new_stats->lonelf ) {
 		/*
 		 * CRLFs would be added by checkout
@@ -205,9 +206,10 @@ static void check_global_conv_flags_eol(const char *path,
 		if (conv_flags & CONV_EOL_RNDTRP_DIE)
 			die(_("LF would be replaced by CRLF in %s"), path);
 		else if (conv_flags & CONV_EOL_RNDTRP_WARN)
-			warning(_("LF will be replaced by CRLF in %s.\n"
-				  "The file will have its original line"
-				  " endings in your working directory"), path);
+			warning(_("LF will be replaced by CRLF in %s the next"
+				  " time you check it out.\n"
+				  "For now, the file still has LF line"
+				  " endings in your working directory."), path);
 	}
 }
 
diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index 0feb41a23f..31f471c4b1 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -82,7 +82,7 @@ check_warning () {
 	'')	                                                 >"$2".expect ;;
 	*) echo >&2 "Illegal 1": "$1" ; return false ;;
 	esac
-	grep "will be replaced by" "$2" | sed -e "s/\(.*\) in [^ ]*$/\1/" | uniq  >"$2".actual
+	grep "will be replaced by" "$2" | sed -e "s/\(.*\) in .*$/\1/" | uniq  >"$2".actual
 	test_cmp "$2".expect "$2".actual
 }
 
-- 
2.35.1

Re: [PATCH] convert: clarify line ending conversion warning

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 03 2022, Alex Henrie wrote:

> The warning about converting line endings is extremely confusing. Its
> two sentences each use the word "will" without specifying a timeframe,
> which makes it sound like both sentences are referring to the same
> timeframe. On top of that, it uses the term "original line endings"
> without saying whether "original" means LF or CRLF.
>
> Rephrase the warning to be clear that line endings have not been changed
> in the working directory but will be changed on the next checkout, and
> explicitly say which line endings the file currently has in the working
> directory.
>
> Example commands to trigger the warning on Linux:
>
> git config core.autocrlf true
> echo 'Hello world!' > hello.txt
> git add hello.txt
> git commit -m "Add hello.txt"

Just eyeballing this, while there might be some improvements here one
thing to consider is atht now %s isn't at the end of the line.

It's fine for hello.txt, but consider longer paths, before:

	LF will be replaced by CRLF in /home/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello.txt.

V.s.:

	LF will be replaced by CRLF in /home/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello/hello.txt the next time you check it out.

Re: [PATCH] convert: clarify line ending conversion warning

[Tao Klerks]

On Mon, Apr 4, 2022 at 7:52 AM Alex Henrie <alexhenrie24@gmail.com> wrote:
>
> The warning about converting line endings is extremely confusing. Its
> two sentences each use the word "will" without specifying a timeframe,
> which makes it sound like both sentences are referring to the same
> timeframe. On top of that, it uses the term "original line endings"
> without saying whether "original" means LF or CRLF.
>
> Rephrase the warning to be clear that line endings have not been changed
> in the working directory but will be changed on the next checkout, and
> explicitly say which line endings the file currently has in the working
> directory.
>

I think this change is generally a great idea (I agree the existing
message is confusing / not as helpful as it could be).

I think we could do slightly better than "the next time you check it
out", however, in terms of clarity. I just tested and the way I
personally would inelegantly describe it is "the next time you switch
to, merge in, or rebase a branch that has different content for this
file, or explicitly check the file out". I understand "check it out"
is probably technically correct, but as a user, I'm left wondering
whether that's the next time I check it out from scratch (i.e., switch
to a branch that does not have the file, and then switch back - which,
incidentally, is what I originally *assumed* would happen before your
patch - I had never checked), or the next time I switch branches at
all, or the next time I switch to (/merge in /rebase onto) a branch
that has different content for that *specific file*.

Re: [PATCH] convert: clarify line ending conversion warning

[Alex Henrie]

On Mon, Apr 4, 2022 at 9:22 AM Tao Klerks <tao@klerks.biz> wrote:
>
> On Mon, Apr 4, 2022 at 7:52 AM Alex Henrie <alexhenrie24@gmail.com> wrote:
> >
> > The warning about converting line endings is extremely confusing. Its
> > two sentences each use the word "will" without specifying a timeframe,
> > which makes it sound like both sentences are referring to the same
> > timeframe. On top of that, it uses the term "original line endings"
> > without saying whether "original" means LF or CRLF.
> >
> > Rephrase the warning to be clear that line endings have not been changed
> > in the working directory but will be changed on the next checkout, and
> > explicitly say which line endings the file currently has in the working
> > directory.
> >
>
> I think this change is generally a great idea (I agree the existing
> message is confusing / not as helpful as it could be).
>
> I think we could do slightly better than "the next time you check it
> out", however, in terms of clarity. I just tested and the way I
> personally would inelegantly describe it is "the next time you switch
> to, merge in, or rebase a branch that has different content for this
> file, or explicitly check the file out". I understand "check it out"
> is probably technically correct, but as a user, I'm left wondering
> whether that's the next time I check it out from scratch (i.e., switch
> to a branch that does not have the file, and then switch back - which,
> incidentally, is what I originally *assumed* would happen before your
> patch - I had never checked), or the next time I switch branches at
> all, or the next time I switch to (/merge in /rebase onto) a branch
> that has different content for that *specific file*.

I would prefer a short message to a long-winded one. How about "the
next time Git touches it"?

-Alex

Re: [PATCH] convert: clarify line ending conversion warning

[Tao Klerks]

On Mon, Apr 4, 2022 at 7:23 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
>
> On Mon, Apr 4, 2022 at 9:22 AM Tao Klerks <tao@klerks.biz> wrote:
> >
> > I think we could do slightly better than "the next time you check it
> > out", however, in terms of clarity.
>
> I would prefer a short message to a long-winded one. How about "the
> next time Git touches it"?
>
> -Alex

Works for me!

Re: [PATCH] convert: clarify line ending conversion warning

[Junio C Hamano]

Alex Henrie <alexhenrie24@gmail.com> writes:

> Rephrase the warning to be clear that line endings have not been changed
> in the working directory but will be changed on the next checkout, and
> explicitly say which line endings the file currently has in the working
> directory.
>
> Example commands to trigger the warning on Linux:
>
> git config core.autocrlf true
> echo 'Hello world!' > hello.txt
> git add hello.txt
> git commit -m "Add hello.txt"

While the "example" does not hurt, because the log message is not
executable, it would not help to its potential unless you add its
expected output to go with it.

    On a platform whose native line endings are not CRLF
    (e.g. Linux), the "git add" step in the sequence triggers the
    waring in question.

    $ git config core.autocrlf true
    $ echo 'Hello world!' >hello.txt
    $ git add hello.txt
    warning: LF will be replaced by CRLF in hello.txt
    The file will have its original line endings in your working directory

or something like that.

I think the recent trend is to enclose end-user supplied strings
(like misspelt option names, arguments to options, and pathnames)
inside single quote, so

    warning: LF will be replaced by CRLF in 'hello.txt'.

would probably be a good idea, on top of what you are aiming for.
Also, in a multi-sentence warning message like this, I do not think
it makes sense to omit the end-of-sentence full-stop.

> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
> diff --git a/convert.c b/convert.c
> index 8e39731efb..b024d74222 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -195,9 +195,10 @@ static void check_global_conv_flags_eol(const char *path,
>  		if (conv_flags & CONV_EOL_RNDTRP_DIE)
>  			die(_("CRLF would be replaced by LF in %s"), path);
>  		else if (conv_flags & CONV_EOL_RNDTRP_WARN)
> -			warning(_("CRLF will be replaced by LF in %s.\n"
> -				  "The file will have its original line"
> -				  " endings in your working directory"), path);
> +			warning(_("CRLF will be replaced by LF in %s the next"
> +				  " time you check it out.\n"
> +				  "For now, the file still has CRLF line"
> +				  " endings in your working directory."), path);

I have mixed feelings with this change, even though I agree that the
original is not good.  The first sentence of the updated text
already says that right now, the file ends with CRLF, and that the
conversion happen the next time you check out the file to the
working tree.  And that makes "For now ... still" totally redundant.

Perhaps a single sentence, nothing more than

	warning: in '%s', CRLF will be replaced by LF the next time
	you check it out

is sufficient?  I dunno.

Thanks.

Re: [PATCH] convert: clarify line ending conversion warning

[Alex Henrie]

On Mon, Apr 4, 2022 at 10:53 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > Rephrase the warning to be clear that line endings have not been changed
> > in the working directory but will be changed on the next checkout, and
> > explicitly say which line endings the file currently has in the working
> > directory.
> >
> > Example commands to trigger the warning on Linux:
> >
> > git config core.autocrlf true
> > echo 'Hello world!' > hello.txt
> > git add hello.txt
> > git commit -m "Add hello.txt"
>
> While the "example" does not hurt, because the log message is not
> executable, it would not help to its potential unless you add its
> expected output to go with it.
>
>     On a platform whose native line endings are not CRLF
>     (e.g. Linux), the "git add" step in the sequence triggers the
>     waring in question.
>
>     $ git config core.autocrlf true
>     $ echo 'Hello world!' >hello.txt
>     $ git add hello.txt
>     warning: LF will be replaced by CRLF in hello.txt
>     The file will have its original line endings in your working directory
>
> or something like that.
>
> I think the recent trend is to enclose end-user supplied strings
> (like misspelt option names, arguments to options, and pathnames)
> inside single quote, so
>
>     warning: LF will be replaced by CRLF in 'hello.txt'.
>
> would probably be a good idea, on top of what you are aiming for.
> Also, in a multi-sentence warning message like this, I do not think
> it makes sense to omit the end-of-sentence full-stop.

Sure. I'll do all of that in the v2.

> I have mixed feelings with this change, even though I agree that the
> original is not good.  The first sentence of the updated text
> already says that right now, the file ends with CRLF, and that the
> conversion happen the next time you check out the file to the
> working tree.  And that makes "For now ... still" totally redundant.
>
> Perhaps a single sentence, nothing more than
>
>         warning: in '%s', CRLF will be replaced by LF the next time
>         you check it out
>
> is sufficient?  I dunno.

I like your idea to move the file name to the start of the message,
and I think that will address Ævar's concern as well.

It doesn't matter to me whether the message is one sentence or two
sentences. If you have a slight preference for one sentence then let's
just do that.

Thanks for the feedback!

-Alex

Re: [PATCH] convert: clarify line ending conversion warning

[Junio C Hamano]

Alex Henrie <alexhenrie24@gmail.com> writes:

>> I have mixed feelings with this change, even though I agree that the
>> original is not good.  The first sentence of the updated text
>> already says that right now, the file ends with CRLF, and that the
>> conversion happen the next time you check out the file to the
>> working tree.  And that makes "For now ... still" totally redundant.
>>
>> Perhaps a single sentence, nothing more than
>>
>>         warning: in '%s', CRLF will be replaced by LF the next time
>>         you check it out
>>
>> is sufficient?  I dunno.
>
> I like your idea to move the file name to the start of the message,
> and I think that will address Ævar's concern as well.
>
> It doesn't matter to me whether the message is one sentence or two
> sentences. If you have a slight preference for one sentence then let's
> just do that.

"For now" smelled like sending a wrong and/or confusing message.  Is
it "for now" until we update the software, and with future versions
of Git, "git add" will pretend as if the user immediately checked it
out?

While I do not care too deeply between one and two sentences,
getting rid of the seemingly redundant "For now" sentence will
remove the source of such confusion, so ....