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

[Brandon Williams <bmwill@google.com>]

On 12/11, Eric Sunshine wrote:
> 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