[PATCH v2] convert: clarify line ending conversion warning

[PATCH v2] 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 about when the line endings will be
changed and what they will be changed to.

On a platform whose native line endings are not CRLF (e.g. Linux), the
"git add" step in the following sequence triggers the warning 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

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
v2:
- Quote the confusing warning in the above commit message
- Move the file name to the beginning of the warning message
- Put the file name in single quotes
- Use the arguably broader verb "touch" instead of "check out"
- Remove the period at the end of the sentence
- Remove the second sentence entirely
---
 convert.c            | 10 ++++------
 t/t0027-auto-crlf.sh |  8 ++++----
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/convert.c b/convert.c
index 8e39731efb..733e581cb9 100644
--- a/convert.c
+++ b/convert.c
@@ -195,9 +195,8 @@ 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(_("In '%s', CRLF will be replaced by LF the"
+				  " next time Git touches it"), path);
 	} else if (old_stats->lonelf && !new_stats->lonelf ) {
 		/*
 		 * CRLFs would be added by checkout
@@ -205,9 +204,8 @@ 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(_("In '%s', LF will be replaced by CRLF the"
+				  " next time Git touches it"), path);
 	}
 }
 
diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index 0feb41a23f..7f80f46393 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -77,12 +77,12 @@ create_NNO_MIX_files () {
 
 check_warning () {
 	case "$1" in
-	LF_CRLF) echo "warning: LF will be replaced by CRLF" >"$2".expect ;;
-	CRLF_LF) echo "warning: CRLF will be replaced by LF" >"$2".expect ;;
-	'')	                                                 >"$2".expect ;;
+	LF_CRLF) echo "LF will be replaced by CRLF" >"$2".expect ;;
+	CRLF_LF) echo "CRLF will be replaced by LF" >"$2".expect ;;
+	'')	                                    >"$2".expect ;;
 	*) echo >&2 "Illegal 1": "$1" ; return false ;;
 	esac
-	grep "will be replaced by" "$2" | sed -e "s/\(.*\) in [^ ]*$/\1/" | uniq  >"$2".actual
+	sed -e "s/^.* \([^ ]* will be replaced by [^ ]*\) .*$/\1/" "$2" | uniq  >"$2".actual
 	test_cmp "$2".expect "$2".actual
 }
 
-- 
2.35.1

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

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 04 2022, Alex Henrie wrote:

Friendly hint: just for the future: Please use the --in-reply-to of
git-format-patch when submitting re-rolls, so threading with the
original thread won't be broken.

> 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 about when the line endings will be
> changed and what they will be changed to.
>
> On a platform whose native line endings are not CRLF (e.g. Linux), the
> "git add" step in the following sequence triggers the warning 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
>
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
> v2:
> - Quote the confusing warning in the above commit message
> - Move the file name to the beginning of the warning message

In case that's in response to my v1 review, I was pointing out pretty
much the opposite, i.e. try with this:
	
	diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
	index 7f80f463930..26b939566d4 100755
	--- a/t/t0027-auto-crlf.sh
	+++ b/t/t0027-auto-crlf.sh
	@@ -100,7 +100,12 @@ commit_check_warn () {
	 	do
	 		fname=${pfx}_$f.txt &&
	 		cp $f $fname &&
	-		git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err"
	+		cp $f $fname &&
	+		cp $f $fname.1 &&
	+		cp $f $fname.2 &&
	+		cp $f $fname.3 &&
	+
	+		git -c core.autocrlf=$crlf add $fname*
	 	done &&
	 	git commit -m "core.autocrlf $crlf" &&
	 	check_warning "$lfname" ${pfx}_LF.err &&

Then run with -vixd, we'll without your change emit e.g.:
	
	+ git -c core.autocrlf=true add crlf_true_attr__CRLF_mix_LF.txt crlf_true_attr__CRLF_mix_LF.txt.1 crlf_true_attr__CRLF_mix_LF.txt.2 crlf_true_attr__CRLF_mix_LF.txt.3
	warning: LF will be replaced by CRLF in crlf_true_attr__CRLF_mix_LF.txt.
	The file will have its original line endings in your working directory
	warning: LF will be replaced by CRLF in crlf_true_attr__CRLF_mix_LF.txt.1.
	The file will have its original line endings in your working directory
	warning: LF will be replaced by CRLF in crlf_true_attr__CRLF_mix_LF.txt.2.
	The file will have its original line endings in your working directory
	warning: LF will be replaced by CRLF in crlf_true_attr__CRLF_mix_LF.txt.3.
	The file will have its original line endings in your working directory

But with your change applied:

	+ git -c core.autocrlf=true add crlf_true_attr__CRLF_mix_LF.txt crlf_true_attr__CRLF_mix_LF.txt.1 crlf_true_attr__CRLF_mix_LF.txt.2 crlf_true_attr__CRLF_mix_LF.txt.3
	warning: In 'crlf_true_attr__CRLF_mix_LF.txt', LF will be replaced by CRLF the next time Git touches it
	warning: In 'crlf_true_attr__CRLF_mix_LF.txt.1', LF will be replaced by CRLF the next time Git touches it
	warning: In 'crlf_true_attr__CRLF_mix_LF.txt.2', LF will be replaced by CRLF the next time Git touches it
	warning: In 'crlf_true_attr__CRLF_mix_LF.txt.3', LF will be replaced by CRLF the next time Git touches it

Now, the existing message really sucks, we should really should
buffering this up somehow and saying "here's the problem with these N
files\n<list of N>".

See Tao's recent e4921d877ab (tracking branches: add advice to ambiguous
refspec error, 2022-04-01) for an example

But having a message in the form of:

    %s blah blah

As opposed to:

    blah blah %s

Where %s is going to be expanded as a path is IMO worse, because both
are repetative, but at least with the latter the constant part of the
message is aligned.

But anyway, while I'm still iffy about this change because it seems like
a bit of improvement and a bit of regression.

Especially as we don't offer the working directory advice at all, and
because the clear explanation about what happens to the file has been
replaced by some "the next time Git touches it" suggestion.

Which, if I wasn't looking at the pre- and post-image would completely
confuse me.

Anyway.

I'd much rather have us focus less on re-arranging the deck chairs on
the Titanic here.

The real issue is that we're emitting this long message in a loop. Now,
I was expecting that it would be painful to do something like Tao's
change since we're deep in the guts of convert.c, but if I set a
breakpoint on warning in gdb I'll get:
	
	(gdb) bt
	#0  warning (warn=0x77ed12 "In '%s', LF will be replaced by CRLF the next time Git touches it") at usage.c:285
	#1  0x0000000000572479 in check_global_conv_flags_eol (path=0x8a1e84 "LF", old_stats=0x7fffffff7af0, new_stats=0x7fffffff7ac8, conv_flags=18) at convert.c:207
	#2  0x000000000056fd49 in crlf_to_git (istate=0x855f70 <the_index>, path=0x8a1e84 "LF", src=0x8a21b0 "$Id: ", '0' <repeats 40 times>, " $\nLINEONE\nLINETWO\nLINETHREEHREE", len=73,
	    buf=0x7fffffff7bf8, crlf_action=CRLF_AUTO_CRLF, conv_flags=18) at convert.c:537
	#3  0x000000000056f88d in convert_to_git (istate=0x855f70 <the_index>, path=0x8a1e84 "LF", src=0x8a21b0 "$Id: ", '0' <repeats 40 times>, " $\nLINEONE\nLINETWO\nLINETHREEHREE", len=73,
	    dst=0x7fffffff7bf8, conv_flags=18) at convert.c:1428
	#4  0x0000000000619a4b in index_mem (istate=0x855f70 <the_index>, oid=0x7ffff7bfda98, buf=0x8a21b0, size=73, type=OBJ_BLOB, path=0x8a1e84 "LF", flags=1) at object-file.c:2179
	#5  0x0000000000616650 in index_core (istate=0x855f70 <the_index>, oid=0x7ffff7bfda98, fd=4, size=73, type=OBJ_BLOB, path=0x8a1e84 "LF", flags=1) at object-file.c:2265
	#6  0x000000000061621c in index_fd (istate=0x855f70 <the_index>, oid=0x7ffff7bfda98, fd=4, st=0x7fffffff7e88, type=OBJ_BLOB, path=0x8a1e84 "LF", flags=1) at object-file.c:2313
	#7  0x000000000061682e in index_path (istate=0x855f70 <the_index>, oid=0x7ffff7bfda98, path=0x8a1e84 "LF", st=0x7fffffff7e88, flags=1) at object-file.c:2334
	#8  0x0000000000657b26 in add_to_index (istate=0x855f70 <the_index>, path=0x8a1e84 "LF", st=0x7fffffff7e88, flags=0) at read-cache.c:830
	#9  0x0000000000658183 in add_file_to_index (istate=0x855f70 <the_index>, path=0x8a1e84 "LF", flags=0) at read-cache.c:863
	#10 0x000000000040a48a in add_files (dir=0x7fffffff8020, flags=0) at builtin/add.c:469
	#11 0x0000000000409aa1 in cmd_add (argc=428, argv=0x7fffffff8518, prefix=0x0) at builtin/add.c:681
	#12 0x0000000000407cbb in run_builtin (p=0x8237f0 <commands>, argc=429, argv=0x7fffffff8510) at git.c:465
	#13 0x0000000000406772 in handle_builtin (argc=429, argv=0x7fffffff8510) at git.c:719
	#14 0x0000000000407696 in run_argv (argcp=0x7fffffff83bc, argv=0x7fffffff83b0) at git.c:786
	#15 0x0000000000406531 in cmd_main (argc=429, argv=0x7fffffff8510) at git.c:917
	#16 0x000000000051202a in main (argc=432, argv=0x7fffffff84f8) at common-main.c:56
	(gdb)

Which was a pleseant surprise, i.e. here we're dealing with the "struct
index_state" all the way down to the warning caller.

So that seems like a good candidate for this. Let's try:
	
	diff --git a/builtin/add.c b/builtin/add.c
	index 3ffb86a4338..189a7337fa1 100644
	--- a/builtin/add.c
	+++ b/builtin/add.c
	@@ -447,6 +447,11 @@ static int add_files(struct dir_struct *dir, int flags)
	 {
	 	int i, exit_status = 0;
	 	struct string_list matched_sparse_paths = STRING_LIST_INIT_NODUP;
	+	struct string_list crlf2lf = STRING_LIST_INIT_NODUP;
	+	struct string_list lf2crlf = STRING_LIST_INIT_NODUP;
	+
	+	the_index.crlf2lf = &crlf2lf;
	+	the_index.lf2crlf = &lf2crlf;
	 
	 	if (dir->ignored_nr) {
	 		fprintf(stderr, _(ignore_error));
	@@ -480,7 +485,26 @@ static int add_files(struct dir_struct *dir, int flags)
	 		exit_status = 1;
	 	}
	 
	+	if (crlf2lf.nr) {
	+		struct string_list_item *item;
	+
	+		warning("ohes noes crlf2lf:");
	+		for_each_string_list_item(item, &crlf2lf)
	+			warning("\t%s", item->string);
	+	}
	+	if (lf2crlf.nr) {
	+		struct string_list_item *item;
	+
	+		warning("ohes noes lf2crlf:");
	+		for_each_string_list_item(item, &lf2crlf)
	+			warning("\t%s", item->string);
	+	}
	+
	 	string_list_clear(&matched_sparse_paths, 0);
	+	string_list_clear(&crlf2lf, 0);
	+	string_list_clear(&lf2crlf, 0);
	+	the_index.crlf2lf = NULL;
	+	the_index.lf2crlf = NULL;
	 
	 	return exit_status;
	 }
	diff --git a/cache.h b/cache.h
	index 6226f6a8a53..3538cdebd61 100644
	--- a/cache.h
	+++ b/cache.h
	@@ -341,6 +341,8 @@ struct index_state {
	 	struct progress *progress;
	 	struct repository *repo;
	 	struct pattern_list *sparse_checkout_patterns;
	+	struct string_list *crlf2lf;
	+	struct string_list *lf2crlf;
	 };
	 
	 /* Name hashing */
	diff --git a/convert.c b/convert.c
	index 733e581cb98..e1f8386185a 100644
	--- a/convert.c
	+++ b/convert.c
	@@ -185,8 +185,11 @@ static enum eol output_eol(enum convert_crlf_action crlf_action)
	 }
	 
	 static void check_global_conv_flags_eol(const char *path,
	-			    struct text_stat *old_stats, struct text_stat *new_stats,
	-			    int conv_flags)
	+					struct text_stat *old_stats,
	+					struct text_stat *new_stats,
	+					int conv_flags,
	+					struct string_list *crlf2lf,
	+					struct string_list *lf2crlf)
	 {
	 	if (old_stats->crlf && !new_stats->crlf ) {
	 		/*
	@@ -194,18 +197,20 @@ 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 && crlf2lf)
	+			string_list_append(crlf2lf, path);
	 		else if (conv_flags & CONV_EOL_RNDTRP_WARN)
	-			warning(_("In '%s', CRLF will be replaced by LF the"
	-				  " next time Git touches it"), path);
	+			BUG("unreachable");
	 	} else if (old_stats->lonelf && !new_stats->lonelf ) {
	 		/*
	 		 * CRLFs would be added by checkout
	 		 */
	 		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 && lf2crlf)
	+			string_list_append(lf2crlf, path);
	 		else if (conv_flags & CONV_EOL_RNDTRP_WARN)
	-			warning(_("In '%s', LF will be replaced by CRLF the"
	-				  " next time Git touches it"), path);
	+			BUG("unreachable");
	 	}
	 }
	 
	@@ -534,7 +539,8 @@ static int crlf_to_git(struct index_state *istate,
	 			new_stats.crlf += new_stats.lonelf;
	 			new_stats.lonelf = 0;
	 		}
	-		check_global_conv_flags_eol(path, &stats, &new_stats, conv_flags);
	+		check_global_conv_flags_eol(path, &stats, &new_stats, conv_flags,
	+					    istate->crlf2lf, istate->lf2crlf);
	 	}
	 	if (!convert_crlf_into_lf)
	 		return 0;

Which, with the test change above emits:


	$ git reset; ~/g/git/git -c core.autocrlf=true add *
	Unstaged changes after reset:
	M       .gitattributes
	warning: ohes noes lf2crlf:
	warning:        CRLF_mix_LF
	warning:        LF
	warning:        crlf_true_attr__LF.err.expect
	warning:        tmp

Now, obviously that's crappy WIP code as far as the message goes., and
the BUG() will be hit by the other caller in diff.c (which hooks in just
before a call to convert_to_git(), or we'd fall back on that.

Also, i18n with _() is needed, and the message needs to be prettier, but
with just a tiny bit of polishing (hint hint!) this would be sooo much
better.

You could even use e4921d877ab as a template to add advice() output for
this, so we could save ourselves the verbosity/output for users who'd
like to configure this away.

Maybe that's all a bit too much, sorry. I just get excited about
improving some of the crappy parts of our UX sometimes. If you don't
want to run with this I suppose it could be done as a follow-up.

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

[Alex Henrie]

On Tue, Apr 5, 2022 at 3:35 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> On Mon, Apr 04 2022, Alex Henrie wrote:
>
> Friendly hint: just for the future: Please use the --in-reply-to of
> git-format-patch when submitting re-rolls, so threading with the
> original thread won't be broken.
>
> > 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 about when the line endings will be
> > changed and what they will be changed to.
> >
> > On a platform whose native line endings are not CRLF (e.g. Linux), the
> > "git add" step in the following sequence triggers the warning 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
> >
> > Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> > ---
> > v2:
> > - Quote the confusing warning in the above commit message
> > - Move the file name to the beginning of the warning message
>
> In case that's in response to my v1 review, I was pointing out pretty
> much the opposite, i.e. try with this:
>
>         diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
>         index 7f80f463930..26b939566d4 100755
>         --- a/t/t0027-auto-crlf.sh
>         +++ b/t/t0027-auto-crlf.sh
>         @@ -100,7 +100,12 @@ commit_check_warn () {
>                 do
>                         fname=${pfx}_$f.txt &&
>                         cp $f $fname &&
>         -               git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err"
>         +               cp $f $fname &&
>         +               cp $f $fname.1 &&
>         +               cp $f $fname.2 &&
>         +               cp $f $fname.3 &&
>         +
>         +               git -c core.autocrlf=$crlf add $fname*
>                 done &&
>                 git commit -m "core.autocrlf $crlf" &&
>                 check_warning "$lfname" ${pfx}_LF.err &&
>
> Then run with -vixd, we'll without your change emit e.g.:
>
>         + git -c core.autocrlf=true add crlf_true_attr__CRLF_mix_LF.txt crlf_true_attr__CRLF_mix_LF.txt.1 crlf_true_attr__CRLF_mix_LF.txt.2 crlf_true_attr__CRLF_mix_LF.txt.3
>         warning: LF will be replaced by CRLF in crlf_true_attr__CRLF_mix_LF.txt.
>         The file will have its original line endings in your working directory
>         warning: LF will be replaced by CRLF in crlf_true_attr__CRLF_mix_LF.txt.1.
>         The file will have its original line endings in your working directory
>         warning: LF will be replaced by CRLF in crlf_true_attr__CRLF_mix_LF.txt.2.
>         The file will have its original line endings in your working directory
>         warning: LF will be replaced by CRLF in crlf_true_attr__CRLF_mix_LF.txt.3.
>         The file will have its original line endings in your working directory
>
> But with your change applied:
>
>         + git -c core.autocrlf=true add crlf_true_attr__CRLF_mix_LF.txt crlf_true_attr__CRLF_mix_LF.txt.1 crlf_true_attr__CRLF_mix_LF.txt.2 crlf_true_attr__CRLF_mix_LF.txt.3
>         warning: In 'crlf_true_attr__CRLF_mix_LF.txt', LF will be replaced by CRLF the next time Git touches it
>         warning: In 'crlf_true_attr__CRLF_mix_LF.txt.1', LF will be replaced by CRLF the next time Git touches it
>         warning: In 'crlf_true_attr__CRLF_mix_LF.txt.2', LF will be replaced by CRLF the next time Git touches it
>         warning: In 'crlf_true_attr__CRLF_mix_LF.txt.3', LF will be replaced by CRLF the next time Git touches it
>
> Now, the existing message really sucks, we should really should
> buffering this up somehow and saying "here's the problem with these N
> files\n<list of N>".
>
> See Tao's recent e4921d877ab (tracking branches: add advice to ambiguous
> refspec error, 2022-04-01) for an example
>
> But having a message in the form of:
>
>     %s blah blah
>
> As opposed to:
>
>     blah blah %s
>
> Where %s is going to be expanded as a path is IMO worse, because both
> are repetative, but at least with the latter the constant part of the
> message is aligned.
>
> But anyway, while I'm still iffy about this change because it seems like
> a bit of improvement and a bit of regression.
>
> Especially as we don't offer the working directory advice at all, and
> because the clear explanation about what happens to the file has been
> replaced by some "the next time Git touches it" suggestion.
>
> Which, if I wasn't looking at the pre- and post-image would completely
> confuse me.
>
> Anyway.
>
> I'd much rather have us focus less on re-arranging the deck chairs on
> the Titanic here.
>
> The real issue is that we're emitting this long message in a loop. Now,
> I was expecting that it would be painful to do something like Tao's
> change since we're deep in the guts of convert.c, but if I set a
> breakpoint on warning in gdb I'll get:
>
>         (gdb) bt
>         #0  warning (warn=0x77ed12 "In '%s', LF will be replaced by CRLF the next time Git touches it") at usage.c:285
>         #1  0x0000000000572479 in check_global_conv_flags_eol (path=0x8a1e84 "LF", old_stats=0x7fffffff7af0, new_stats=0x7fffffff7ac8, conv_flags=18) at convert.c:207
>         #2  0x000000000056fd49 in crlf_to_git (istate=0x855f70 <the_index>, path=0x8a1e84 "LF", src=0x8a21b0 "$Id: ", '0' <repeats 40 times>, " $\nLINEONE\nLINETWO\nLINETHREEHREE", len=73,
>             buf=0x7fffffff7bf8, crlf_action=CRLF_AUTO_CRLF, conv_flags=18) at convert.c:537
>         #3  0x000000000056f88d in convert_to_git (istate=0x855f70 <the_index>, path=0x8a1e84 "LF", src=0x8a21b0 "$Id: ", '0' <repeats 40 times>, " $\nLINEONE\nLINETWO\nLINETHREEHREE", len=73,
>             dst=0x7fffffff7bf8, conv_flags=18) at convert.c:1428
>         #4  0x0000000000619a4b in index_mem (istate=0x855f70 <the_index>, oid=0x7ffff7bfda98, buf=0x8a21b0, size=73, type=OBJ_BLOB, path=0x8a1e84 "LF", flags=1) at object-file.c:2179
>         #5  0x0000000000616650 in index_core (istate=0x855f70 <the_index>, oid=0x7ffff7bfda98, fd=4, size=73, type=OBJ_BLOB, path=0x8a1e84 "LF", flags=1) at object-file.c:2265
>         #6  0x000000000061621c in index_fd (istate=0x855f70 <the_index>, oid=0x7ffff7bfda98, fd=4, st=0x7fffffff7e88, type=OBJ_BLOB, path=0x8a1e84 "LF", flags=1) at object-file.c:2313
>         #7  0x000000000061682e in index_path (istate=0x855f70 <the_index>, oid=0x7ffff7bfda98, path=0x8a1e84 "LF", st=0x7fffffff7e88, flags=1) at object-file.c:2334
>         #8  0x0000000000657b26 in add_to_index (istate=0x855f70 <the_index>, path=0x8a1e84 "LF", st=0x7fffffff7e88, flags=0) at read-cache.c:830
>         #9  0x0000000000658183 in add_file_to_index (istate=0x855f70 <the_index>, path=0x8a1e84 "LF", flags=0) at read-cache.c:863
>         #10 0x000000000040a48a in add_files (dir=0x7fffffff8020, flags=0) at builtin/add.c:469
>         #11 0x0000000000409aa1 in cmd_add (argc=428, argv=0x7fffffff8518, prefix=0x0) at builtin/add.c:681
>         #12 0x0000000000407cbb in run_builtin (p=0x8237f0 <commands>, argc=429, argv=0x7fffffff8510) at git.c:465
>         #13 0x0000000000406772 in handle_builtin (argc=429, argv=0x7fffffff8510) at git.c:719
>         #14 0x0000000000407696 in run_argv (argcp=0x7fffffff83bc, argv=0x7fffffff83b0) at git.c:786
>         #15 0x0000000000406531 in cmd_main (argc=429, argv=0x7fffffff8510) at git.c:917
>         #16 0x000000000051202a in main (argc=432, argv=0x7fffffff84f8) at common-main.c:56
>         (gdb)
>
> Which was a pleseant surprise, i.e. here we're dealing with the "struct
> index_state" all the way down to the warning caller.
>
> So that seems like a good candidate for this. Let's try:
>
>         diff --git a/builtin/add.c b/builtin/add.c
>         index 3ffb86a4338..189a7337fa1 100644
>         --- a/builtin/add.c
>         +++ b/builtin/add.c
>         @@ -447,6 +447,11 @@ static int add_files(struct dir_struct *dir, int flags)
>          {
>                 int i, exit_status = 0;
>                 struct string_list matched_sparse_paths = STRING_LIST_INIT_NODUP;
>         +       struct string_list crlf2lf = STRING_LIST_INIT_NODUP;
>         +       struct string_list lf2crlf = STRING_LIST_INIT_NODUP;
>         +
>         +       the_index.crlf2lf = &crlf2lf;
>         +       the_index.lf2crlf = &lf2crlf;
>
>                 if (dir->ignored_nr) {
>                         fprintf(stderr, _(ignore_error));
>         @@ -480,7 +485,26 @@ static int add_files(struct dir_struct *dir, int flags)
>                         exit_status = 1;
>                 }
>
>         +       if (crlf2lf.nr) {
>         +               struct string_list_item *item;
>         +
>         +               warning("ohes noes crlf2lf:");
>         +               for_each_string_list_item(item, &crlf2lf)
>         +                       warning("\t%s", item->string);
>         +       }
>         +       if (lf2crlf.nr) {
>         +               struct string_list_item *item;
>         +
>         +               warning("ohes noes lf2crlf:");
>         +               for_each_string_list_item(item, &lf2crlf)
>         +                       warning("\t%s", item->string);
>         +       }
>         +
>                 string_list_clear(&matched_sparse_paths, 0);
>         +       string_list_clear(&crlf2lf, 0);
>         +       string_list_clear(&lf2crlf, 0);
>         +       the_index.crlf2lf = NULL;
>         +       the_index.lf2crlf = NULL;
>
>                 return exit_status;
>          }
>         diff --git a/cache.h b/cache.h
>         index 6226f6a8a53..3538cdebd61 100644
>         --- a/cache.h
>         +++ b/cache.h
>         @@ -341,6 +341,8 @@ struct index_state {
>                 struct progress *progress;
>                 struct repository *repo;
>                 struct pattern_list *sparse_checkout_patterns;
>         +       struct string_list *crlf2lf;
>         +       struct string_list *lf2crlf;
>          };
>
>          /* Name hashing */
>         diff --git a/convert.c b/convert.c
>         index 733e581cb98..e1f8386185a 100644
>         --- a/convert.c
>         +++ b/convert.c
>         @@ -185,8 +185,11 @@ static enum eol output_eol(enum convert_crlf_action crlf_action)
>          }
>
>          static void check_global_conv_flags_eol(const char *path,
>         -                           struct text_stat *old_stats, struct text_stat *new_stats,
>         -                           int conv_flags)
>         +                                       struct text_stat *old_stats,
>         +                                       struct text_stat *new_stats,
>         +                                       int conv_flags,
>         +                                       struct string_list *crlf2lf,
>         +                                       struct string_list *lf2crlf)
>          {
>                 if (old_stats->crlf && !new_stats->crlf ) {
>                         /*
>         @@ -194,18 +197,20 @@ 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 && crlf2lf)
>         +                       string_list_append(crlf2lf, path);
>                         else if (conv_flags & CONV_EOL_RNDTRP_WARN)
>         -                       warning(_("In '%s', CRLF will be replaced by LF the"
>         -                                 " next time Git touches it"), path);
>         +                       BUG("unreachable");
>                 } else if (old_stats->lonelf && !new_stats->lonelf ) {
>                         /*
>                          * CRLFs would be added by checkout
>                          */
>                         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 && lf2crlf)
>         +                       string_list_append(lf2crlf, path);
>                         else if (conv_flags & CONV_EOL_RNDTRP_WARN)
>         -                       warning(_("In '%s', LF will be replaced by CRLF the"
>         -                                 " next time Git touches it"), path);
>         +                       BUG("unreachable");
>                 }
>          }
>
>         @@ -534,7 +539,8 @@ static int crlf_to_git(struct index_state *istate,
>                                 new_stats.crlf += new_stats.lonelf;
>                                 new_stats.lonelf = 0;
>                         }
>         -               check_global_conv_flags_eol(path, &stats, &new_stats, conv_flags);
>         +               check_global_conv_flags_eol(path, &stats, &new_stats, conv_flags,
>         +                                           istate->crlf2lf, istate->lf2crlf);
>                 }
>                 if (!convert_crlf_into_lf)
>                         return 0;
>
> Which, with the test change above emits:
>
>
>         $ git reset; ~/g/git/git -c core.autocrlf=true add *
>         Unstaged changes after reset:
>         M       .gitattributes
>         warning: ohes noes lf2crlf:
>         warning:        CRLF_mix_LF
>         warning:        LF
>         warning:        crlf_true_attr__LF.err.expect
>         warning:        tmp
>
> Now, obviously that's crappy WIP code as far as the message goes., and
> the BUG() will be hit by the other caller in diff.c (which hooks in just
> before a call to convert_to_git(), or we'd fall back on that.
>
> Also, i18n with _() is needed, and the message needs to be prettier, but
> with just a tiny bit of polishing (hint hint!) this would be sooo much
> better.
>
> You could even use e4921d877ab as a template to add advice() output for
> this, so we could save ourselves the verbosity/output for users who'd
> like to configure this away.
>
> Maybe that's all a bit too much, sorry. I just get excited about
> improving some of the crappy parts of our UX sometimes. If you don't
> want to run with this I suppose it could be done as a follow-up.

Thanks for the extensive feedback. So, there are two problems here:

1. The current warning is very confusingly worded

2. The current warning is repetitive if there are a lot of files

For the moment, I'm only trying to address problem #1. Junio asked for
the file name to be moved to near the beginning of the message and for
the second sentence to be removed. Tao asked to avoid the verb "check
out" and said that "touch" was better. What wording would you suggest?

-Alex

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

[Junio C Hamano]

Alex Henrie <alexhenrie24@gmail.com> writes:

>> Now, the existing message really sucks, we should really should
>> buffering this up somehow and saying "here's the problem with these N
>> files\n<list of N>".
> ...
> Thanks for the extensive feedback. So, there are two problems here:
>
> 1. The current warning is very confusingly worded
>
> 2. The current warning is repetitive if there are a lot of files
>
> For the moment, I'm only trying to address problem #1. Junio asked for
> the file name to be moved to near the beginning of the message and for
> the second sentence to be removed. Tao asked to avoid the verb "check
> out" and said that "touch" was better. What wording would you suggest?

Heh, I do not ask contributors to do anything.  When there is only
one right way to write a code, I may tell them to do exactly that
way, but that is rare.  I only suggest an alternative or two and
have contributors to think themselves.  Ah, I am asking them to
think for themselves in that case, but that does not count ;-)

Anyway.

I do not have a strong opinion between checkout and touch myself.
Switching to "touch" did make it less technically incorrect than
saying "checkout" by making it a bit more vague, which can cut both
ways, but if "touch" can squelch complaints from hair-splitters like
"I did 'git checkout' (no arguments) immediately after I saw the
message but nothing changed", that may be a good change.

Between "we have a problem with long description in 'this file'"
that pushes the most crucial information to the far right in the
output that is necessary to tell where to look for the problem, and
"in 'this file', we have a problem with long description", I would
imagine that readers prefer to see the former, simply because it
lets them spot the pathname easier.  The repetitiveness is an issue
that may need to be addressed separately, and the repetitiveness
might make it look as if repeating the long problem description in
the front part of the message is better by aligning the repetitive
message and by numbing readers' mind, but I do not think that
benefit outweighs the downside of hiding more important information
by pushing it far to the right.

Just my two cents.

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

[Torsten Bögershausen]

On Mon, Apr 04, 2022 at 11:35:59PM -0600, 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 about when the line endings will be
> changed and what they will be changed to.
>
> On a platform whose native line endings are not CRLF (e.g. Linux), the
> "git add" step in the following sequence triggers the warning 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
>
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
> v2:
> - Quote the confusing warning in the above commit message
> - Move the file name to the beginning of the warning message
> - Put the file name in single quotes
> - Use the arguably broader verb "touch" instead of "check out"
> - Remove the period at the end of the sentence
> - Remove the second sentence entirely
> ---
>  convert.c            | 10 ++++------
>  t/t0027-auto-crlf.sh |  8 ++++----
>  2 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/convert.c b/convert.c
> index 8e39731efb..733e581cb9 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -195,9 +195,8 @@ 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(_("In '%s', CRLF will be replaced by LF the"
> +				  " next time Git touches it"), path);

(Somewhat late to the discussions), thanks for picking this up.
May be we can use "updates" instead of "touches" ?

"In '%s', CRLF will be replaced by LF the next time Git updates it"

Or may be

"In '%s', CRLF will be replaced by LF the next time a `git checkout` updates it"


[snip]

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

[Tao Klerks]

On Wed, Apr 6, 2022 at 8:04 PM Torsten Bögershausen <tboegi@web.de> wrote:

> May be we can use "updates" instead of "touches" ?
>
> "In '%s', CRLF will be replaced by LF the next time Git updates it"

Makes sense to me.

>
> Or may be
>
> "In '%s', CRLF will be replaced by LF the next time a `git checkout` updates it"
>

I believe we should stay away from the word "checkout" because it's
neither accurate nor clear, for at least a couple of reasons:
1. We have long, in principle, been promoting the use of "git switch",
and it's not obvious in a message like this one that this is
considered to be equivalent.
2. Files can be touched/updated by other commands/processes, like "git
pull" (and "rebase", and probably others I can't think of)

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

[Alex Henrie]

On Wed, Apr 6, 2022 at 12:18 PM Tao Klerks <tao@klerks.biz> wrote:
>
> On Wed, Apr 6, 2022 at 8:04 PM Torsten Bögershausen <tboegi@web.de> wrote:
>
> > May be we can use "updates" instead of "touches" ?
> >
> > "In '%s', CRLF will be replaced by LF the next time Git updates it"
>
> Makes sense to me.

I don't like this wording as much because "update" could more easily
be taken to mean something that happens in the index or in a commit,
not in the working directory. But maybe it needs more clarification
anyway, e.g. "In the working copy of '%s'..."

Ævar, would the phrase "In the working copy" resolve your concern
about the new message "[not offering] the working directory advice at
all"?

> > Or may be
> >
> > "In '%s', CRLF will be replaced by LF the next time a `git checkout` updates it"
> >
>
> I believe we should stay away from the word "checkout" because it's
> neither accurate nor clear, for at least a couple of reasons:
> 1. We have long, in principle, been promoting the use of "git switch",
> and it's not obvious in a message like this one that this is
> considered to be equivalent.
> 2. Files can be touched/updated by other commands/processes, like "git
> pull" (and "rebase", and probably others I can't think of)

I agree: Talking about the `git checkout` command is too specific.

-Alex

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

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> Or may be
>
> "In '%s', CRLF will be replaced by LF the next time a `git checkout` updates it"

As this is a warning(_("")) message, usual "after 'warning:'
heading, the message does not begin in a capital" rule would apply,
I think.

It may not be "git checkout" that updates the path.  And running
"git checkout" may not necessarily update the path.  "... the next
time Git updates it" would probably be more widely applicable.

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

[Junio C Hamano]

Tao Klerks <tao@klerks.biz> writes:

> On Wed, Apr 6, 2022 at 8:04 PM Torsten Bögershausen <tboegi@web.de> wrote:
>
>> May be we can use "updates" instead of "touches" ?
>>
>> "In '%s', CRLF will be replaced by LF the next time Git updates it"
>
> Makes sense to me.
>
>>
>> Or may be
>>
>> "In '%s', CRLF will be replaced by LF the next time a `git checkout` updates it"
>>
>
> I believe we should stay away from the word "checkout" because it's
> neither accurate nor clear, for at least a couple of reasons:
> 1. We have long, in principle, been promoting the use of "git switch",
> and it's not obvious in a message like this one that this is
> considered to be equivalent.

It is not an equivalent.  "switch" is a strict subset of "checkout"
and branch switching is not the only case that would correct the
line ending on such a file.  Checking the contents out of the index
or a named commit with "git checkout -- path" would also fix the
line endings.

> 2. Files can be touched/updated by other commands/processes, like "git
> pull" (and "rebase", and probably others I can't think of)

Yes.

[PATCH v3] 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 about when the line endings will be
changed and what they will be changed to.

On a platform whose native line endings are not CRLF (e.g. Linux), the
"git add" step in the following sequence triggers the warning 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

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
v3:
- Start the warning with a lowercase letter
- Add the phrase "the working copy of" - hopefully this resolves the
  last concerns about clarity
---
 convert.c            | 12 ++++++------
 t/t0027-auto-crlf.sh |  8 ++++----
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/convert.c b/convert.c
index 8e39731efb..4d153729da 100644
--- a/convert.c
+++ b/convert.c
@@ -195,9 +195,9 @@ 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(_("in the working copy of '%s', CRLF will be"
+				  " replaced by LF the next time Git touches"
+				  " it"), path);
 	} else if (old_stats->lonelf && !new_stats->lonelf ) {
 		/*
 		 * CRLFs would be added by checkout
@@ -205,9 +205,9 @@ 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(_("in the working copy of '%s', LF will be"
+				  " replaced by CRLF the next time Git touches"
+				  " it"), path);
 	}
 }
 
diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index 0feb41a23f..7f80f46393 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -77,12 +77,12 @@ create_NNO_MIX_files () {
 
 check_warning () {
 	case "$1" in
-	LF_CRLF) echo "warning: LF will be replaced by CRLF" >"$2".expect ;;
-	CRLF_LF) echo "warning: CRLF will be replaced by LF" >"$2".expect ;;
-	'')	                                                 >"$2".expect ;;
+	LF_CRLF) echo "LF will be replaced by CRLF" >"$2".expect ;;
+	CRLF_LF) echo "CRLF will be replaced by LF" >"$2".expect ;;
+	'')	                                    >"$2".expect ;;
 	*) echo >&2 "Illegal 1": "$1" ; return false ;;
 	esac
-	grep "will be replaced by" "$2" | sed -e "s/\(.*\) in [^ ]*$/\1/" | uniq  >"$2".actual
+	sed -e "s/^.* \([^ ]* will be replaced by [^ ]*\) .*$/\1/" "$2" | uniq  >"$2".actual
 	test_cmp "$2".expect "$2".actual
 }
 
-- 
2.35.1