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

From: Jeff King <peff@peff.net>
Date: Mar 3, 2017, 04:24 UTC
Message-ID: <20170303042422.s3dnbgbjszhygwqz@sigill.intra.peff.net>
In-Reply-To: <49f66f2b723af38a58c17bc8ae413bcee69e2a2f.1488506615.git.johannes.schindelin@gmx.de>

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

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

Show 7 quoted lines

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

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.

Show 7 quoted lines

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

Could the problem be this:

Show 6 quoted lines

> +	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;

It works the same as before in the dos-drive case, but we'd get ceil_offset = -1 otherwise. Which seems weird, but maybe I don't understand how ceil_offset is supposed to work.

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;

Here, "offset" is the length of the working tree path. The root directory case differs from normal in that "offset" already accounts for the trailing slash.

So I think the bug comes from:

Show 6 quoted lines

> -			ret = setup_discovered_git_dir(gitdirenv,
> -						       &cwd, offset,
> -						       nongit_ok);
> [...]
> +		prefix = setup_discovered_git_dir(gitdir.buf, &cwd, dir.len,
> +						  nongit_ok);

The original knew that "offset" took into account the off-by-one for the root, but that's lost when we use dir.len. I haven't studied it enough to know the best solution, but I suspect you will.

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

-Peff