mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Brandon Williams <>
To: Eric Sunshine <>
Cc: "Git List" <>,
	"Marc-André Lureau" <>
Subject: Re: [PATCH] clone: support 'clone --shared' from a worktree
Date: Mon, 11 Dec 2017 16:52:21 -0800	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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

  reply	other threads:[~2017-12-12  0:52 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-11 23:16 [PATCH] clone: support 'clone --shared' from a worktree Eric Sunshine
2017-12-12  0:01 ` Junio C Hamano
2017-12-12  0:18 ` Brandon Williams
2017-12-12  0:40   ` Eric Sunshine
2017-12-12  0:52     ` Brandon Williams [this message]
2017-12-13 18:28       ` [PATCH] path: document path functions Brandon Williams
2017-12-13 19:14         ` Junio C Hamano
2017-12-20  8:58         ` Eric Sunshine

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).