Re: [PATCH v2 5/6] setup: teach discover_git_directory to respect the commondir

From: Jeff King <peff@peff.net>
Date: Jun 14, 2017, 06:15 UTC
Message-ID: <20170614061548.uqxtbmwizfdyivv7@sigill.intra.peff.net>
In-Reply-To: <20170613210321.152978-6-bmwill@google.com>

On Tue, Jun 13, 2017 at 02:03:20PM -0700, Brandon Williams wrote:

Show 10 quoted lines

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

Show 11 quoted lines

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

Show 7 quoted lines

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