Re: [PATCH v2 3/9] setup_git_directory(): avoid changing global state during discovery

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

Hi Peff,

On Thu, 2 Mar 2017, Jeff King wrote:

> On Fri, Mar 03, 2017 at 03:04:11AM +0100, Johannes Schindelin wrote:
> 
> > For historical reasons, Git searches for the .git/ directory (or the
> > .git file) by changing the working directory successively to the
> > parent directory of the current directory, until either anything was
> > found or until a ceiling or a mount point is hit.
> 
> This is starting to get into the meat of changes we've been putting off
> writing for ages. I'll read with my fingers crossed. :)

Heh.

> > Further global state may be changed, depending on the actual type of
> > discovery, e.g. the global variable
> > `repository_format_precious_objects` is set in the
> > `check_repository_format_gently()` function (which is a bit
> > surprising, given the function name).
> 
> It's gentle in the sense that if it does not find a valid repo, it
> touches no state. I do suspect the functions you want are the ones it
> builds on: read_repository_format() and verify_repository_format(),
> which I added not too long ago for the exact purpose of checking repo
> config without touching anything global.

Okay. But I think that my interpretation of the word "gently" is as valid
as Git's source code's.

> > We do have a use case where we would like to find the .git/ directory
> > without having any global state touched, though: when we read the early
> > config e.g. for the pager or for alias expansion.
> > 
> > Let's just rename the function `setup_git_directory_gently_1()` to
> > `discover_git_directory()` and move all code that changes any global
> > state back into `setup_git_directory_gently()`.
> 
> Given the earlier paragraph, it sounds like you want to move the
> global-state-changing parts out of check_repository_format_gently(). But
> that wouldn't be right; it's triggered separate from the discovery
> process by things like enter_repo().

Oh, right. I really only meant to move the global-state-changing parts out
of the discover_git_directory().

> However, I don't see that happening in the patch, which is good. I just
> wonder if the earlier paragraph should really be complaining about how
> setup_git_directory() (and its callees) touches a lot of global state,
> not check_repository_format_gently(), whose use is but one of multiple
> global-state modifications.

Okay, I'll try my best to rephrase the commit message.

For good measure, I will also keep the name setup_git_directory_gently_1()
because it won't get exported directly (I made up my mind about wrapping
that function to allow for an easier interface that does *not* return the
"cdup").

> > Note: the new loop is a *little* tricky, as we have to handle the root
> > directory specially: we cannot simply strip away the last component
> > including the slash, as the root directory only has that slash. To
> > remedy that, we introduce the `min_offset` variable that holds the
> > minimal length of an absolute path, and using that to special-case the
> > root directory, including an early exit before trying to find the
> > parent of the root directory.
> 
> I wondered how t1509 fared with this, as it is the only test of
> repositories at the root directory (and it is not run by default because
> it involves a bunch of flaky and expensive chroot setup).

Oh, thanks. I allowed myself to forget about that test script (and did a
lot of testing by hand, but of course *during* the development of v2, not
when I had finished...).

> Unfortunately it seems to fail with your patch:
> 
>   expecting success: 
>   		test_cmp_val "foo/" "$(git rev-parse --show-prefix)"
>   	
>   --- expected
>   +++ result
>   @@ -1 +1 @@
>   -foo/
>   +oo/
>   not ok 66 - auto gitdir, foo: prefix

I can reproduce this failure here.

Side note: it took a while until I realized that the prepare-chroot.sh
script has to be run *every time* I change *anything* in either Git's
source code or in the test script.

> Could the problem be this:
> 
> > +	int ceil_offset = -1, min_offset = has_dos_drive_prefix(dir->buf) ? 3 : 1;
> > [...]
> > -	if (ceil_offset < 0 && has_dos_drive_prefix(cwd.buf))
> > -		ceil_offset = 1;
> > +	if (ceil_offset < 0)
> > +		ceil_offset = min_offset - 2;

Yes. The previous code did not need cwd.buf[0..offset] to be a valid path,
but it needed the offset to point to the trailing slash, if any.

> Interestingly, I don't think this is the bug, though. We still correctly
> find /.git as a repository. The problem seems to happen later, in
> setup_discovered_git_dir(), which does this:
> 
>   /* Make "offset" point to past the '/', and add a '/' at the end */
>   offset++;
>   strbuf_addch(cwd, '/');
>   return cwd->buf + offset;

I fixed this by ensuring that we only increment the offset if it is not
already pointing at the end of the first offset (which handles Windows
paths correctly, too).

> Other than this bug, I very much like the direction that this patch is
> taking us.

Awesome. I was anxious to hear something like that.

Ciao,
Dscho