# [PATCHv2] fetch: Only call a new ref a "branch" if it's under refs/heads/. 27 messages from 2012-04-13 to 2012-04-17. Participants: marcnarc@xiplink.com, Jonathan Nieder, Jeff King, Junio C Hamano, Marc Branchaud, Zbigniew Jędrzejewski-Szmek. Thread: https://gitlist.dev/t/30233 ## marcnarc@xiplink.com, 2012-04-13 17:08 Subject: [PATCHv2] fetch: Only call a new ref a "branch" if it's under refs/heads/. Message-ID: <1334336904-18649-1-git-send-email-marcnarc@xiplink.com> URL: https://gitlist.dev/e/1334336904-18649-1-git-send-email-marcnarc%40xiplink.com ``` From: Marc Branchaud Signed-off-by: Marc Branchaud --- Re-rolled atop of Jens's fix. Thanks guys -- glad I finally itched this scratch! M. builtin/fetch.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index cfb43df..b6f737e 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -293,14 +293,18 @@ static int update_local_ref(struct ref *ref, const char *msg; const char *what; int r; - if (!strncmp(ref->name, "refs/tags/", 10)) { + if (!prefixcmp(ref->name, "refs/tags/")) { msg = "storing tag"; what = _("[new tag]"); } - else { + else if (!prefixcmp(ref->name, "refs/heads/")) { msg = "storing head"; what = _("[new branch]"); } + else { + msg = "storing ref"; + what = _("[new ref]"); + } if ((recurse_submodules != RECURSE_SUBMODULES_OFF) && (recurse_submodules != RECURSE_SUBMODULES_ON)) -- 1.7.10.2.ge89da ``` ## Jonathan Nieder, 2012-04-13 20:07 Subject: Re: [PATCHv2] fetch: Only call a new ref a "branch" if it's under refs/heads/. Message-ID: <20120413200721.GA3549@burratino> URL: https://gitlist.dev/e/20120413200721.GA3549%40burratino In-Reply-To: <1334336904-18649-1-git-send-email-marcnarc@xiplink.com> ``` marcnarc@xiplink.com wrote: > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -293,14 +293,18 @@ static int update_local_ref(struct ref *ref, > const char *msg; > const char *what; > int r; > - if (!strncmp(ref->name, "refs/tags/", 10)) { > + if (!prefixcmp(ref->name, "refs/tags/")) { This part is just a clean-up, right? > msg = "storing tag"; > what = _("[new tag]"); > } > - else { > + else if (!prefixcmp(ref->name, "refs/heads/")) { > msg = "storing head"; > what = _("[new branch]"); > } > + else { > + msg = "storing ref"; > + what = _("[new ref]"); > + } Neat. I like it, for what it's worth. Sincerely, Jonathan ``` ## Jeff King, 2012-04-13 21:13 Subject: Re: [PATCHv2] fetch: Only call a new ref a "branch" if it's under refs/heads/. Message-ID: <20120413211350.GD7919@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120413211350.GD7919%40sigill.intra.peff.net In-Reply-To: <1334336904-18649-1-git-send-email-marcnarc@xiplink.com> ``` On Fri, Apr 13, 2012 at 01:08:24PM -0400, marcnarc@xiplink.com wrote: > diff --git a/builtin/fetch.c b/builtin/fetch.c > index cfb43df..b6f737e 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -293,14 +293,18 @@ static int update_local_ref(struct ref *ref, > const char *msg; > const char *what; > int r; > - if (!strncmp(ref->name, "refs/tags/", 10)) { > + if (!prefixcmp(ref->name, "refs/tags/")) { > msg = "storing tag"; > what = _("[new tag]"); > } > - else { > + else if (!prefixcmp(ref->name, "refs/heads/")) { > msg = "storing head"; > what = _("[new branch]"); > } > + else { > + msg = "storing ref"; > + what = _("[new ref]"); > + } Hmm. The ref->name we are comparing here is the local side. So if I am fetching a new branch "foo" from the remote into a local "refs/remotes/origin/foo" tracking ref, it used to say: From ../parent * [new branch] master -> origin/master Now it says: From ../parent * [new ref] master -> origin/master while refs/remotes/* are not technically branches in our side, I think from the user's perspective, it is true that we have fetched a branch. Should we be calling refs/remotes/* branches, too? Should we be checking the remote's name for the item instead of the local one? -Peff ``` ## Jonathan Nieder, 2012-04-13 21:53 Subject: Re: [PATCHv2] fetch: Only call a new ref a "branch" if it's under refs/heads/. Message-ID: <20120413215316.GA19826@burratino> URL: https://gitlist.dev/e/20120413215316.GA19826%40burratino In-Reply-To: <20120413211350.GD7919@sigill.intra.peff.net> ``` Jeff King wrote: > Hmm. The ref->name we are comparing here is the local side. So if I am > fetching a new branch "foo" from the remote into a local > "refs/remotes/origin/foo" tracking ref, it used to say: > > From ../parent > * [new branch] master -> origin/master > > Now it says: > > From ../parent > * [new ref] master -> origin/master > > while refs/remotes/* are not technically branches in our side, I think > from the user's perspective, it is true that we have fetched a branch. > Should we be calling refs/remotes/* branches, too? Should we be checking > the remote's name for the item instead of the local one? The former sounds sensible. Then once the default refspec learns to fetch into separate refs/remotes/origin/heads/* and refs/remotes/origin/notes/* namespaces the logic could be updated to write [new branch] or [new note collection] according to the situation. Jonathan ``` ## Junio C Hamano, 2012-04-13 22:39 Subject: Re: [PATCHv2] fetch: Only call a new ref a "branch" if it's under refs/heads/. Message-ID: <7vy5pz1cjk.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vy5pz1cjk.fsf%40alter.siamese.dyndns.org In-Reply-To: <20120413215316.GA19826@burratino> ``` Jonathan Nieder writes: > Jeff King wrote: > >> Hmm. The ref->name we are comparing here is the local side. So if I am >> fetching a new branch "foo" from the remote into a local >> "refs/remotes/origin/foo" tracking ref, it used to say: >> >> From ../parent >> * [new branch] master -> origin/master >> >> Now it says: >> >> From ../parent >> * [new ref] master -> origin/master >> >> while refs/remotes/* are not technically branches in our side, I think >> from the user's perspective, it is true that we have fetched a branch. >> Should we be calling refs/remotes/* branches, too? Should we be checking >> the remote's name for the item instead of the local one? > > The former sounds sensible. Then once the default refspec learns to > fetch into separate refs/remotes/origin/heads/* and > refs/remotes/origin/notes/* namespaces the logic could be updated to > write [new branch] or [new note collection] according to the > situation. If we give 'new branch' label for this case because we store it in our 'refs/remotes/*', a natural extension of it would be to redefine the rule to narrow it to 'refs/remotes/*/heads/*' for using 'branch' when we introduce 'new notes collection' label to give refs we are going to store in 'refs/remotes/origin/notes/*'. That is consistent with the former. If we give 'new branch' label because refs/heads/master on the originating end is what is shown on the line, a natural extension would be to use 'new notes collection' label when we are fetching from refs/notes/* on the originating end, and it does not matter where we store it, either our own refs/notes/* or refs/remotes/origin/notes/*. That is consistent with the latter. There is no concensus if refs/remotes/origin/notes/* hierarchy is a good idea or not, but your argument does not support either side between the former or the latter anyway, so I think it is irrelevant point to raise in this discussion. The choice between the two really depends on what information we are trying to convey with this label. Are we saying "Hey, we now have a new 'branch' on our side"? Or are we saying "We found a new 'branch' over there"? It is unclear and you can argue both ways. Although I personally think it is the latter, I do not have a strong opinion either way. I am actually fine with just saying '[new]' without indicating what kind at all, because the label is there only to fill the space where old..new object names are usually shown. We don't even say "[rejected branch]", just "[rejected]" in the same place. ``` ## Marc Branchaud, 2012-04-16 14:26 Subject: Re: [PATCHv2] fetch: Only call a new ref a "branch" if it's under refs/heads/. Message-ID: <4F8C2C20.6080300@xiplink.com> URL: https://gitlist.dev/e/4F8C2C20.6080300%40xiplink.com In-Reply-To: <20120413200721.GA3549@burratino> ``` On 12-04-13 04:07 PM, Jonathan Nieder wrote: > marcnarc@xiplink.com wrote: > >> --- a/builtin/fetch.c >> +++ b/builtin/fetch.c >> @@ -293,14 +293,18 @@ static int update_local_ref(struct ref *ref, >> const char *msg; >> const char *what; >> int r; >> - if (!strncmp(ref->name, "refs/tags/", 10)) { >> + if (!prefixcmp(ref->name, "refs/tags/")) { > > This part is just a clean-up, right? Yes. It seems most of the code in fetch.c uses prefixcmp(), so I thought I'd use it for my patch. But then I thought it looked weird for one check to use strncmp() and the other to use prefixcmp(). After a cursory search for strncmp() and strcmp() in fetch.c, I think this is the only place where prefixcmp() should've been used but wasn't. It didn't seem worth making an extra patch for this change, but I've happy to break it out if that's what folks want. >> msg = "storing tag"; >> what = _("[new tag]"); >> } >> - else { >> + else if (!prefixcmp(ref->name, "refs/heads/")) { >> msg = "storing head"; >> what = _("[new branch]"); >> } >> + else { >> + msg = "storing ref"; >> + what = _("[new ref]"); >> + } > > Neat. I like it, for what it's worth. Thanks! M. ``` ## Marc Branchaud, 2012-04-16 14:58 Subject: Re: [PATCHv2] fetch: Only call a new ref a "branch" if it's under refs/heads/. Message-ID: <4F8C338D.1050805@xiplink.com> URL: https://gitlist.dev/e/4F8C338D.1050805%40xiplink.com In-Reply-To: <7vy5pz1cjk.fsf@alter.siamese.dyndns.org> ``` On 12-04-13 06:39 PM, Junio C Hamano wrote: > Jonathan Nieder writes: > >> Jeff King wrote: >> >>> Hmm. The ref->name we are comparing here is the local side. So if I am >>> fetching a new branch "foo" from the remote into a local >>> "refs/remotes/origin/foo" tracking ref, it used to say: >>> >>> From ../parent >>> * [new branch] master -> origin/master >>> >>> Now it says: >>> >>> From ../parent >>> * [new ref] master -> origin/master >>> >>> while refs/remotes/* are not technically branches in our side, I think >>> from the user's perspective, it is true that we have fetched a branch. >>> Should we be calling refs/remotes/* branches, too? Should we be checking >>> the remote's name for the item instead of the local one? >> >> The former sounds sensible. Then once the default refspec learns to >> fetch into separate refs/remotes/origin/heads/* and >> refs/remotes/origin/notes/* namespaces the logic could be updated to >> write [new branch] or [new note collection] according to the >> situation. > > If we give 'new branch' label for this case because we store it in our > 'refs/remotes/*', a natural extension of it would be to redefine the rule > to narrow it to 'refs/remotes/*/heads/*' for using 'branch' when we > introduce 'new notes collection' label to give refs we are going to store > in 'refs/remotes/origin/notes/*'. That is consistent with the former. > > If we give 'new branch' label because refs/heads/master on the originating > end is what is shown on the line, a natural extension would be to use 'new > notes collection' label when we are fetching from refs/notes/* on the > originating end, and it does not matter where we store it, either our own > refs/notes/* or refs/remotes/origin/notes/*. That is consistent with the > latter. > > There is no concensus if refs/remotes/origin/notes/* hierarchy is a good > idea or not, but your argument does not support either side between the > former or the latter anyway, so I think it is irrelevant point to raise in > this discussion. > > The choice between the two really depends on what information we are > trying to convey with this label. Are we saying "Hey, we now have a new > 'branch' on our side"? Or are we saying "We found a new 'branch' over > there"? It is unclear and you can argue both ways. Although I personally > think it is the latter, I do not have a strong opinion either way. I think git should describe what it finds in the remote repo, because as a published repo it's refs are more likely to follow the standard layout. The local repo is more likely to be configured with a fetch refspec like +refs/heads/*:refs/crazy/* In such a case there's no point in keying off of the local names. Git is better off describing what's appeared in the remote repo, and not worrying about describing how the user might've mapped those things to local refs. (That said, patching fetch.c to do that is a bit beyond me at the moment. Where would I find the remote's name for the ref?) > I am actually fine with just saying '[new]' without indicating what kind > at all, because the label is there only to fill the space where old..new > object names are usually shown. We don't even say "[rejected branch]", > just "[rejected]" in the same place. I'd be disappointed if git didn't take the extra step to tell me a bit more about what's going on. I like to see what kinds of new refs the remote has. M. ``` ## Jeff King, 2012-04-16 15:00 Subject: Re: [PATCHv2] fetch: Only call a new ref a "branch" if it's under refs/heads/. Message-ID: <20120416150036.GA15009@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120416150036.GA15009%40sigill.intra.peff.net In-Reply-To: <4F8C338D.1050805@xiplink.com> ``` On Mon, Apr 16, 2012 at 10:58:21AM -0400, Marc Branchaud wrote: > Git is better off describing what's appeared in the remote repo, and not > worrying about describing how the user might've mapped those things to local > refs. That's my preference, as well, so it sounds like you, Junio, and I are all in agreement. > (That said, patching fetch.c to do that is a bit beyond me at the moment. > Where would I find the remote's name for the ref?) During transfer operations, the remote side of the pair is generally pointed to by the peer_ref member of "struct ref" (so just use "ref->peer_ref->name" instead of "ref->name"). -Peff ``` ## marcnarc@xiplink.com, 2012-04-16 15:52 Subject: [PATCHv3] fetch: Use the remote's ref name to decide how to describe new refs. Message-ID: <1334591542-25136-1-git-send-email-marcnarc@xiplink.com> URL: https://gitlist.dev/e/1334591542-25136-1-git-send-email-marcnarc%40xiplink.com In-Reply-To: <20120416150036.GA15009@sigill.intra.peff.net> ``` From: Marc Branchaud Also, only call a new ref a "branch" if it's under refs/heads/. Signed-off-by: Marc Branchaud --- Re-rolled to work with the remote's ref names. As before, this is atop of Jens's submodule-recursion fix. Technically there are now 3 different changes in this patch: 1. Switch to using remote ref names. 2. Use prefixcomp() consistently. 3. Only call a new ref a "branch" if its' under refs/heads. Should I split this up? M. builtin/fetch.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index cfb43df..063c63b 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -293,14 +293,23 @@ static int update_local_ref(struct ref *ref, const char *msg; const char *what; int r; - if (!strncmp(ref->name, "refs/tags/", 10)) { + /* + * Nicely describe what we're fetching. + * Base this on the remote's ref names, as they're + * more likely to follow a standard layout. + */ + if (!prefixcmp(ref->peer_ref->name, "refs/tags/")) { msg = "storing tag"; what = _("[new tag]"); } - else { + else if (!prefixcmp(ref->peer_ref->name, "refs/heads/")) { msg = "storing head"; what = _("[new branch]"); } + else { + msg = "storing ref"; + what = _("[new ref]"); + } if ((recurse_submodules != RECURSE_SUBMODULES_OFF) && (recurse_submodules != RECURSE_SUBMODULES_ON)) -- 1.7.10.2.ge89da.dirty ``` ## Jonathan Nieder, 2012-04-16 16:08 Subject: Re: [PATCHv2] fetch: Only call a new ref a "branch" if it's under refs/heads/. Message-ID: <20120416160850.GN5813@burratino> URL: https://gitlist.dev/e/20120416160850.GN5813%40burratino In-Reply-To: <20120416150036.GA15009@sigill.intra.peff.net> ``` Jeff King wrote: > On Mon, Apr 16, 2012 at 10:58:21AM -0400, Marc Branchaud wrote: >> Git is better off describing what's appeared in the remote repo, and not >> worrying about describing how the user might've mapped those things to local >> refs. > > That's my preference, as well, so it sounds like you, Junio, and I are > all in agreement. My preference after reading Junio's message was "[new] ", for what it's worth. ``` ## Jonathan Nieder, 2012-04-16 16:10 Subject: Re: [PATCHv3] fetch: Use the remote's ref name to decide how to describe new refs. Message-ID: <20120416161048.GO5813@burratino> URL: https://gitlist.dev/e/20120416161048.GO5813%40burratino In-Reply-To: <1334591542-25136-1-git-send-email-marcnarc@xiplink.com> ``` marcnarc@xiplink.com wrote: > Also, only call a new ref a "branch" if it's under refs/heads/. Looks good (though I haven't tested it). Perhaps some new tests would help make sure this doesn't get broken in the future. ``` ## Junio C Hamano, 2012-04-16 17:59 Subject: Re: [PATCHv3] fetch: Use the remote's ref name to decide how to describe new refs. Message-ID: <7vy5pvv9q0.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vy5pvv9q0.fsf%40alter.siamese.dyndns.org In-Reply-To: <1334591542-25136-1-git-send-email-marcnarc@xiplink.com> ``` marcnarc@xiplink.com writes: > Re-rolled to work with the remote's ref names. > > As before, this is atop of Jens's submodule-recursion fix. Thanks; will queue. As Jonathan mentioned, it would probably be a good idea to protect this with a new test or two, but I am fine if it is done as a separate patch. ``` ## Marc Branchaud, 2012-04-16 20:21 Subject: Re: [PATCHv3] fetch: Use the remote's ref name to decide how to describe new refs. Message-ID: <4F8C7F3A.9070801@xiplink.com> URL: https://gitlist.dev/e/4F8C7F3A.9070801%40xiplink.com In-Reply-To: <1334591542-25136-1-git-send-email-marcnarc@xiplink.com> ``` Apologies - this breaks quite a few tests! For example, t1507-rev-parse-upstream.sh test 5 segfaults. I'll take another stab at it... M. On 12-04-16 11:52 AM, marcnarc@xiplink.com wrote: > From: Marc Branchaud > > Also, only call a new ref a "branch" if it's under refs/heads/. > > Signed-off-by: Marc Branchaud > --- > > Re-rolled to work with the remote's ref names. > > As before, this is atop of Jens's submodule-recursion fix. > > Technically there are now 3 different changes in this patch: > 1. Switch to using remote ref names. > 2. Use prefixcomp() consistently. > 3. Only call a new ref a "branch" if its' under refs/heads. > > Should I split this up? > > M. > > > builtin/fetch.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/builtin/fetch.c b/builtin/fetch.c > index cfb43df..063c63b 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -293,14 +293,23 @@ static int update_local_ref(struct ref *ref, > const char *msg; > const char *what; > int r; > - if (!strncmp(ref->name, "refs/tags/", 10)) { > + /* > + * Nicely describe what we're fetching. > + * Base this on the remote's ref names, as they're > + * more likely to follow a standard layout. > + */ > + if (!prefixcmp(ref->peer_ref->name, "refs/tags/")) { > msg = "storing tag"; > what = _("[new tag]"); > } > - else { > + else if (!prefixcmp(ref->peer_ref->name, "refs/heads/")) { > msg = "storing head"; > what = _("[new branch]"); > } > + else { > + msg = "storing ref"; > + what = _("[new ref]"); > + } > > if ((recurse_submodules != RECURSE_SUBMODULES_OFF) && > (recurse_submodules != RECURSE_SUBMODULES_ON)) ``` ## marcnarc@xiplink.com, 2012-04-16 22:08 Subject: [PATCHv4 0/3] fetch: Only call a new ref a "branch" if it's under refs/heads/. Message-ID: <1334614130-31826-1-git-send-email-marcnarc@xiplink.com> URL: https://gitlist.dev/e/1334614130-31826-1-git-send-email-marcnarc%40xiplink.com In-Reply-To: <1334591542-25136-1-git-send-email-marcnarc@xiplink.com> ``` It turns out that ref->peer_ref is always NULL in update_local_ref(). So I made its caller pass in the full remote ref as a new parameter. I also added a unit test. This series is a complete resend of all the patches discussed in these threads, including Jens's submodule recursion fix. builtin/fetch.c | 39 +++++++++++++++++++++++++-------------- t/t5510-fetch.sh | 28 ++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 14 deletions(-) Jens Lehmann (1): submodules: recursive fetch also checks new tags for submodule commits Marc Branchaud (2): fetch: Pass both the full remote ref and its short name to update_local_ref(). fetch: Use the remote's ref name to decide how to describe new refs. M. ``` ## marcnarc@xiplink.com, 2012-04-16 22:08 Subject: [PATCH 1/3] submodules: recursive fetch also checks new tags for submodule commits Message-ID: <1334614130-31826-2-git-send-email-marcnarc@xiplink.com> URL: https://gitlist.dev/e/1334614130-31826-2-git-send-email-marcnarc%40xiplink.com In-Reply-To: <1334614130-31826-1-git-send-email-marcnarc@xiplink.com> ``` From: Jens Lehmann Since 88a21979c (fetch/pull: recurse into submodules when necessary) all fetched commits are examined if they contain submodule changes (unless configuration or command line options inhibit that). If a newly recorded submodule commit is not present in the submodule, a fetch is run inside it to download that commit. Checking new refs was done in an else branch where it wasn't executed for tags. This normally isn't a problem because tags are only fetched with the branches they live on, then checking the new commits in the fetched branches for submodule commits will also process all tags. But when a specific tag is fetched (or the refspec contains refs/tags/) commits only reachable by tags won't be searched for submodule commits, which is a bug. Fix that by moving the code outside the if/else construct to handle new tags just like any other ref. The performance impact of adding tags that most of the time lie on a branch which is checked anyway for new submodule commit should be minimal, as since 6859de4 (fetch: avoid quadratic loop checking for updated submodules) all ref-tips are collected first and then fed to a single rev-list. Spotted-by: Jeff King Signed-off-by: Jens Lehmann --- builtin/fetch.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 65f5f9b..cfb43df 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -300,11 +300,11 @@ static int update_local_ref(struct ref *ref, else { msg = "storing head"; what = _("[new branch]"); - if ((recurse_submodules != RECURSE_SUBMODULES_OFF) && - (recurse_submodules != RECURSE_SUBMODULES_ON)) - check_for_new_submodule_commits(ref->new_sha1); } + if ((recurse_submodules != RECURSE_SUBMODULES_OFF) && + (recurse_submodules != RECURSE_SUBMODULES_ON)) + check_for_new_submodule_commits(ref->new_sha1); r = s_update_ref(msg, ref, 0); strbuf_addf(display, "%c %-*s %-*s -> %s%s", r ? '!' : '*', -- 1.7.10.2.ge89da.dirty ``` ## marcnarc@xiplink.com, 2012-04-16 22:08 Subject: [PATCH 2/3] fetch: Pass both the full remote ref and its short name to update_local_ref(). Message-ID: <1334614130-31826-3-git-send-email-marcnarc@xiplink.com> URL: https://gitlist.dev/e/1334614130-31826-3-git-send-email-marcnarc%40xiplink.com In-Reply-To: <1334614130-31826-1-git-send-email-marcnarc@xiplink.com> ``` From: Marc Branchaud Signed-off-by: Marc Branchaud --- builtin/fetch.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index cfb43df..6c19975 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -239,7 +239,8 @@ static int s_update_ref(const char *action, #define REFCOL_WIDTH 10 static int update_local_ref(struct ref *ref, - const char *remote, + const char *remote_ref_shortname, + const struct ref *remote_ref, struct strbuf *display) { struct commit *current = NULL, *updated; @@ -256,7 +257,7 @@ static int update_local_ref(struct ref *ref, strbuf_addf(display, "= %-*s %-*s -> %s", TRANSPORT_SUMMARY_WIDTH, _("[up to date]"), REFCOL_WIDTH, - remote, pretty_ref); + remote_ref_shortname, pretty_ref); return 0; } @@ -271,7 +272,7 @@ static int update_local_ref(struct ref *ref, strbuf_addf(display, _("! %-*s %-*s -> %s (can't fetch in current branch)"), TRANSPORT_SUMMARY_WIDTH, _("[rejected]"), - REFCOL_WIDTH, remote, pretty_ref); + REFCOL_WIDTH, remote_ref_shortname, pretty_ref); return 1; } @@ -282,7 +283,7 @@ static int update_local_ref(struct ref *ref, strbuf_addf(display, "%c %-*s %-*s -> %s%s", r ? '!' : '-', TRANSPORT_SUMMARY_WIDTH, _("[tag update]"), - REFCOL_WIDTH, remote, pretty_ref, + REFCOL_WIDTH, remote_ref_shortname, pretty_ref, r ? _(" (unable to update local ref)") : ""); return r; } @@ -309,7 +310,7 @@ static int update_local_ref(struct ref *ref, strbuf_addf(display, "%c %-*s %-*s -> %s%s", r ? '!' : '*', TRANSPORT_SUMMARY_WIDTH, what, - REFCOL_WIDTH, remote, pretty_ref, + REFCOL_WIDTH, remote_ref_shortname, pretty_ref, r ? _(" (unable to update local ref)") : ""); return r; } @@ -327,7 +328,7 @@ static int update_local_ref(struct ref *ref, strbuf_addf(display, "%c %-*s %-*s -> %s%s", r ? '!' : ' ', TRANSPORT_SUMMARY_WIDTH, quickref, - REFCOL_WIDTH, remote, pretty_ref, + REFCOL_WIDTH, remote_ref_shortname, pretty_ref, r ? _(" (unable to update local ref)") : ""); return r; } else if (force || ref->force) { @@ -343,13 +344,13 @@ static int update_local_ref(struct ref *ref, strbuf_addf(display, "%c %-*s %-*s -> %s (%s)", r ? '!' : '+', TRANSPORT_SUMMARY_WIDTH, quickref, - REFCOL_WIDTH, remote, pretty_ref, + REFCOL_WIDTH, remote_ref_shortname, pretty_ref, r ? _("unable to update local ref") : _("forced update")); return r; } else { strbuf_addf(display, "! %-*s %-*s -> %s %s", TRANSPORT_SUMMARY_WIDTH, _("[rejected]"), - REFCOL_WIDTH, remote, pretty_ref, + REFCOL_WIDTH, remote_ref_shortname, pretty_ref, _("(non-fast-forward)")); return 1; } @@ -466,7 +467,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, strbuf_reset(¬e); if (ref) { - rc |= update_local_ref(ref, what, ¬e); + rc |= update_local_ref(ref, what, rm, ¬e); free(ref); } else strbuf_addf(¬e, "* %-*s %-*s -> FETCH_HEAD", -- 1.7.10.2.ge89da.dirty ``` ## marcnarc@xiplink.com, 2012-04-16 22:08 Subject: [PATCH 3/3] fetch: Use the remote's ref name to decide how to describe new refs. Message-ID: <1334614130-31826-4-git-send-email-marcnarc@xiplink.com> URL: https://gitlist.dev/e/1334614130-31826-4-git-send-email-marcnarc%40xiplink.com In-Reply-To: <1334614130-31826-1-git-send-email-marcnarc@xiplink.com> ``` From: Marc Branchaud Also, only call a new ref a "branch" if it's under refs/heads/. Signed-off-by: Marc Branchaud --- builtin/fetch.c | 14 ++++++++++++-- t/t5510-fetch.sh | 28 ++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 6c19975..3ede8fd 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -294,14 +294,24 @@ static int update_local_ref(struct ref *ref, const char *msg; const char *what; int r; - if (!strncmp(ref->name, "refs/tags/", 10)) { + /* + * Nicely describe the new ref we're fetching. + * Base this on the remote's ref name, as it's + * more likely to follow a standard layout. + */ + const char *name = remote_ref ? remote_ref->name : ""; + if (!prefixcmp(name, "refs/tags/")) { msg = "storing tag"; what = _("[new tag]"); } - else { + else if (!prefixcmp(name, "refs/heads/")) { msg = "storing head"; what = _("[new branch]"); } + else { + msg = "storing ref"; + what = _("[new ref]"); + } if ((recurse_submodules != RECURSE_SUBMODULES_OFF) && (recurse_submodules != RECURSE_SUBMODULES_ON)) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 308c02e..d17a2cd 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -162,6 +162,34 @@ test_expect_success 'fetch following tags' ' ' +test_expect_success 'fetch uses remote ref names to describe new refs' ' + cd "$D" && + mkdir descriptive && + ( + cd descriptive && + git init && + git config remote.o.url .. && + git config remote.o.fetch "refs/heads/*:refs/crazyheads/*" && + git config --add remote.o.fetch "refs/others/*:refs/heads/*" && + git fetch o + ) && + git tag -a -m "Descriptive tag" descriptive-tag && + git branch descriptive-branch && + git checkout descriptive-branch && + echo "Nuts" >> crazy && + git add crazy && + git commit -a -m "descriptive commit" && + git update-ref refs/others/crazy HEAD && + ( + cd descriptive && + git fetch o 2> actual + grep descriptive-branch actual | grep "[new branch]" && + grep descriptive-tag actual | grep "[new tag]" && + grep others/crazy actual | grep "[new ref]" + ) && + git checkout master +' + test_expect_success 'fetch must not resolve short tag name' ' cd "$D" && -- 1.7.10.2.ge89da.dirty ``` ## Jonathan Nieder, 2012-04-16 22:34 Subject: Re: [PATCH 3/3] fetch: Use the remote's ref name to decide how to describe new refs. Message-ID: <20120416223429.GA13440@burratino> URL: https://gitlist.dev/e/20120416223429.GA13440%40burratino In-Reply-To: <1334614130-31826-4-git-send-email-marcnarc@xiplink.com> ``` marcnarc@xiplink.com wrote: > + grep descriptive-branch actual | grep "[new branch]" && > + grep descriptive-tag actual | grep "[new tag]" && > + grep others/crazy actual | grep "[new ref]" Careful. :) I suppose the simplest fix would be to leave out the brackets so the '[new ref]' tag is not misinterpreted as a character class, like so: grep "new branch.*descriptive-branch" actual && grep "new tag.*descriptive-tag" actual && grep "new ref.*others/crazy" actual ``` ## Zbigniew Jędrzejewski-Szmek, 2012-04-17 07:53 Subject: Re: [PATCH 3/3] fetch: Use the remote's ref name to decide how to describe new refs. Message-ID: <4F8D2195.9030507@in.waw.pl> URL: https://gitlist.dev/e/4F8D2195.9030507%40in.waw.pl In-Reply-To: <20120416223429.GA13440@burratino> ``` On 04/17/2012 12:34 AM, Jonathan Nieder wrote: > marcnarc@xiplink.com wrote: > >> + grep descriptive-branch actual | grep "[new branch]"&& >> + grep descriptive-tag actual | grep "[new tag]"&& >> + grep others/crazy actual | grep "[new ref]" > > Careful. :) I suppose the simplest fix would be to leave out the > brackets so the '[new ref]' tag is not misinterpreted as a character > class, like so: > > grep "new branch.*descriptive-branch" actual&& > grep "new tag.*descriptive-tag" actual&& > grep "new ref.*others/crazy" actual s/grep/test_i18ngrep/ Also: >> + echo "Nuts" >> crazy && >> + git add crazy && >> + git commit -a -m "descriptive commit" && >> + git update-ref refs/others/crazy HEAD && >> + ( >> + cd descriptive && >> + git fetch o 2> actual redirections should be without spaces between '>' and the filename (>>crazy, 2>actual), for portability. Zbyszek ``` ## Jonathan Nieder, 2012-04-17 07:57 Subject: Re: [PATCH 3/3] fetch: Use the remote's ref name to decide how to describe new refs. Message-ID: <20120417075740.GE20017@burratino> URL: https://gitlist.dev/e/20120417075740.GE20017%40burratino In-Reply-To: <4F8D2195.9030507@in.waw.pl> ``` Zbigniew Jędrzejewski-Szmek wrote: > On 04/17/2012 12:34 AM, Jonathan Nieder wrote: >> marcnarc@xiplink.com wrote: >>> + echo "Nuts" >> crazy && >>> + git add crazy && >>> + git commit -a -m "descriptive commit" && >>> + git update-ref refs/others/crazy HEAD && >>> + ( >>> + cd descriptive && >>> + git fetch o 2> actual > > redirections should be without spaces between '>' and the filename > (>>crazy, 2>actual), for portability. I think you mean for consistency. A space between the operator and filename is perfectly portable, though git's tests tend to use a style without the space. Thanks, Jonathan ``` ## Zbigniew Jędrzejewski-Szmek, 2012-04-17 08:39 Subject: Re: [PATCH 3/3] fetch: Use the remote's ref name to decide how to describe new refs. Message-ID: <4F8D2C50.3060502@in.waw.pl> URL: https://gitlist.dev/e/4F8D2C50.3060502%40in.waw.pl In-Reply-To: <20120417075740.GE20017@burratino> ``` On 04/17/2012 09:57 AM, Jonathan Nieder wrote: > Zbigniew Jędrzejewski-Szmek wrote: >> On 04/17/2012 12:34 AM, Jonathan Nieder wrote: >>> marcnarc@xiplink.com wrote: > >>>> + echo "Nuts" >> crazy && >>>> + git add crazy && >>>> + git commit -a -m "descriptive commit" && >>>> + git update-ref refs/others/crazy HEAD && >>>> + ( >>>> + cd descriptive && >>>> + git fetch o 2> actual >> >> redirections should be without spaces between '>' and the filename >> (>>crazy, 2>actual), for portability. > > I think you mean for consistency. A space between the operator and > filename is perfectly portable, though git's tests tend to use a > style without the space. Yes, you're right. I mixed up the motivation for two different rules... Quoting Junio C Hamano: > - Strictly speaking, the target of I/O redirection (e.g. >"$name") does > not have to have quotes around it, but some versions of bash are known > to give misguided warnings against it; > > - We do not write SP between the redirection and filename, but we do have > one SP before the redirection; and So rule #2 is for consistency, rule #1 for portability. - Zbyszek ``` ## Marc Branchaud, 2012-04-17 14:23 Subject: Re: [PATCH 3/3] fetch: Use the remote's ref name to decide how to describe new refs. Message-ID: <4F8D7CD5.3080508@xiplink.com> URL: https://gitlist.dev/e/4F8D7CD5.3080508%40xiplink.com In-Reply-To: <4F8D2195.9030507@in.waw.pl> ``` On 12-04-17 03:53 AM, Zbigniew Jędrzejewski-Szmek wrote: > On 04/17/2012 12:34 AM, Jonathan Nieder wrote: >> marcnarc@xiplink.com wrote: >> >>> + grep descriptive-branch actual | grep "[new branch]"&& >>> + grep descriptive-tag actual | grep "[new tag]"&& >>> + grep others/crazy actual | grep "[new ref]" >> >> Careful. :) I suppose the simplest fix would be to leave out the >> brackets so the '[new ref]' tag is not misinterpreted as a character >> class, like so: >> >> grep "new branch.*descriptive-branch" actual&& >> grep "new tag.*descriptive-tag" actual&& >> grep "new ref.*others/crazy" actual > > s/grep/test_i18ngrep/ With the switch to test_i18ngrep do I have to worry about possible regexp misinterpretations of "[new branch]" et al? Jonathan, I figured regexps aren't an issue with plain "grep" (unlike "egrep" or "grep -e"). I take it this is a portability concern -- are there systems that actually replace plain "grep" like a "egrep"? Also, in my test's pipes I believe only the second "grep" needs to be "test_i18ngrep", right? (Only strings like "[new branch]" are internationalized.) > Also: >>> + echo "Nuts" >> crazy && >>> + git add crazy && >>> + git commit -a -m "descriptive commit" && >>> + git update-ref refs/others/crazy HEAD && >>> + ( >>> + cd descriptive && >>> + git fetch o 2> actual > redirections should be without spaces between '>' and the filename > (>>crazy, 2>actual), for portability. Consistency; got it. Thanks guys! M. ``` ## Jonathan Nieder, 2012-04-17 15:18 Subject: Re: [PATCH 3/3] fetch: Use the remote's ref name to decide how to describe new refs. Message-ID: <20120417151826.GA21534@burratino> URL: https://gitlist.dev/e/20120417151826.GA21534%40burratino In-Reply-To: <4F8D7CD5.3080508@xiplink.com> ``` Hi again, Marc Branchaud wrote: > Jonathan, I figured regexps aren't an issue with plain "grep" (unlike "egrep" > or "grep -e"). I take it this is a portability concern -- are there systems > that actually replace plain "grep" like a "egrep"? No, "grep" always means to globally match against a regexp and print. There is an "fgrep" command to search for a fixed string, but why not take advantage of regexps while they're available and use one grep instead of two? > Also, in my test's pipes I believe only the second "grep" needs to be > "test_i18ngrep", right? (Only strings like "[new branch]" are > internationalized.) I'm not sure what the point would be. The exit status from the upstream of a pipe doesn't affect the outcome of the test, so no one would be able to tell the difference. [...] > Thanks guys! Thank you. Sincerely, Jonathan ``` ## Junio C Hamano, 2012-04-17 15:26 Subject: Re: [PATCHv4 0/3] fetch: Only call a new ref a "branch" if it's under refs/heads/. Message-ID: <7vbomqpef1.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vbomqpef1.fsf%40alter.siamese.dyndns.org In-Reply-To: <1334614130-31826-1-git-send-email-marcnarc@xiplink.com> ``` marcnarc@xiplink.com writes: > It turns out that ref->peer_ref is always NULL in update_local_ref(). So I > made its caller pass in the full remote ref as a new parameter. I also added > a unit test. > > This series is a complete resend of all the patches discussed in these > threads, including Jens's submodule recursion fix. Thanks, but I'd prefer to keep unrelated things as separate unless there is a compelling reason not to. Also I do not think renaming of the existing parameter in the first patch is warranted, especially when the new parameter you are adding is more descriptive (i.e. "remote_ref" in the context of that function makes it clear enough that it is not just a string but is a pointer to a ref structure). So let's do this. -- >8 -- From: Marc Branchaud Date: Mon, 16 Apr 2012 18:08:49 -0400 Subject: [PATCH 1/2] fetch: Give remote_ref to update_local_ref() as well This way, the function can look at the remote side to adjust the informational message it gives. Signed-off-by: Marc Branchaud Signed-off-by: Junio C Hamano --- builtin/fetch.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 8ec4eae..06d71e4 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -240,6 +240,7 @@ static int s_update_ref(const char *action, static int update_local_ref(struct ref *ref, const char *remote, + const struct ref *remote_ref, struct strbuf *display) { struct commit *current = NULL, *updated; @@ -466,7 +467,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, strbuf_reset(¬e); if (ref) { - rc |= update_local_ref(ref, what, ¬e); + rc |= update_local_ref(ref, what, rm, ¬e); free(ref); } else strbuf_addf(¬e, "* %-*s %-*s -> FETCH_HEAD", -- 1.7.10.332.g1863c ``` ## Junio C Hamano, 2012-04-17 15:28 Subject: Re: [PATCHv4 0/3] fetch: Only call a new ref a "branch" if it's under refs/heads/. Message-ID: <7v7gxepecd.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v7gxepecd.fsf%40alter.siamese.dyndns.org In-Reply-To: <7vbomqpef1.fsf@alter.siamese.dyndns.org> ``` From: Marc Branchaud Date: Mon, 16 Apr 2012 18:08:50 -0400 Subject: [PATCH 2/2] fetch: describe new refs based on where it came from update_local_ref() used to say "[new branch]" when we stored a new ref outside refs/tags/ hierarchy, but the message is more about what we fetched, so use the refname at the origin to make that decision. Also, only call a new ref a "branch" if it's under refs/heads/. Signed-off-by: Marc Branchaud Signed-off-by: Junio C Hamano --- * And this is a re-roll of yours on top of the updated 1/2; I also fixed the new test here and there. builtin/fetch.c | 14 +++++++++++--- t/t5510-fetch.sh | 30 ++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 06d71e4..0f80cf8 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -294,16 +294,24 @@ static int update_local_ref(struct ref *ref, const char *msg; const char *what; int r; - if (!strncmp(ref->name, "refs/tags/", 10)) { + /* + * Nicely describe the new ref we're fetching. + * Base this on the remote's ref name, as it's + * more likely to follow a standard layout. + */ + const char *name = remote_ref ? remote_ref->name : ""; + if (!prefixcmp(name, "refs/tags/")) { msg = "storing tag"; what = _("[new tag]"); - } - else { + } else if (!prefixcmp(name, "refs/heads/")) { msg = "storing head"; what = _("[new branch]"); if ((recurse_submodules != RECURSE_SUBMODULES_OFF) && (recurse_submodules != RECURSE_SUBMODULES_ON)) check_for_new_submodule_commits(ref->new_sha1); + } else { + msg = "storing ref"; + what = _("[new ref]"); } r = s_update_ref(msg, ref, 0); diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 308c02e..d7a19a1 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -162,6 +162,36 @@ test_expect_success 'fetch following tags' ' ' +test_expect_success 'fetch uses remote ref names to describe new refs' ' + cd "$D" && + git init descriptive && + ( + cd descriptive && + git config remote.o.url .. && + git config remote.o.fetch "refs/heads/*:refs/crazyheads/*" && + git config --add remote.o.fetch "refs/others/*:refs/heads/*" && + git fetch o + ) && + git tag -a -m "Descriptive tag" descriptive-tag && + git branch descriptive-branch && + git checkout descriptive-branch && + echo "Nuts" >crazy && + git add crazy && + git commit -a -m "descriptive commit" && + git update-ref refs/others/crazy HEAD && + ( + cd descriptive && + git fetch o 2>actual && + grep " -> refs/crazyheads/descriptive-branch$" actual | + test_i18ngrep "new branch" && + grep " -> descriptive-tag$" actual | + test_i18ngrep "new tag" && + grep " -> crazy$" actual | + test_i18ngrep "new ref" + ) && + git checkout master +' + test_expect_success 'fetch must not resolve short tag name' ' cd "$D" && -- 1.7.10.332.g1863c ``` ## Marc Branchaud, 2012-04-17 19:30 Subject: Re: [PATCHv4 0/3] fetch: Only call a new ref a "branch" if it's under refs/heads/. Message-ID: <4F8DC4CD.9090106@xiplink.com> URL: https://gitlist.dev/e/4F8DC4CD.9090106%40xiplink.com In-Reply-To: <7vbomqpef1.fsf@alter.siamese.dyndns.org> ``` On 12-04-17 11:26 AM, Junio C Hamano wrote: > marcnarc@xiplink.com writes: > >> It turns out that ref->peer_ref is always NULL in update_local_ref(). So I >> made its caller pass in the full remote ref as a new parameter. I also added >> a unit test. >> >> This series is a complete resend of all the patches discussed in these >> threads, including Jens's submodule recursion fix. > > Thanks, but I'd prefer to keep unrelated things as separate unless there > is a compelling reason not to. > > Also I do not think renaming of the existing parameter in the first patch > is warranted, especially when the new parameter you are adding is more > descriptive (i.e. "remote_ref" in the context of that function makes it > clear enough that it is not just a string but is a pointer to a ref > structure). > > So let's do this. I'm good with both your patches. Thanks much for fixing my work! M. ``` ## Jeff King, 2012-04-17 22:29 Subject: Re: [PATCHv4 0/3] fetch: Only call a new ref a "branch" if it's under refs/heads/. Message-ID: <20120417222959.GD10797@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120417222959.GD10797%40sigill.intra.peff.net In-Reply-To: <1334614130-31826-1-git-send-email-marcnarc@xiplink.com> ``` On Mon, Apr 16, 2012 at 06:08:47PM -0400, marcnarc@xiplink.com wrote: > > It turns out that ref->peer_ref is always NULL in update_local_ref(). So I > made its caller pass in the full remote ref as a new parameter. I also added > a unit test. Hrm. So yeah, it is because the "struct ref" we create is not from the ref_map, but is a newly created ref based on the local side (and I had my refs backwards before; the remote ref is the "real" ref, and the local version is found in peer_ref, not the other way around). But I couldn't help but notice that store_updated_refs already has this exact same "what do we call it to the user" logic in it, which is what goes into FETCH_HEAD. Shouldn't we just be passing this "kind" flag down to update_local_ref, and then this copy of the logic can go away entirely? -Peff ```