On Thu, Sep 01, 2016 at 09:07:00PM +0100, Thomas Gummerer wrote:

> > Related problem: `t3700-add.sh` currently fails in 2.9.3. I can
> > provide more debug information if you don't already know this problem.
> I noticed this problem as well, when I'm compiling with USE_NSEC = 1
> in my config.mak.

I can replicate this even without USE_NSEC with my stress-tester[1].
That makes sense why it would show up with the profiling run; git runs
slower and therefore increases the chances of crossing the 1-second
boundary and losing the race.

[1] https://github.com/peff/git/blob/meta/stress

> Tracking this problem down a bit, it happens because the --chmod=[+-]x
> option introduced in 4e55ed32 ("add: add --chmod=+x / --chmod=-x
> options") only works if the file on disk is modified.  When the test
> was changed to work on one single file, instead of doing chmod=+x on
> one file and chmod=-x on another file in b38ab197c ("t3700: merge two
> tests into one"), this test started breaking when the mtime of the
> file and the index file weren't the same (in other words, if the file
> was not racily clean and thus was not smudged).

That certainly sounds buggy. A less racy way of verifying this is just:

  # guarantee not-racy state
  echo content >file
  test-chmtime -60 file
  git add file

  # now check --chmod; file will still be 100644!
  git add --chmod=+x file
  git ls-files -s

> One possible fix for the test is to smudge the entry as showed below,
> though I'm not sure it's the right fix.  The other way I can think of
> is to change the file in the index regardless of whether the file was
> changed in some other way before issuing the git add command, as that
> might fit the user expectation better.  Thoughts?

Yeah, I think we should _always_ act on the --chmod, no matter if the
file is racy or not, or whether it has a content change or not. I.e.,
the race is not the problem, but rather the behavior of 4e55ed32. Your
second proposal there sounds more like the right approach.


Reply via email to