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

[Brandon Williams <bmwill@google.com>]

On 06/14, Jeff King wrote:
> 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?

Yes I'll update the docstring to reflect the change.  And yes if you
aren't in a worktree, then both strbufs would hold 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).

I agree with that.  I can tweak the return value to return a success
code.

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

Yeah I think that Dschos series has less review to still take place (as
he just sent out a v3) so I'll just rebase ontop of his series.

-- 
Brandon Williams