On Tue, Jun 13, 2017 at 02:03:20PM -0700, Brandon Williams wrote:
> Currently 'discover_git_directory' only looks at the gitdir to determine
> if a git directory was discovered. This causes a problem in the event
> that the gitdir which was discovered was in fact a per-worktree git
> directory and not the common git directory. This is because the
> repository config, which is checked to verify the repository's format,
> is stored in the commondir and not in the per-worktree gitdir. Correct
> this behavior by checking the config stored in the commondir.
>
> It will also be of use for callers to have access to the commondir, so
> lets also return that upon successfully discovering a git directory.
This makes sense, and I agree is the correct path forward for handling
the config code's needs.
> diff --git a/cache.h b/cache.h
> index fd45b8c55..a4176436d 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -530,7 +530,8 @@ extern void setup_work_tree(void);
> * appended to gitdir. The return value is either NULL if no repository was
> * found, or pointing to the path inside gitdir's buffer.
> */
> -extern const char *discover_git_directory(struct strbuf *gitdir);
> +extern const char *discover_git_directory(struct strbuf *commondir,
> + struct strbuf *gitdir);
Does the docstring above the function need updating? What happens when
you are not in a worktree? Are both strbufs filled out with the same
value?
That's what I'd assume (and what it looks like looking at the code), but
it's probably worth being explicit.
We also return a pointer. I think this still points into the gitdir
strbuf. Which I guess is reasonable, though probably should be
documented.
Given that the callers only ever look at whether it's non-NULL, it
probably would be better to just return a true/false value. This might
be a good time to do that, because the function signature is changing
already (so if we switch to the usual "0 is success", a newly added call
won't silently do the wrong thing).
> @@ -983,6 +987,7 @@ const char *discover_git_directory(struct strbuf *gitdir)
> warning("ignoring git dir '%s': %s",
> gitdir->buf + gitdir_offset, err.buf);
> strbuf_release(&err);
> + strbuf_setlen(commondir, commondir_offset);
> return NULL;
> }
I'd have expected these resetting setlens to be paired between gitdir
and commondir. And indeed, they should be; this is the same case that
Dscho fixed in the first patch of his series.
I kind of wonder if one of you should take ownership and do a combined
series.
-Peff