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