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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 7, 2017, 14:31 UTC
Message-ID: <alpine.DEB.2.20.1703071314180.3767@virtualbox>
In-Reply-To: <20170304073952.drfgy7jacnlm6tum@sigill.intra.peff.net>

Hi Peff,

On Sat, 4 Mar 2017, Jeff King wrote:

Show 19 quoted lines

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

Show 13 quoted lines

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

Show 13 quoted lines

> > +/* 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