threads / discuss / 30233

[PATCHv2] fetch: Only call a new ref a "branch" if it's under refs/heads/.

Subject: [PATCHv2] fetch: Only call a new ref a "branch" if it's under refs/heads/.

## tl;dr

27 messages between Apr 13, 2012 and Apr 17, 2012.

replies: 26people: 5as markdown or json

marcnarc@xiplink.com· Apr 13, 2012, 17:08 UTC · lore
From: Marc Branchaud <marcnarc@xiplink.com>
Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
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· Apr 13, 2012, 20:07 UTC · re: marcnarc@xiplink.com · lore

Re: [PATCHv2] fetch: Only call a new ref a "branch" if it's under refs/heads/.

marcnarc@xiplink.com wrote:
Show 8 quoted lines
> --- 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?
Show 12 quoted lines
>  			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

Marc Branchaud· Apr 16, 2012, 14:26 UTC · re: Jonathan Nieder · lore

Re: [PATCHv2] fetch: Only call a new ref a "branch" if it's under refs/heads/.

On 12-04-13 04:07 PM, Jonathan Nieder wrote:
Show 12 quoted lines
> 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.

Show 14 quoted lines
>>  			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.
Jeff King· Apr 13, 2012, 21:13 UTC · re: marcnarc@xiplink.com · lore

Re: [PATCHv2] fetch: Only call a new ref a "branch" if it's under refs/heads/.

On Fri, Apr 13, 2012 at 01:08:24PM -0400, marcnarc@xiplink.com wrote:
Show 22 quoted lines
> 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· Apr 13, 2012, 21:53 UTC · re: Jeff King · lore

Re: [PATCHv2] fetch: Only call a new ref a "branch" if it's under refs/heads/.

Jeff King wrote:
Show 16 quoted lines
> 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· Apr 13, 2012, 22:39 UTC · re: Jonathan Nieder · lore

Re: [PATCHv2] fetch: Only call a new ref a "branch" if it's under refs/heads/.

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 24 quoted lines
> 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· Apr 16, 2012, 14:58 UTC · re: Junio C Hamano · lore

Re: [PATCHv2] fetch: Only call a new ref a "branch" if it's under refs/heads/.

On 12-04-13 06:39 PM, Junio C Hamano wrote:
Show 50 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> 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· Apr 16, 2012, 15:00 UTC · re: Marc Branchaud · lore

Re: [PATCHv2] fetch: Only call a new ref a "branch" if it's under refs/heads/.

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· Apr 16, 2012, 15:52 UTC · re: Jeff King · lore

[PATCHv3] fetch: Use the remote's ref name to decide how to describe new refs.

From: Marc Branchaud <marcnarc@xiplink.com>
Also, only call a new ref a "branch" if it's under refs/heads/.
Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
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· Apr 16, 2012, 16:10 UTC · re: marcnarc@xiplink.com · lore

Re: [PATCHv3] fetch: Use the remote's ref name to decide how to describe new refs.

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· Apr 16, 2012, 17:59 UTC · re: marcnarc@xiplink.com · lore

Re: [PATCHv3] fetch: Use the remote's ref name to decide how to describe new refs.

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· Apr 16, 2012, 20:21 UTC · re: marcnarc@xiplink.com · lore

Re: [PATCHv3] fetch: Use the remote's ref name to decide how to describe new refs.

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:
Show 54 quoted lines
> From: Marc Branchaud <marcnarc@xiplink.com>
> 
> Also, only call a new ref a "branch" if it's under refs/heads/.
> 
> Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
> ---
> 
> 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· Apr 16, 2012, 22:08 UTC · re: marcnarc@xiplink.com · lore

[PATCHv4 0/3] fetch: Only call a new ref a "branch" if it's under refs/heads/.

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· Apr 16, 2012, 22:08 UTC · re: marcnarc@xiplink.com · lore

[PATCH 1/3] submodules: recursive fetch also checks new tags for submodule commits

From: Jens Lehmann <Jens.Lehmann@web.de>

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 <peff@peff.net>
Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
 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· Apr 16, 2012, 22:08 UTC · re: marcnarc@xiplink.com · lore

[PATCH 2/3] fetch: Pass both the full remote ref and its short name to update_local_ref().

From: Marc Branchaud <marcnarc@xiplink.com>
Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
 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(&note);
 			if (ref) {
-				rc |= update_local_ref(ref, what, &note);
+			  rc |= update_local_ref(ref, what, rm, &note);
 				free(ref);
 			} else
 				strbuf_addf(&note, "* %-*s %-*s -> FETCH_HEAD",

-- 
1.7.10.2.ge89da.dirty
marcnarc@xiplink.com· Apr 16, 2012, 22:08 UTC · re: marcnarc@xiplink.com · lore

[PATCH 3/3] fetch: Use the remote's ref name to decide how to describe new refs.

From: Marc Branchaud <marcnarc@xiplink.com>
Also, only call a new ref a "branch" if it's under refs/heads/.
Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
 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· Apr 16, 2012, 22:34 UTC · re: marcnarc@xiplink.com · lore

Re: [PATCH 3/3] fetch: Use the remote's ref name to decide how to describe new refs.

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· Apr 17, 2012, 07:53 UTC · re: Jonathan Nieder · lore

Re: [PATCH 3/3] fetch: Use the remote's ref name to decide how to describe new refs.

On 04/17/2012 12:34 AM, Jonathan Nieder wrote:
Show 13 quoted lines
> 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:
Show 7 quoted lines
>> +	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· Apr 17, 2012, 07:57 UTC · re: Zbigniew Jędrzejewski-Szmek · lore

Re: [PATCH 3/3] fetch: Use the remote's ref name to decide how to describe new refs.

Zbigniew Jędrzejewski-Szmek wrote:
> On 04/17/2012 12:34 AM, Jonathan Nieder wrote:
>> marcnarc@xiplink.com wrote:
Show 10 quoted lines
>>> +	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· Apr 17, 2012, 08:39 UTC · re: Jonathan Nieder · lore

Re: [PATCH 3/3] fetch: Use the remote's ref name to decide how to describe new refs.

On 04/17/2012 09:57 AM, Jonathan Nieder wrote:
Show 18 quoted lines
> 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:
Show 6 quoted lines
>  - 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· Apr 17, 2012, 14:23 UTC · re: Zbigniew Jędrzejewski-Szmek · lore

Re: [PATCH 3/3] fetch: Use the remote's ref name to decide how to describe new refs.

On 12-04-17 03:53 AM, Zbigniew Jędrzejewski-Szmek wrote:
Show 16 quoted lines
> 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.)

Show 10 quoted lines
> 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· Apr 17, 2012, 15:18 UTC · re: Marc Branchaud · lore

Re: [PATCH 3/3] fetch: Use the remote's ref name to decide how to describe new refs.

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· Apr 17, 2012, 15:26 UTC · re: marcnarc@xiplink.com · lore

Re: [PATCHv4 0/3] fetch: Only call a new ref a "branch" if it's under refs/heads/.

marcnarc@xiplink.com writes:
Show 6 quoted lines
> 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 <marcnarc@xiplink.com>
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 <marcnarc@xiplink.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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(&note);
 			if (ref) {
-				rc |= update_local_ref(ref, what, &note);
+				rc |= update_local_ref(ref, what, rm, &note);
 				free(ref);
 			} else
 				strbuf_addf(&note, "* %-*s %-*s -> FETCH_HEAD",

-- 
1.7.10.332.g1863c
Junio C Hamano· Apr 17, 2012, 15:28 UTC · re: Junio C Hamano · lore

Re: [PATCHv4 0/3] fetch: Only call a new ref a "branch" if it's under refs/heads/.

From: Marc Branchaud <marcnarc@xiplink.com>
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 <marcnarc@xiplink.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * 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· Apr 17, 2012, 19:30 UTC · re: Junio C Hamano · lore

Re: [PATCHv4 0/3] fetch: Only call a new ref a "branch" if it's under refs/heads/.

On 12-04-17 11:26 AM, Junio C Hamano wrote:
Show 19 quoted lines
> 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· Apr 17, 2012, 22:29 UTC · re: marcnarc@xiplink.com · lore

Re: [PATCHv4 0/3] fetch: Only call a new ref a "branch" if it's under refs/heads/.

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
Jonathan Nieder· Apr 16, 2012, 16:08 UTC · re: Jeff King · lore

Re: [PATCHv2] fetch: Only call a new ref a "branch" if it's under refs/heads/.

Jeff King wrote:
> On Mon, Apr 16, 2012 at 10:58:21AM -0400, Marc Branchaud wrote:
Show 6 quoted lines
>> 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.

← back to recent threads