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