Re: [PATCH] t0021: make sure clean filter runs

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Wed, Aug 21, 2019 at 08:23:23PM +0200, Johannes Sixt wrote:
> Am 21.08.19 um 16:56 schrieb Thomas Gummerer:
> > On 08/20, Johannes Sixt wrote:
> >> Am 20.08.19 um 08:56 schrieb Thomas Gummerer:
> >>> Fix the test by updating the mtime of test.r, ...
> >>
> >>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> >>> index e10f5f787f..66f75005d5 100755
> >>> --- a/t/t0021-conversion.sh
> >>> +++ b/t/t0021-conversion.sh
> >>> @@ -390,6 +390,7 @@ test_expect_success PERL 'required process filter should filter data' '
> >>>  		EOF
> >>>  		test_cmp_exclude_clean expected.log debug.log &&
> >>>  
> >>> +		touch test.r &&
> >>
> >> 		test-tool chmtime +10 test.r
> >>
> >> would be more reliable.
> > 
> > Hmm, is touch unreliable on some platforms?  I didn't think of
> > 'test-tool chmtime', but I'm also not sure it's better than touch in
> > this case.
> > 
> > To me te 'touch' signifies that the timestamp must be updated after
> > the previous checkout, so git thinks it could possibly have been
> > changed, which I think is clearer in this case than setting the mtime
> > to a future time.
> 
> touch does not guarantee that the current time is different from the
> timestamp that the file already carries, particularly not when the
> filesystem stores just a resolution of 1 second, and commands are
> executed quickly.

This 'touch' must ensure that the timestamp of the file is not older
than the timestamp of the index, and to achive that it doesn't
necessarily have to modify the timestamp.

The file is modified first, then the index is updated, and finally
comes this 'touch'.  Consequently, if 'touch' doesn't modify the
timestamp of the file, then it must have the same timestamp as the
index, IOW it's racily clean, and the subsequent 'git checkout' has to
look at the file content and has to run the filter, and that's what we
want to see here.

However, I'm not sure what would happens if the system clock were to
jump back in between, but since it's only a test I don't think it's
worth caring about.

> But when we use test-tool chmtime +10, then the timestamp is definitely
> different.

'test-tool chmtime +10' adjusts the timestamp of the file relative to
its current timestamp.  So yeah, the file's timestamp definitely
changes, but that's not enough, because it doesn't ensure that the new
timestamp is not older than the timestamp of the index.  Just imagine
the arguably pathological situation that right after the file was last
modified the system miraculously comes to a complete stall, and only
manages to resume after 15 seconds to continue with updating the
index.  This means that the timestamp of the file will be 15s older
than the index, and after that 'chmtime +10' it will still be 5s
older.  Consequently, 'git checkout' will think that the file is
clean, it won't run the filter that we expect, and the test will fail.
So instead of '+10' it should be '=+10' to set the new timestamp
relative to the current time, but I'm not too keen about the timestamp
in the future either (though the file is about to be deleted anyway).

I think it would be best to explicitly set the timestamp of the file
and the index sort-of relative to each other and add an in-code
comment as well, e.g.:

  # Make sure that the file appears dirty, so checkout below has to
  # run the configured filter.
  test-tool chmtime =-10 .git/index &&
  test-tool chmtime =+0 test.r &&