On 12/11, Eric Sunshine wrote: > On Mon, Dec 11, 2017 at 7:18 PM, Brandon Williams <bmw...@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
