Re: [PATCH v3 0/9] Fix the early config

From: Jeff King <peff@peff.net>
Date: Mar 8, 2017, 07:30 UTC
Message-ID: <20170308073035.pt5ysp6bzpozoqps@sigill.intra.peff.net>
In-Reply-To: <alpine.DEB.2.20.1703071314180.3767@virtualbox>

On Tue, Mar 07, 2017 at 03:31:43PM +0100, Johannes Schindelin wrote:

Show 11 quoted lines

> >   /*
> >    * Find GIT_DIR of the repository that contains the current working
> >    * directory, without changing the working directory or other global
> >    * state. The result is appended to gitdir. The return value is NULL
> >    * if no repository was found, or gitdir->buf otherwise.
> >    */
> 
> I changed it a little bit more. In particular, I changed the
> discover_git_directory() function to return the pointer to the path
> itself: it provides additional value, and if that is not what the caller
> wants, they can use git_dir->buf just as well.

Yes, that makes much more sense.

Show 7 quoted lines

> There is one more thing I included in v4: when I (re-)implemented that
> pre-command/post-command hook I was hinting at earlier, the test suite
> identified a problem where an invalid .git file would prevent even `git
> init` from working (it was actually much more complicated than that, but
> the gist is that `git -p init` would fail, no matter how much sense it
> may make to you to paginate an `init` run, it should still not fail,
> right?). So I added a patch on top to fix that.

Good catch. Another "non-gentle" thing I noticed here while looking at another thread: the repository-format version check uses the config parser, which will die() in certain circumstances. So for instance:

  $ git init
  $ git rev-parse && echo ok
  ok

  $ echo '[core]repositoryformatversion = 10' >.git/config
  $ git rev-parse && echo ok
  fatal: Expected git repo version <= 1, found 10

  $ echo '[core]repositoryformatversion = foobar' >.git/config
  $ git rev-parse && echo ok
  fatal: bad numeric config value 'foobar' for 'core.repositoryformatversion' in file .git/config: invalid unit

  $ echo '[co' >.git/config
  $ git rev-parse && echo ok
  fatal: bad config line 1 in file .git/config

Your series correctly avoids the first failure by calling the read/verify_repository_format functions correctly. But I think it would get tripped up by the other two.

Fixing the first one is probably not too hard; check_repo_format() should use a more forgiving parser than git_config_int().

The second one I thought would be tricky, but it looks like we added a die_on_error flag in b2dc09455. That does what we want, but it needs to be plumbed through to git_config_from_file().

> And another change: the GIT_DIR_NONE value was handled incorrectly in
> discover_git_directory().

This is the "if (setup_git_directory_1() <= 0)" change from the interdiff? That's subtle. The compiler could have noticed if we used a switch statement here. But then any new error conditions would have to be added to that switch statement.

> I am slightly disappointed that the these additional problems were not
> spotted in any review but my own. And I had not even included a Duck.

Get used to being disappointed, I guess. A non-zero number of bugs will slip through when writing code _and_ when reviewing it.

> [ceil_offset]
> Hopefully that clears up the picture?

Yes, it does. Thanks.

-Peff