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.
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.
> 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.