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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Peff,

On Sat, 4 Mar 2017, Jeff King wrote:

> On Fri, Mar 03, 2017 at 06:31:55PM +0100, Johannes Schindelin wrote:
> 
> > Interdiff vs v2:
> > [...]
> >  +	 * When we are not about to create a repository ourselves (init or
> >  +	 * clone) and when no .git/ directory was set up yet (in which case
> >  +	 * git_config_with_options() would already have picked up the
> >  +	 * repository config), we ask discover_git_directory() to figure out
> >  +	 * whether there is any repository config we should use (but unlike
> >  +	 * setup_git_directory_gently(), no global state is changed, most
> >  +	 * notably, the current working directory is still the same after
> >  +	 * the call).
> >   	 */
> >  -	if (!startup_info->creating_repository && !have_git_dir() &&
> >  -	    discover_git_directory(&buf)) {
> >  +	if (!have_git_dir() && discover_git_directory(&buf)) {
> 
> I think this "when we are not about to..." part of the comment is no
> longer true, given the second part of the hunk.

Yep, that was a stale part of that patch. Thanks for noticing!

> >  @@ -721,8 +721,10 @@ static const char *setup_discovered_git_dir(const char *gitdir,
> >   	if (offset == cwd->len)
> >   		return NULL;
> >   
> >  -	/* Make "offset" point to past the '/', and add a '/' at the end */
> >  -	offset++;
> >  +	/* Make "offset" point past the '/' (already the case for root dirs) */
> >  +	if (offset != offset_1st_component(cwd->buf))
> >  +		offset++;
> 
> Nice. I was worried we would have to have a hacky "well, sometimes we
> don't add one here..." code, but using offset_1st_component says
> exactly what we mean.

Right. I also wanted to avoid that very, very much. My initial version
actually tried to detect whether cwd already has a trailing slash, but
then I figured that we can be much, much more precise here (and I am
really pleased how offset_1st_component() is *semantically* precise, i.e.
it describes very well what the code is supposed to do here).

> > +/* Find GIT_DIR without changing the working directory or other global state */
> >  extern const char *discover_git_directory(struct strbuf *gitdir);
> 
> The parts that actually confused me were the parameters (mostly whether
> gitdir was a directory to start looking in, or an output parameter). So
> maybe:
> 
>   /*
>    * 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.

> This looks good to me aside from those few comment nits.

Thanks.

It is not obvious from the interdiff, but I had an incorrect fixup to 8/9
that actually wanted to go to 5/9: the code in
discover_git_repository() tests the repository version should be part of
the initial version of this function, of course.

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.

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

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.

> I'm still not sure I understand how ceil_offset works in
> setup_git_directory_gently_1(), but I don't think your patch actually
> changed it. I can live with my confusion.

Yes, that code is very confusing. It also does not help that the naming is
inconsistent in that it abbreviates "ceiling" but not "offset". What makes
it even worse is that the function name `longest_ancestor_length()` is
highly misleading: in Git's context, "ancestor" of sth pretty much always
refers to a commit reachable from sth, but in this context it refers to
the path of a directory containing sth.

So basically, we set ceil_offset to the offset of the last directory
separator in our path that corresponds to the most precise match in
GIT_CEILING_DIRECTORIES.

Example: given GIT_CEILING_DIRECTORIES /foo:/:/bar and a path of /foo/bar,
ceil_offset would be 4, pointing to the slash at the end of /foo/ because
that is the most precise match in GIT_CEILING_DIRECTORIES ("/" would also
match, but is less precise).

Later, setup_git_directory_gently_1() ensures that it does not go beyond
ceil_offset when looking for the parent directory as the next candidate to
test for .git/.

Hopefully that clears up the picture?

Ciao,
Dscho