t0027 racy?

t0027 racy?

[Johannes Schindelin]

Hi Torsten,

I remember that you did a ton of work on t0027. Now I see problems, and
not only that the entire script now takes a whopping 4 minutes 20 seconds
to run on my high-end Windows machine.

It appears that t0027 fails randomly for me, in seemingly random places.
Sometimes all 1388 cases pass. Sometimes "29 - commit NNO files crlf=true
attr=auto LF" fails. Sometimes it is "24 - commit NNO files crlf=false
attr=auto LF". Sometimes it is "114 - commit NNO files crlf=false
attr=auto LF", and sometimes "111 - commit NNO files attr=auto aeol=lf
crlf=false CRLF_mix_LF".

When I run it with -i -v -x --tee, it passes every single time (taking
over 5 minutes, just to make things worse)...

Any idea about any possible races?

Ciao,
Dscho

Re: t0027 racy?

[Jeff King]

On Mon, Aug 08, 2016 at 05:05:07PM +0200, Johannes Schindelin wrote:

> I remember that you did a ton of work on t0027. Now I see problems, and
> not only that the entire script now takes a whopping 4 minutes 20 seconds
> to run on my high-end Windows machine.
> 
> It appears that t0027 fails randomly for me, in seemingly random places.
> Sometimes all 1388 cases pass. Sometimes "29 - commit NNO files crlf=true
> attr=auto LF" fails. Sometimes it is "24 - commit NNO files crlf=false
> attr=auto LF". Sometimes it is "114 - commit NNO files crlf=false
> attr=auto LF", and sometimes "111 - commit NNO files attr=auto aeol=lf
> crlf=false CRLF_mix_LF".
> 
> When I run it with -i -v -x --tee, it passes every single time (taking
> over 5 minutes, just to make things worse)...
> 
> Any idea about any possible races?

Try:

  https://github.com/peff/git/blob/meta/stress

which you can run as "sh /path/to/stress t0027" in the top-level of your
git repository. I got failure within about 30 seconds on t0027 (though 5
minutes? Yeesh. It runs in 9s on my laptop. I weep for you).

The verbose output is not very exciting, though:

	expecting success: 
	                check_warning "$lfwarn" ${pfx}_LF.err

	--- NNO_attr_auto_aeol_crlf_false_LF.err.expect 2016-08-08 15:26:37.061701392 +0000
	+++ NNO_attr_auto_aeol_crlf_false_LF.err.actual 2016-08-08 15:26:37.061701392 +0000
	@@ -1 +0,0 @@
	-warning: LF will be replaced by CRLF
	not ok 114 - commit NNO files crlf=false attr=auto LF

-Peff

Re: t0027 racy?

[Torsten Bögershausen]

On 2016-08-08 17.05, Johannes Schindelin wrote:
> Hi Torsten,
> 
> I remember that you did a ton of work on t0027. Now I see problems, and
> not only that the entire script now takes a whopping 4 minutes 20 seconds
> to run on my high-end Windows machine.
> 
> It appears that t0027 fails randomly for me, in seemingly random places.
> Sometimes all 1388 cases pass. Sometimes "29 - commit NNO files crlf=true
> attr=auto LF" fails. Sometimes it is "24 - commit NNO files crlf=false
> attr=auto LF". Sometimes it is "114 - commit NNO files crlf=false
> attr=auto LF", and sometimes "111 - commit NNO files attr=auto aeol=lf
> crlf=false CRLF_mix_LF".
> 
> When I run it with -i -v -x --tee, it passes every single time (taking
> over 5 minutes, just to make things worse)...
> 
> Any idea about any possible races?
Just to double-check: I assume that you have this
commit ded2444ad8ab8128cae2b91b8efa57ea2dd8c7a5
Author: Torsten Bögershausen <tboegi@web.de>
Date:   Mon Apr 25 18:56:27 2016 +0200

    t0027: make commit_chk_wrnNNO() reliable
in your tree ?


Is there a special pattern ?
Did you
a) Update the machine ?
b) Update Git code base ?
or both ?
(Yes, I know that this may be stupid questions)

Is it only the NNO tests that fail ?
Did they ever pass ?
(I think I run them some time ago on a Virtual machine running Windows 7)

I see only "commit NNO files...." in you report, they belong to
check_warning(), which is called around line 126 in t0027.


check_warning() does a grep on a file which has been re-directed from stderr.

How reproducible is the problem ?
If you add
exit 0
After the last "commit_chk_wrnNNO" line (line 418),
does
ls -l crlf*.err
give you any hint ?

Re: t0027 racy?

[Torsten Bögershausen]

On Mon, Aug 08, 2016 at 11:29:26AM -0400, Jeff King wrote:
> On Mon, Aug 08, 2016 at 05:05:07PM +0200, Johannes Schindelin wrote:
> 
> > I remember that you did a ton of work on t0027. Now I see problems, and
> > not only that the entire script now takes a whopping 4 minutes 20 seconds
> > to run on my high-end Windows machine.
> > 
> > It appears that t0027 fails randomly for me, in seemingly random places.
> > Sometimes all 1388 cases pass. Sometimes "29 - commit NNO files crlf=true
> > attr=auto LF" fails. Sometimes it is "24 - commit NNO files crlf=false
> > attr=auto LF". Sometimes it is "114 - commit NNO files crlf=false
> > attr=auto LF", and sometimes "111 - commit NNO files attr=auto aeol=lf
> > crlf=false CRLF_mix_LF".
> > 
> > When I run it with -i -v -x --tee, it passes every single time (taking
> > over 5 minutes, just to make things worse)...
> > 
> > Any idea about any possible races?
> 
> Try:
> 
>   https://github.com/peff/git/blob/meta/stress
> 
> which you can run as "sh /path/to/stress t0027" in the top-level of your
> git repository. I got failure within about 30 seconds on t0027 (though 5
> minutes? Yeesh. It runs in 9s on my laptop. I weep for you).
> 
> The verbose output is not very exciting, though:
> 
> 	expecting success: 
> 	                check_warning "$lfwarn" ${pfx}_LF.err
> 
> 	--- NNO_attr_auto_aeol_crlf_false_LF.err.expect 2016-08-08 15:26:37.061701392 +0000
> 	+++ NNO_attr_auto_aeol_crlf_false_LF.err.actual 2016-08-08 15:26:37.061701392 +0000
> 	@@ -1 +0,0 @@
> 	-warning: LF will be replaced by CRLF
> 	not ok 114 - commit NNO files crlf=false attr=auto LF

(I realized that t0027 is not yet self-explaining, I have it on my list)

NNO_attr_auto_aeol_crlf_false_LF means:

NNO:           "Not NOrmalized" A file had been commited with CRLF in the repo
attr_auto:     .gitattributes has "* text=auto"
aeol_eol       .gitattributes has "eol=crlf"
crlf_false     git config core.autocrlf = false
LF             We commit a file with LF line endings.

This should happend:
- The file is commited "as is", with LF line endings.
- While commiting, git should print the warning
- "warning: LF will be replaced by CRLF" to stderr
- stderr is piped (redirected) into NNO_attr_auto_aeol_crlf_false_LF.err
- we grep for "will be replaced by" in xx.err and pipe it into xx.err.actual

The rest is test_cmp, this is what you see.
The warning is missing, but should be there:

The file has LF, and after commit and a new checkout these LF will
be convertet into CRLF.

So why isn't the warning there (but here on my oldish machines)

Re: t0027 racy?

[Jeff King]

On Mon, Aug 08, 2016 at 08:32:24PM +0000, Torsten Bögershausen wrote:

> > The verbose output is not very exciting, though:
> > 
> > 	expecting success: 
> > 	                check_warning "$lfwarn" ${pfx}_LF.err
> > 
> > 	--- NNO_attr_auto_aeol_crlf_false_LF.err.expect 2016-08-08 15:26:37.061701392 +0000
> > 	+++ NNO_attr_auto_aeol_crlf_false_LF.err.actual 2016-08-08 15:26:37.061701392 +0000
> > 	@@ -1 +0,0 @@
> > 	-warning: LF will be replaced by CRLF
> > 	not ok 114 - commit NNO files crlf=false attr=auto LF
> [...]
> The warning is missing, but should be there:
> 
> The file has LF, and after commit and a new checkout these LF will
> be convertet into CRLF.
> 
> So why isn't the warning there (but here on my oldish machines)

To be clear, the warning _is_ there when I just run t0027 by itself, and
the test passes.  It's only under heavy load that it isn't. So it's a
race condition either in the test script or in git itself.

Usually race conditions like these are due to one of:

  - git dying from SIGPIPE before it has a chance to output the command.
    But I don't see any pipes being used in the test script.

  - index raciness causing us to avoid reading file content. For
    example, if you do:

      echo foo >bar
      git add bar

    Then _usually_ "bar" and the index will have the same mtime. And
    therefore subsequent commands that need to refresh the index will
    re-read the content of "bar", because they cannot tell from the stat
    information if we have the latest version of "bar" in the index or
    not (it could have been written after the index update, but in the
    same second).

    But on a slow or heavily loaded system (or if you simply get unlucky
    in crossing the boundary to a new second), they'll have different
    mtimes. And therefore git knows it can skip reading the content from
    the filesystem.

    So if your test relies on git actually re-converting the file
    content, it would sometimes randomly fail.

The second one seems plausible, given the history of issues with
changing CRLF settings for an existing checkout. I'm not sure if it
would be feasible to reset the index completely before each tested
command, but that would probably solve it.

-Peff

Re: t0027 racy?

[Jeff King]

On Tue, Aug 09, 2016 at 02:51:10AM -0400, Jeff King wrote:

>   - index raciness causing us to avoid reading file content. For
>     example, if you do:
> 
>       echo foo >bar
>       git add bar
> 
>     Then _usually_ "bar" and the index will have the same mtime. And
>     therefore subsequent commands that need to refresh the index will
>     re-read the content of "bar", because they cannot tell from the stat
>     information if we have the latest version of "bar" in the index or
>     not (it could have been written after the index update, but in the
>     same second).
> 
>     But on a slow or heavily loaded system (or if you simply get unlucky
>     in crossing the boundary to a new second), they'll have different
>     mtimes. And therefore git knows it can skip reading the content from
>     the filesystem.
> 
>     So if your test relies on git actually re-converting the file
>     content, it would sometimes randomly fail.
> 
> The second one seems plausible, given the history of issues with
> changing CRLF settings for an existing checkout. I'm not sure if it
> would be feasible to reset the index completely before each tested
> command, but that would probably solve it.

And indeed, this seems to fix it for me (at least it has been running in
a 16-way loop for 5 minutes, whereas before it died after 30 seconds or
so):

diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index 2860d2d..9f057ff 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -120,6 +120,7 @@ commit_chk_wrnNNO () {
 		cp $f $fname &&
 		printf Z >>"$fname" &&
 		git -c core.autocrlf=$crlf add $fname 2>/dev/null &&
+		touch $fname && # ensure index raciness
 		git -c core.autocrlf=$crlf commit -m "commit_$fname" $fname >"${pfx}_$f.err" 2>&1
 	done
 

I'm not sure if there is a more elegant solution, though (for instance,
why not collect the output from "git add", which should have the same
warning, I would think).

-Peff

Re: t0027 racy?

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2557 bytes --]

Hi Torsten,

On Mon, 8 Aug 2016, Torsten Bögershausen wrote:

> On 2016-08-08 17.05, Johannes Schindelin wrote:
> > 
> > I remember that you did a ton of work on t0027. Now I see problems,
> > and not only that the entire script now takes a whopping 4 minutes 20
> > seconds to run on my high-end Windows machine.
> > 
> > It appears that t0027 fails randomly for me, in seemingly random
> > places.  Sometimes all 1388 cases pass. Sometimes "29 - commit NNO
> > files crlf=true attr=auto LF" fails. Sometimes it is "24 - commit NNO
> > files crlf=false attr=auto LF". Sometimes it is "114 - commit NNO
> > files crlf=false attr=auto LF", and sometimes "111 - commit NNO files
> > attr=auto aeol=lf crlf=false CRLF_mix_LF".
> > 
> > When I run it with -i -v -x --tee, it passes every single time (taking
> > over 5 minutes, just to make things worse)...
> > 
> > Any idea about any possible races?
>
> Just to double-check: I assume that you have this
> commit ded2444ad8ab8128cae2b91b8efa57ea2dd8c7a5
> Author: Torsten Bögershausen <tboegi@web.de>
> Date:   Mon Apr 25 18:56:27 2016 +0200
> 
>     t0027: make commit_chk_wrnNNO() reliable
> in your tree ?

I tested this with multiple branches, but yes, the one I tested most was
the shears/pu branch of git-for-windows/git (which has all
Windows-specific patches of our master branch rebased on top of pu). I
also tested with the pu branch as of yesterday.

> Is there a special pattern ?

No. Just "make -j15 DEVELOPER=1 -k test".

> Did you
> a) Update the machine ?

Yep, it's up-to-date. Windows 10 Anniversary Update.

> b) Update Git code base ?

As I said, several.

> Is it only the NNO tests that fail ?

As I said, the failures are random, I just picked the 4 most recent ones.

> Did they ever pass ?

As I said, if I run with -i -v -x --tee, everything passes.

> I see only "commit NNO files...." in you report, they belong to
> check_warning(), which is called around line 126 in t0027.

I believe this is true. Some race, probably, leading to the commit *not*
refreshing the files. Or some such, this is just a guess on my side.

> How reproducible is the problem ?

Not very. That is, about half of the time t0027 passes even *without* -i
-v -x --tee. And when it fails, it is anybody's guess which case fails.

> If you add
> exit 0
> After the last "commit_chk_wrnNNO" line (line 418),
> does
> ls -l crlf*.err
> give you any hint ?

No. It simply does not contain that warning that is expected.

Ciao,
Dscho

Re: t0027 racy?

[Johannes Schindelin]

Hi Peff,

On Tue, 9 Aug 2016, Jeff King wrote:

> And indeed, this seems to fix it for me (at least it has been running in
> a 16-way loop for 5 minutes, whereas before it died after 30 seconds or
> so):
> 
> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
> index 2860d2d..9f057ff 100755
> --- a/t/t0027-auto-crlf.sh
> +++ b/t/t0027-auto-crlf.sh
> @@ -120,6 +120,7 @@ commit_chk_wrnNNO () {
>  		cp $f $fname &&
>  		printf Z >>"$fname" &&
>  		git -c core.autocrlf=$crlf add $fname 2>/dev/null &&
> +		touch $fname && # ensure index raciness
>  		git -c core.autocrlf=$crlf commit -m "commit_$fname" $fname >"${pfx}_$f.err" 2>&1
>  	done

I will test this patch as soon as my poor machine has less work.

> I'm not sure if there is a more elegant solution, though (for instance,
> why not collect the output from "git add", which should have the same
> warning, I would think).

That should work, too. I am a bit overwhelmed with t0027, though, I do not
really understand what is going on. It tests *so many* things, and in no
test case is it clear to me what it tests and what it expects let alone
why those expectations make sense.

Ciao,
Dscho

Re: t0027 racy?

[Johannes Schindelin]

Hi Peff,

On Mon, 8 Aug 2016, Jeff King wrote:

> I got failure within about 30 seconds on t0027 (though 5 minutes? Yeesh.
> It runs in 9s on my laptop. I weep for you).

Yep. That is the price I (and all other Git for Windows developers) pay
for the decision to implement Git's entire test suite in Shell script,
including expensive tests with over a 1,000 test cases such as t0027.

Ciao,
Dscho

Re: t0027 racy?

[Torsten Bögershausen]

On Tue, Aug 09, 2016 at 02:51:10AM -0400, Jeff King wrote:
> On Mon, Aug 08, 2016 at 08:32:24PM +0000, Torsten Bögershausen wrote:
> 
> > > The verbose output is not very exciting, though:
> > > 
> > > 	expecting success: 
> > > 	                check_warning "$lfwarn" ${pfx}_LF.err
> > > 
> > > 	--- NNO_attr_auto_aeol_crlf_false_LF.err.expect 2016-08-08 15:26:37.061701392 +0000
> > > 	+++ NNO_attr_auto_aeol_crlf_false_LF.err.actual 2016-08-08 15:26:37.061701392 +0000
> > > 	@@ -1 +0,0 @@
> > > 	-warning: LF will be replaced by CRLF
> > > 	not ok 114 - commit NNO files crlf=false attr=auto LF
> > [...]
> > The warning is missing, but should be there:
> > 
> > The file has LF, and after commit and a new checkout these LF will
> > be convertet into CRLF.
> > 
> > So why isn't the warning there (but here on my oldish machines)
> 
> To be clear, the warning _is_ there when I just run t0027 by itself, and
> the test passes.  It's only under heavy load that it isn't. So it's a
> race condition either in the test script or in git itself.
> 
> Usually race conditions like these are due to one of:
> 
>   - git dying from SIGPIPE before it has a chance to output the command.
>     But I don't see any pipes being used in the test script.
> 
>   - index raciness causing us to avoid reading file content. For
>     example, if you do:
> 
>       echo foo >bar
>       git add bar
> 
>     Then _usually_ "bar" and the index will have the same mtime. And
>     therefore subsequent commands that need to refresh the index will
>     re-read the content of "bar", because they cannot tell from the stat
>     information if we have the latest version of "bar" in the index or
>     not (it could have been written after the index update, but in the
>     same second).
> 
>     But on a slow or heavily loaded system (or if you simply get unlucky
>     in crossing the boundary to a new second), they'll have different
>     mtimes. And therefore git knows it can skip reading the content from
>     the filesystem.
> 
>     So if your test relies on git actually re-converting the file
>     content, it would sometimes randomly fail.
> 
> The second one seems plausible, given the history of issues with
> changing CRLF settings for an existing checkout. I'm not sure if it
> would be feasible to reset the index completely before each tested
> command, but that would probably solve it.
The content of the file has been changed (we appended the letter 'Z' to it,
so even if mtime is the same, st.st_size should differ.
And it seems as if the commit is triggered, see below.
> 

(I got the stress script working; no I can reproduce it nicely)

Is it possible to vote for a 3rd option ?
I added some more warnings, to have always some output in stderr. 

This is the good case, this test case passed:
cat NNO_attr_auto_aeol_crlf_true_LF.err 

warning: check_safe_crlf in NNO_attr_auto_aeol_crlf_true_LF.txt 2
warning: LF was here in NNO_attr_auto_aeol_crlf_true_LF.txt..
warning: LF will be replaced by CRLF in NNO_attr_auto_aeol_crlf_true_LF.txt.
The file will have its original line endings in your working directory.
warning: check_safe_crlf in NNO_attr__aeol_crlf_true_CRLF.txt 0
warning: check_safe_crlf in NNO_attr__aeol_crlf_true_CRLF_mix_LF.txt 0
warning: check_safe_crlf in NNO_attr__aeol_crlf_true_LF.txt 0
warning: check_safe_crlf in NNO_attr__aeol_lf_true_CRLF.txt 0
warning: check_safe_crlf in NNO_attr__aeol_lf_true_CRLF_mix_LF.txt 0
warning: check_safe_crlf in NNO_attr__aeol_lf_true_LF.txt 0
warning: check_safe_crlf in NNO_attr_auto_aeol_lf_true_LF.txt 0
[master 2decee0] commit_NNO_attr_auto_aeol_crlf_true_LF.txt
 Author: A U Thor <author@example.com>
 1 file changed, 2 insertions(+), 2 deletions(-)
---------------------------
And this one failed, the same code base, but a different file:
cat commit_NNO_attr_auto_aeol_crlf_input_LF.

[master ce2045a] commit_NNO_attr_auto_aeol_crlf_input_LF.txt
 Author: A U Thor <author@example.com>
 1 file changed, 2 insertions(+), 2 deletions(-)

---------------------------
Both had been generated with a patched convert.c:
git diff convert.c
index 67d69b5..ae64a58 100644
--- a/convert.c
+++ b/convert.c
@@ -191,6 +191,7 @@ static enum eol output_eol(enum crlf_action crlf_action)
 static void check_safe_crlf(const char *path, enum crlf_action crlf_action,
                             struct text_stat *stats, enum safe_crlf checksafe)
 {
+       warning("check_safe_crlf in %s %d", path, (int)checksafe);
        if (!checksafe)
                return;
 
@@ -210,6 +211,7 @@ static void check_safe_crlf(const char *path, enum crlf_action crlf_action,
                 * CRLFs would be added by checkout:
                 * check if we have "naked" LFs
                 */
+               warning("LF was here in %s..", path);
                if (stats->lonelf) {
                        if (checksafe == SAFE_CRLF_WARN)
                                warning("LF will be replaced by CRLF in %s.\nThe

---------------------

In the failing case, there is no warning at all printed,
even if I would have expected the list of files to be printed.

The commit has been done, I think.

See t0027, around line 124:
		cp $f $fname &&
		printf Z >>"$fname" &&
		git -c core.autocrlf=$crlf add $fname 2>/dev/null &&
		git -c core.autocrlf=$crlf commit -m "commit_$fname" $fname >"${pfx}_$f.err" 2>&1

Both stdout and stderr are piped into the .err file.
The content of stdout seems to be there always.
The content of stderr seems to be there most of the time.
When the test case fails, the content of stderr seems to be completely missing.

Reverting  f4c3edc0b156362a92bf9de4f0ec794e90a757f does not fix the problem
Reverting f4c3edc0b15 and 3b331e9267146961466 does not fix the problem.

Re: t0027 racy?

[Jeff King]

On Tue, Aug 09, 2016 at 01:33:04PM +0200, Johannes Schindelin wrote:

> On Mon, 8 Aug 2016, Jeff King wrote:
> 
> > I got failure within about 30 seconds on t0027 (though 5 minutes? Yeesh.
> > It runs in 9s on my laptop. I weep for you).
> 
> Yep. That is the price I (and all other Git for Windows developers) pay
> for the decision to implement Git's entire test suite in Shell script,
> including expensive tests with over a 1,000 test cases such as t0027.

To be fair, it does not run unless you set GIT_TEST_LONG. Even _I_ don't
do that by default.

-Peff

Re: t0027 racy?

[Jeff King]

On Tue, Aug 09, 2016 at 11:33:37AM +0000, Torsten Bögershausen wrote:

> > The second one seems plausible, given the history of issues with
> > changing CRLF settings for an existing checkout. I'm not sure if it
> > would be feasible to reset the index completely before each tested
> > command, but that would probably solve it.
> The content of the file has been changed (we appended the letter 'Z' to it,
> so even if mtime is the same, st.st_size should differ.
> And it seems as if the commit is triggered, see below.

I don't think I made myself clear. It's not a question of whether there
is something to commit. It's that when git asks the index "what is the
sha1 of the content at this path?", the index may be able to answer
directly (the file is up-to-date, so we return the index value), or it
may have to go to the filesystem and read the file content. It is this
latter which triggers convert_to_git(), which is what generates the
message in question.

For a more stripped-down example, try:

  git add foo
  git commit -m msg

versus:

  git add foo
  sleep 1
  git commit -m msg

In the latter case, we should not generally need convert_to_git() in the
"commit" step. It was already done by "git add", and we reuse the cached
result.

Whereas in the first one, we may run into the racy-index problem and
have to re-read the file to be on the safe side.

-Peff

Re: t0027 racy?

[Torsten Bögershausen]

On Tue, Aug 09, 2016 at 07:49:38AM -0400, Jeff King wrote:
> On Tue, Aug 09, 2016 at 11:33:37AM +0000, Torsten Bögershausen wrote:
> 
> > > The second one seems plausible, given the history of issues with
> > > changing CRLF settings for an existing checkout. I'm not sure if it
> > > would be feasible to reset the index completely before each tested
> > > command, but that would probably solve it.
> > The content of the file has been changed (we appended the letter 'Z' to it,
> > so even if mtime is the same, st.st_size should differ.
> > And it seems as if the commit is triggered, see below.
> 
> I don't think I made myself clear. It's not a question of whether there
> is something to commit. It's that when git asks the index "what is the
> sha1 of the content at this path?", the index may be able to answer
> directly (the file is up-to-date, so we return the index value), or it
> may have to go to the filesystem and read the file content. It is this
> latter which triggers convert_to_git(), which is what generates the
> message in question.
> 
> For a more stripped-down example, try:
> 
>   git add foo
>   git commit -m msg
> 
> versus:
> 
>   git add foo
>   sleep 1
>   git commit -m msg
> 
> In the latter case, we should not generally need convert_to_git() in the
> "commit" step. It was already done by "git add", and we reuse the cached
> result.
> 
> Whereas in the first one, we may run into the racy-index problem and
> have to re-read the file to be on the safe side.
> 
> -Peff

Thanks for the explanation, so there are 2 chances for a race.

I assume that the suggested "touch" will fix race#2 in most cases.
In my understanding, the change of the file size will be more reliable:
 

diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index 2860d2d..9933a9b 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -120,6 +120,7 @@ commit_chk_wrnNNO () {
                cp $f $fname &&
                printf Z >>"$fname" &&
                git -c core.autocrlf=$crlf add $fname 2>/dev/null &&
+               printf Z >>"$fname" &&
                git -c core.autocrlf=$crlf commit -m "commit_$fname" $fname >"${pfx}_$f.err" 2>&1
        done
-------------------
Does anybody agree ?
And, by the way, the convert warning may be issued twice, once in
"git add" and once in "git commit".

Re: t0027 racy?

[Jeff King]

On Tue, Aug 09, 2016 at 12:59:58PM +0000, Torsten Bögershausen wrote:

> Thanks for the explanation, so there are 2 chances for a race.
> 
> I assume that the suggested "touch" will fix race#2 in most cases.
> In my understanding, the change of the file size will be more reliable:
>  
> 
> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
> index 2860d2d..9933a9b 100755
> --- a/t/t0027-auto-crlf.sh
> +++ b/t/t0027-auto-crlf.sh
> @@ -120,6 +120,7 @@ commit_chk_wrnNNO () {
>                 cp $f $fname &&
>                 printf Z >>"$fname" &&
>                 git -c core.autocrlf=$crlf add $fname 2>/dev/null &&
> +               printf Z >>"$fname" &&
>                 git -c core.autocrlf=$crlf commit -m "commit_$fname" $fname >"${pfx}_$f.err" 2>&1
>         done
> -------------------
> Does anybody agree ?

I think the mtime change is reliable. We know that the mtime on the file
will be greater than or equal to the index mtime (because it happened
afterwards), so git will always look at the on-disk contents.

With your change, "git commit" will also always re-read the file from
disk, because it actually has new content (and you provide the filename
on the command line, so it is stage-and-commit, not just
"commit-the-index").

So either is fine.

> And, by the way, the convert warning may be issued twice, once in
> "git add" and once in "git commit".

Yes, but you only save it from "git commit", so we can ignore what
happens from "add" here. But that's why I wondered if:

  git -c core.autocrlf=$crlf add $fname >"${pfx}_$f.err" 2>&1

would make more sense. We _know_ that we have to do convert_to_git() in
that step because the content is changed. And then you can ignore the
warnings from "git commit" (which are racy), or you can simply commit as
a whole later, as some other loops do.

But like Dscho, I do not actually understand what this test is checking.
The function is called commit_chk_wrnNNO(), so perhaps you really are
interested in what "commit" has to say. But IMHO that is not an
interesting test. We know that if it has to read the content from disk,
it will call convert_to_git(), which is the exact same code path used by
"git add". So I do not understand what it is accomplishing to make a
commit at all here.

-Peff

Re: t0027 racy?

[Torsten Bögershausen]

>   git -c core.autocrlf=$crlf add $fname >"${pfx}_$f.err" 2>&1
> 
> would make more sense. We _know_ that we have to do convert_to_git() in
> that step because the content is changed. And then you can ignore the
> warnings from "git commit" (which are racy), or you can simply commit as
> a whole later, as some other loops do.
> 
> But like Dscho, I do not actually understand what this test is checking.
> The function is called commit_chk_wrnNNO(), so perhaps you really are
> interested in what "commit" has to say. But IMHO that is not an
> interesting test. We know that if it has to read the content from disk,
> it will call convert_to_git(), which is the exact same code path used by
> "git add". So I do not understand what it is accomplishing to make a
> commit at all here.

It seems as if the test has been written without understanding the raciness.
It should commit files with different line endings on top of
a file with mixed line endings.
The warning should be checked (and here "git add" can be used,
or the file can be commited directly).
I'm not sure why the test ended up in doing both.

However, doing it the right way triggers a bug in convert.c,
(some warnings are missing, so I need some days to come up
with a proper patch)

Thanks for reporting & digging.

Re: t0027 racy?

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2281 bytes --]

Hi Torsten,

On Tue, 9 Aug 2016, Torsten Bögershausen wrote:

> >   git -c core.autocrlf=$crlf add $fname >"${pfx}_$f.err" 2>&1
> > 
> > would make more sense. We _know_ that we have to do convert_to_git() in
> > that step because the content is changed. And then you can ignore the
> > warnings from "git commit" (which are racy), or you can simply commit as
> > a whole later, as some other loops do.
> > 
> > But like Dscho, I do not actually understand what this test is checking.
> > The function is called commit_chk_wrnNNO(), so perhaps you really are
> > interested in what "commit" has to say. But IMHO that is not an
> > interesting test. We know that if it has to read the content from disk,
> > it will call convert_to_git(), which is the exact same code path used by
> > "git add". So I do not understand what it is accomplishing to make a
> > commit at all here.
> 
> It seems as if the test has been written without understanding the raciness.
> It should commit files with different line endings on top of
> a file with mixed line endings.
> The warning should be checked (and here "git add" can be used,
> or the file can be commited directly).
> I'm not sure why the test ended up in doing both.
> 
> However, doing it the right way triggers a bug in convert.c,
> (some warnings are missing, so I need some days to come up
> with a proper patch)

FWIW I would strongly prefer to use the warning of `git add` and not even
bother with `git commit`. What we are interested in is the warning
message, generated by convert_to_git(). Not using the first one and
triggering a second one merely adds unnecessary churn that increases the
CO2 budget of running the test.

On that matter, I wonder whether there would be a chance to revamp t0027
in a major way, with the following goals:

- to make it very obvious to the casual reader what is being tested

- to combine Git invocations when possible, e.g. running one big `git add`
  on a couple of files and then verify the relevant parts of the output

- dramatically decreasing the time required to run the test, without
  sacrificing correctness (I would wager a bet that not only a few of
  those 1388 test cases essentially exercise identical code paths)

Ciao,
Dscho

Re: t0027 racy?

[Torsten Bögershausen]

[]
> FWIW I would strongly prefer to use the warning of `git add` and not even
> bother with `git commit`. What we are interested in is the warning
> message, generated by convert_to_git(). 
The commit is needed, because we check the content of the commit later.
> Not using the first one and
[]
> 
> On that matter, I wonder whether there would be a chance to revamp t0027
> in a major way, with the following goals:
> 
> - to make it very obvious to the casual reader what is being tested
> 
> - to combine Git invocations when possible, e.g. running one big `git add`
>   on a couple of files and then verify the relevant parts of the output
> 
> - dramatically decreasing the time required to run the test, without
>   sacrificing correctness (I would wager a bet that not only a few of
>   those 1388 test cases essentially exercise identical code paths)
Good ideas, I will work on a series that fixes bugs first, and then we
can see if there is room for optimization.

What do you think about this as a starting point,
more things will follow.
I like to here comments about the commit msg first ;-)

commit 3754404d3d1ea4a0cbbed4986cc4ac1b5fe6b66e
Author: Torsten Bögershausen <tboegi@web.de>
Date:   Thu Aug 11 18:47:29 2016 +0200

    t0027: Correct raciness in NNO test
    
    When a non-reversible CRLF conversion is done in "git add",
    a warning is printed on stderr.
    
    The commit_chk_wrnNNO() function  in t0027 was written to test this,
    but did the wrong thing: Instead of looking at the warning
    from "git add", it looked at the warning from "git commit".
    
    Correct this and replace the commit for each and every file with a commit
    of all files in one go.
    
    The function commit_chk_wrnNNO() will to be renamed in a separate commit.
    Thanks to Jeff King <peff@peff.net> for analizing t0027.
    Reporyed-By: Johannes Schindelin <Johannes.Schindelin@gmx.de>

diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index 2860d2d..6e44382 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -119,8 +119,7 @@ commit_chk_wrnNNO () {
 		fname=${pfx}_$f.txt &&
 		cp $f $fname &&
 		printf Z >>"$fname" &&
-		git -c core.autocrlf=$crlf add $fname 2>/dev/null &&
-		git -c core.autocrlf=$crlf commit -m "commit_$fname" $fname >"${pfx}_$f.err" 2>&1
+		git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err"
 	done
 
 	test_expect_success "commit NNO files crlf=$crlf attr=$attr LF" '
@@ -394,11 +393,10 @@ test_expect_success 'commit files attr=crlf' '
 
 #                 attr                    LF        CRLF      CRLFmixLF   LF_mix_CR   CRLFNUL
 commit_chk_wrnNNO ""      ""      false   ""        ""        ""          ""          ""
-commit_chk_wrnNNO ""      ""      true    LF_CRLF   ""        ""          ""          ""
+commit_chk_wrnNNO ""      ""      true    ""        ""        ""          ""          ""
 commit_chk_wrnNNO ""      ""      input   ""        ""        ""          ""          ""
-
-commit_chk_wrnNNO "auto"  ""      false   "$WILC"   ""        ""          ""          ""
-commit_chk_wrnNNO "auto"  ""      true    LF_CRLF   ""        ""          ""          ""
+commit_chk_wrnNNO "auto"  ""      false   ""        ""        ""          ""          ""
+commit_chk_wrnNNO "auto"  ""      true    ""        ""        ""          ""          ""
 commit_chk_wrnNNO "auto"  ""      input   ""        ""        ""          ""          ""
 for crlf in true false input
 do
@@ -408,7 +406,7 @@ do
 	commit_chk_wrnNNO ""    lf      $crlf   ""       CRLF_LF    CRLF_LF      ""         CRLF_LF
 	commit_chk_wrnNNO ""    crlf    $crlf   LF_CRLF   ""        LF_CRLF     LF_CRLF     ""
 	commit_chk_wrnNNO auto  lf    	$crlf   ""        ""        ""          ""          ""
-	commit_chk_wrnNNO auto  crlf  	$crlf   LF_CRLF   ""        ""          ""          ""
+	commit_chk_wrnNNO auto  crlf  	$crlf   ""        ""        ""          ""          ""
 	commit_chk_wrnNNO text  lf    	$crlf   ""       CRLF_LF    CRLF_LF     ""          CRLF_LF
 	commit_chk_wrnNNO text  crlf  	$crlf   LF_CRLF   ""        LF_CRLF     LF_CRLF     ""
 done
@@ -417,7 +415,8 @@ commit_chk_wrnNNO "text"  ""      false   "$WILC"   "$WICL"   "$WAMIX"    "$WILC
 commit_chk_wrnNNO "text"  ""      true    LF_CRLF   ""        LF_CRLF     LF_CRLF     ""
 commit_chk_wrnNNO "text"  ""      input   ""        CRLF_LF   CRLF_LF     ""          CRLF_LF
 
-test_expect_success 'create files cleanup' '
+test_expect_success 'commit NNO and cleanup' '
+	git commit -m "commit files on top of NNO" &&
 	rm -f *.txt &&
 	git -c core.autocrlf=false reset --hard
 '

Re: t0027 racy?

[Junio C Hamano]

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

> Good ideas, I will work on a series that fixes bugs first, and then we
> can see if there is room for optimization.
>
> What do you think about this as a starting point, more things will
> follow.
> I like to here comments about the commit msg first ;-)

Throughout t0027 there is no mention on what NNO stands for.  Are
they about operations that result in un-normalized index entries?

> commit 3754404d3d1ea4a0cbbed4986cc4ac1b5fe6b66e
> Author: Torsten Bögershausen <tboegi@web.de>
> Date:   Thu Aug 11 18:47:29 2016 +0200
>
>     t0027: Correct raciness in NNO test
>     
>     When a non-reversible CRLF conversion is done in "git add",
>     a warning is printed on stderr.
>     
>     The commit_chk_wrnNNO() function  in t0027 was written to test this,
>     but did the wrong thing: Instead of looking at the warning
>     from "git add", it looked at the warning from "git commit".
>     
>     Correct this and replace the commit for each and every file with a commit
>     of all files in one go.
>     
>     The function commit_chk_wrnNNO() will to be renamed in a separate commit.
>     Thanks to Jeff King <peff@peff.net> for analizing t0027.
>     Reporyed-By: Johannes Schindelin <Johannes.Schindelin@gmx.de>

Reporyed?  

An obligatory "comments-about-the-commit-msg";-)

Re: t0027 racy?

[Jeff King]

On Thu, Aug 11, 2016 at 06:58:12PM +0000, Torsten Bögershausen wrote:

> commit 3754404d3d1ea4a0cbbed4986cc4ac1b5fe6b66e
> Author: Torsten Bögershausen <tboegi@web.de>
> Date:   Thu Aug 11 18:47:29 2016 +0200
> 
>     t0027: Correct raciness in NNO test
>     
>     When a non-reversible CRLF conversion is done in "git add",
>     a warning is printed on stderr.
>     
>     The commit_chk_wrnNNO() function  in t0027 was written to test this,
>     but did the wrong thing: Instead of looking at the warning
>     from "git add", it looked at the warning from "git commit".

Maybe add:

  This is racy because "git commit" may not have to do CRLF conversion
  at all if it can use the sha1 value from the index (which depends on
  whether "add" and "commit" run in a single second).

>     Thanks to Jeff King <peff@peff.net> for analizing t0027.
>     Reporyed-By: Johannes Schindelin <Johannes.Schindelin@gmx.de>

s/analizing/analyzing/; s/Reporyed/Reported/

> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
> index 2860d2d..6e44382 100755
> --- a/t/t0027-auto-crlf.sh
> +++ b/t/t0027-auto-crlf.sh
> @@ -119,8 +119,7 @@ commit_chk_wrnNNO () {
>  		fname=${pfx}_$f.txt &&
>  		cp $f $fname &&
>  		printf Z >>"$fname" &&
> -		git -c core.autocrlf=$crlf add $fname 2>/dev/null &&
> -		git -c core.autocrlf=$crlf commit -m "commit_$fname" $fname >"${pfx}_$f.err" 2>&1
> +		git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err"
>  	done

So this is the meat of the fix.

> @@ -394,11 +393,10 @@ test_expect_success 'commit files attr=crlf' '
>  
>  #                 attr                    LF        CRLF      CRLFmixLF   LF_mix_CR   CRLFNUL
>  commit_chk_wrnNNO ""      ""      false   ""        ""        ""          ""          ""
> -commit_chk_wrnNNO ""      ""      true    LF_CRLF   ""        ""          ""          ""
> +commit_chk_wrnNNO ""      ""      true    ""        ""        ""          ""          ""
>  commit_chk_wrnNNO ""      ""      input   ""        ""        ""          ""          ""
> -
> -commit_chk_wrnNNO "auto"  ""      false   "$WILC"   ""        ""          ""          ""
> -commit_chk_wrnNNO "auto"  ""      true    LF_CRLF   ""        ""          ""          ""
> +commit_chk_wrnNNO "auto"  ""      false   ""        ""        ""          ""          ""
> +commit_chk_wrnNNO "auto"  ""      true    ""        ""        ""          ""          ""
>  commit_chk_wrnNNO "auto"  ""      input   ""        ""        ""          ""          ""

I am not sure I understand why this reordering is necessary, but I guess
it's to group like things together in a single commit? Might be worth a
mention in the commit message.

-Peff

[PATCH v1 0/2] Fix conversion warnings

[tboegi]

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

conversion warnings in convert.c and t0027
- Fix a raciness in t0027
- Fix an older problem with the conversion warnings when a file
  has CRLF in the index, and is now commited with LF.
- Do not yet rename NNO to something different.
  NNO means Not NOrmalized in the index.
  The files are changed and content with different line endings
  are added on top of the existing version.
  A replacement for commit_chk_wrnNNO will be send after these bugfixes.
  The term commit_chk_wrnNNO could be changed to
  NNO_in_repo_add_file_check_warning.
  Would  NNO_addX__chk_wrn be more readable ?
  Or CRLFindex_add_wrn ?

Changes against v0:
- Added raciness comment from Peff, and removed the removal of a line.
Torsten Bögershausen (2):
  t0027: Correct raciness in NNO test
  convert: missing `LF will be replaced by CRLF

 convert.c            | 97 ++++++++++++++++++++++++++++++----------------------
 t/t0027-auto-crlf.sh | 10 +++---
 2 files changed, 62 insertions(+), 45 deletions(-)

-- 
2.0.0.rc1.6318.g0c2c796

[PATCH v1 1/2] t0027: Correct raciness in NNO test

[tboegi]

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

When a non-reversible CRLF conversion is done in "git add",
a warning is printed on stderr (or Git dies, depending on checksafe)

The function commit_chk_wrnNNO() in t0027 was written to test this,
but did the wrong thing: Instead of looking at the warning
from "git add", it looked at the warning from "git commit".

This is racy because "git commit" may not have to do CRLF conversion
at all if it can use the sha1 value from the index (which depends on
whether "add" and "commit" run in a single second).

Correct this and replace the commit for each and every file with a commit
of all files in one go.

The function commit_chk_wrnNNO() will to be renamed in a separate commit.
Thanks to Jeff King <peff@peff.net> for analyzing t0027.

Reported-By: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
 t/t0027-auto-crlf.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index 2860d2d..ab6e962 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -119,8 +119,7 @@ commit_chk_wrnNNO () {
 		fname=${pfx}_$f.txt &&
 		cp $f $fname &&
 		printf Z >>"$fname" &&
-		git -c core.autocrlf=$crlf add $fname 2>/dev/null &&
-		git -c core.autocrlf=$crlf commit -m "commit_$fname" $fname >"${pfx}_$f.err" 2>&1
+		git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err"
 	done
 
 	test_expect_success "commit NNO files crlf=$crlf attr=$attr LF" '
@@ -394,11 +393,11 @@ test_expect_success 'commit files attr=crlf' '
 
 #                 attr                    LF        CRLF      CRLFmixLF   LF_mix_CR   CRLFNUL
 commit_chk_wrnNNO ""      ""      false   ""        ""        ""          ""          ""
-commit_chk_wrnNNO ""      ""      true    LF_CRLF   ""        ""          ""          ""
+commit_chk_wrnNNO ""      ""      true    ""        ""        ""          ""          ""
 commit_chk_wrnNNO ""      ""      input   ""        ""        ""          ""          ""
 
-commit_chk_wrnNNO "auto"  ""      false   "$WILC"   ""        ""          ""          ""
-commit_chk_wrnNNO "auto"  ""      true    LF_CRLF   ""        ""          ""          ""
+commit_chk_wrnNNO "auto"  ""      false   ""        ""        ""          ""          ""
+commit_chk_wrnNNO "auto"  ""      true    ""        ""        ""          ""          ""
 commit_chk_wrnNNO "auto"  ""      input   ""        ""        ""          ""          ""
 for crlf in true false input
 do
@@ -408,7 +407,7 @@ do
 	commit_chk_wrnNNO ""    lf      $crlf   ""       CRLF_LF    CRLF_LF      ""         CRLF_LF
 	commit_chk_wrnNNO ""    crlf    $crlf   LF_CRLF   ""        LF_CRLF     LF_CRLF     ""
 	commit_chk_wrnNNO auto  lf    	$crlf   ""        ""        ""          ""          ""
-	commit_chk_wrnNNO auto  crlf  	$crlf   LF_CRLF   ""        ""          ""          ""
+	commit_chk_wrnNNO auto  crlf  	$crlf   ""        ""        ""          ""          ""
 	commit_chk_wrnNNO text  lf    	$crlf   ""       CRLF_LF    CRLF_LF     ""          CRLF_LF
 	commit_chk_wrnNNO text  crlf  	$crlf   LF_CRLF   ""        LF_CRLF     LF_CRLF     ""
 done
@@ -417,7 +416,8 @@ commit_chk_wrnNNO "text"  ""      false   "$WILC"   "$WICL"   "$WAMIX"    "$WILC
 commit_chk_wrnNNO "text"  ""      true    LF_CRLF   ""        LF_CRLF     LF_CRLF     ""
 commit_chk_wrnNNO "text"  ""      input   ""        CRLF_LF   CRLF_LF     ""          CRLF_LF
 
-test_expect_success 'create files cleanup' '
+test_expect_success 'commit NNO and cleanup' '
+	git commit -m "commit files on top of NNO" &&
 	rm -f *.txt &&
 	git -c core.autocrlf=false reset --hard
 '
-- 
2.0.0.rc1.6318.g0c2c796

[PATCH v1 2/2] convert: missing `LF will be replaced by CRLF

[tboegi]

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

When the "new safer autocrlf-handling" was introduced in c4805393, the logic
around check_safe_crlf() should have been changed:
Once a file has been commited with CRLF (and core.autocrlf is true),
line endings are no longer converted by "git add" or "git checkout".
The line endings will not normalized by Git.
The user may run e.g. dos2unix and commit the file.

Factor out will_convert_lf_to_crlf() which returns when LF are converted
into CRLF at checkout.

Update the logic around check_safe_crlf() and do the possible CRLF-LF
conversion in "git add".
Simulate the checkout (and a possible LF-CRLF conversion) with help of
will_convert_lf_to_crlf().
---
 convert.c            | 97 ++++++++++++++++++++++++++++++----------------------
 t/t0027-auto-crlf.sh |  6 ++--
 2 files changed, 60 insertions(+), 43 deletions(-)

diff --git a/convert.c b/convert.c
index 67d69b5..077f5e6 100644
--- a/convert.c
+++ b/convert.c
@@ -189,33 +189,25 @@ static enum eol output_eol(enum crlf_action crlf_action)
 }
 
 static void check_safe_crlf(const char *path, enum crlf_action crlf_action,
-                            struct text_stat *stats, enum safe_crlf checksafe)
+			    struct text_stat *old_stats, struct text_stat *new_stats,
+			    enum safe_crlf checksafe)
 {
-	if (!checksafe)
-		return;
-
-	if (output_eol(crlf_action) == EOL_LF) {
+	if (old_stats->crlf && !new_stats->crlf ) {
 		/*
-		 * CRLFs would not be restored by checkout:
-		 * check if we'd remove CRLFs
+		 * CRLFs would not be restored by checkout
 		 */
-		if (stats->crlf) {
-			if (checksafe == SAFE_CRLF_WARN)
-				warning("CRLF will be replaced by LF in %s.\nThe file will have its original line endings in your working directory.", path);
-			else /* i.e. SAFE_CRLF_FAIL */
-				die("CRLF would be replaced by LF in %s.", path);
-		}
-	} else if (output_eol(crlf_action) == EOL_CRLF) {
+		if (checksafe == SAFE_CRLF_WARN)
+			warning("CRLF will be replaced by LF in %s.\nThe file will have its original line endings in your working directory.", path);
+		else /* i.e. SAFE_CRLF_FAIL */
+			die("CRLF would be replaced by LF in %s.", path);
+	} else if (old_stats->lonelf && !new_stats->lonelf ) {
 		/*
-		 * CRLFs would be added by checkout:
-		 * check if we have "naked" LFs
+		 * CRLFs would be added by checkout
 		 */
-		if (stats->lonelf) {
-			if (checksafe == SAFE_CRLF_WARN)
-				warning("LF will be replaced by CRLF in %s.\nThe file will have its original line endings in your working directory.", path);
-			else /* i.e. SAFE_CRLF_FAIL */
-				die("LF would be replaced by CRLF in %s", path);
-		}
+		if (checksafe == SAFE_CRLF_WARN)
+			warning("LF will be replaced by CRLF in %s.\nThe file will have its original line endings in your working directory.", path);
+		else /* i.e. SAFE_CRLF_FAIL */
+			die("LF would be replaced by CRLF in %s", path);
 	}
 }
 
@@ -233,12 +225,35 @@ static int has_cr_in_index(const char *path)
 	return has_cr;
 }
 
+static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,
+				   enum crlf_action crlf_action)
+{
+	if (output_eol(crlf_action) != EOL_CRLF)
+		return 0;
+	/* No "naked" LF? Nothing to convert, regardless. */
+	if (!stats->lonelf)
+		return 0;
+
+	if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || crlf_action == CRLF_AUTO_CRLF) {
+		/* If we have any CR or CRLF line endings, we do not touch it */
+		/* This is the new safer autocrlf-handling */
+		if (stats->lonecr || stats->crlf)
+			return 0;
+
+		if (convert_is_binary(len, stats))
+			return 0;
+	}
+	return 1;
+
+}
+
 static int crlf_to_git(const char *path, const char *src, size_t len,
 		       struct strbuf *buf,
 		       enum crlf_action crlf_action, enum safe_crlf checksafe)
 {
 	struct text_stat stats;
 	char *dst;
+	int convert_crlf_into_lf;
 
 	if (crlf_action == CRLF_BINARY ||
 	    (src && !len))
@@ -252,6 +267,8 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
 		return 1;
 
 	gather_stats(src, len, &stats);
+	/* Optimization: No CRLF? Nothing to convert, regardless. */
+	convert_crlf_into_lf = !!stats.crlf;
 
 	if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || crlf_action == CRLF_AUTO_CRLF) {
 		if (convert_is_binary(len, &stats))
@@ -263,12 +280,24 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
 		if (checksafe == SAFE_CRLF_RENORMALIZE)
 			checksafe = SAFE_CRLF_FALSE;
 		else if (has_cr_in_index(path))
-			return 0;
+			convert_crlf_into_lf = 0;
 	}
-	check_safe_crlf(path, crlf_action, &stats, checksafe);
-
-	/* Optimization: No CRLF? Nothing to convert, regardless. */
-	if (!stats.crlf)
+	if (checksafe && len) {
+		struct text_stat new_stats;
+		memcpy(&new_stats, &stats, sizeof(new_stats));
+		/* simulate "git add" */
+		if (convert_crlf_into_lf) {
+			new_stats.lonelf += new_stats.crlf;
+			new_stats.crlf = 0;
+		}
+		/* simulate "git checkout" */
+		if (will_convert_lf_to_crlf(len, &new_stats, crlf_action)) {
+			new_stats.crlf += new_stats.lonelf;
+			new_stats.lonelf = 0;
+		}
+		check_safe_crlf(path, crlf_action, &stats, &new_stats, checksafe);
+	}
+	if (!convert_crlf_into_lf)
 		return 0;
 
 	/*
@@ -314,21 +343,9 @@ static int crlf_to_worktree(const char *path, const char *src, size_t len,
 		return 0;
 
 	gather_stats(src, len, &stats);
-
-	/* No "naked" LF? Nothing to convert, regardless. */
-	if (!stats.lonelf)
+	if (!will_convert_lf_to_crlf(len, &stats, crlf_action))
 		return 0;
 
-	if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || crlf_action == CRLF_AUTO_CRLF) {
-		/* If we have any CR or CRLF line endings, we do not touch it */
-		/* This is the new safer autocrlf-handling */
-		if (stats.lonecr || stats.crlf )
-			return 0;
-
-		if (convert_is_binary(len, &stats))
-			return 0;
-	}
-
 	/* are we "faking" in place editing ? */
 	if (src == buf->buf)
 		to_free = strbuf_detach(buf, NULL);
diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index ab6e962..7de8364 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -393,11 +393,11 @@ test_expect_success 'commit files attr=crlf' '
 
 #                 attr                    LF        CRLF      CRLFmixLF   LF_mix_CR   CRLFNUL
 commit_chk_wrnNNO ""      ""      false   ""        ""        ""          ""          ""
-commit_chk_wrnNNO ""      ""      true    ""        ""        ""          ""          ""
+commit_chk_wrnNNO ""      ""      true    LF_CRLF   ""        ""          ""          ""
 commit_chk_wrnNNO ""      ""      input   ""        ""        ""          ""          ""
 
 commit_chk_wrnNNO "auto"  ""      false   ""        ""        ""          ""          ""
-commit_chk_wrnNNO "auto"  ""      true    ""        ""        ""          ""          ""
+commit_chk_wrnNNO "auto"  ""      true    LF_CRLF   ""        ""          ""          ""
 commit_chk_wrnNNO "auto"  ""      input   ""        ""        ""          ""          ""
 for crlf in true false input
 do
@@ -407,7 +407,7 @@ do
 	commit_chk_wrnNNO ""    lf      $crlf   ""       CRLF_LF    CRLF_LF      ""         CRLF_LF
 	commit_chk_wrnNNO ""    crlf    $crlf   LF_CRLF   ""        LF_CRLF     LF_CRLF     ""
 	commit_chk_wrnNNO auto  lf    	$crlf   ""        ""        ""          ""          ""
-	commit_chk_wrnNNO auto  crlf  	$crlf   ""        ""        ""          ""          ""
+	commit_chk_wrnNNO auto  crlf  	$crlf   LF_CRLF  ""        ""          ""          ""
 	commit_chk_wrnNNO text  lf    	$crlf   ""       CRLF_LF    CRLF_LF     ""          CRLF_LF
 	commit_chk_wrnNNO text  crlf  	$crlf   LF_CRLF   ""        LF_CRLF     LF_CRLF     ""
 done
-- 
2.0.0.rc1.6318.g0c2c796

Re: [PATCH v1 2/2] convert: missing `LF will be replaced by CRLF

[Junio C Hamano]

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>
>
> When the "new safer autocrlf-handling" was introduced in c4805393, the logic
> around check_safe_crlf() should have been changed:

Should have been changed from what to what?  And instead it was
changed to which other logic?  What is the difference between the
ideal this change tries to bring in and what c4805393 did?

> Once a file has been commited with CRLF (and core.autocrlf is true),
> line endings are no longer converted by "git add" or "git checkout".
> The line endings will not normalized by Git.

s/will not/& be/;

> The user may run e.g. dos2unix and commit the file.

It is unclear what this sentence wants to say.  The user may overwrite
the file with random contents and commit it, too.  So what?

What is missing from this statement is what problem is being worked
around.  Do you mean: "Because Git does not normalize once CRLF is
in the index, if the user wants to have a normalized content in the
repository to correct that mistake, the user needs to do dos2unix to
fix it and commit the fixed result"?

> Factor out will_convert_lf_to_crlf() which returns when LF are converted
> into CRLF at checkout.
>
> Update the logic around check_safe_crlf() and do the possible CRLF-LF
> conversion in "git add".
> Simulate the checkout (and a possible LF-CRLF conversion) with help of
> will_convert_lf_to_crlf().

These two paragraphs are all "what I did to solve X", but what X is
is not very clear.

> ---

Missing sign-off.

>  convert.c            | 97 ++++++++++++++++++++++++++++++----------------------
>  t/t0027-auto-crlf.sh |  6 ++--
>  2 files changed, 60 insertions(+), 43 deletions(-)
>
> diff --git a/convert.c b/convert.c
> index 67d69b5..077f5e6 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -189,33 +189,25 @@ static enum eol output_eol(enum crlf_action crlf_action)
>  }
>  
>  static void check_safe_crlf(const char *path, enum crlf_action crlf_action,
> -                            struct text_stat *stats, enum safe_crlf checksafe)
> +			    struct text_stat *old_stats, struct text_stat *new_stats,
> +			    enum safe_crlf checksafe)
>  {
> -	if (!checksafe)
> -		return;
> -
> -	if (output_eol(crlf_action) == EOL_LF) {
> +	if (old_stats->crlf && !new_stats->crlf ) {

Space before ")"

Re: [PATCH v1 1/2] t0027: Correct raciness in NNO test

[Junio C Hamano]

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>
>
> When a non-reversible CRLF conversion is done in "git add",
> a warning is printed on stderr (or Git dies, depending on checksafe)
>
> The function commit_chk_wrnNNO() in t0027 was written to test this,
> but did the wrong thing: Instead of looking at the warning
> from "git add", it looked at the warning from "git commit".
>
> This is racy because "git commit" may not have to do CRLF conversion
> at all if it can use the sha1 value from the index (which depends on
> whether "add" and "commit" run in a single second).
>
> Correct this and replace the commit for each and every file with a commit
> of all files in one go.
>
> The function commit_chk_wrnNNO() will to be renamed in a separate commit.
> Thanks to Jeff King <peff@peff.net> for analyzing t0027.
>
> Reported-By: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> ---

Missing sign-off.  The reasoning above looks very clearly explained.

Thanks.


>  t/t0027-auto-crlf.sh | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
> index 2860d2d..ab6e962 100755
> --- a/t/t0027-auto-crlf.sh
> +++ b/t/t0027-auto-crlf.sh
> @@ -119,8 +119,7 @@ commit_chk_wrnNNO () {
>  		fname=${pfx}_$f.txt &&
>  		cp $f $fname &&
>  		printf Z >>"$fname" &&
> -		git -c core.autocrlf=$crlf add $fname 2>/dev/null &&
> -		git -c core.autocrlf=$crlf commit -m "commit_$fname" $fname >"${pfx}_$f.err" 2>&1
> +		git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err"
>  	done
>  
>  	test_expect_success "commit NNO files crlf=$crlf attr=$attr LF" '
> @@ -394,11 +393,11 @@ test_expect_success 'commit files attr=crlf' '
>  
>  #                 attr                    LF        CRLF      CRLFmixLF   LF_mix_CR   CRLFNUL
>  commit_chk_wrnNNO ""      ""      false   ""        ""        ""          ""          ""
> -commit_chk_wrnNNO ""      ""      true    LF_CRLF   ""        ""          ""          ""
> +commit_chk_wrnNNO ""      ""      true    ""        ""        ""          ""          ""
>  commit_chk_wrnNNO ""      ""      input   ""        ""        ""          ""          ""
>  
> -commit_chk_wrnNNO "auto"  ""      false   "$WILC"   ""        ""          ""          ""
> -commit_chk_wrnNNO "auto"  ""      true    LF_CRLF   ""        ""          ""          ""
> +commit_chk_wrnNNO "auto"  ""      false   ""        ""        ""          ""          ""
> +commit_chk_wrnNNO "auto"  ""      true    ""        ""        ""          ""          ""
>  commit_chk_wrnNNO "auto"  ""      input   ""        ""        ""          ""          ""
>  for crlf in true false input
>  do
> @@ -408,7 +407,7 @@ do
>  	commit_chk_wrnNNO ""    lf      $crlf   ""       CRLF_LF    CRLF_LF      ""         CRLF_LF
>  	commit_chk_wrnNNO ""    crlf    $crlf   LF_CRLF   ""        LF_CRLF     LF_CRLF     ""
>  	commit_chk_wrnNNO auto  lf    	$crlf   ""        ""        ""          ""          ""
> -	commit_chk_wrnNNO auto  crlf  	$crlf   LF_CRLF   ""        ""          ""          ""
> +	commit_chk_wrnNNO auto  crlf  	$crlf   ""        ""        ""          ""          ""
>  	commit_chk_wrnNNO text  lf    	$crlf   ""       CRLF_LF    CRLF_LF     ""          CRLF_LF
>  	commit_chk_wrnNNO text  crlf  	$crlf   LF_CRLF   ""        LF_CRLF     LF_CRLF     ""
>  done
> @@ -417,7 +416,8 @@ commit_chk_wrnNNO "text"  ""      false   "$WILC"   "$WICL"   "$WAMIX"    "$WILC
>  commit_chk_wrnNNO "text"  ""      true    LF_CRLF   ""        LF_CRLF     LF_CRLF     ""
>  commit_chk_wrnNNO "text"  ""      input   ""        CRLF_LF   CRLF_LF     ""          CRLF_LF
>  
> -test_expect_success 'create files cleanup' '
> +test_expect_success 'commit NNO and cleanup' '
> +	git commit -m "commit files on top of NNO" &&
>  	rm -f *.txt &&
>  	git -c core.autocrlf=false reset --hard
>  '

Re: [PATCH v1 1/2] t0027: Correct raciness in NNO test

[Johannes Sixt]

Am 12.08.2016 um 18:51 schrieb tboegi@web.de:
> From: Torsten Bögershausen <tboegi@web.de>
>
> When a non-reversible CRLF conversion is done in "git add",
> a warning is printed on stderr (or Git dies, depending on checksafe)
>
> The function commit_chk_wrnNNO() in t0027 was written to test this,
> but did the wrong thing: Instead of looking at the warning
> from "git add", it looked at the warning from "git commit".
>
> This is racy because "git commit" may not have to do CRLF conversion
> at all if it can use the sha1 value from the index (which depends on
> whether "add" and "commit" run in a single second).
>
> Correct this and replace the commit for each and every file with a commit
> of all files in one go.

The new test code does not only fix the race condition, but also tests 
different things, or prepares test cases in a different manner. I would 
have appreciated an explanation why this is necessary. Is it "on my 
machine, the race condition was triggered consistently for a bunch of 
tests, and so I recorded the wrong expected output in the test cases"?

>
> The function commit_chk_wrnNNO() will to be renamed in a separate commit.
> Thanks to Jeff King <peff@peff.net> for analyzing t0027.
>
> Reported-By: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> ---
>   t/t0027-auto-crlf.sh | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
> index 2860d2d..ab6e962 100755
> --- a/t/t0027-auto-crlf.sh
> +++ b/t/t0027-auto-crlf.sh
> @@ -119,8 +119,7 @@ commit_chk_wrnNNO () {
>   		fname=${pfx}_$f.txt &&
>   		cp $f $fname &&
>   		printf Z >>"$fname" &&
> -		git -c core.autocrlf=$crlf add $fname 2>/dev/null &&
> -		git -c core.autocrlf=$crlf commit -m "commit_$fname" $fname >"${pfx}_$f.err" 2>&1
> +		git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err"
>   	done
>
>   	test_expect_success "commit NNO files crlf=$crlf attr=$attr LF" '
> @@ -394,11 +393,11 @@ test_expect_success 'commit files attr=crlf' '
>
>   #                 attr                    LF        CRLF      CRLFmixLF   LF_mix_CR   CRLFNUL
>   commit_chk_wrnNNO ""      ""      false   ""        ""        ""          ""          ""
> -commit_chk_wrnNNO ""      ""      true    LF_CRLF   ""        ""          ""          ""
> +commit_chk_wrnNNO ""      ""      true    ""        ""        ""          ""          ""
>   commit_chk_wrnNNO ""      ""      input   ""        ""        ""          ""          ""
>
> -commit_chk_wrnNNO "auto"  ""      false   "$WILC"   ""        ""          ""          ""
> -commit_chk_wrnNNO "auto"  ""      true    LF_CRLF   ""        ""          ""          ""
> +commit_chk_wrnNNO "auto"  ""      false   ""        ""        ""          ""          ""
> +commit_chk_wrnNNO "auto"  ""      true    ""        ""        ""          ""          ""
>   commit_chk_wrnNNO "auto"  ""      input   ""        ""        ""          ""          ""
>   for crlf in true false input
>   do
> @@ -408,7 +407,7 @@ do
>   	commit_chk_wrnNNO ""    lf      $crlf   ""       CRLF_LF    CRLF_LF      ""         CRLF_LF
>   	commit_chk_wrnNNO ""    crlf    $crlf   LF_CRLF   ""        LF_CRLF     LF_CRLF     ""
>   	commit_chk_wrnNNO auto  lf    	$crlf   ""        ""        ""          ""          ""
> -	commit_chk_wrnNNO auto  crlf  	$crlf   LF_CRLF   ""        ""          ""          ""
> +	commit_chk_wrnNNO auto  crlf  	$crlf   ""        ""        ""          ""          ""
>   	commit_chk_wrnNNO text  lf    	$crlf   ""       CRLF_LF    CRLF_LF     ""          CRLF_LF
>   	commit_chk_wrnNNO text  crlf  	$crlf   LF_CRLF   ""        LF_CRLF     LF_CRLF     ""
>   done
> @@ -417,7 +416,8 @@ commit_chk_wrnNNO "text"  ""      false   "$WILC"   "$WICL"   "$WAMIX"    "$WILC
>   commit_chk_wrnNNO "text"  ""      true    LF_CRLF   ""        LF_CRLF     LF_CRLF     ""
>   commit_chk_wrnNNO "text"  ""      input   ""        CRLF_LF   CRLF_LF     ""          CRLF_LF
>
> -test_expect_success 'create files cleanup' '
> +test_expect_success 'commit NNO and cleanup' '
> +	git commit -m "commit files on top of NNO" &&
>   	rm -f *.txt &&
>   	git -c core.autocrlf=false reset --hard
>   '
>

Re: [PATCH v1 1/2] t0027: Correct raciness in NNO test

[Torsten Bögershausen]

On Sat, Aug 13, 2016 at 06:50:19PM +0200, Johannes Sixt wrote:
> Am 12.08.2016 um 18:51 schrieb tboegi@web.de:
> >From: Torsten Bögershausen <tboegi@web.de>
> >
> >When a non-reversible CRLF conversion is done in "git add",
> >a warning is printed on stderr (or Git dies, depending on checksafe)
> >
> >The function commit_chk_wrnNNO() in t0027 was written to test this,
> >but did the wrong thing: Instead of looking at the warning
> >from "git add", it looked at the warning from "git commit".
> >
> >This is racy because "git commit" may not have to do CRLF conversion
> >at all if it can use the sha1 value from the index (which depends on
> >whether "add" and "commit" run in a single second).
> >
> >Correct this and replace the commit for each and every file with a commit
> >of all files in one go.
> 
> The new test code does not only fix the race condition, but also
> tests different things, or prepares test cases in a different
> manner. I would have appreciated an explanation why this is
> necessary. Is it "on my machine, the race condition was triggered
> consistently for a bunch of tests, and so I recorded the wrong
> expected output in the test cases"?
> 
Good point. The origanal intention was to let t0027 pass, and fix
the bug in convert.c in a later commit.
(and rename NNO in a 3rd commit, that is postponed until we figured this out).

Looking at t0027, it turns out that the changes in the test matrix done in 1/2
are reverted in 2/2.
This is not a good thing.
Either (a) they should be marked as test_expect_failure in 1/2 and
corrected in 2/2,
or (b) 1/2 and 2/2 are squashed together and the noise in t0027 is minimal.
I'll send a patch for (b) in a second.

[PATCH v2 0/1] convert: Correct NNO tests and missing `LF will be replaced by CRLF`

[tboegi]

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

Change since v1:
- The changes done in 1/2 in t0027 needed to be reverted in 2/2.
  Put both changes for convert.c and t0027 into a single commit
Thanks to Johannes Sixt.
Torsten Bögershausen (1):
  convert: Correct NNO tests and missing `LF will be replaced by CRLF`

 convert.c            | 97 ++++++++++++++++++++++++++++++----------------------
 t/t0027-auto-crlf.sh |  6 ++--
 2 files changed, 60 insertions(+), 43 deletions(-)

-- 
2.9.3.599.g2376d31.dirty

[PATCH v2 1/1] convert: Correct NNO tests and missing `LF will be replaced by CRLF`

[tboegi]

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

When a non-reversible CRLF conversion is done in "git add",
a warning is printed on stderr (or Git dies, depending on checksafe)

The function commit_chk_wrnNNO() in t0027 was written to test this,
but did the wrong thing: Instead of looking at the warning
from "git add", it looked at the warning from "git commit".

This is racy because "git commit" may not have to do CRLF conversion
at all if it can use the sha1 value from the index (which depends on
whether "add" and "commit" run in a single second).

Correct t0027 and replace the commit for each and every file with a commit
of all files in one go.
The function commit_chk_wrnNNO() should be renamed in a separate commit.

Now that t0027 does the right thing, it detects a bug in covert.c:
This sequence should generate the warning `LF will be replaced by CRLF`,
but does not:

$ git init
$ git config core.autocrlf false
$ printf "Line\r\n" >file
$ git add file
$ git commit -m "commit with CRLF"
$ git config core.autocrlf true
$ printf "Line\n" >file
$ git add file

"git add" calls crlf_to_git() in convert.c, which calls check_safe_crlf().
When has_cr_in_index(path) is true, crlf_to_git() returns too early and
check_safe_crlf() is not called at all.

Factor out the code which determines if "git checkout" converts LF->CRLF
into will_convert_lf_to_crlf().

Update the logic around check_safe_crlf() and "simulate" the possible
LF->CRLF conversion at "git checkout" with help of will_convert_lf_to_crlf().
Thanks to Jeff King <peff@peff.net> for analyzing t0027.

Reported-By: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 convert.c            | 97 ++++++++++++++++++++++++++++++----------------------
 t/t0027-auto-crlf.sh |  6 ++--
 2 files changed, 60 insertions(+), 43 deletions(-)

diff --git a/convert.c b/convert.c
index 67d69b5..077f5e6 100644
--- a/convert.c
+++ b/convert.c
@@ -189,33 +189,25 @@ static enum eol output_eol(enum crlf_action crlf_action)
 }
 
 static void check_safe_crlf(const char *path, enum crlf_action crlf_action,
-                            struct text_stat *stats, enum safe_crlf checksafe)
+			    struct text_stat *old_stats, struct text_stat *new_stats,
+			    enum safe_crlf checksafe)
 {
-	if (!checksafe)
-		return;
-
-	if (output_eol(crlf_action) == EOL_LF) {
+	if (old_stats->crlf && !new_stats->crlf ) {
 		/*
-		 * CRLFs would not be restored by checkout:
-		 * check if we'd remove CRLFs
+		 * CRLFs would not be restored by checkout
 		 */
-		if (stats->crlf) {
-			if (checksafe == SAFE_CRLF_WARN)
-				warning("CRLF will be replaced by LF in %s.\nThe file will have its original line endings in your working directory.", path);
-			else /* i.e. SAFE_CRLF_FAIL */
-				die("CRLF would be replaced by LF in %s.", path);
-		}
-	} else if (output_eol(crlf_action) == EOL_CRLF) {
+		if (checksafe == SAFE_CRLF_WARN)
+			warning("CRLF will be replaced by LF in %s.\nThe file will have its original line endings in your working directory.", path);
+		else /* i.e. SAFE_CRLF_FAIL */
+			die("CRLF would be replaced by LF in %s.", path);
+	} else if (old_stats->lonelf && !new_stats->lonelf ) {
 		/*
-		 * CRLFs would be added by checkout:
-		 * check if we have "naked" LFs
+		 * CRLFs would be added by checkout
 		 */
-		if (stats->lonelf) {
-			if (checksafe == SAFE_CRLF_WARN)
-				warning("LF will be replaced by CRLF in %s.\nThe file will have its original line endings in your working directory.", path);
-			else /* i.e. SAFE_CRLF_FAIL */
-				die("LF would be replaced by CRLF in %s", path);
-		}
+		if (checksafe == SAFE_CRLF_WARN)
+			warning("LF will be replaced by CRLF in %s.\nThe file will have its original line endings in your working directory.", path);
+		else /* i.e. SAFE_CRLF_FAIL */
+			die("LF would be replaced by CRLF in %s", path);
 	}
 }
 
@@ -233,12 +225,35 @@ static int has_cr_in_index(const char *path)
 	return has_cr;
 }
 
+static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,
+				   enum crlf_action crlf_action)
+{
+	if (output_eol(crlf_action) != EOL_CRLF)
+		return 0;
+	/* No "naked" LF? Nothing to convert, regardless. */
+	if (!stats->lonelf)
+		return 0;
+
+	if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || crlf_action == CRLF_AUTO_CRLF) {
+		/* If we have any CR or CRLF line endings, we do not touch it */
+		/* This is the new safer autocrlf-handling */
+		if (stats->lonecr || stats->crlf)
+			return 0;
+
+		if (convert_is_binary(len, stats))
+			return 0;
+	}
+	return 1;
+
+}
+
 static int crlf_to_git(const char *path, const char *src, size_t len,
 		       struct strbuf *buf,
 		       enum crlf_action crlf_action, enum safe_crlf checksafe)
 {
 	struct text_stat stats;
 	char *dst;
+	int convert_crlf_into_lf;
 
 	if (crlf_action == CRLF_BINARY ||
 	    (src && !len))
@@ -252,6 +267,8 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
 		return 1;
 
 	gather_stats(src, len, &stats);
+	/* Optimization: No CRLF? Nothing to convert, regardless. */
+	convert_crlf_into_lf = !!stats.crlf;
 
 	if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || crlf_action == CRLF_AUTO_CRLF) {
 		if (convert_is_binary(len, &stats))
@@ -263,12 +280,24 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
 		if (checksafe == SAFE_CRLF_RENORMALIZE)
 			checksafe = SAFE_CRLF_FALSE;
 		else if (has_cr_in_index(path))
-			return 0;
+			convert_crlf_into_lf = 0;
 	}
-	check_safe_crlf(path, crlf_action, &stats, checksafe);
-
-	/* Optimization: No CRLF? Nothing to convert, regardless. */
-	if (!stats.crlf)
+	if (checksafe && len) {
+		struct text_stat new_stats;
+		memcpy(&new_stats, &stats, sizeof(new_stats));
+		/* simulate "git add" */
+		if (convert_crlf_into_lf) {
+			new_stats.lonelf += new_stats.crlf;
+			new_stats.crlf = 0;
+		}
+		/* simulate "git checkout" */
+		if (will_convert_lf_to_crlf(len, &new_stats, crlf_action)) {
+			new_stats.crlf += new_stats.lonelf;
+			new_stats.lonelf = 0;
+		}
+		check_safe_crlf(path, crlf_action, &stats, &new_stats, checksafe);
+	}
+	if (!convert_crlf_into_lf)
 		return 0;
 
 	/*
@@ -314,21 +343,9 @@ static int crlf_to_worktree(const char *path, const char *src, size_t len,
 		return 0;
 
 	gather_stats(src, len, &stats);
-
-	/* No "naked" LF? Nothing to convert, regardless. */
-	if (!stats.lonelf)
+	if (!will_convert_lf_to_crlf(len, &stats, crlf_action))
 		return 0;
 
-	if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || crlf_action == CRLF_AUTO_CRLF) {
-		/* If we have any CR or CRLF line endings, we do not touch it */
-		/* This is the new safer autocrlf-handling */
-		if (stats.lonecr || stats.crlf )
-			return 0;
-
-		if (convert_is_binary(len, &stats))
-			return 0;
-	}
-
 	/* are we "faking" in place editing ? */
 	if (src == buf->buf)
 		to_free = strbuf_detach(buf, NULL);
diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index 2860d2d..90db54c 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -119,8 +119,7 @@ commit_chk_wrnNNO () {
 		fname=${pfx}_$f.txt &&
 		cp $f $fname &&
 		printf Z >>"$fname" &&
-		git -c core.autocrlf=$crlf add $fname 2>/dev/null &&
-		git -c core.autocrlf=$crlf commit -m "commit_$fname" $fname >"${pfx}_$f.err" 2>&1
+		git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err"
 	done
 
 	test_expect_success "commit NNO files crlf=$crlf attr=$attr LF" '
@@ -417,7 +416,8 @@ commit_chk_wrnNNO "text"  ""      false   "$WILC"   "$WICL"   "$WAMIX"    "$WILC
 commit_chk_wrnNNO "text"  ""      true    LF_CRLF   ""        LF_CRLF     LF_CRLF     ""
 commit_chk_wrnNNO "text"  ""      input   ""        CRLF_LF   CRLF_LF     ""          CRLF_LF
 
-test_expect_success 'create files cleanup' '
+test_expect_success 'commit NNO and cleanup' '
+	git commit -m "commit files on top of NNO" &&
 	rm -f *.txt &&
 	git -c core.autocrlf=false reset --hard
 '
-- 
2.9.3.599.g2376d31.dirty

Re: [PATCH v1 1/2] t0027: Correct raciness in NNO test

[Junio C Hamano]

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

> Looking at t0027, it turns out that the changes in the test matrix done in 1/2
> are reverted in 2/2.
> This is not a good thing.
> Either (a) they should be marked as test_expect_failure in 1/2 and
> corrected in 2/2,
> or (b) 1/2 and 2/2 are squashed together and the noise in t0027 is minimal.

Probably (a) is a bit more kosher and useful when somebody wants to
inspect the history of the test ("the test was flaky and that hid a
few bugs, but we fix the timing-dependency which exposes the bugs"
in 1/2, "this is the fix for the bug that the previous one exposed"
in 2/2).  (b) might be a bit less work, though.

Re: [PATCH v2 0/1] convert: Correct NNO tests and missing `LF will be replaced by CRLF`

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 446 bytes --]

Hi Torsten,

On Sat, 13 Aug 2016, tboegi@web.de wrote:

> From: Torsten Bögershausen <tboegi@web.de>
> 
> Change since v1:
> - The changes done in 1/2 in t0027 needed to be reverted in 2/2.
>   Put both changes for convert.c and t0027 into a single commit
> Thanks to Johannes Sixt.
> Torsten Bögershausen (1):
>   convert: Correct NNO tests and missing `LF will be replaced by CRLF`

Thank you so much for the fix!

Ciao,
Dscho

[PATCH v1 0/1] Rename NotNormalized (NNO) into CRLF in index

[tboegi]

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

Here comes the promised cleanup of t0027:
- The wording NNO is removed and replaced by CRI
- No code changes
- Needs to go on top of next or pu or tb/t0027-raciness-fix
Torsten Bögershausen (1):
  t0027: Rename NotNormalized (NNO) into CRLF in index

 t/t0027-auto-crlf.sh | 122 +++++++++++++++++++++++++--------------------------
 1 file changed, 61 insertions(+), 61 deletions(-)

-- 
2.9.3.599.g2376d31.dirty

[PATCH v1 1/1] t0027: Rename NotNormalized (NNO) into CRLF in index

[tboegi]

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

Originally NNO stands for content, that had been commited
"Not NOrmalized", in other words files with CRLF in the index.

Make more clear what should be tested:
- commit a file with CRLF into the index
- Change the content in the working tree
- Run "git add" and check for the conversion warnings
- Repeat for different content (text, LF, CRLF, mixed) and
  binary (LF and lone CR, CRLF with NUL)

Rename commit_chk_wrnNNO() into CRI_add_chk_wrn() and rename NNO into CRI.

Integrate create_NNO_files() into 'setup master'

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 t/t0027-auto-crlf.sh | 122 +++++++++++++++++++++++++--------------------------
 1 file changed, 61 insertions(+), 61 deletions(-)

diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index 90db54c..bfcf14b 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -49,24 +49,6 @@ create_gitattributes () {
 	} >.gitattributes
 }
 
-create_NNO_files () {
-	for crlf in false true input
-	do
-		for attr in "" auto text -text
-		do
-			for aeol in "" lf crlf
-			do
-				pfx=NNO_attr_${attr}_aeol_${aeol}_${crlf}
-				cp CRLF_mix_LF ${pfx}_LF.txt &&
-				cp CRLF_mix_LF ${pfx}_CRLF.txt &&
-				cp CRLF_mix_LF ${pfx}_CRLF_mix_LF.txt &&
-				cp CRLF_mix_LF ${pfx}_LF_mix_CR.txt &&
-				cp CRLF_mix_LF ${pfx}_CRLF_nul.txt
-			done
-		done
-	done
-}
-
 check_warning () {
 	case "$1" in
 	LF_CRLF) echo "warning: LF will be replaced by CRLF" >"$2".expect ;;
@@ -102,7 +84,7 @@ commit_check_warn () {
 	check_warning "$crlfnul" ${pfx}_CRLF_nul.err
 }
 
-commit_chk_wrnNNO () {
+CRI_add_chk_wrn () {
 	attr=$1 ; shift
 	aeol=$1 ; shift
 	crlf=$1 ; shift
@@ -111,7 +93,7 @@ commit_chk_wrnNNO () {
 	lfmixcrlf=$1 ; shift
 	lfmixcr=$1 ; shift
 	crlfnul=$1 ; shift
-	pfx=NNO_attr_${attr}_aeol_${aeol}_${crlf}
+	pfx=CRI_attr_${attr}_aeol_${aeol}_${crlf}
 	#Commit files on top of existing file
 	create_gitattributes "$attr" $aeol &&
 	for f in LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul
@@ -122,22 +104,22 @@ commit_chk_wrnNNO () {
 		git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err"
 	done
 
-	test_expect_success "commit NNO files crlf=$crlf attr=$attr LF" '
+	test_expect_success "CRLF in index add file crlf=$crlf attr=$attr LF" '
 		check_warning "$lfwarn" ${pfx}_LF.err
 	'
-	test_expect_success "commit NNO files attr=$attr aeol=$aeol crlf=$crlf CRLF" '
+	test_expect_success "CRLF in index add file attr=$attr aeol=$aeol crlf=$crlf CRLF" '
 		check_warning "$crlfwarn" ${pfx}_CRLF.err
 	'
 
-	test_expect_success "commit NNO files attr=$attr aeol=$aeol crlf=$crlf CRLF_mix_LF" '
+	test_expect_success "CRLF in index add file attr=$attr aeol=$aeol crlf=$crlf CRLF_mix_LF" '
 		check_warning "$lfmixcrlf" ${pfx}_CRLF_mix_LF.err
 	'
 
-	test_expect_success "commit NNO files attr=$attr aeol=$aeol crlf=$crlf LF_mix_cr" '
+	test_expect_success "CRLF in index add file attr=$attr aeol=$aeol crlf=$crlf LF_mix_cr" '
 		check_warning "$lfmixcr" ${pfx}_LF_mix_CR.err
 	'
 
-	test_expect_success "commit NNO files attr=$attr aeol=$aeol crlf=$crlf CRLF_nul" '
+	test_expect_success "CRLF in index add file attr=$attr aeol=$aeol crlf=$crlf CRLF_nul" '
 		check_warning "$crlfnul" ${pfx}_CRLF_nul.err
 	'
 }
@@ -199,7 +181,7 @@ check_files_in_repo () {
 	compare_files $crlfnul ${pfx}CRLF_nul.txt
 }
 
-check_in_repo_NNO () {
+check_in_repo_CRI () {
 	attr=$1 ; shift
 	aeol=$1 ; shift
 	crlf=$1 ; shift
@@ -208,7 +190,7 @@ check_in_repo_NNO () {
 	lfmixcrlf=$1 ; shift
 	lfmixcr=$1 ; shift
 	crlfnul=$1 ; shift
-	pfx=NNO_attr_${attr}_aeol_${aeol}_${crlf}
+	pfx=CRI_attr_${attr}_aeol_${aeol}_${crlf}
 	test_expect_success "compare_files $lfname ${pfx}_LF.txt" '
 		compare_files $lfname ${pfx}_LF.txt
 	'
@@ -329,8 +311,22 @@ test_expect_success 'setup master' '
 	printf "\$Id: 0000000000000000000000000000000000000000 \$\r\nLINEONE\r\nLINETWO\rLINETHREE"   >CRLF_mix_CR &&
 	printf "\$Id: 0000000000000000000000000000000000000000 \$\r\nLINEONEQ\r\nLINETWO\r\nLINETHREE" | q_to_nul >CRLF_nul &&
 	printf "\$Id: 0000000000000000000000000000000000000000 \$\nLINEONEQ\nLINETWO\nLINETHREE" | q_to_nul >LF_nul &&
-	create_NNO_files CRLF_mix_LF CRLF_mix_LF CRLF_mix_LF CRLF_mix_LF CRLF_mix_LF &&
-	git -c core.autocrlf=false add NNO_*.txt &&
+	for crlf in false true input
+	do
+		for attr in "" auto text -text
+		do
+			for aeol in "" lf crlf
+			do
+				pfx=CRI_attr_${attr}_aeol_${aeol}_${crlf}
+				cp CRLF_mix_LF ${pfx}_LF.txt &&
+				cp CRLF_mix_LF ${pfx}_CRLF.txt &&
+				cp CRLF_mix_LF ${pfx}_CRLF_mix_LF.txt &&
+				cp CRLF_mix_LF ${pfx}_LF_mix_CR.txt &&
+				cp CRLF_mix_LF ${pfx}_CRLF_nul.txt
+			done
+		done
+	done &&
+	git -c core.autocrlf=false add CRI_*.txt &&
 	git commit -m "mixed line endings" &&
 	test_tick
 '
@@ -391,33 +387,37 @@ test_expect_success 'commit files attr=crlf' '
 	commit_check_warn input "crlf" "LF_CRLF" ""        "LF_CRLF" "LF_CRLF" ""
 '
 
-#                 attr                    LF        CRLF      CRLFmixLF   LF_mix_CR   CRLFNUL
-commit_chk_wrnNNO ""      ""      false   ""        ""        ""          ""          ""
-commit_chk_wrnNNO ""      ""      true    LF_CRLF   ""        ""          ""          ""
-commit_chk_wrnNNO ""      ""      input   ""        ""        ""          ""          ""
-
-commit_chk_wrnNNO "auto"  ""      false   "$WILC"   ""        ""          ""          ""
-commit_chk_wrnNNO "auto"  ""      true    LF_CRLF   ""        ""          ""          ""
-commit_chk_wrnNNO "auto"  ""      input   ""        ""        ""          ""          ""
+# Files had been commited with CRLF. run "git add" with different content and
+# check if
+# warning: LF will be replaced by CRLF or
+# warning: CRLF will be replaced by LF are written to stderr
+#               attr                    LF        CRLF      CRLFmixLF   LF_mix_CR   CRLFNUL
+CRI_add_chk_wrn ""      ""      false   ""        ""        ""          ""          ""
+CRI_add_chk_wrn ""      ""      true    LF_CRLF   ""        ""          ""          ""
+CRI_add_chk_wrn ""      ""      input   ""        ""        ""          ""          ""
+
+CRI_add_chk_wrn "auto"  ""      false   "$WILC"   ""        ""          ""          ""
+CRI_add_chk_wrn "auto"  ""      true    LF_CRLF   ""        ""          ""          ""
+CRI_add_chk_wrn "auto"  ""      input   ""        ""        ""          ""          ""
 for crlf in true false input
 do
-	commit_chk_wrnNNO -text ""      $crlf   ""        ""        ""          ""          ""
-	commit_chk_wrnNNO -text lf      $crlf   ""        ""        ""          ""          ""
-	commit_chk_wrnNNO -text crlf    $crlf   ""        ""        ""          ""          ""
-	commit_chk_wrnNNO ""    lf      $crlf   ""       CRLF_LF    CRLF_LF      ""         CRLF_LF
-	commit_chk_wrnNNO ""    crlf    $crlf   LF_CRLF   ""        LF_CRLF     LF_CRLF     ""
-	commit_chk_wrnNNO auto  lf    	$crlf   ""        ""        ""          ""          ""
-	commit_chk_wrnNNO auto  crlf  	$crlf   LF_CRLF   ""        ""          ""          ""
-	commit_chk_wrnNNO text  lf    	$crlf   ""       CRLF_LF    CRLF_LF     ""          CRLF_LF
-	commit_chk_wrnNNO text  crlf  	$crlf   LF_CRLF   ""        LF_CRLF     LF_CRLF     ""
+	CRI_add_chk_wrn -text ""      $crlf   ""        ""        ""          ""          ""
+	CRI_add_chk_wrn -text lf      $crlf   ""        ""        ""          ""          ""
+	CRI_add_chk_wrn -text crlf    $crlf   ""        ""        ""          ""          ""
+	CRI_add_chk_wrn ""    lf      $crlf   ""        CRLF_LF   CRLF_LF      ""         CRLF_LF
+	CRI_add_chk_wrn ""    crlf    $crlf   LF_CRLF   ""        LF_CRLF     LF_CRLF     ""
+	CRI_add_chk_wrn auto  lf    	$crlf   ""        ""        ""          ""          ""
+	CRI_add_chk_wrn auto  crlf  	$crlf   LF_CRLF   ""        ""          ""          ""
+	CRI_add_chk_wrn text  lf    	$crlf   ""        CRLF_LF   CRLF_LF     ""          CRLF_LF
+	CRI_add_chk_wrn text  crlf  	$crlf   LF_CRLF   ""        LF_CRLF     LF_CRLF     ""
 done
 
-commit_chk_wrnNNO "text"  ""      false   "$WILC"   "$WICL"   "$WAMIX"    "$WILC"     "$WICL"
-commit_chk_wrnNNO "text"  ""      true    LF_CRLF   ""        LF_CRLF     LF_CRLF     ""
-commit_chk_wrnNNO "text"  ""      input   ""        CRLF_LF   CRLF_LF     ""          CRLF_LF
+CRI_add_chk_wrn   text  ""      false   "$WILC"   "$WICL"   "$WAMIX"    "$WILC"     "$WICL"
+CRI_add_chk_wrn   text  ""      true    LF_CRLF   ""        LF_CRLF     LF_CRLF     ""
+CRI_add_chk_wrn   text  ""      input   ""        CRLF_LF   CRLF_LF     ""          CRLF_LF
 
-test_expect_success 'commit NNO and cleanup' '
-	git commit -m "commit files on top of NNO" &&
+test_expect_success 'commit files with CRLF in index CRI and cleanup' '
+	git commit -m "commit files on top of CRLF" &&
 	rm -f *.txt &&
 	git -c core.autocrlf=false reset --hard
 '
@@ -449,16 +449,16 @@ test_expect_success 'commit -text' '
 for crlf in true false input
 do
 	#                 attr  aeol           LF  CRLF  CRLF_mix_LF  LF_mix_CR  CRLFNUL
-	check_in_repo_NNO ""    ""     $crlf   LF  CRLF  CRLF_mix_LF  LF_mix_CR  CRLF_nul
-	check_in_repo_NNO -text ""     $crlf   LF  CRLF  CRLF_mix_LF  LF_mix_CR  CRLF_nul
-	check_in_repo_NNO -text lf     $crlf   LF  CRLF  CRLF_mix_LF  LF_mix_CR  CRLF_nul
-	check_in_repo_NNO -text crlf   $crlf   LF  CRLF  CRLF_mix_LF  LF_mix_CR  CRLF_nul
-	check_in_repo_NNO auto  ""     $crlf   LF  CRLF  CRLF_mix_LF  LF_mix_CR  CRLF_nul
-	check_in_repo_NNO auto  lf     $crlf   LF  CRLF  CRLF_mix_LF  LF_mix_CR  CRLF_nul
-	check_in_repo_NNO auto  crlf   $crlf   LF  CRLF  CRLF_mix_LF  LF_mix_CR  CRLF_nul
-	check_in_repo_NNO text  ""     $crlf   LF  LF    LF           LF_mix_CR  LF_nul
-	check_in_repo_NNO text  lf     $crlf   LF  LF    LF           LF_mix_CR  LF_nul
-	check_in_repo_NNO text  crlf   $crlf   LF  LF    LF           LF_mix_CR  LF_nul
+	check_in_repo_CRI ""    ""     $crlf   LF  CRLF  CRLF_mix_LF  LF_mix_CR  CRLF_nul
+	check_in_repo_CRI -text ""     $crlf   LF  CRLF  CRLF_mix_LF  LF_mix_CR  CRLF_nul
+	check_in_repo_CRI -text lf     $crlf   LF  CRLF  CRLF_mix_LF  LF_mix_CR  CRLF_nul
+	check_in_repo_CRI -text crlf   $crlf   LF  CRLF  CRLF_mix_LF  LF_mix_CR  CRLF_nul
+	check_in_repo_CRI auto  ""     $crlf   LF  CRLF  CRLF_mix_LF  LF_mix_CR  CRLF_nul
+	check_in_repo_CRI auto  lf     $crlf   LF  CRLF  CRLF_mix_LF  LF_mix_CR  CRLF_nul
+	check_in_repo_CRI auto  crlf   $crlf   LF  CRLF  CRLF_mix_LF  LF_mix_CR  CRLF_nul
+	check_in_repo_CRI text  ""     $crlf   LF  LF    LF           LF_mix_CR  LF_nul
+	check_in_repo_CRI text  lf     $crlf   LF  LF    LF           LF_mix_CR  LF_nul
+	check_in_repo_CRI text  crlf   $crlf   LF  LF    LF           LF_mix_CR  LF_nul
 done
 ################################################################################
 # Check how files in the repo are changed when they are checked out
-- 
2.9.3.599.g2376d31.dirty

Re: [PATCH v1 0/1] Rename NotNormalized (NNO) into CRLF in index

[Junio C Hamano]

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>
>
> Here comes the promised cleanup of t0027:
> - The wording NNO is removed and replaced by CRI
> - No code changes
> - Needs to go on top of next or pu or tb/t0027-raciness-fix
> Torsten Bögershausen (1):
>   t0027: Rename NotNormalized (NNO) into CRLF in index
>
>  t/t0027-auto-crlf.sh | 122 +++++++++++++++++++++++++--------------------------
>  1 file changed, 61 insertions(+), 61 deletions(-)

Until these acronyms are shown expanded upfront in the file (not in
the log message), this patch does not help the readers at all.  They
used to say "What the heck is NNO?"  Now they will be left wondering
"What the heck is CRI?".

Have a comment before the first use of the acronym, perhaps?

@@ -102,7 +84,7 @@ commit_check_warn () {
 	check_warning "$crlfnul" ${pfx}_CRLF_nul.err
 }
 
-commit_chk_wrnNNO () {
+CRI_add_chk_wrn () {
 	attr=$1 ; shift
 	aeol=$1 ; shift
 	crlf=$1 ; shift


Incidentally it is a good place to describe what this humongous
helper function is meant to do.

# Having CR in the indexed contents (CRI in short) poses such and
# such tricky cases; this helper tests combinations of xyzzy, frotz
# and nitfol exhaustively by running xyzzy through (a, b, c),
# flipping frotz between (x, y), and ...

or something like that (of course the blanks need to be filled),
perhaps?

[PATCH v1 0/3] Update eol documentation

[tboegi]

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

Sorry for posting this so late:
While reviewing another patch I realized that the eol related
documentation was not updated as it should be.

Torsten Bögershausen (2):
  git ls-files: text=auto eol=lf is supported in Git 2.10
  gitattributes: Document the unified "auto" handling

 Documentation/git-ls-files.txt  |  3 +--
 Documentation/gitattributes.txt | 24 ++++++++++++++++--------
 2 files changed, 17 insertions(+), 10 deletions(-)

-- 
2.9.3.599.g2376d31.dirty

[PATCH v1 1/2] git ls-files: text=auto eol=lf is supported in Git 2.10

[tboegi]

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

The man page for `git ls-files --eol` mentions the combination
of text attributes "text=auto eol=lf" or "text=auto eol=crlf" as not
supported yet, but may be in the future.
Now they are supported

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 Documentation/git-ls-files.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 078b556..0d933ac 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -159,8 +159,7 @@ not accessible in the working tree.
 +
 <eolattr> is the attribute that is used when checking out or committing,
 it is either "", "-text", "text", "text=auto", "text eol=lf", "text eol=crlf".
-Note: Currently Git does not support "text=auto eol=lf" or "text=auto eol=crlf",
-that may change in the future.
+Since Git 2.10 "text=auto eol=lf" and "text=auto eol=crlf" are supported.
 +
 Both the <eolinfo> in the index ("i/<eolinfo>")
 and in the working tree ("w/<eolinfo>") are shown for regular files,
-- 
2.9.3.599.g2376d31.dirty

[PATCH v1 2/2] gitattributes: Document the unified "auto" handling

[tboegi]

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

Update the documentation about text=auto:
text=auto now follows the core.autocrlf handling when files are not
normalized in the repository.

For a cross platform project recommend the usage of attributes for
line-ending conversions.

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 Documentation/gitattributes.txt | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 807577a..4012661 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -213,27 +213,35 @@ that text files that you introduce to the repository have their line
 endings normalized to LF when they are added, and that files that are
 already normalized in the repository stay normalized.
 
+If you want to ensure that text files that any contributor introduces to
+the repository have their line endings normalized, you could set the
+`text` attribute to "auto" for _all_ files.
+
+------------------------
+*	text=auto
+------------------------
+
 If you want to interoperate with a source code management system that
 enforces end-of-line normalization, or you simply want all text files
 in your repository to be normalized, you should instead set the `text`
-attribute to "auto" for _all_ files.
+attribute to "text" for text files.
 
 ------------------------
-*	text=auto
+*.txt	text
 ------------------------
 
-This ensures that all files that Git considers to be text will have
+This ensures that all files marked as text will have
 normalized (LF) line endings in the repository.  The `core.eol`
 configuration variable controls which line endings Git will use for
 normalized files in your working directory; the default is to use the
 native line ending for your platform, or CRLF if `core.autocrlf` is
 set.
 
-NOTE: When `text=auto` normalization is enabled in an existing
-repository, any text files containing CRLFs should be normalized.  If
-they are not they will be normalized the next time someone tries to
-change them, causing unfortunate misattribution.  From a clean working
-directory:
+NOTE: When you have a cross-platform project using push and pull
+to a central repository the text files containing CRLFs should be
+normalized. All text files should have a text attribute, either
+`text` or `text=auto`.
+From a clean working directory:
 
 -------------------------------------------------
 $ echo "* text=auto" >>.gitattributes
-- 
2.9.3.599.g2376d31.dirty

Re: [PATCH v1 0/3] Update eol documentation

[Junio C Hamano]

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>
>
> Sorry for posting this so late:
> While reviewing another patch I realized that the eol related
> documentation was not updated as it should be.
>
> Torsten Bögershausen (2):
>   git ls-files: text=auto eol=lf is supported in Git 2.10
>   gitattributes: Document the unified "auto" handling
>
>  Documentation/git-ls-files.txt  |  3 +--
>  Documentation/gitattributes.txt | 24 ++++++++++++++++--------
>  2 files changed, 17 insertions(+), 10 deletions(-)

This [0/3] is meant to be a cover for [1/2] and [2/2]?

I am trying to see if we broke format-patch recently, or it is a
manual editing error.  The latter I do not care about; the former I
do.

Re: [PATCH v1 1/2] git ls-files: text=auto eol=lf is supported in Git 2.10

[Junio C Hamano]

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>
>
> The man page for `git ls-files --eol` mentions the combination
> of text attributes "text=auto eol=lf" or "text=auto eol=crlf" as not
> supported yet, but may be in the future.
> Now they are supported

Thanks. I'll finish the sentence with a full-stop here ;-).

>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>  Documentation/git-ls-files.txt | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> index 078b556..0d933ac 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -159,8 +159,7 @@ not accessible in the working tree.
>  +
>  <eolattr> is the attribute that is used when checking out or committing,
>  it is either "", "-text", "text", "text=auto", "text eol=lf", "text eol=crlf".
> -Note: Currently Git does not support "text=auto eol=lf" or "text=auto eol=crlf",
> -that may change in the future.
> +Since Git 2.10 "text=auto eol=lf" and "text=auto eol=crlf" are supported.

It may be a good idea to have this for a while.  Having this
sentence would only help those who have been dissuaded by the
existing Note by telling them that the limitation is no longer
there, but that will quickly become unnecessary.

We'd eventually want to remove this sentence.  I wonder if it is a
better alternative to just remove the Note without adding new text,
though.

>  Both the <eolinfo> in the index ("i/<eolinfo>")
>  and in the working tree ("w/<eolinfo>") are shown for regular files,

Re: [PATCH v1 2/2] gitattributes: Document the unified "auto" handling

[Junio C Hamano]

tboegi@web.de writes:

> +If you want to ensure that text files that any contributor introduces to
> +the repository have their line endings normalized, you could set the
> +`text` attribute to "auto" for _all_ files.
> +
> +------------------------
> +*	text=auto
> +------------------------
> +

That is very understandable, especially that the text before this
added paragraph is about "core.autocrlf" configuration that is about
"your" changes.  It contrasts gitconfig vs gitattributes very well.

However, it is no longer clear what "you should instead" in the
existing paragraph attempts to contrast with.  "If you want all text
files, then use '* text=auto'" is what is said previously.  And your
updated example below says "If you do not want that, and instead you
want X, do '*.txt text'".  But the value of X is reads the same as
the above one: "you want all text files to be normalized".

>  If you want to interoperate with a source code management system that
>  enforces end-of-line normalization, or you simply want all text files
>  in your repository to be normalized, you should instead set the `text`
> -attribute to "auto" for _all_ files.
> +attribute to "text" for text files.
>  
>  ------------------------
> -*	text=auto
> +*.txt	text
>  ------------------------

In short, the above is incoherent and not understandable, without
updating the three lines of introductory text you left untouched at
the beginning of the paragraph, when read in the (updated) context.

> -This ensures that all files that Git considers to be text will have
> +This ensures that all files marked as text will have

This is a good update of the description to match the updated example.

Re: [PATCH v1 0/3] Update eol documentation

[Jacob Keller]

On Thu, Aug 25, 2016 at 1:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
> tboegi@web.de writes:
>
>> From: Torsten Bögershausen <tboegi@web.de>
>>
>> Sorry for posting this so late:
>> While reviewing another patch I realized that the eol related
>> documentation was not updated as it should be.
>>
>> Torsten Bögershausen (2):
>>   git ls-files: text=auto eol=lf is supported in Git 2.10
>>   gitattributes: Document the unified "auto" handling
>>
>>  Documentation/git-ls-files.txt  |  3 +--
>>  Documentation/gitattributes.txt | 24 ++++++++++++++++--------
>>  2 files changed, 17 insertions(+), 10 deletions(-)
>
> This [0/3] is meant to be a cover for [1/2] and [2/2]?
>
> I am trying to see if we broke format-patch recently, or it is a
> manual editing error.  The latter I do not care about; the former I
> do.
> --

Yes. I recently changed some of format patch and would like to make
sure I didn't break it...

Thanks,
Jake

Re: [PATCH v1 0/3] Update eol documentation

[Torsten Bögershausen]

On 25/08/16 22:31, Junio C Hamano wrote:
> []

> This [0/3] is meant to be a cover for [1/2] and [2/2]?
>
> I am trying to see if we broke format-patch recently, or it is a
> manual editing error.  The latter I do not care about; the former I
> do.
>

No worry, 0/3 is patched by me by hand, sorry for the confusion.

[PATCH v2 0/2] Adjust the documentation to the unified "auto" handling

[tboegi]

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

Changes since v1:
- 1/2 is left unchanged
- 2/2 is re-written and should be more consistant to read.

Torsten Bögershausen (2):
  git ls-files: text=auto eol=lf is supported in Git 2.10
  gitattributes: Document the unified "auto" handling

 Documentation/git-ls-files.txt  |  3 +--
 Documentation/gitattributes.txt | 58 +++++++++++++++++------------------------
 2 files changed, 25 insertions(+), 36 deletions(-)

-- 
2.9.0.243.g5c589a7

[PATCH v2 1/2] git ls-files: text=auto eol=lf is supported in Git 2.10

[tboegi]

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

The man page for `git ls-files --eol` mentions the combination
of text attributes "text=auto eol=lf" or "text=auto eol=crlf" as not
supported yet, but may be in the future.
Now they are supported

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 Documentation/git-ls-files.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 078b556..0d933ac 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -159,8 +159,7 @@ not accessible in the working tree.
 +
 <eolattr> is the attribute that is used when checking out or committing,
 it is either "", "-text", "text", "text=auto", "text eol=lf", "text eol=crlf".
-Note: Currently Git does not support "text=auto eol=lf" or "text=auto eol=crlf",
-that may change in the future.
+Since Git 2.10 "text=auto eol=lf" and "text=auto eol=crlf" are supported.
 +
 Both the <eolinfo> in the index ("i/<eolinfo>")
 and in the working tree ("w/<eolinfo>") are shown for regular files,
-- 
2.9.0.243.g5c589a7

[PATCH v2 2/2] gitattributes: Document the unified "auto" handling

[tboegi]

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

Update the documentation about text=auto:
text=auto now follows the core.autocrlf handling when files are not
normalized in the repository.

For a cross platform project recommend the usage of attributes for
line-ending conversions.

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 Documentation/gitattributes.txt | 58 +++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 34 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 807577a..7aff940 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -182,23 +182,6 @@ While Git normally leaves file contents alone, it can be configured to
 normalize line endings to LF in the repository and, optionally, to
 convert them to CRLF when files are checked out.
 
-Here is an example that will make Git normalize .txt, .vcproj and .sh
-files, ensure that .vcproj files have CRLF and .sh files have LF in
-the working directory, and prevent .jpg files from being normalized
-regardless of their content.
-
-------------------------
-*               text=auto
-*.txt		text
-*.vcproj	text eol=crlf
-*.sh		text eol=lf
-*.jpg		-text
-------------------------
-
-Other source code management systems normalize all text files in their
-repositories, and there are two ways to enable similar automatic
-normalization in Git.
-
 If you simply want to have CRLF line endings in your working directory
 regardless of the repository you are working with, you can set the
 config variable "core.autocrlf" without using any attributes.
@@ -208,35 +191,42 @@ config variable "core.autocrlf" without using any attributes.
 	autocrlf = true
 ------------------------
 
-This does not force normalization of all text files, but does ensure
+This does not force normalization of text files, but does ensure
 that text files that you introduce to the repository have their line
 endings normalized to LF when they are added, and that files that are
 already normalized in the repository stay normalized.
 
-If you want to interoperate with a source code management system that
-enforces end-of-line normalization, or you simply want all text files
-in your repository to be normalized, you should instead set the `text`
-attribute to "auto" for _all_ files.
+If you want to ensure that text files that any contributor introduces to
+the repository have their line endings normalized, you can set the
+`text` attribute to "auto" for _all_ files.
 
 ------------------------
 *	text=auto
 ------------------------
 
-This ensures that all files that Git considers to be text will have
-normalized (LF) line endings in the repository.  The `core.eol`
-configuration variable controls which line endings Git will use for
-normalized files in your working directory; the default is to use the
-native line ending for your platform, or CRLF if `core.autocrlf` is
-set.
+The attributes allow a fine-grained control, how the line endings
+are converted.
+Here is an example that will make Git normalize .txt, .vcproj and .sh
+files, ensure that .vcproj files have CRLF and .sh files have LF in
+the working directory, and prevent .jpg files from being normalized
+regardless of their content.
+
+------------------------
+*               text=auto
+*.txt		text
+*.vcproj	text eol=crlf
+*.sh		text eol=lf
+*.jpg		-text
+------------------------
+
+NOTE: When `text=auto` conversion is enabled in a cross-platform
+project using push and pull to a central repository the text files
+containing CRLFs should be normalized.
 
-NOTE: When `text=auto` normalization is enabled in an existing
-repository, any text files containing CRLFs should be normalized.  If
-they are not they will be normalized the next time someone tries to
-change them, causing unfortunate misattribution.  From a clean working
-directory:
+From a clean working directory:
 
 -------------------------------------------------
-$ echo "* text=auto" >>.gitattributes
+$ echo "* text=auto" >.gitattributes
 $ rm .git/index     # Remove the index to force Git to
 $ git reset         # re-scan the working directory
 $ git status        # Show files that will be normalized
-- 
2.9.0.243.g5c589a7

Re: [PATCH v2 2/2] gitattributes: Document the unified "auto" handling

[Junio C Hamano]

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>
>
> Update the documentation about text=auto:
> text=auto now follows the core.autocrlf handling when files are not
> normalized in the repository.
>
> For a cross platform project recommend the usage of attributes for
> line-ending conversions.
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>  Documentation/gitattributes.txt | 58 +++++++++++++++++------------------------
>  1 file changed, 24 insertions(+), 34 deletions(-)

Looks good.  Let me merge this to 'master' before cutting -rc2.

Thanks.