On Tue, Mar 07, 2017 at 03:31:43PM +0100, Johannes Schindelin wrote: > > /* > > * 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. > 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
