[PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

Use the Travis-CI cache feature to store prove test results and make them
available in subsequent builds. This allows to run previously failed tests
first and run remaining tests in slowest to fastest order. As a result it
is less likely that Travis-CI needs to wait for a single test at the end
which speeds up the test suite execution by ~2 min.

Unfortunately the cache feature is only available (for free) on the
Travis-CI Linux environment.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 .travis.yml | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/.travis.yml b/.travis.yml
index c3bf9c6..f34726b 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -1,5 +1,9 @@
 language: c

+cache:
+  directories:
+    - $HOME/.prove-cache
+
 os:
   - linux
   - osx
@@ -18,7 +22,7 @@ env:
     - P4_VERSION="15.2"
     - GIT_LFS_VERSION="1.1.0"
     - DEFAULT_TEST_TARGET=prove
-    - GIT_PROVE_OPTS="--timer --jobs 3"
+    - GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
     - GIT_TEST_OPTS="--verbose --tee"
     - CFLAGS="-g -O2 -Wall -Werror"
     - GIT_TEST_CLONE_2GB=YesPlease
@@ -67,6 +71,8 @@ before_install:
     p4 -V | grep Rev.;
     echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)";
     git-lfs version;
+    mkdir -p $HOME/.prove-cache;
+    ln -s $HOME/.prove-cache/.prove t/.prove;

 before_script: make --jobs=2

--
2.5.1

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Jeff King]

On Tue, Jan 19, 2016 at 10:24:29AM +0100, larsxschneider@gmail.com wrote:

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> Use the Travis-CI cache feature to store prove test results and make them
> available in subsequent builds. This allows to run previously failed tests
> first and run remaining tests in slowest to fastest order. As a result it
> is less likely that Travis-CI needs to wait for a single test at the end
> which speeds up the test suite execution by ~2 min.

Thanks, this makes sense, and the patch looks good.

> @@ -18,7 +22,7 @@ env:
>      - P4_VERSION="15.2"
>      - GIT_LFS_VERSION="1.1.0"
>      - DEFAULT_TEST_TARGET=prove
> -    - GIT_PROVE_OPTS="--timer --jobs 3"
> +    - GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"

Have you tried bumping --jobs here? I usually use "16" on my local box.

I also looked into the Travis "container" thing. It's not clear to me
from their page:

  https://docs.travis-ci.com/user/workers/container-based-infrastructure/

whether we're using the new, faster container infrastructure or not. It
depends on when Travis "recognized" the repo, but I'm not quite sure
what that means. Should we be adding "sudo: false" to the top-level of
the yaml file?

-Peff

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Junio C Hamano]

larsxschneider@gmail.com writes:

> From: Lars Schneider <larsxschneider@gmail.com>
>
> Use the Travis-CI cache feature to store prove test results and make them
> available in subsequent builds. This allows to run previously failed tests
> first and run remaining tests in slowest to fastest order. As a result it
> is less likely that Travis-CI needs to wait for a single test at the end
> which speeds up the test suite execution by ~2 min.
>
> Unfortunately the cache feature is only available (for free) on the
> Travis-CI Linux environment.
>
> Suggested-by: Jeff King <peff@peff.net>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>  .travis.yml | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

This is cute, but isn't it useful even outside Travis's context?  I
am not suggesting to touch anything other than .travis.yml file in
this patch, but if I wanted to get the benefit from the idea in this
patch when I run my tests manually, I can just tell prove to use the
cached states, no?

IOW, I am confused by the beginning of the log message that says
this is taking advantage of "the Travis-CI cache feature".  This
improvement looks to me like using the feature of "prove" that
allows us to run slower tests first, and does not have much to do
with Travis.

You are relying on the assumption that things under $HOME/ is stable
while things under t/ (or in our source tree in general) are not,
and I think that is a sensible thing to take advantage of, but are
we sure that they are running in an environment where "ln -s" would
work?  Otherwise, it may be more robust to copy $HOME/.prove to
t/.prove before starting to test and then copy it back once the
tests are done.

Thanks.

>
> diff --git a/.travis.yml b/.travis.yml
> index c3bf9c6..f34726b 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -1,5 +1,9 @@
>  language: c
>
> +cache:
> +  directories:
> +    - $HOME/.prove-cache
> +
>  os:
>    - linux
>    - osx
> @@ -18,7 +22,7 @@ env:
>      - P4_VERSION="15.2"
>      - GIT_LFS_VERSION="1.1.0"
>      - DEFAULT_TEST_TARGET=prove
> -    - GIT_PROVE_OPTS="--timer --jobs 3"
> +    - GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
>      - GIT_TEST_OPTS="--verbose --tee"
>      - CFLAGS="-g -O2 -Wall -Werror"
>      - GIT_TEST_CLONE_2GB=YesPlease
> @@ -67,6 +71,8 @@ before_install:
>      p4 -V | grep Rev.;
>      echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)";
>      git-lfs version;
> +    mkdir -p $HOME/.prove-cache;
> +    ln -s $HOME/.prove-cache/.prove t/.prove;
>
>  before_script: make --jobs=2
>
> --
> 2.5.1

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> This is cute, but isn't it useful even outside Travis's context?  I
> am not suggesting to touch anything other than .travis.yml file in
> this patch, but if I wanted to get the benefit from the idea in this
> patch when I run my tests manually, I can just tell prove to use the
> cached states, no?

It seems that exporting something like

    GIT_PROVE_OPTS="--timer --state=slow,save -j8" 

when running "make DEFAULT_TEST_TARGET=prove test" does give me the
same benefit by leaving the stats from the previous run in t/.prove
when making the test scheduling decisions.

One thing I noticed but didn't dig further to fix was that this
"prove --state" business did not seem to work well together with

    make T="...list of tests..." test

that limits the set of tests to perform.  For example:

    $ rm -f t/.prove
    $ make -j4 GIT_PROVE_OPTS="--timer --state=slow,save -j8" \
	   T="$( cd t && echo t0???-*.sh)" \
           DEFAULT_TEST_TARGET=prove test

runs all test in 0xxx series and populates t/.prove with them.  And
immediately after that, with t/.prove still there:

    $ make -j4 GIT_PROVE_OPTS="--timer --state=slow,save -j8" \
         DEFAULT_TEST_TARGET=prove \
         T=t0000-basic.sh test

does not limit the test to only 0000, but ends up running all the
others recorded in t/.prove file, it seems.

I would imagine that this would not affect your use case negatively,
as it is unlikely that your automated tests are skipping different
set of tests in each run.

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Jan 19, 2016 at 10:24:29AM +0100, larsxschneider@gmail.com wrote:
>
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> Use the Travis-CI cache feature to store prove test results and make them
>> available in subsequent builds. This allows to run previously failed tests
>> first and run remaining tests in slowest to fastest order. As a result it
>> is less likely that Travis-CI needs to wait for a single test at the end
>> which speeds up the test suite execution by ~2 min.
>
> Thanks, this makes sense, and the patch looks good.
>
>> @@ -18,7 +22,7 @@ env:
>>      - P4_VERSION="15.2"
>>      - GIT_LFS_VERSION="1.1.0"
>>      - DEFAULT_TEST_TARGET=prove
>> -    - GIT_PROVE_OPTS="--timer --jobs 3"
>> +    - GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
>
> Have you tried bumping --jobs here? I usually use "16" on my local box.

I think 3 comes from this:

  http://thread.gmane.org/gmane.comp.version-control.git/279348/focus=279674

>
> I also looked into the Travis "container" thing. It's not clear to me
> from their page:
>
>   https://docs.travis-ci.com/user/workers/container-based-infrastructure/
>
> whether we're using the new, faster container infrastructure or not.
> ...
> depends on when Travis "recognized" the repo, but I'm not quite sure
> what that means. Should we be adding "sudo: false" to the top-level of
> the yaml file?

In an earlier discussion

  http://thread.gmane.org/gmane.comp.version-control.git/279348/focus=279495

I found that we were not eligible for container-based sandbox as the
version of travis-yaml back then used "sudo".  I do not seem to find
the use of sudo in the recent one we have in my tree, so it would be
beneficial if somebody interested in Travis CI look into this.

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Jeff King]

On Tue, Jan 19, 2016 at 02:53:00PM -0800, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > This is cute, but isn't it useful even outside Travis's context?  I
> > am not suggesting to touch anything other than .travis.yml file in
> > this patch, but if I wanted to get the benefit from the idea in this
> > patch when I run my tests manually, I can just tell prove to use the
> > cached states, no?
> 
> It seems that exporting something like
> 
>     GIT_PROVE_OPTS="--timer --state=slow,save -j8" 
> 
> when running "make DEFAULT_TEST_TARGET=prove test" does give me the
> same benefit by leaving the stats from the previous run in t/.prove
> when making the test scheduling decisions.

Yes, I've been using this on my local machine for years (which is why I
suggested it to Lars for the Travis build). I have also noticed that my
test runs take about as much time as the longest-running test, and do
not fully utilize all of my processors. I suspect we could drop the
run-time of the test suite substantially by splitting a few of the
longer tests.

You also wrote earlier:

> IOW, I am confused by the beginning of the log message that says
> this is taking advantage of "the Travis-CI cache feature".  This
> improvement looks to me like using the feature of "prove" that
> allows us to run slower tests first, and does not have much to do
> with Travis.

The interesting Travis feature we are using is that we are allowed to
store some data from run-to-run. So we use the Travis feature that lets
us use the prove feature. :)

> One thing I noticed but didn't dig further to fix was that this
> "prove --state" business did not seem to work well together with
> 
>     make T="...list of tests..." test
> 
> that limits the set of tests to perform.

Right, it does not do what you want.  The "prove --state" feature is not
just about ordering, but also about selecting. When run via "make", we
always give prove the full list of tests. But you can also do:

  prove --state=failed

manually to just run whatever failed on the last run.

I don't know if there is a way to tell prove "use the state for
ordering, but don't otherwise select from it", which would do what you
want above.

You can also note that if we ever delete a test script, it will still be
mentioned in prove's state file. I think prove is smart enough to
realize it went away and not bother you.

-Peff

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> You can also note that if we ever delete a test script, it will still be
> mentioned in prove's state file. I think prove is smart enough to
> realize it went away and not bother you.

The inverse might be more problematic.  When we add a new test
script (which we still do from time to time), does prove notice
that we asked it to run more tests than it already knows about?

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Jeff King]

On Tue, Jan 19, 2016 at 06:06:33PM -0500, Jeff King wrote:

> > It seems that exporting something like
> > 
> >     GIT_PROVE_OPTS="--timer --state=slow,save -j8" 
> > 
> > when running "make DEFAULT_TEST_TARGET=prove test" does give me the
> > same benefit by leaving the stats from the previous run in t/.prove
> > when making the test scheduling decisions.
> 
> Yes, I've been using this on my local machine for years (which is why I
> suggested it to Lars for the Travis build). I have also noticed that my
> test runs take about as much time as the longest-running test, and do
> not fully utilize all of my processors. I suspect we could drop the
> run-time of the test suite substantially by splitting a few of the
> longer tests.

Here are the numbers for that:

  $ time make ;# configured to use prove --state=slow,save -j16
  [...]
  real    0m47.035s
  user    1m6.884s
  sys     0m19.892s

  $ grep -v '^\.\.\.' .prove |
    perl -MYAML -e '
      local $/;
      $x = YAML::Load(<>)->{tests};
      print int($x->{$_}->{elapsed}), " $_\n" for keys(%$x)
    ' |
    sort -rn |
    head
  39 t3404-rebase-interactive.sh
  29 t3421-rebase-topology-linear.sh
  27 t9001-send-email.sh
  16 t9500-gitweb-standalone-no-errors.sh
  15 t3425-rebase-topology-merges.sh
  14 t6030-bisect-porcelain.sh
  13 t7610-mergetool.sh
  13 t5572-pull-submodule.sh
  13 t3426-rebase-submodule.sh
  12 t3415-rebase-autosquash.sh

So we're running t3404 for the majority of the time. I guess that
doesn't tell us how full our pipelines are for the rest of the time,
though. It could be worth splitting some of those long tests and seeing
if that improves run-time, though.

-Peff

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Jeff King]

On Tue, Jan 19, 2016 at 03:26:04PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > You can also note that if we ever delete a test script, it will still be
> > mentioned in prove's state file. I think prove is smart enough to
> > realize it went away and not bother you.
> 
> The inverse might be more problematic.  When we add a new test
> script (which we still do from time to time), does prove notice
> that we asked it to run more tests than it already knows about?

Yes. It runs the union of the state-file and what you give it on the
command line. E.g.:

  $ rm .prove
  $ prove --state=slow,save t0000-basic.sh
  No saved state, selection will be empty
  t0000-basic.sh .. ok    
  All tests successful.
  Files=1, Tests=77,  1 wallclock secs ( 0.03 usr  0.00 sys +  0.08 cusr 0.06 csys =  0.17 CPU)
  Result: PASS

  $ prove --state=slow,save t0001-init.sh
  t0000-basic.sh .. ok    
  t0001-init.sh ... ok    
  All tests successful.
  Files=2, Tests=113,  1 wallclock secs ( 0.03 usr  0.00 sys +  0.06 cusr  0.10 csys =  0.19 CPU)
  Result: PASS

-Peff

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> You can also note that if we ever delete a test script, it will still be
>> mentioned in prove's state file. I think prove is smart enough to
>> realize it went away and not bother you.
>
> The inverse might be more problematic.  When we add a new test
> script (which we still do from time to time), does prove notice
> that we asked it to run more tests than it already knows about?

Heh, I should have tested before sending it out. It seems that it
does notice what's missing from t/.prove so it is safe.

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Mike Hommey]

On Tue, Jan 19, 2016 at 03:00:52PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > On Tue, Jan 19, 2016 at 10:24:29AM +0100, larsxschneider@gmail.com wrote:
> >
> >> From: Lars Schneider <larsxschneider@gmail.com>
> >> 
> >> Use the Travis-CI cache feature to store prove test results and make them
> >> available in subsequent builds. This allows to run previously failed tests
> >> first and run remaining tests in slowest to fastest order. As a result it
> >> is less likely that Travis-CI needs to wait for a single test at the end
> >> which speeds up the test suite execution by ~2 min.
> >
> > Thanks, this makes sense, and the patch looks good.
> >
> >> @@ -18,7 +22,7 @@ env:
> >>      - P4_VERSION="15.2"
> >>      - GIT_LFS_VERSION="1.1.0"
> >>      - DEFAULT_TEST_TARGET=prove
> >> -    - GIT_PROVE_OPTS="--timer --jobs 3"
> >> +    - GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
> >
> > Have you tried bumping --jobs here? I usually use "16" on my local box.
> 
> I think 3 comes from this:
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/279348/focus=279674

Having recently looked into this, the relevant travis-ci documentation
is:
https://docs.travis-ci.com/user/ci-environment/

which says all environments have 2 cores, so you won't get much from
anything higher than -j3.

The following document also says something slightly different:
https://docs.travis-ci.com/user/speeding-up-the-build#Parallelizing-your-build-on-one-VM

"Travis CI VMs run on 1.5 virtual cores."

> > I also looked into the Travis "container" thing. It's not clear to me
> > from their page:
> >
> >   https://docs.travis-ci.com/user/workers/container-based-infrastructure/
> >
> > whether we're using the new, faster container infrastructure or not.
> > ...
> > depends on when Travis "recognized" the repo, but I'm not quite sure
> > what that means. Should we be adding "sudo: false" to the top-level of
> > the yaml file?
> 
> In an earlier discussion
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/279348/focus=279495
> 
> I found that we were not eligible for container-based sandbox as the
> version of travis-yaml back then used "sudo".  I do not seem to find
> the use of sudo in the recent one we have in my tree, so it would be
> beneficial if somebody interested in Travis CI look into this.

The https://docs.travis-ci.com/user/ci-environment/ document says the
default is "sudo: false" for repositories enabled in 2015 or later, which
I assume is the case for the git repository. "sudo: required" is the
default for repositories enabled before 2015.

Mike

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

> On Tue, Jan 19, 2016 at 03:00:52PM -0800, Junio C Hamano wrote:
>
>> I think 3 comes from this:
>> 
>>   http://thread.gmane.org/gmane.comp.version-control.git/279348/focus=279674
>
> Having recently looked into this, the relevant travis-ci documentation
> is:
> https://docs.travis-ci.com/user/ci-environment/
>
> which says all environments have 2 cores, so you won't get much from
> anything higher than -j3.
>
> The following document also says something slightly different:
> https://docs.travis-ci.com/user/speeding-up-the-build#Parallelizing-your-build-on-one-VM
>
> "Travis CI VMs run on 1.5 virtual cores."

Yup, that 1.5 was already mentioned in the earlier thread, but many
tests are mostly I/O bound, so 1.5 (or 2 for that matter) does not
mean we should not go higher than -j2 or -j3.  What I meant was that
the 3 comes from the old discussion "let's be nice to those who
offer this to us for free".

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Jeff King]

On Wed, Jan 20, 2016 at 09:26:06AM +0900, Mike Hommey wrote:

> Having recently looked into this, the relevant travis-ci documentation
> is:
> https://docs.travis-ci.com/user/ci-environment/
> 
> which says all environments have 2 cores, so you won't get much from
> anything higher than -j3.

FWIW, I settled on "-j16" on my 8-core (well, hyperthreaded quad-core)
machine after experimenting. That's running the tests on a RAM-disk,
though. On a slower filesystem where fsync() actually does something,
you're going to get a lot more I/O stalls, and want a bigger CPU to
process multiplier.

Here are actual numbers from my machine:

  -j | time (user+sys)
  ---+------------------
   1 | 5m18s (41s+17s)
   2 | 2m24s (41s+14s)
   4 | 1m15s (46s+13s)
   8 | 0m56s (65s+18s)
  16 | 0m53s (76s+24s)
  32 | 0m57s (78s+25s)

Note that the CPU-second times will go up with more threads because of
the frequency scaling.

So yeah, -j3 might not be that unreasonable, depending on the filesystem
response times.

> The https://docs.travis-ci.com/user/ci-environment/ document says the
> default is "sudo: false" for repositories enabled in 2015 or later, which
> I assume is the case for the git repository. "sudo: required" is the
> default for repositories enabled before 2015.

Thanks. The document I saw used the word "recognized", and I didn't
quite know what they meant. We just enabled this a month or two ago, so
we should be running on the new format.

-Peff

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Jeff King]

On Tue, Jan 19, 2016 at 05:46:55PM -0800, Junio C Hamano wrote:

> > "Travis CI VMs run on 1.5 virtual cores."
> 
> Yup, that 1.5 was already mentioned in the earlier thread, but many
> tests are mostly I/O bound, so 1.5 (or 2 for that matter) does not
> mean we should not go higher than -j2 or -j3.  What I meant was that
> the 3 comes from the old discussion "let's be nice to those who
> offer this to us for free".

I am very appreciative that we can use Travis for free, but I doubt they
care much one way or the other how we parallelize. Everything is
sandboxed enough that we should not be able to cause problems for them
or other customers. It's all CPU seconds to them (or should be, anyway).

The thing that _would_ probably bother them is throwing too many builds
at it (right now we are building the integration branches; it would be
useful information to build individual topics, too, but that would
increase the number of CPU seconds we ask them for).

-Peff

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Johannes Schindelin]

Hi Junio,

On Tue, 19 Jan 2016, Junio C Hamano wrote:

> larsxschneider@gmail.com writes:
> 
> > From: Lars Schneider <larsxschneider@gmail.com>
> >
> > Use the Travis-CI cache feature to store prove test results and make
> > them available in subsequent builds. This allows to run previously
> > failed tests first and run remaining tests in slowest to fastest
> > order. As a result it is less likely that Travis-CI needs to wait for
> > a single test at the end which speeds up the test suite execution by
> > ~2 min.
> >
> > Unfortunately the cache feature is only available (for free) on the
> > Travis-CI Linux environment.
> >
> > Suggested-by: Jeff King <peff@peff.net>
> > Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> > ---
> >  .travis.yml | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> This is cute, but isn't it useful even outside Travis's context?  I am
> not suggesting to touch anything other than .travis.yml file in this
> patch, but if I wanted to get the benefit from the idea in this patch
> when I run my tests manually, I can just tell prove to use the cached
> states, no?

You are basically talking about prove's support to modify behavior based
on previous runs. This patch does not introduce that support, it was
introduced long ago: 5099b99 (test-lib: Adjust output to be valid TAP
format, 2010-06-24). This commit also advertises that feature in t/README.

> IOW, I am confused by the beginning of the log message that says
> this is taking advantage of "the Travis-CI cache feature".

Lars' patch is really about Travis and its useful feature to retain state
from previous runs. AFAICT this is the feature Lars is talking about, and
it is absolutely necessary to make use of this prove feature (don't try
this with BuildHive, for example, its workspaces are typically
garbage-collected before the next CI run).

As such, I think it makes sense to talk about the Travis-CI feature in the
commit message (and not about the prove feature because we introduced
support for prove much, much earlier in Git's history, and advertised its
benefits at that time, as I stated above).

Ciao,
Dscho

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Lars Schneider]

On 19 Jan 2016, at 21:00, Junio C Hamano <gitster@pobox.com> wrote:

> larsxschneider@gmail.com writes:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> Use the Travis-CI cache feature to store prove test results and make them
>> available in subsequent builds. This allows to run previously failed tests
>> first and run remaining tests in slowest to fastest order. As a result it
>> is less likely that Travis-CI needs to wait for a single test at the end
>> which speeds up the test suite execution by ~2 min.
>> 
>> Unfortunately the cache feature is only available (for free) on the
>> Travis-CI Linux environment.
>> 
>> Suggested-by: Jeff King <peff@peff.net>
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>> .travis.yml | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
> 
> This is cute, but isn't it useful even outside Travis's context?  I
> am not suggesting to touch anything other than .travis.yml file in
> this patch, but if I wanted to get the benefit from the idea in this
> patch when I run my tests manually, I can just tell prove to use the
> cached states, no?
> 
> IOW, I am confused by the beginning of the log message that says
> this is taking advantage of "the Travis-CI cache feature".  This
> improvement looks to me like using the feature of "prove" that
> allows us to run slower tests first, and does not have much to do
> with Travis.
> 
> You are relying on the assumption that things under $HOME/ is stable
> while things under t/ (or in our source tree in general) are not,
> and I think that is a sensible thing to take advantage of, but are
> we sure that they are running in an environment where "ln -s" would
> work?  Otherwise, it may be more robust to copy $HOME/.prove to
> t/.prove before starting to test and then copy it back once the
> tests are done.

OK, looks like my wording was not ideal. One important thing to know is that 
$HOME is *not* stable. These TravisCI machines start *always* in a completely 
clean state. That's why prove cannot store and use it's cache. With the following 
statement I instruct Travis to cache my ".prove-cache" directory. As a consequence
Travis CI will automatically restore this directory whenever it starts a new instance
for the git job. It will also save the content of this directory when the job is done.

>> +cache:
>> +  directories:
>> +    - $HOME/.prove-cache

The Travis CI cache works only on a directory basis. Since I don't want to cache
the entire /t directory I came up with the $HOME/.prove-cache directory. I also used
a file link to leverage the automated save/restore feature for the $HOME/.prove-cache
directory. If I would not use a link then I would need to copy the updated .prove file 
from t/ to .prove-cache after the test run.

Would the following first sentence for the commit message be less ambiguous?

"Use the Travis-CI cache feature to make the prove test results cache persistent 
across subsequent build jobs. This allows to run previously..."

Thanks,
Lars


>> 
>> diff --git a/.travis.yml b/.travis.yml
>> index c3bf9c6..f34726b 100644
>> --- a/.travis.yml
>> +++ b/.travis.yml
>> @@ -1,5 +1,9 @@
>> language: c
>> 
>> +cache:
>> +  directories:
>> +    - $HOME/.prove-cache
>> +
>> os:
>>   - linux
>>   - osx
>> @@ -18,7 +22,7 @@ env:
>>     - P4_VERSION="15.2"
>>     - GIT_LFS_VERSION="1.1.0"
>>     - DEFAULT_TEST_TARGET=prove
>> -    - GIT_PROVE_OPTS="--timer --jobs 3"
>> +    - GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
>>     - GIT_TEST_OPTS="--verbose --tee"
>>     - CFLAGS="-g -O2 -Wall -Werror"
>>     - GIT_TEST_CLONE_2GB=YesPlease
>> @@ -67,6 +71,8 @@ before_install:
>>     p4 -V | grep Rev.;
>>     echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)";
>>     git-lfs version;
>> +    mkdir -p $HOME/.prove-cache;
>> +    ln -s $HOME/.prove-cache/.prove t/.prove;
>> 
>> before_script: make --jobs=2
>> 
>> --
>> 2.5.1

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Lars Schneider]

On 20 Jan 2016, at 01:26, Mike Hommey <mh@glandium.org> wrote:

> On Tue, Jan 19, 2016 at 03:00:52PM -0800, Junio C Hamano wrote:
>> Jeff King <peff@peff.net> writes:
>> 
>>> On Tue, Jan 19, 2016 at 10:24:29AM +0100, larsxschneider@gmail.com wrote:
>>> 
>>>> From: Lars Schneider <larsxschneider@gmail.com>
>>>> 
>>>> Use the Travis-CI cache feature to store prove test results and make them
>>>> available in subsequent builds. This allows to run previously failed tests
>>>> first and run remaining tests in slowest to fastest order. As a result it
>>>> is less likely that Travis-CI needs to wait for a single test at the end
>>>> which speeds up the test suite execution by ~2 min.
>>> 
>>> Thanks, this makes sense, and the patch looks good.
>>> 
>>>> @@ -18,7 +22,7 @@ env:
>>>>     - P4_VERSION="15.2"
>>>>     - GIT_LFS_VERSION="1.1.0"
>>>>     - DEFAULT_TEST_TARGET=prove
>>>> -    - GIT_PROVE_OPTS="--timer --jobs 3"
>>>> +    - GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
>>> 
>>> Have you tried bumping --jobs here? I usually use "16" on my local box.
>> 
>> I think 3 comes from this:
>> 
>>  http://thread.gmane.org/gmane.comp.version-control.git/279348/focus=279674
> 
> Having recently looked into this, the relevant travis-ci documentation
> is:
> https://docs.travis-ci.com/user/ci-environment/
> 
> which says all environments have 2 cores, so you won't get much from
> anything higher than -j3.
> 
> The following document also says something slightly different:
> https://docs.travis-ci.com/user/speeding-up-the-build#Parallelizing-your-build-on-one-VM
> 
> "Travis CI VMs run on 1.5 virtual cores."
> 
>>> I also looked into the Travis "container" thing. It's not clear to me
>>> from their page:
>>> 
>>>  https://docs.travis-ci.com/user/workers/container-based-infrastructure/
>>> 
>>> whether we're using the new, faster container infrastructure or not.
>>> ...
>>> depends on when Travis "recognized" the repo, but I'm not quite sure
>>> what that means. Should we be adding "sudo: false" to the top-level of
>>> the yaml file?
>> 
>> In an earlier discussion
>> 
>>  http://thread.gmane.org/gmane.comp.version-control.git/279348/focus=279495
>> 
>> I found that we were not eligible for container-based sandbox as the
>> version of travis-yaml back then used "sudo".  I do not seem to find
>> the use of sudo in the recent one we have in my tree, so it would be
>> beneficial if somebody interested in Travis CI look into this.
> 
> The https://docs.travis-ci.com/user/ci-environment/ document says the
> default is "sudo: false" for repositories enabled in 2015 or later, which
> I assume is the case for the git repository. "sudo: required" is the
> default for repositories enabled before 2015.
> 

I made the Git job run on the new container-based infrastructure for Linux.
We can add "sudo: false" to make this more explicit!

Thanks,
Lars

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Lars Schneider]

On 20 Jan 2016, at 02:46, Junio C Hamano <gitster@pobox.com> wrote:

> Mike Hommey <mh@glandium.org> writes:
> 
>> On Tue, Jan 19, 2016 at 03:00:52PM -0800, Junio C Hamano wrote:
>> 
>>> I think 3 comes from this:
>>> 
>>>  http://thread.gmane.org/gmane.comp.version-control.git/279348/focus=279674
>> 
>> Having recently looked into this, the relevant travis-ci documentation
>> is:
>> https://docs.travis-ci.com/user/ci-environment/
>> 
>> which says all environments have 2 cores, so you won't get much from
>> anything higher than -j3.
>> 
>> The following document also says something slightly different:
>> https://docs.travis-ci.com/user/speeding-up-the-build#Parallelizing-your-build-on-one-VM
>> 
>> "Travis CI VMs run on 1.5 virtual cores."
> 
> Yup, that 1.5 was already mentioned in the earlier thread, but many
> tests are mostly I/O bound, so 1.5 (or 2 for that matter) does not
> mean we should not go higher than -j2 or -j3.  What I meant was that
> the 3 comes from the old discussion "let's be nice to those who
> offer this to us for free".

I tested different settings and found that running prove with "-j5" seems to be
the fastest option for the Travis CI machines. However, I also noticed that 
I got more test failures with higher parallelism (Dscho reported similar 
observations [1]).

Especially t0025-crlf-auto.sh failed multiple times ([2], [3]) on the OS X builds
when I increase the parallelism:

not ok 4 - text=true causes a CRLF file to be normalized 
not ok 9 - text=auto, autocrlf=true _does_ normalize CRLF files 

Anyone an idea why that might be the case?

Thanks,
Lars

[1] http://article.gmane.org/gmane.comp.version-control.git/279660
[2] https://travis-ci.org/larsxschneider/git/jobs/103461538
[3] https://travis-ci.org/larsxschneider/git/jobs/103461458

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

> On 19 Jan 2016, at 21:00, Junio C Hamano <gitster@pobox.com> wrote:
>
>> IOW, I am confused by the beginning of the log message that says
>> this is taking advantage of "the Travis-CI cache feature".  This
>> improvement looks to me like using the feature of "prove" that
>> allows us to run slower tests first, and does not have much to do
>> with Travis.
>> 
>> You are relying on the assumption that things under $HOME/ is stable
>> while things under t/ (or in our source tree in general) are not,
>> and I think that is a sensible thing to take advantage of, but are
>> we sure that they are running in an environment where "ln -s" would
>> work?  Otherwise, it may be more robust to copy $HOME/.prove to
>> t/.prove before starting to test and then copy it back once the
>> tests are done.
>
> OK, looks like my wording was not ideal. One important thing to know is that 
> $HOME is *not* stable. These TravisCI machines start *always* in a completely 
> clean state.

Ah, that is what I missed.  Travis makes everything transient by
default (which is a sensible thing to do for CI), but it lets you
declare some things are to be made stable, and that is the "cache"
feature you are taking advantage of in Travis.

The log message needs to be clarified in a reroll, but thanks for
clarifying it for me in advance ;-)

That only leaves one question from me: Is 'ln -s' safe enough?
Would copying back and forth make it more robust?

I am guessint the answers are Yes and No, in which case the patch
text can (and should) stay as-is.

Thanks.

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[brian m. carlson]

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

On Wed, Jan 20, 2016 at 10:22:16AM +0100, Lars Schneider wrote:
> I tested different settings and found that running prove with "-j5" seems to be
> the fastest option for the Travis CI machines. However, I also noticed that
> I got more test failures with higher parallelism (Dscho reported similar
> observations [1]).
> 
> Especially t0025-crlf-auto.sh failed multiple times ([2], [3]) on the OS X builds
> when I increase the parallelism:
> 
> not ok 4 - text=true causes a CRLF file to be normalized
> not ok 9 - text=auto, autocrlf=true _does_ normalize CRLF files
> 
> Anyone an idea why that might be the case?

I've seen this on my personal box too[0] when running make -j4 all test.
I wasn't able to pin down why it was occurring, but if we're going to
run the tests in parallel, it's probably worth spending some time
figuring it out.

[0] Debian amd64/sid, ThinkPad X220.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Jeff King]

On Fri, Jan 22, 2016 at 02:33:59AM +0000, brian m. carlson wrote:

> > Especially t0025-crlf-auto.sh failed multiple times ([2], [3]) on the OS X builds
> > when I increase the parallelism:
> > 
> > not ok 4 - text=true causes a CRLF file to be normalized
> > not ok 9 - text=auto, autocrlf=true _does_ normalize CRLF files
> > 
> > Anyone an idea why that might be the case?
> 
> I've seen this on my personal box too[0] when running make -j4 all test.
> I wasn't able to pin down why it was occurring, but if we're going to
> run the tests in parallel, it's probably worth spending some time
> figuring it out.

Interesting.  I run the test suite in parallel probably a dozen times
per day, and I've never seen this. However, I was able to trigger it
eventually with:

  for i in 1 2 3 4 5 6 7 8
  do
    (while ./t0025-crlf-auto.sh --root=/var/ram/git-tests/foo-$i -v -i >/tmp/foo-$i 2>&1
    do
      : nothing
    done
    echo FAILED $i
    ) &
  done

I get a few of the threads failing (in test 4) after 2-3 minutes. The
"-v" output is pretty unenlightening, though. I don't see anything
racy-looking in the test unless it is something with "read-tree" and
stat mtimes.

-Peff

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Jeff King]

On Fri, Jan 22, 2016 at 12:52:55AM -0500, Jeff King wrote:

> I get a few of the threads failing (in test 4) after 2-3 minutes. The
> "-v" output is pretty unenlightening, though. I don't see anything
> racy-looking in the test unless it is something with "read-tree" and
> stat mtimes.

And indeed, doing this:

diff --git a/t/t0025-crlf-auto.sh b/t/t0025-crlf-auto.sh
index c164b46..d34775b 100755
--- a/t/t0025-crlf-auto.sh
+++ b/t/t0025-crlf-auto.sh
@@ -56,6 +56,7 @@ test_expect_success 'text=true causes a CRLF file to be normalized' '
 
 	rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL &&
 	echo "CRLFonly text" > .gitattributes &&
+	sleep 1 &&
 	git read-tree --reset -u HEAD &&
 
 	# Note, "normalized" means that git will normalize it if added

let me run for over 5 minutes with no failures in test 4 (I eventually
did hit one in test 9). I don't claim to understand what is going on,
though.

-Peff

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Thomas Gummerer]

On 01/22, Jeff King wrote:
> On Fri, Jan 22, 2016 at 12:52:55AM -0500, Jeff King wrote:
>
> > I get a few of the threads failing (in test 4) after 2-3 minutes. The
> > "-v" output is pretty unenlightening, though. I don't see anything
> > racy-looking in the test unless it is something with "read-tree" and
> > stat mtimes.
>
> And indeed, doing this:
>
> diff --git a/t/t0025-crlf-auto.sh b/t/t0025-crlf-auto.sh
> index c164b46..d34775b 100755
> --- a/t/t0025-crlf-auto.sh
> +++ b/t/t0025-crlf-auto.sh
> @@ -56,6 +56,7 @@ test_expect_success 'text=true causes a CRLF file to be normalized' '
>
>  	rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL &&
>  	echo "CRLFonly text" > .gitattributes &&
> +	sleep 1 &&
>  	git read-tree --reset -u HEAD &&
>
>  	# Note, "normalized" means that git will normalize it if added
>
> let me run for over 5 minutes with no failures in test 4 (I eventually
> did hit one in test 9). I don't claim to understand what is going on,
> though.

I don't think this is the right solution though, I think this mostly
lessens the load on the filesystem so the flakiness doesn't occur,
especially on your system, where it seems hard to trigger in the first
place :)

I actually hit the same problem occasionally when running the test
suite before, but was always to lazy to try to reproduce it.  Thanks
to your reproduction I think I was able to track the underlying
problem down.

My analysis is in the commit message below.

--->8---
Subject: [PATCH] entry: fix up to date marking

write_entry always marks cache entries up to date when
state->refresh_cache is set.  This is however not always accurate,
if core.autocrlf is set in the config, a file with cr and lf line
endings exists and the file attribute is set to text or crlf in the
gitattributes.

Most notably this makes t0025 flaky.  When calling deleting the files
that will be adjusted through the automated crlf handling, and then
calling `git read-tree --reset -u HEAD`, this leads to a race between
git read-tree and the filesystem.  The test currently only passes
most of the time, because the filesystem usually isn't synced between
the call to unpack_trees() and write_locked_index().

Currently the sequence of 1) remove files with cr and lf as line
endings, 2) `git read-tree --reset -u HEAD` 3) checking the status of
the changed files succeeds, because the index and the files are written
at the same time, so they have the same mtime.  Thus when reading the
index the next time, the files are recognized as racy, and the actual
contents on the disk are checked for changes.

If the index and the files have different mtimes however, the entry is
written to the index as up to date because of the flag set in
entry.c:write_entry(), and the contents on the filesystem are not
actually checked again, because the stat data in the index matches.

The failures in t0025 can be consistently reproduced by introducing a
call to sync() between the call to unpack_trees() and
write_index_locked().

Instead of blindly marking and entry up to date in write_entry(), check
if the contents may change on disk first, and strip the CE_UPTODATE flag
in that case.  Because the flag is not set, the cache entry will go
through the racy check when writing the index the first time, and
smudged if appropriate.

This fixes the flaky test as well as the underlying problem.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 entry.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/entry.c b/entry.c
index 582c400..102fdfa 100644
--- a/entry.c
+++ b/entry.c
@@ -214,6 +214,8 @@ finish:
 		if (!fstat_done)
 			lstat(ce->name, &st);
 		fill_stat_cache_info(ce, &st);
+		if (would_convert_to_git(ce->name))
+			ce->ce_flags &= ~CE_UPTODATE;
 		ce->ce_flags |= CE_UPDATE_IN_BASE;
 		state->istate->cache_changed |= CE_ENTRY_CHANGED;
 	}
--
2.7.0.75.g3ee1e0f.dirty

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> My analysis is in the commit message below.
>
> --->8---
> Subject: [PATCH] entry: fix up to date marking
>
> write_entry always marks cache entries up to date when
> state->refresh_cache is set.  This is however not always accurate,
> if core.autocrlf is set in the config, a file with cr and lf line
> endings exists and the file attribute is set to text or crlf in the
> gitattributes.
>
> Most notably this makes t0025 flaky.  When calling deleting the files
> that will be adjusted through the automated crlf handling, and then
> calling `git read-tree --reset -u HEAD`, this leads to a race between
> git read-tree and the filesystem.  The test currently only passes
> most of the time, because the filesystem usually isn't synced between
> the call to unpack_trees() and write_locked_index().
>
> Currently the sequence of 1) remove files with cr and lf as line
> endings, 2) `git read-tree --reset -u HEAD` 3) checking the status of
> the changed files succeeds, because the index and the files are written
> at the same time, so they have the same mtime.  Thus when reading the
> index the next time, the files are recognized as racy, and the actual
> contents on the disk are checked for changes.
>
> If the index and the files have different mtimes however, the entry is
> written to the index as up to date because of the flag set in
> entry.c:write_entry(), and the contents on the filesystem are not
> actually checked again, because the stat data in the index matches.
>
> The failures in t0025 can be consistently reproduced by introducing a
> call to sync() between the call to unpack_trees() and
> write_index_locked().
>
> Instead of blindly marking and entry up to date in write_entry(), check
> if the contents may change on disk first, and strip the CE_UPTODATE flag
> in that case.  Because the flag is not set, the cache entry will go
> through the racy check when writing the index the first time, and
> smudged if appropriate.

Sorry, but I am confused by all of the above.

We write the thing out with write_entry(), possibly applying smudge
filters and eol conversion to the "git" representation to create the
"working tree" representation in this codepath, right?  The resulting
file matches what the user's configuration told us to produce.

Until the working tree file is changed by somebody after the above
happens, we shouldn't have to check the contents of the file to see
if there is a difference.  By definition, that has to match the
contents expected to be there by Git.

The only case I can think of that the above does not hold is when
the smuge/clean and the eol conversion are not a correct round-trip
operation pairs, but that would be a misconfiguration.  Otherwise,
we'd be _always_ comparing the contents without relying on the
cached stat info for any paths whose in-core and working tree
representations are different, not just those that are configured
with misbehaving smudge/clean pair, no?

Puzzled...  In this case, my hunch says that the patch is correct,
your analysis also is and it is only me who is missing some crucial
bits in the analysis and getting confused.

Enlightenment, please?

>
> This fixes the flaky test as well as the underlying problem.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>  entry.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/entry.c b/entry.c
> index 582c400..102fdfa 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -214,6 +214,8 @@ finish:
>  		if (!fstat_done)
>  			lstat(ce->name, &st);
>  		fill_stat_cache_info(ce, &st);
> +		if (would_convert_to_git(ce->name))
> +			ce->ce_flags &= ~CE_UPTODATE;
>  		ce->ce_flags |= CE_UPDATE_IN_BASE;
>  		state->istate->cache_changed |= CE_ENTRY_CHANGED;
>  	}
> --
> 2.7.0.75.g3ee1e0f.dirty

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Sorry, but I am confused by all of the above.
>
> We write the thing out with write_entry(), possibly applying smudge
> filters and eol conversion to the "git" representation to create the
> "working tree" representation in this codepath, right?  The resulting
> file matches what the user's configuration told us to produce.
>
> Until the working tree file is changed by somebody after the above
> happens, we shouldn't have to check the contents of the file to see
> if there is a difference.  By definition, that has to match the
> contents expected to be there by Git.
>
> The only case I can think of that the above does not hold is when
> the smuge/clean and the eol conversion are not a correct round-trip
> operation pairs, but that would be a misconfiguration.  Otherwise,
> we'd be _always_ comparing the contents without relying on the
> cached stat info for any paths whose in-core and working tree
> representations are different, not just those that are configured
> with misbehaving smudge/clean pair, no?

To put it differently, if a blob stored at path has CRLF line
endings and .gitattributes is changed after the fact to say that it
must have LF line endings, we should treat it as a broken transitory
state.  There may have to be a way to "fix" an already "wrong" blob
in the index that is milder than "rm --cached && add .", but I do
not think write_entry(), which is shared by all the normal codepaths
that writes out to the working tree, is the best place to do so, if
doing so forces the contents of the paths to be always re-checked,
just in case the user is in such a broken transitory state.

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Thomas Gummerer]

On 01/24, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Sorry, but I am confused by all of the above.
> >
> > We write the thing out with write_entry(), possibly applying smudge
> > filters and eol conversion to the "git" representation to create the
> > "working tree" representation in this codepath, right?  The resulting
> > file matches what the user's configuration told us to produce.
> >
> > Until the working tree file is changed by somebody after the above
> > happens, we shouldn't have to check the contents of the file to see
> > if there is a difference.  By definition, that has to match the
> > contents expected to be there by Git.
> >
> > The only case I can think of that the above does not hold is when
> > the smuge/clean and the eol conversion are not a correct round-trip
> > operation pairs, but that would be a misconfiguration.  Otherwise,
> > we'd be _always_ comparing the contents without relying on the
> > cached stat info for any paths whose in-core and working tree
> > representations are different, not just those that are configured
> > with misbehaving smudge/clean pair, no?
>
> To put it differently, if a blob stored at path has CRLF line
> endings and .gitattributes is changed after the fact to say that it
> must have LF line endings, we should treat it as a broken transitory
> state.

Right, I wasn't considering this as a broken state, because t0025 uses
just this to transition between the states.

> There may have to be a way to "fix" an already "wrong" blob
> in the index that is milder than "rm --cached && add .", but I do
> not think write_entry(), which is shared by all the normal codepaths
> that writes out to the working tree, is the best place to do so, if
> doing so forces the contents of the paths to be always re-checked,
> just in case the user is in such a broken transitory state.

Maybe I'm misunderstanding something, but the contents of the paths
are only re-checked if we are in such a broken transition state, and
the file stored in git has crlf line endings, and thus would be
normalized.  In this case we currently re-check the contents of that
file anyway, at least when the file and the index have the same mtime,
and we actually show the correct output.

I'm not too familiar with the eol conversion code, so I might be
missing something.

--
Thomas

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> On 01/24, Junio C Hamano wrote:
>> To put it differently, if a blob stored at path has CRLF line
>> endings and .gitattributes is changed after the fact to say that it
>> must have LF line endings, we should treat it as a broken transitory
>> state.
>
> Right, I wasn't considering this as a broken state, because t0025 uses
> just this to transition between the states.
>
>> There may have to be a way to "fix" an already "wrong" blob
>> in the index that is milder than "rm --cached && add .", but I do
>> not think write_entry(), which is shared by all the normal codepaths
>> that writes out to the working tree, is the best place to do so, if
>> doing so forces the contents of the paths to be always re-checked,
>> just in case the user is in such a broken transitory state.
>
> Maybe I'm misunderstanding something, but the contents of the paths
> are only re-checked if we are in such a broken transition state, and

What I do not understand here is how the added check ensures that
"only if in such a broken transition state".  would_convert_to_git()
does not take the contents but is called only with the pathname to
key into the attributes, so in a typical cross platform project
where all the source files are "text" and the repository can set
core.eol to adjust the end of line convention for its working tree,
the added check has no way to differentiate the paths that are
recorded with CRLF line endings in the object database by mistake,
i.e. the ones in the broken transitory state, and all the other
paths that are following the "text" and storing their blobs with LF
line endings.  The added check would trigger "is it racy" check to
re-reads the contents that we have written out from the working tree
for the paths with "wrong" blobs, but how would it avoid doing so
for the paths whose blobs are already stored correctly?

The new code affects not just "reset --hard", but everybody who
writes out from the object database to the working tree and records
that these paths are checked out in the index.  How does the new
code avoid destroying the performance for all paths?

> the file stored in git has crlf line endings, and thus would be
> normalized.  In this case we currently re-check the contents of that
> file anyway, at least when the file and the index have the same mtime,
> and we actually show the correct output.

The right way to at that "correct output", I think, is that it
happens to be shown that way by accident.  It is questionable that
it is correct to report that such a path is modified.  Immediately
after you check out a path to the working tree out of the index, via
"reset --hard" and "checkout $path", by definition it ought to be
the same between the working tree file and the index.

Unless it is in this transititory broken state, that is.

The "by accident" happens only because racy-git avoidance is being
implemented in one particular way.  Is about protecting people from
making changes to the working tree files immediately after their
previous contents are registered to the index (and the index files
written to the disk), and immediately after we write things out of
the index and by definition the result and the indexed blob ought to
match there is no reason to re-read and recheck unless the working
tree files are further edited.

The current way the racy-git avoidance works by re-reading and
re-checking the contents when the timestamps match is merely one
possible implementation, and that is the only thing that produces
your "correct" output most of the time in this test.  We could have
waited after writing the index time for a second before giving the
control back to the user instead, which would have also allowed us
to implement the racy-git avoidance, and in that alternate world,
all these transitory broken paths would have been correctly reported
as unmodified.

IOW, I would think the test in question is insisting a single
outcome for an operation whose result is undefined, and it is
harmful to twist the system by pessimizing the common cases just
to cater to this transititory broken state.

I am not saying that we shouldn't have support for users to fix
their repository and get out of this transititory broken state.  A
recent work by Torsten Bögershausen to have ls-files report the end
of line convention used in the blob in the index and the settings
that affect conversion for each path (among other things) is a step
in the right direction.  With a support like that, those who noticed
that they by mistake added CRLF files to the index as-is when they
wanted their project to be cross platform can recover from it by
setting necessary attributes (i.e. mark them as "text") and then
find paths that are broken out of "ls-files --eol" output to see
which ones are not using lf end-of-line in the index.

I do not think there is a canned command to help dealing with these
broken paths right now.  You would have to check them out of the
index (you would get a CRLF file in the working tree in the example
we are discussing), fix the line endings (you would run dos2unix on
it in this example, as you would want "text=true" attribute) and
"git add" them to recover manually, but I can imagine that Torsten's
work can be extended to do all of these, without molesting the
working tree files, with minimum work by the end user.  That is:

 * Reuse Torsten's "ls-files --eol" code to find paths that record
   the blob in the index that does not follow the eol convention
   specified for the path;

 * For each of these index entries, run convert_to_working_tree() on
   the indexed contents, and then on the result of it, run
   convert_to_git().  The result is the blob that the index ought to
   have had, if it were to be consistent with the attribute
   settings.  So add that to the index.

 * Write the index out.

 * Tell the user to commit or commit it automatically with a canned
   log message "fix broken encoding" or something.

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I am not saying that we shouldn't have support for users to fix
> their repository and get out of this transititory broken state.  A
> recent work by Torsten Bögershausen to have ls-files report the end
> of line convention used in the blob in the index and the settings
> that affect conversion for each path (among other things) is a step
> in the right direction.  With a support like that, those who noticed
> that they by mistake added CRLF files to the index as-is when they
> wanted their project to be cross platform can recover from it by
> setting necessary attributes (i.e. mark them as "text") and then
> find paths that are broken out of "ls-files --eol" output to see
> which ones are not using lf end-of-line in the index.
>
> I do not think there is a canned command to help dealing with these
> broken paths right now.  You would have to check them out of the
> index (you would get a CRLF file in the working tree in the example
> we are discussing), fix the line endings (you would run dos2unix on
> it in this example, as you would want "text=true" attribute) and
> "git add" them to recover manually, but I can imagine that Torsten's
> work can be extended to do all of these, without molesting the
> working tree files, with minimum work by the end user.  That is:
>
>  * Reuse Torsten's "ls-files --eol" code to find paths that record
>    the blob in the index that does not follow the eol convention
>    specified for the path;
>
>  * For each of these index entries, run convert_to_working_tree() on
>    the indexed contents, and then on the result of it, run
>    convert_to_git().  The result is the blob that the index ought to
>    have had, if it were to be consistent with the attribute
>    settings.  So add that to the index.
>
>  * Write the index out.
>
>  * Tell the user to commit or commit it automatically with a canned
>    log message "fix broken encoding" or something.

Here is what I whipped up as a lunch-break hack.  I do not claim
that "git add" would be the best place to do this, but it should be
sufficient to illustrate the overall idea.

The user can say "git add --fix-index" and have a simplified version
of the above happen, i.e. for each path in the index, if the
contents recorded there does not round-trip to the identical
contents when first converted to the working tree representation
(i.e. passing through core.eol and smudge filter conversion) and
then converted back to the Git blob representation (i.e. clean
filter and core.crlf), and when the result is different from what we
started from, we know we have an unnormalized blob registered in the
index, so we replace it.  After this, "git diff --cached" would show
the correction made by this operation, and committing it would let
you fix the earlier mistake that added CRLF content when the path
was marked with text=true attribute.

We could go even fancier and attempt the round-trip twice or more.
It is possible that the in-index representation will not converge
when you use a misconfigured pair of clean/smudge filters (e.g.
using "gzip -d -c" as the smudge filter, and then using "gzip -c"
without "-n" option as the clean filter would most likely make the
in-index representation fuzzy, as each time the cycle is run, the
compressed contents will be made with different timestamps, even
though the working tree representation will be the same), and an
operation "we screwed up the filters, please repair the damage!"
like this "add --fix-index" is probably the best place to catch such
a misconfiguration.

 builtin/add.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/builtin/add.c b/builtin/add.c
index 145f06e..36d3915 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -233,6 +233,7 @@ N_("The following paths are ignored by one of your .gitignore files:\n");
 
 static int verbose, show_only, ignored_too, refresh_only;
 static int ignore_add_errors, intent_to_add, ignore_missing;
+static int fix_index;
 
 #define ADDREMOVE_DEFAULT 1
 static int addremove = ADDREMOVE_DEFAULT;
@@ -263,6 +264,7 @@ static struct option builtin_add_options[] = {
 	OPT_BOOL( 0 , "refresh", &refresh_only, N_("don't add, only refresh the index")),
 	OPT_BOOL( 0 , "ignore-errors", &ignore_add_errors, N_("just skip files which cannot be added because of errors")),
 	OPT_BOOL( 0 , "ignore-missing", &ignore_missing, N_("check if - even missing - files are ignored in dry run")),
+	OPT_BOOL( 0 , "fix-index", &fix_index, N_("fix contents in the index that is inconsistent with the eol and clean/smudge filters")),
 	OPT_END(),
 };
 
@@ -297,6 +299,64 @@ static int add_files(struct dir_struct *dir, int flags)
 	return exit_status;
 }
 
+static int fix_index_roundtrip(int ac, const char **av, const char *prefix)
+{
+	int i;
+
+	if (ac)
+		die(_("git add --fix-index does not take any other argument"));
+
+	if (read_cache() < 0)
+		die(_("index file corrupt"));
+
+	for (i = 0; i < active_nr; i++) {
+		struct cache_entry *ce = active_cache[i];
+		struct strbuf buf = STRBUF_INIT;
+		char *contents;
+		unsigned long size;
+		enum object_type type;
+		unsigned char sha1[20];
+
+		if (ce_stage(ce) || !S_ISREG(ce->ce_mode))
+			continue;
+		if (!would_convert_to_git(ce->name))
+			continue;
+
+		contents = read_sha1_file(ce->sha1, &type, &size);
+		if (type != OBJ_BLOB)
+			die(_("object in the index at path '%s' is not a blob"),
+			    ce->name);
+
+		/*
+		 * Round-trip conversion; act as if we wrote it out to the
+		 * working tree and then re-read it, with clean/smudge and
+		 * eol conversions.  Do we get the same result?
+		 */
+		if (convert_to_working_tree(ce->name, contents, size, &buf))
+			strbuf_add(&buf, contents, size);
+		free(contents);
+
+		contents = strbuf_detach(&buf, &size);
+
+		if (!convert_to_git(ce->name, contents, size, &buf, 0))
+			strbuf_add(&buf, contents, size);
+		free(contents);
+
+		/* Hash the result - does it match? */
+		hash_sha1_file(buf.buf, buf.len, "blob", sha1);
+		if (hashcmp(sha1, ce->sha1)) {
+			hashcpy(ce->sha1, sha1);
+			active_cache_changed = 1;
+		}
+		strbuf_release(&buf);
+	}
+
+	if (active_cache_changed)
+		if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
+			die(_("Unable to write new index file"));
+	return 0;
+}
+
 int cmd_add(int argc, const char **argv, const char *prefix)
 {
 	int exit_status = 0;
@@ -318,6 +378,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	if (edit_interactive)
 		return(edit_patch(argc, argv, prefix));
+
+	if (fix_index)
+		exit(fix_index_roundtrip(argc, argv, prefix));
+
 	argc--;
 	argv++;
 

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Thomas Gummerer]

On 01/25, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > On 01/24, Junio C Hamano wrote:
> >> To put it differently, if a blob stored at path has CRLF line
> >> endings and .gitattributes is changed after the fact to say that it
> >> must have LF line endings, we should treat it as a broken transitory
> >> state.
> >
> > Right, I wasn't considering this as a broken state, because t0025 uses
> > just this to transition between the states.
> >
> >> There may have to be a way to "fix" an already "wrong" blob
> >> in the index that is milder than "rm --cached && add .", but I do
> >> not think write_entry(), which is shared by all the normal codepaths
> >> that writes out to the working tree, is the best place to do so, if
> >> doing so forces the contents of the paths to be always re-checked,
> >> just in case the user is in such a broken transitory state.
> >
> > Maybe I'm misunderstanding something, but the contents of the paths
> > are only re-checked if we are in such a broken transition state, and
>
> What I do not understand here is how the added check ensures that
> "only if in such a broken transition state".  would_convert_to_git()
> does not take the contents but is called only with the pathname to
> key into the attributes, so in a typical cross platform project
> where all the source files are "text" and the repository can set
> core.eol to adjust the end of line convention for its working tree,
> the added check has no way to differentiate the paths that are
> recorded with CRLF line endings in the object database by mistake,
> i.e. the ones in the broken transitory state, and all the other
> paths that are following the "text" and storing their blobs with LF
> line endings.  The added check would trigger "is it racy" check to
> re-reads the contents that we have written out from the working tree
> for the paths with "wrong" blobs, but how would it avoid doing so
> for the paths whose blobs are already stored correctly?
>
> The new code affects not just "reset --hard", but everybody who
> writes out from the object database to the working tree and records
> that these paths are checked out in the index.  How does the new
> code avoid destroying the performance for all paths?

I misunderstood the way would_convert_to_git() works, I should have
actually read the code, instead of just relying on my test, which was
even wrong.  Sorry about the confusion, my patch does indeed hurt
the performance.

> > the file stored in git has crlf line endings, and thus would be
> > normalized.  In this case we currently re-check the contents of that
> > file anyway, at least when the file and the index have the same mtime,
> > and we actually show the correct output.
>
> The right way to at that "correct output", I think, is that it
> happens to be shown that way by accident.  It is questionable that
> it is correct to report that such a path is modified.  Immediately
> after you check out a path to the working tree out of the index, via
> "reset --hard" and "checkout $path", by definition it ought to be
> the same between the working tree file and the index.
>
> Unless it is in this transititory broken state, that is.
>
> The "by accident" happens only because racy-git avoidance is being
> implemented in one particular way.  Is about protecting people from
> making changes to the working tree files immediately after their
> previous contents are registered to the index (and the index files
> written to the disk), and immediately after we write things out of
> the index and by definition the result and the indexed blob ought to
> match there is no reason to re-read and recheck unless the working
> tree files are further edited.
>
> The current way the racy-git avoidance works by re-reading and
> re-checking the contents when the timestamps match is merely one
> possible implementation, and that is the only thing that produces
> your "correct" output most of the time in this test.  We could have
> waited after writing the index time for a second before giving the
> control back to the user instead, which would have also allowed us
> to implement the racy-git avoidance, and in that alternate world,
> all these transitory broken paths would have been correctly reported
> as unmodified.
>
> IOW, I would think the test in question is insisting a single
> outcome for an operation whose result is undefined, and it is
> harmful to twist the system by pessimizing the common cases just
> to cater to this transititory broken state.
>
> I am not saying that we shouldn't have support for users to fix
> their repository and get out of this transititory broken state.  A
> recent work by Torsten Bögershausen to have ls-files report the end
> of line convention used in the blob in the index and the settings
> that affect conversion for each path (among other things) is a step
> in the right direction.  With a support like that, those who noticed
> that they by mistake added CRLF files to the index as-is when they
> wanted their project to be cross platform can recover from it by
> setting necessary attributes (i.e. mark them as "text") and then
> find paths that are broken out of "ls-files --eol" output to see
> which ones are not using lf end-of-line in the index.
>
> I do not think there is a canned command to help dealing with these
> broken paths right now.  You would have to check them out of the
> index (you would get a CRLF file in the working tree in the example
> we are discussing), fix the line endings (you would run dos2unix on
> it in this example, as you would want "text=true" attribute) and
> "git add" them to recover manually, but I can imagine that Torsten's
> work can be extended to do all of these, without molesting the
> working tree files, with minimum work by the end user.  That is:
>
>  * Reuse Torsten's "ls-files --eol" code to find paths that record
>    the blob in the index that does not follow the eol convention
>    specified for the path;
>
>  * For each of these index entries, run convert_to_working_tree() on
>    the indexed contents, and then on the result of it, run
>    convert_to_git().  The result is the blob that the index ought to
>    have had, if it were to be consistent with the attribute
>    settings.  So add that to the index.
>
>  * Write the index out.
>
>  * Tell the user to commit or commit it automatically with a canned
>    log message "fix broken encoding" or something.

Thanks for the thorough explanation, and the patch in the next email,
I'll have a look at that tomorrow.

--
Thomas

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Clemens Buchacher]

I think Junio pointed me to this thread from "[PATCH] optionally disable
gitattributes". Since I am not sure I am following everything correctly
in this thread, allow me to recapitulate what I understood so far.

Firstly, I think the racy'ness of t0025 is understood. It is due to the
is_racy_timestamp check in read-cache.c's ie_match_stat. But for the
moment I would like to put this aside, because the issue can be
reproduced reliably with this change to t0025:

diff --git a/t/t0025-crlf-auto.sh b/t/t0025-crlf-auto.sh
index c164b46..e30e9b3 100755
--- a/t/t0025-crlf-auto.sh
+++ b/t/t0025-crlf-auto.sh
@@ -55,8 +55,11 @@ test_expect_success 'crlf=true causes a CRLF file to be normalized' '
 test_expect_success 'text=true causes a CRLF file to be normalized' '
 
        rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL &&
-       echo "CRLFonly text" > .gitattributes &&
        git read-tree --reset -u HEAD &&
+       sleep 1 &&
+       rm .git/index &&
+       git reset &&
+       echo "CRLFonly text" > .gitattributes &&
 
        # Note, "normalized" means that git will normalize it if added
        has_cr CRLFonly &&

I intentionally wait for one second and then I remove and re-read the
index. Now the timestamps of CRLFonly and .git/index are different, so
we avoid the is_racy_timestamp check. From now on Git will not read the
contents of CRLFonly from disk again until either the index entry or the
mtime of CRLFonly changes (maybe we also check the size, I am not sure).

Now we add .gitattributes. This does not change the index entry, nor
does it change the mtime of CRLFonly. Therefore the subsequent git diff
turns out empty, and the test fails.

I believe this behavior is expected. In gitattributes(5) we therefore
recommend using rm .git/index and git reset to "force Git to rescan the
working directory." The test should be fixed accordingly, something
like:

diff --git a/t/t0025-crlf-auto.sh b/t/t0025-crlf-auto.sh
index c164b46..2917591 100755
--- a/t/t0025-crlf-auto.sh
+++ b/t/t0025-crlf-auto.sh
@@ -57,6 +57,8 @@ test_expect_success 'text=true causes a CRLF file to be normalized' '
        rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL &&
        echo "CRLFonly text" > .gitattributes &&
        git read-tree --reset -u HEAD &&
+       rm .git/index &&
+       git reset &&
 
        # Note, "normalized" means that git will normalize it if added
        has_cr CRLFonly &&



On Mon, Jan 25, 2016 at 01:52:18PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I do not think there is a canned command to help dealing with these
> > broken paths right now.

I think (rm .git/index && git reset) works well enough in most cases,
but not all:

> We could go even fancier and attempt the round-trip twice or more.
> It is possible that the in-index representation will not converge
> when you use a misconfigured pair of clean/smudge filters

This can also happen with eol conversion, for example if you have files
with CRCRLF line endings. The eol conversion will remove only one CR.
Two conversions would be needed to achieve a normalized format. But
iterating (rm .git/index && git reset) does not help. Since we do not
touch the file on disk, after the first round, we have CRCRLF on disk
and CRLF in the index. During the second round, Git reads CRCRLF from
disk again, converts it to CRLF, which matches the index. Even
git reset --hard will not checkout the CRLF version to the worktree.

A possible solution is to iterate (rm -r * && git checkout -- . && git
add -u) until the work tree is clean. Quite ugly.

A command like git add --fix-index would make this conversion less
painful.  It should be ok if the user has to run it several times in
corner cases like CRCRLF, but it would be nice to issue a warning if the
index is still not normalized after running git add --fix-index.

Regarding the name of the option, maybe git add --renormalize-index
would be more consistent, since we also have the related merge option
"renormalize", which is very similar. In fact possibly you can share
some code with it.

Your patch looks good to me otherwise.


Coming back to "[PATCH] optionally disable gitattributes": The topics
are related, because they both deal with the situation where the work
tree has files which are not normalized according to gitattributes. But
my patch is more about saying: ok, I know I may have files which need to
be normalized, but I want to ignore this issue for now. Please disable
gitattributes for now, because I want to work with the files as they are
committed. Conversely, the discussion here is about how to reliably
detect and fix files which are not normalized.

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> Coming back to "[PATCH] optionally disable gitattributes": The topics
> are related, because they both deal with the situation where the work
> tree has files which are not normalized according to gitattributes. But
> my patch is more about saying: ok, I know I may have files which need to
> be normalized, but I want to ignore this issue for now. Please disable
> gitattributes for now, because I want to work with the files as they are
> committed. Conversely, the discussion here is about how to reliably
> detect and fix files which are not normalized.

I primarily wanted to make sure that you understood the underlying
issue, so that I do not have to go back to the basics in the other
thread.  And it is clear that you obviously do, which is good.

Here, you seem to think that what t0025 wants to see happen is
sensible, judging by the fact that you call "rm .git/index && git
reset" a "fix".

My take on this is quite different.  After a "reset --hard HEAD", we
should be able to trust the cached stat information and have "diff
HEAD" say "no changes".  That is what you essentially want in the
other thread, if I understand you correctly, and in an ideal world
where the filesystem timestamp has infinite precision, that is what
would happen in t0025, always "breaking" its expectation.  The real
world has much coarser timestamp granularity than ideal, and that is
why the test appear to be "flaky", failing to give "correct" outcome
some of the time--but I'd say that it is expecting a wrong thing.

An index entry that has data that does not round-trip when it goes
through convert_to_working_tree() and then convert_to_git() "breaks"
this arrangement, and I'd view it as the user having an inconsistent
data.  It is like you are in a repository that still has an unmerged
paths--you cannot proceed before you resolve them.

Anyway.

As to your patch in the other thread, here is what I think:

 (1) When you know (or perhaps your CI knows) that the working tree
     has never been modified since you did "reset --hard HEAD" (or
     its equivalent, like "git checkout $branch" from a clean
     state), these paths with inconsistent data would break the
     usual check to ask "is the working tree clean?"  That is a
     problem and we need a way to ensure that the working tree is
     always judged to be clean immediately after "reset --hard
     HEAD".  IOW, I agree with you that the issue you are trying to
     solve is worth solving.

 (2) Regardless of the "inconsistent data breaking the cleanliness
     check" issue, it may be handy to have a way to temporarily
     disable the attributes, i.e. allow us to ask "what happens if
     there is no attributes defined?"  IOW, I am saying that the
     change in the patch is not without merit.

In addition to (1), I further think that this sequence should not
report that the path F is modified:

     # Write F from HEAD to the working tree, after passing it
     # through convert_to_working_tree()
     $ git reset --hard HEAD

     # Force the re-reading, without changing the contents at all
     $ cp F F.new
     $ mv F.new F

     $ git diff HEAD

which is broken by paths with inconsistent data.  Your CI would want
a way to make that happen.

However, I do not think disabling attributes (i.e. (2)) is a
solution to the issue (i.e. (1)), which we just agreed to be an
issue that is worth solving, for at least two reasons.

 * Even without any attributes, core.autocrlf setting can get the
   data in your index (whose lines can be terminated with CRLF) into
   the same "inconsistent data" situation.  Disabling attribute
   handling would not have any effect on that codepath, I think.

 * The indexed data and the contents in the working tree file may
   match only because the clean/smudge transformation is done.  If
   you disable attributes, re-checking by passing the working tree
   contents through convert_to_git() and comparing the result with
   what is in the index would tell you that they are different, even
   if the clean/smudge filter pair implements round-trip operations
   correctly.

One way to solve (1) I can think of is to change the definition of
ce_compare_data(), which is called by the code that does not trust
the cached stat data (including but not limited to the Racy Git
codepath).  The current semantics of that function asks this
question:

    We do not know if the working tree file and the indexed data
    match.  Let's see if "git add" of that path would record the
    data that is identical to what is in the index.

This definition was cast in stone by 29e4d363 (Racy GIT, 2005-12-20)
and has been with us since Git v1.0.0.  But that does not have to be
the only sensible definition of this check.  I wonder what would
break if we ask this question instead:

    We do not know if the working tree file and the indexed data
    match.  Let's see if "git checkout" of that path would leave the
    same data as what currently is in the working tree file.

If we did this, "reset --hard HEAD" followed by "diff HEAD" will by
definition always report "is clean" as long as nobody changes files
in the working tree, even with the inconsistent data in the index.

This still requires that convert_to_working_tree(), i.e. your smudge
filter, is deterministic, though, but I think that is a sensible
assumption for sane people, even for those with inconsistent data in
the index.

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> One way to solve (1) I can think of is to change the definition of
> ce_compare_data(), which is called by the code that does not trust
> the cached stat data (including but not limited to the Racy Git
> codepath).  The current semantics of that function asks this
> question:
>
>     We do not know if the working tree file and the indexed data
>     match.  Let's see if "git add" of that path would record the
>     data that is identical to what is in the index.
>
> This definition was cast in stone by 29e4d363 (Racy GIT, 2005-12-20)
> and has been with us since Git v1.0.0.  But that does not have to be
> the only sensible definition of this check.  I wonder what would
> break if we ask this question instead:
>
>     We do not know if the working tree file and the indexed data
>     match.  Let's see if "git checkout" of that path would leave the
>     same data as what currently is in the working tree file.
>
> If we did this, "reset --hard HEAD" followed by "diff HEAD" will by
> definition always report "is clean" as long as nobody changes files
> in the working tree, even with the inconsistent data in the index.
>
> This still requires that convert_to_working_tree(), i.e. your smudge
> filter, is deterministic, though, but I think that is a sensible
> assumption for sane people, even for those with inconsistent data in
> the index.

Just a few additional comments.

The primary reason why I originally chose "does 'git add' of what is
in the working tree give us the same blob in the index?" as opposed
to "does 'git checkout' from the index again will give the same
result in the working tree?" is because it is a lot less resource
intensive and also is simpler.  Back then I do not think we had a
streaming interface to hash huge contents from a file in the working
tree, but it requires us to read the entire file from the filesystem
just once, apply the convert_to_git() processing and then hash the
result, whether we keep the whole thing in core at once or process
the data in streaming fashion.  Doing the other check will have to
inflate the blob data and apply the convert_to_working_tree()
processing, and also read the whole thing from the filesystem and
compare, which is more work at runtime.  And for a sane set-up where
the data in the index does not contradict with the clean/smudge
filter and EOL settings, both would yield the same result.

If we were to switch the semantics of ce_compare_data(), we would
want a new sibling interface next to stream_blob_to_fd() that takes
a file descriptor opened on the file in the working tree for reading
(fd), the object name (sha1), and the output filter, and works very
similarly to stream_blob_to_fd().  The difference would be that we
would be reading from the fd (i.e. the file in the working tree) as
we read from the istream (i.e. the contents of the blob in the
index, after passing the convert_to_working_tree() filter) and
comparing them in the main loop.  The filter parameter to the
function would be obtained by calling get_stream_filter() just like
how write_entry() uses it to prepare the filter parameter to call
streaming_write_entry() with.  That way, we can rely on future
improvement of the streaming interface to make sure we keep the data
we have to keep in core to the minimum.

IOW, I am saying that the "add --fix-index" lunchbreak patch I sent
earlier in the thread that has to hold the data in-core while
processing is not a production quality patch ;-)

Re: eol round trip Was: [PATCH] travis-ci: run previously failed ....

[Torsten Bögershausen]

On 01/27/2016 08:05 PM, Junio C Hamano wrote:
(Changed the topic, 2 notes inside)
> Clemens Buchacher <drizzd@aon.at> writes:
>
>> Coming back to "[PATCH] optionally disable gitattributes": The topics
>> are related, because they both deal with the situation where the work
>> tree has files which are not normalized according to gitattributes. But
>> my patch is more about saying: ok, I know I may have files which need to
>> be normalized, but I want to ignore this issue for now. Please disable
>> gitattributes for now, because I want to work with the files as they are
>> committed. Conversely, the discussion here is about how to reliably
>> detect and fix files which are not normalized.
git ls-files --eol can detect that (as Junio pointed out)

> I primarily wanted to make sure that you understood the underlying
> issue, so that I do not have to go back to the basics in the other
> thread.  And it is clear that you obviously do, which is good.
>
> Here, you seem to think that what t0025 wants to see happen is
> sensible, judging by the fact that you call "rm .git/index && git
> reset" a "fix".
>
> My take on this is quite different.  After a "reset --hard HEAD", we
> should be able to trust the cached stat information and have "diff
> HEAD" say "no changes".  That is what you essentially want in the
> other thread, if I understand you correctly, and in an ideal world
> where the filesystem timestamp has infinite precision, that is what
> would happen in t0025, always "breaking" its expectation.  The real
> world has much coarser timestamp granularity than ideal, and that is
> why the test appear to be "flaky", failing to give "correct" outcome
> some of the time--but I'd say that it is expecting a wrong thing.
>
> An index entry that has data that does not round-trip when it goes
> through convert_to_working_tree() and then convert_to_git() "breaks"
> this arrangement, and I'd view it as the user having an inconsistent
> data.  It is like you are in a repository that still has an unmerged
> paths--you cannot proceed before you resolve them.
This is actually bringing some light to me: the round-trip test.
There are this "well known but less well document" situations where we 
break that rule:
- files are checked in with CRLF into the repo.
- .gittatributes is set to "text" later.
2 different ways to handle it:
- keep the eol at checkout, normalize at checkin -> roundtrip broken
- keep the eol at checkout and checkin -> roundtrip OK

- files with mixed line endings in the repo:
Same here: 2 different ways to handle it:
- keep the eol at checkout, normalize at checkin -> roundtrip broken
- keep the eol at checkout and checkin -> roundtrip OK

- files with CRCRLF line endings in the repo:
Same here: 2 different ways to handle it:
- keep the eol at checkout, normalize at checkin -> roundtrip broken
- keep the eol at checkout and checkin -> roundtrip OK

My feeling is that we should simply say:
You user set attribute to "text" and by doing that, you promised to have 
files
with LF only in the index.
If you break that promise, Git does  not know, what you really want.
- It may be a situation where you write a shell script which for some 
reasons
   needs a '\015' at the end of a line, and Git may treat it wrong by 
assuming
   that this is a CRLF line ending (end converts it into LF)
- It may be that you want CRLF because you added a Windows .BAT file.
   It may be that you use git.git and another implementation of Git, 
which doesn't
   support attributes at all, so that a save way to do this is to just 
commit CRLF.
- It may be that this is a historical issue.
   Everybody using the project uses git that understands .gitattributes,
   so someone may fix it some day.

Can Git make this decision ?

When core.autocrlf is true (and no attributes are set), then the 
conversion of line ending is disabled.
On 01/27/2016 08:05 PM, Junio C Hamano wrote:
(Changed the topic, 2 notes inside)
> Clemens Buchacher <drizzd@aon.at> writes:
>
>> Coming back to "[PATCH] optionally disable gitattributes": The topics
>> are related, because they both deal with the situation where the work
>> tree has files which are not normalized according to gitattributes. But
>> my patch is more about saying: ok, I know I may have files which need to
>> be normalized, but I want to ignore this issue for now. Please disable
>> gitattributes for now, because I want to work with the files as they are
>> committed. Conversely, the discussion here is about how to reliably
>> detect and fix files which are not normalized.
git ls-files --eol can detect that (as Junio pointed out)

> I primarily wanted to make sure that you understood the underlying
> issue, so that I do not have to go back to the basics in the other
> thread.  And it is clear that you obviously do, which is good.
>
> Here, you seem to think that what t0025 wants to see happen is
> sensible, judging by the fact that you call "rm .git/index && git
> reset" a "fix".
>
> My take on this is quite different.  After a "reset --hard HEAD", we
> should be able to trust the cached stat information and have "diff
> HEAD" say "no changes".  That is what you essentially want in the
> other thread, if I understand you correctly, and in an ideal world
> where the filesystem timestamp has infinite precision, that is what
> would happen in t0025, always "breaking" its expectation.  The real
> world has much coarser timestamp granularity than ideal, and that is
> why the test appear to be "flaky", failing to give "correct" outcome
> some of the time--but I'd say that it is expecting a wrong thing.
>
> An index entry that has data that does not round-trip when it goes
> through convert_to_working_tree() and then convert_to_git() "breaks"
> this arrangement, and I'd view it as the user having an inconsistent
> data.  It is like you are in a repository that still has an unmerged
> paths--you cannot proceed before you resolve them.
This is actually bringing some light to me: the round-trip test.
There are this "well known but less well document" situations where we 
break that rule:
- files are checked in with CRLF into the repo.
- .gittatributes is set to "text" later.
2 different ways to handle it:
- keep the eol at checkout, normalize at checkin -> roundtrip broken
- keep the eol at checkout and checkin -> roundtrip OK

- files with mixed line endings in the repo:
Same here: 2 different ways to handle it:
- keep the eol at checkout, normalize at checkin -> roundtrip broken
- keep the eol at checkout and checkin -> roundtrip OK

- files with CRCRLF line endings in the repo:
Same here: 2 different ways to handle it:
- keep the eol at checkout, normalize at checkin -> roundtrip broken
- keep the eol at checkout and checkin -> roundtrip OK

My feeling is that we should simply say:
You user set attribute to "text" and by doing that, you promised to have 
files
with LF only in the index.
If you break that promise, Git does  not know, what you really want.
- It may be a situation where you write a shell script which for some 
reasons
   needs a '\015' at the end of a line, and Git may treat it wrong by 
assuming
   that this is a CRLF line ending (end converts it into LF)
- It may be that you want CRLF because you added a Windows .BAT file.
   It may be that you use git.git and another implementation of Git, 
which doesn't
   support attributes at all, so that a save way to do this is to just 
commit CRLF.
- It may be that this is a historical issue.
   Everybody using the project uses git that understands .gitattributes,
   so someone may fix it some day.

Can Git make this decision ?

When core.autocrlf is true (and no attributes are set), then the 
conversion of line ending is disabled.
On 01/27/2016 08:05 PM, Junio C Hamano wrote:
(Changed the topic, 2 notes inside)
> Clemens Buchacher <drizzd@aon.at> writes:
>
>> Coming back to "[PATCH] optionally disable gitattributes": The topics
>> are related, because they both deal with the situation where the work
>> tree has files which are not normalized according to gitattributes. But
>> my patch is more about saying: ok, I know I may have files which need to
>> be normalized, but I want to ignore this issue for now. Please disable
>> gitattributes for now, because I want to work with the files as they are
>> committed. Conversely, the discussion here is about how to reliably
>> detect and fix files which are not normalized.
git ls-files --eol can detect that (as Junio pointed out)

> I primarily wanted to make sure that you understood the underlying
> issue, so that I do not have to go back to the basics in the other
> thread.  And it is clear that you obviously do, which is good.
>
> Here, you seem to think that what t0025 wants to see happen is
> sensible, judging by the fact that you call "rm .git/index && git
> reset" a "fix".
>
> My take on this is quite different.  After a "reset --hard HEAD", we
> should be able to trust the cached stat information and have "diff
> HEAD" say "no changes".  That is what you essentially want in the
> other thread, if I understand you correctly, and in an ideal world
> where the filesystem timestamp has infinite precision, that is what
> would happen in t0025, always "breaking" its expectation.  The real
> world has much coarser timestamp granularity than ideal, and that is
> why the test appear to be "flaky", failing to give "correct" outcome
> some of the time--but I'd say that it is expecting a wrong thing.
>
> An index entry that has data that does not round-trip when it goes
> through convert_to_working_tree() and then convert_to_git() "breaks"
> this arrangement, and I'd view it as the user having an inconsistent
> data.  It is like you are in a repository that still has an unmerged
> paths--you cannot proceed before you resolve them.
This is actually bringing some light to me: the round-trip test.
There are this "well known but less well document" situations where we 
break that rule:
- files are checked in with CRLF into the repo.
- .gittatributes is set to "text" later.
2 different ways to handle it:
- keep the eol at checkout, normalize at checkin -> roundtrip broken
- keep the eol at checkout and checkin -> roundtrip OK

- files with mixed line endings in the repo:
Same here: 2 different ways to handle it:
- keep the eol at checkout, normalize at checkin -> roundtrip broken
- keep the eol at checkout and checkin -> roundtrip OK

- files with CRCRLF line endings in the repo:
Same here: 2 different ways to handle it:
- keep the eol at checkout, normalize at checkin -> roundtrip broken
- keep the eol at checkout and checkin -> roundtrip OK

My feeling is that we should simply say:
You user set attribute to "text" and by doing that, you promised to have 
files
with LF only in the index.
If you break that promise, Git does  not know, what you really want.
- It may be a situation where you write a shell script which for some 
reasons
   needs a '\015' at the end of a line, and Git may treat it wrong by 
assuming
   that this is a CRLF line ending (end converts it into LF)
- It may be that you want CRLF because you added a Windows .BAT file.
   It may be that you use git.git and another implementation of Git, 
which doesn't
   support attributes at all, so that a save way to do this is to just 
commit CRLF.
- It may be that this is a historical issue.
   Everybody using the project uses git that understands .gitattributes,
   so someone may fix it some day.

Can Git make this decision ?

When core.autocrlf is true (and no attributes are set), then the 
conversion of line endings is disabled.
See convert.v "This is the new safer autocrlf handling",
commit fd6cce9e

So the round trip is achieved when core.autocrlf=true,
but no longer when attributes are added.
[]


> Anyway.
>
> As to your patch in the other thread, here is what I think:
>
>   (1) When you know (or perhaps your CI knows) that the working tree
>       has never been modified since you did "reset --hard HEAD" (or
>       its equivalent, like "git checkout $branch" from a clean
>       state), these paths with inconsistent data would break the
>       usual check to ask "is the working tree clean?"  That is a
>       problem and we need a way to ensure that the working tree is
>       always judged to be clean immediately after "reset --hard
>       HEAD".  IOW, I agree with you that the issue you are trying to
>       solve is worth solving.
>
>   (2) Regardless of the "inconsistent data breaking the cleanliness
>       check" issue, it may be handy to have a way to temporarily
>       disable the attributes, i.e. allow us to ask "what happens if
>       there is no attributes defined?"  IOW, I am saying that the
>       change in the patch is not without merit.
>
> In addition to (1), I further think that this sequence should not
> report that the path F is modified:
>
>       # Write F from HEAD to the working tree, after passing it
>       # through convert_to_working_tree()
>       $ git reset --hard HEAD
>
>       # Force the re-reading, without changing the contents at all
>       $ cp F F.new
>       $ mv F.new F
>
>       $ git diff HEAD
>
> which is broken by paths with inconsistent data.  Your CI would want
> a way to make that happen.
>
> However, I do not think disabling attributes (i.e. (2)) is a
> solution to the issue (i.e. (1)), which we just agreed to be an
> issue that is worth solving, for at least two reasons.
>
>   * Even without any attributes, core.autocrlf setting can get the
>     data in your index (whose lines can be terminated with CRLF) into
>     the same "inconsistent data" situation.  Disabling attribute
>     handling would not have any effect on that codepath, I think.
>
I don't think so, see above.

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Clemens Buchacher]

On Wed, Jan 27, 2016 at 12:49:31PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I wonder what would break if we ask this question instead:
> >
> >     We do not know if the working tree file and the indexed data
> >     match.  Let's see if "git checkout" of that path would leave the
> >     same data as what currently is in the working tree file.

If we do this, then git diff should show the diff between
convert_to_worktree(index state) and the worktree state. That would be
nice, because the diff would actually show what we have in the worktree.
It keeps confusing me that with eol conversion enabled, git diff does
not actually show me the worktree state.

However, even if the file is clean in that direction, there could be a
mismatch between convert_to_git(worktree state) and the index state.
This will happen for example in t0025.4, where we have a CRLF file in
the index and the worktree, but convert_to_git converts it to a file
with LF line endings. Still, I do not see a problem if we also provide a
command like git add --fix-index, which will force normalization of all
files.

> > If we did this, "reset --hard HEAD" followed by "diff HEAD" will by
> > definition always report "is clean" as long as nobody changes files
> > in the working tree, even with the inconsistent data in the index.

Yes, this is a more elegant and a more complete solution to the problem
which prompted me to submit the GIT_ATTRIBUTES_DISABLED patch.

> > This still requires that convert_to_working_tree(), i.e. your smudge
> > filter, is deterministic, though, but I think that is a sensible
> > assumption for sane people, even for those with inconsistent data in
> > the index.

Deterministic, yes. But not unchanging. When a smudge filter is added,
or modified, or if the filter program changes, we still have to remove
the index before we can trust git diff again. The only way to avoid this
would be to somehow detect if the conversion itself changes. One could
hash the attributes, but changes to the filter configuration or the
filter itself are hard to detect. So I think we have to live with this.

> [...] Doing the other check will have to
> inflate the blob data and apply the convert_to_working_tree()
> processing, and also read the whole thing from the filesystem and
> compare, which is more work at runtime.

If we assume that the smudge filter is deterministic, then we could also
hash the output of convert_to_working_tree, and store the hash in the
index. With this optimization, the comparision would be less work,
because we do not have to apply a filter again, whereas currently we
have to apply convert_to_git.

> IOW, I am saying that the "add --fix-index" lunchbreak patch I sent
> earlier in the thread that has to hold the data in-core while
> processing is not a production quality patch ;-)

Ok. The existing implementation in renormalize_buffer (convert.c) works
for me, though.

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> On Wed, Jan 27, 2016 at 12:49:31PM -0800, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>> > I wonder what would break if we ask this question instead:
>> >
>> >     We do not know if the working tree file and the indexed data
>> >     match.  Let's see if "git checkout" of that path would leave the
>> >     same data as what currently is in the working tree file.
>
> If we do this, then git diff should show the diff between
> convert_to_worktree(index state) and the worktree state.

I agree with you that, when ce_compare_data(), i.e. "does the index
match the working tree?", says "they match", "git diff" (show me the
change to go from the index to the worknig tree) should show empty
to be consistent, and for that to happen under the above definition
of ce_compare_data(), "git diff" needs to be comparing the data in
the index after converting it to the working tree representation
with the data in the working tree.

And that unfortunately is a very good reason why this approach
should not be taken.  "git diff" (show me the change to go from the
index to the working tree) is a preview of what we would see in "git
diff --cached" (show me the change to go from HEAD to the index) if
we did "git add", and it is a preview of what we would see in "git
show" (show me the change of what the last commit did) if we did
"git commit -a".  It is crazy for these latter comparisons to happen
in the working tree (aka "smudged") representation of the data, IOW,
these two must compare the "clean" representation.  It also is crazy
for "git diff" to be using different representation from these two.
This alone makes the above idea a non-starter X-<.

Besides, I do not think the above approach really solves the issue,
either.  After "git reset --hard" to have the contents in the index
dumped to the working tree, if your core.autocrlf is flipped, "git
checkout" of the same path would result in a working tree
representation of the data that is different from what you have in
the working tree, so we would declare that the working tree is not
clean, even though nobody actually touched them in the meantime.
This is less of an issue than having data in the index that is
inconsistent with the convert_to_git() setting (i.e. eol and clean
filter conversion that happens when you "git add"), but it still is
fundamentally the same issue.

Oh, bummer, I thought it was a nice approach.

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Clemens Buchacher]

On Thu, Jan 28, 2016 at 01:32:30PM -0800, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
> 
> > If we do this, then git diff should show the diff between
> > convert_to_worktree(index state) and the worktree state.
> 
> And that unfortunately is a very good reason why this approach
> should not be taken.

Ok, then let's take a step back. I do not actually care if git diff and
friends say the worktree is clean or not. But I know that I did not make
any modifications to the worktree, because I just did git reset --hard.
And now I want to use commands like cherry-pick and checkout without
failure. But they can fail, because they essentially use git diff to
check if there are worktree changes, and if so refuse to overwrite them.

So, if the check "Am I allowed to modify the worktree file?", would go
the extra mile to also check if the worktree is clean in the sense that
convert_to_worktree(index state) matches the worktree. If this is the
case, then it is safe to modify the file because it is the committed
state, and can be recovered.

Regarding performance impact: We only need to do this extra check if the
usual check convert_to_git(work tree) against index state fails, and
conversion is in effect.

> Besides, I do not think the above approach really solves the issue,
> either.  After "git reset --hard" to have the contents in the index
> dumped to the working tree, if your core.autocrlf is flipped,

Indeed, if the user configuration changes, then we cannot always detect
this (at least if the filter is an external program, and the behavior of
that changes). But the user is in control of that, and we can document
this limitation.

On the other hand, a user who simply follows an upstream repository by
doing git pull all the time, and who does not make changes to their
configuration, can still run into this issue, because upstream could
change .gitattributes. This part we could actually detect by hashing the
attributes for each index entry, and if that changes we re-evaluate the
file state.

This is also an issue only if a smudge filter is in place. The eol
conversion which only acts in the convert_to_git direction is not
affected.

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> Ok, then let's take a step back. I do not actually care if git diff and
> friends say the worktree is clean or not.

You may not, but many existing scripts people have do.

> But I know that I did not make
> any modifications to the worktree, because I just did git reset --hard.
> And now I want to use commands like cherry-pick and checkout without
> failure. But they can fail, because they essentially use git diff to
> check if there are worktree changes, and if so refuse to overwrite them.

Yes, exactly.

> So, if the check "Am I allowed to modify the worktree file?", would go
> the extra mile to also check if the worktree is clean in the sense that
> convert_to_worktree(index state) matches the worktree. If this is the
> case, then it is safe to modify the file because it is the committed
> state, and can be recovered.

So in essense, the proposed "fix" is "let's fix it in the right
way"?

The way we defined "would we lose some changes that are only in the
working tree?", aka "is the working tree dirty wrt the index?", has
been to check if "git add -u" would change the states in the index.
And for scripted Porcelains and end-user scripts, "git diff-files",
aka "what change would 'git add -u' make to the states in the
index?", has been the command to do the same check.

Your proposal is to redefine "is the working tree dirty?"; it would
check if "git checkout -f" would change what is in the working tree.

I agree that indeed is "would we lose some changes that are only in
the working tree", and I think we can do that transparently for
"internal" commands, i.e. without any end-user impact, as the new
check would behave identically when they have sane contents--the
difference between the current check and the new check only exists
when the contents in the index contradicts what the user specifies
for to-git conversion via eol or clean filter.

We would need a way for our scripted Porcelains and end-user scripts
to ask that new question, though, but I think that is not something
insurmountable.  A new option to "diff-files" or something, perhaps,
would be workable, but having a new "git require-clean-work-tree"
plumbing, which would replace require_clean_work_tree shell helper
in git-sh-setup, may be conceptually much cleaner, because the new
definition of "working tree being clean" is no longer tied to what
"diff" should say.

I like that as a general direction.

> Regarding performance impact: We only need to do this extra check if the
> usual check convert_to_git(work tree) against index state fails, and
> conversion is in effect.

How would you detect the failure, though?  Having contents in the
index that contradicts the attributes and eol settings affects the
cleanliness both ways.  Running the working tree contents via to-git
conversion and hashing may match the blob in the index, declaring
that the index entry is "clean", but passing the blob to to-worktree
conversion may produce result different from what is in the
worktree, which is "falsely clean".  That is an equally important
case that is opposite from what we have been primarily discussing,
which is "falsely dirty".

>> Besides, I do not think the above approach really solves the issue,
>> either.  After "git reset --hard" to have the contents in the index
>> dumped to the working tree, if your core.autocrlf is flipped,
>
> Indeed, if the user configuration changes, then we cannot always detect
> this (at least if the filter is an external program, and the behavior of
> that changes). But the user is in control of that, and we can document
> this limitation.

That argument does not result in a very useful result, though.
Because the user is in control of what attributes and eol settings
are in effect in her repository, we can just document that the
current check will give unspecified result if the indexed contents
contradict with that setting, e.g. when you have CRLF encoded data
in the index but the eol conversion assumes LF in the repository.
But this discussion is an attempt to do better than that, no?

> On the other hand, a user who simply follows an upstream repository by
> doing git pull all the time, and who does not make changes to their
> configuration, can still run into this issue, because upstream could
> change .gitattributes. This part we could actually detect by hashing the
> attributes for each index entry, and if that changes we re-evaluate the
> file state.

If this has to bloat each index entry, I do not think solving the
problem is worth that cost of that overhead.  I'd rather just say
"if you have inconsistent data, here is a workaround using 'reset'
and then 'reset --hard'" and be done with it.

> This is also an issue only if a smudge filter is in place. The eol
> conversion which only acts in the convert_to_git direction is not
> affected.

IIRC, autocrlf=true would strip CR at the end of line in to-git
conversion, and would add CR in to-worktree conversion.  So some eol
conversion may only act in to-git, but some others do affect both,
and without needing you to touch attributes.

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Clemens Buchacher]

On Mon, Feb 01, 2016 at 10:17:24AM -0800, Junio C Hamano wrote:
> 
> Your proposal is to redefine "is the working tree dirty?"; it would
> check if "git checkout -f" would change what is in the working tree.

I like this definition. Sounds obviously right.

> > Regarding performance impact: We only need to do this extra check if the
> > usual check convert_to_git(work tree) against index state fails, and
> > conversion is in effect.
> 
> How would you detect the failure, though?  Having contents in the
> index that contradicts the attributes and eol settings affects the
> cleanliness both ways.  Running the working tree contents via to-git
> conversion and hashing may match the blob in the index, declaring
> that the index entry is "clean", but passing the blob to to-worktree
> conversion may produce result different from what is in the
> worktree, which is "falsely clean".

True. But this is what we do today, and I thought at first that we have
to keep this behavior. The following enables eol conversion on git add,
but not on checkout:

 printf 'line 1\r\n' >dos.txt
 echo '* text' >.gitattributes
 git add dos.txt
 git commit

After git add the worktree is considered clean, even though dos.txt
still has CRLF line endings, and rm dos.txt && git checkout dos.txt
re-creates dos.txt with LF line endings. If we change the definition as
proposed above, then the worktree would be dirty even though we just
did git add and git commit.

So I concluded that we have to treat the worktree clean if either git
add -u does not change the index state, _or_ git checkout -f does not
change the worktree state.

But doing only the git checkout -f check makes much more sense. Maybe we
can handle the above situation better by doing an implicit
git checkout -f <committed files> after git commit. After all, I would
expect git commit to give me exactly the same state that I get later
when I do git checkout <commit> for the same commit.

> > On the other hand, a user who simply follows an upstream repository by
> > doing git pull all the time, and who does not make changes to their
> > configuration, can still run into this issue, because upstream could
> > change .gitattributes. This part we could actually detect by hashing the
> > attributes for each index entry, and if that changes we re-evaluate the
> > file state.
> 
> If this has to bloat each index entry, I do not think solving the
> problem is worth that cost of that overhead.  I'd rather just say
> "if you have inconsistent data, here is a workaround using 'reset'
> and then 'reset --hard'" and be done with it.

Works for me.

> > This is also an issue only if a smudge filter is in place. The eol
> > conversion which only acts in the convert_to_git direction is not
> > affected.
> 
> IIRC, autocrlf=true would strip CR at the end of line in to-git
> conversion, and would add CR in to-worktree conversion.  So some eol
> conversion may only act in to-git, but some others do affect both,
> and without needing you to touch attributes.

I was somehow under the impression that autocrlf=true is discouraged,
and setting the text attribute to true is the new recommended way to
configure eol conversion. But I see that the Git for Windows installer
still offers autocrlf=true as the default option, so clearly we need to
support it well.

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Torsten Bögershausen]

On 2016-02-01 19.17, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
[]
> 
> IIRC, autocrlf=true would strip CR at the end of line in to-git
> conversion, and would add CR in to-worktree conversion.  So some eol
> conversion may only act in to-git, but some others do affect both,
> and without needing you to touch attributes.
That depends, which version of Git you are running.
It has changed from the first version of core.autocrlf:

commit c4805393d73425a5f467f10fa434fb99bfba17ac
Author: Finn Arne Gangstad <finnag@pvv.org>
Date:   Wed May 12 00:37:57 2010 +0200

    autocrlf: Make it work also for un-normalized repositories

    Previously, autocrlf would only work well for normalized
    repositories. Any text files that contained CRLF in the repository
    would cause problems, and would be modified when handled with
    core.autocrlf set.

    Change autocrlf to not do any conversions to files that in the
    repository already contain a CR. git with autocrlf set will never
    create such a file, or change a LF only file to contain CRs, so the
    (new) assumption is that if a file contains a CR, it is intentional,
    and autocrlf should not change that.

    The following sequence should now always be a NOP even with autocrlf
    set (assuming a clean working directory):

    git checkout <something>
    touch *
    git add -A .    (will add nothing)
    git commit      (nothing to commit)

    Previously this would break for any text file containing a CR.

    Some of you may have been folowing Eyvind's excellent thread about
    trying to make end-of-line translation in git a bit smoother.

    I decided to attack the problem from a different angle: Is it possible
    to make autocrlf behave non-destructively for all the previous problem cases?

    Stealing the problem from Eyvind's initial mail (paraphrased and
    summarized a bit):

    1. Setting autocrlf globally is a pain since autocrlf does not work well
       with CRLF in the repo
    2. Setting it in individual repos is hard since you do it "too late"
       (the clone will get it wrong)
    3. If someone checks in a file with CRLF later, you get into problems again
    4. If a repository once has contained CRLF, you can't tell autocrlf
       at which commit everything is sane again
    5. autocrlf does needless work if you know that all your users want
       the same EOL style.

    I belive that this patch makes autocrlf a safe (and good) default
    setting for Windows, and this solves problems 1-4 (it solves 2 by being
    set by default, which is early enough for clone).

    I implemented it by looking for CR charactes in the index, and
    aborting any conversion attempt if this is found.
-----------------------
And my intention is to do a similar fix for the attributes.
More patches coming.

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> On Mon, Feb 01, 2016 at 10:17:24AM -0800, Junio C Hamano wrote:
>> 
>> Your proposal is to redefine "is the working tree dirty?"; it would
>> check if "git checkout -f" would change what is in the working tree.
>
> I like this definition. Sounds obviously right.

So this is an illustration.  The change to ce_compare_data() has a
room for improvement in that it assumes that it can always slurp the
whole blob in-core; it should try to use the streaming interface
when it makes sense.  Otherwise we would not be able to handle a
blob that we used to be able to (as index_fd() streams), which would
be a regression.

The change to t0023 is merely an example that shows that existing
tests assume the convert_to_git() way of defining the dirtyness of
the working tree.  It used to be OK to have core.autocrlf set to true,
have LF terminated file on the working tree and add it to the index,
and the resulting state was "We just added it to the index, and
nobody touched the index nor the working tree file--by definition
the working tree IS CLEAN".  With your updated semantics, that no
longer is true.  "We just added it, but if we check it out, we would
normalize the line ending to be CRLF on the working tree, so the
working tree is dirty" is what happens.

There are tons of tests that would break the same way all of which
needs to be looked at and fixed if we were to go in this direction.


 read-cache.c       | 53 +++++++++++++++++++++++++++++++++++++++++++++++++----
 t/t0023-crlf-am.sh |  2 +-
 2 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 84616c8..c284f78 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -156,16 +156,61 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
 		ce_mark_uptodate(ce);
 }
 
+/*
+ * Compare the data in buf with the data in the file pointed by fd and
+ * return 0 if they are identical, and non-zero if they differ.
+ */
+static int compare_with_fd(const char *input, ssize_t len, int fd)
+{
+	for (;;) {
+		char buf[1024 * 16];
+		ssize_t chunk_len, read_len;
+
+		chunk_len = sizeof(buf) < len ? sizeof(buf) : len;
+		read_len = xread(fd, buf, chunk_len ? chunk_len : 1);
+
+		if (!read_len)
+			/* EOF on the working tree file */
+			return !len ? 0 : -1;
+
+		if (!len)
+			/* we expected there is nothing left */
+			return -1;
+
+		if (memcmp(buf, input, read_len))
+			return -1;
+		input += read_len;
+		len -= read_len;
+	}
+}
+
+/*
+ * Does the file in the working tree match what is in the index?
+ * That is, do we lose any data from the working tree copy if we
+ * did a new "git checkout" of that path out of the index?
+ */
 static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
 {
 	int match = -1;
 	int fd = open(ce->name, O_RDONLY);
 
 	if (fd >= 0) {
-		unsigned char sha1[20];
-		if (!index_fd(sha1, fd, st, OBJ_BLOB, ce->name, 0))
-			match = hashcmp(sha1, ce->sha1);
-		/* index_fd() closed the file descriptor already */
+		enum object_type type;
+		unsigned long size;
+		void *data = read_sha1_file(ce->sha1, &type, &size);
+
+		if (type == OBJ_BLOB) {
+			struct strbuf worktree = STRBUF_INIT;
+			if (convert_to_working_tree(ce->name, data, size,
+						    &worktree)) {
+				free(data);
+				data = strbuf_detach(&worktree, &size);
+			}
+			if (!compare_with_fd(data, size, fd))
+				match = 0;
+		}
+		free(data);
+		close(fd);
 	}
 	return match;
 }
diff --git a/t/t0023-crlf-am.sh b/t/t0023-crlf-am.sh
index f9bbb91..5c086b4 100755
--- a/t/t0023-crlf-am.sh
+++ b/t/t0023-crlf-am.sh
@@ -27,7 +27,7 @@ EOF
 test_expect_success 'setup' '
 
 	git config core.autocrlf true &&
-	echo foo >bar &&
+	printf "%s\r\n" foo >bar &&
 	git add bar &&
 	test_tick &&
 	git commit -m initial

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> The change to t0023 is merely an example that shows that existing
> tests assume the convert_to_git() way of defining the dirtyness of
> the working tree.  It used to be OK to have core.autocrlf set to true,
> have LF terminated file on the working tree and add it to the index,
> and the resulting state was "We just added it to the index, and
> nobody touched the index nor the working tree file--by definition
> the working tree IS CLEAN".  With your updated semantics, that no
> longer is true.  "We just added it, but if we check it out, we would
> normalize the line ending to be CRLF on the working tree, so the
> working tree is dirty" is what happens.
>
> There are tons of tests that would break the same way all of which
> needs to be looked at and fixed if we were to go in this direction.

That made me think further aloud.  I haven't thought things through,
but I wonder what happens if we do both.  That is, we define the
working tree file is clean if either:

  * the result of running convert_to_git() on the working tree
    contents matches what is in the index (because that would mean
    doing another "git add" on the path is a no-op); OR

  * the result of running convert_to_working_tree() on the content
    in the index matches what is in the working tree (because that
    would mean doing another "git checkout -f" on the path is a
    no-op).

A possible downside (but again, I haven't thought things through, so
this may be a non-issue) of doing this is that it may make it even
harder to "fix" an index entry or a working tree file that is
inconsistent with the user's conversion settings.  Even when "git
add" would allow the user to fix an index entry by applying (an
updated) convert_to_git() filter to the working tree file, because
of the new rule that works in the opposite direction, we would end
up saying "the working tree file is clean, and there is no point
doing 'git add'".  And vice versa for fixing a working tree file by
running "git checkout".

Also this will make "update-index --refresh" potentially take twice
as long for paths that are not known to be clean and indeed dirty,
as they would need to be processed twice.

An updated patch to do so would look like this.  At least we don't
have to update the expectation t0023 makes with this approach.

 read-cache.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/read-cache.c b/read-cache.c
index 84616c8..42d9452 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -156,17 +156,78 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
 		ce_mark_uptodate(ce);
 }
 
+/*
+ * Compare the data in buf with the data in the file pointed by fd and
+ * return 0 if they are identical, and non-zero if they differ.
+ */
+static int compare_with_fd(const char *input, ssize_t len, int fd)
+{
+	for (;;) {
+		char buf[1024 * 16];
+		ssize_t chunk_len, read_len;
+
+		chunk_len = sizeof(buf) < len ? sizeof(buf) : len;
+		read_len = xread(fd, buf, chunk_len ? chunk_len : 1);
+
+		if (!read_len)
+			/* EOF on the working tree file */
+			return !len ? 0 : -1;
+
+		if (!len)
+			/* we expected there is nothing left */
+			return -1;
+
+		if (memcmp(buf, input, read_len))
+			return -1;
+		input += read_len;
+		len -= read_len;
+	}
+}
+
+/*
+ * Does the file in the working tree match what is in the index?
+ */
 static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
 {
 	int match = -1;
 	int fd = open(ce->name, O_RDONLY);
 
+	/*
+	 * Would another "git add" on the path change what is in the
+	 * index for the path?
+	 */
 	if (fd >= 0) {
 		unsigned char sha1[20];
 		if (!index_fd(sha1, fd, st, OBJ_BLOB, ce->name, 0))
 			match = hashcmp(sha1, ce->sha1);
 		/* index_fd() closed the file descriptor already */
 	}
+	if (!match)
+		return match;
+
+	/*
+	 * Would another "git checkout -f" out of the index change
+	 * what is in the working tree file?
+	 */
+	fd = open(ce->name, O_RDONLY);
+	if (fd >= 0) {
+		enum object_type type;
+		unsigned long size;
+		void *data = read_sha1_file(ce->sha1, &type, &size);
+
+		if (type == OBJ_BLOB) {
+			struct strbuf worktree = STRBUF_INIT;
+			if (convert_to_working_tree(ce->name, data, size,
+						    &worktree)) {
+				free(data);
+				data = strbuf_detach(&worktree, &size);
+			}
+			if (!compare_with_fd(data, size, fd))
+				match = 0;
+		}
+		free(data);
+		close(fd);
+	}
 	return match;
 }