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