Re: [PATCH] clone: support 'clone --shared' from a worktree

From: Brandon Williams <bmwill@google.com>
Date: Dec 12, 2017, 00:52 UTC
Message-ID: <20171212005221.GD177995@google.com>
In-Reply-To: <CAPig+cSTsuHdAqdBMvO80ybTSzfxncX8yQODSKd1bmaoNRQOjw@mail.gmail.com>

On 12/11, Eric Sunshine wrote:

Show 31 quoted lines

> On Mon, Dec 11, 2017 at 7:18 PM, Brandon Williams <bmwill@google.com> wrote:
> > On 12/11, Eric Sunshine wrote:
> >>               struct strbuf alt = STRBUF_INIT;
> >> -             strbuf_addf(&alt, "%s/objects", src_repo);
> >> +             get_common_dir(&alt, src_repo);
> >> +             strbuf_addstr(&alt, "/objects");
> >
> > If you wanted to do this in one function call you could either use
> > 'strbuf_git_common_path()' or either 'strbuf_git_path()' or
> > 'strbuf_repo_git_path()' which will do the proper path adjustments when
> > working on a path which should be shared between worktrees (i.e. part of
> > the common git dir).
> 
> Thanks for the pointers, however, the above fix mirrors the fix made
> by 744e469755 (clone: allow --local from a linked checkout,
> 2015-09-28) to code immediately below it in the 'else' arm:
> 
>     get_common_dir(&src, src_repo);
>     get_common_dir(&dest, dest_repo);
>     strbuf_addstr(&src, "/objects");
>     strbuf_addstr(&dest, "/objects");
> 
> It would be poor form and confusing to use one of the mechanisms you
> suggest while leaving the 'else' arm untouched.
> 
> Re-working both arms of the 'if' to use one of the suggested functions
> would make a fine follow-on or preparatory patch, however, I'd rather
> not hold up this fix merely to re-roll for such a minor cleanup. (I
> also considered a follow-on patch to reduce the duplication between
> the two cases but decided against it, for the present, since such a
> patch would almost be noise without much gain.)

I didn't look close enough at what you were trying to fix, you're right I think what you have here is good as the alternative would require a lot more reworking I think (at least to change the above part too).

Either way though, I'm a little worried about what happens if you have GIT_COMMON_DIR set because then both the src and dest repo would share a common dir, I don't know if that is expected or not. Maybe something else to consider later.

> 
> By the way, is there any documentation explaining the differences
> between all these similar functions and when one should be used over
> the others?

I wish, I probably should have done a better job documenting it all in path.h when I added the repo_* flavor of functions. I'll add that to my list of things to do though :)

-- 
Brandon Williams