Re: `make profile-install` fails in 2.9.3

From: Jeff King <peff@peff.net>
Date: Sep 1, 2016, 21:58 UTC
Message-ID: <20160901215810.ez47lqwmfmahyvc7@sigill.intra.peff.net>
In-Reply-To: <20160901200700.GA8254@hank>

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

Show 5 quoted lines

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

Show 8 quoted lines

> 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

Show 5 quoted lines

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

-Peff