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

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

[marcnarc]

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

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

[Jonathan Nieder]

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

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

[Jeff King]

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

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

[Jonathan Nieder]

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

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

[Junio C Hamano]

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 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.

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

[Marc Branchaud]

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.

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

[Marc Branchaud]

On 12-04-13 06:39 PM, Junio C Hamano wrote:
> 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.

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

[Jeff King]

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

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

[marcnarc]

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

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

[Jonathan Nieder]

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.

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

[Jonathan Nieder]

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.

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

[Junio C Hamano]

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.

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

[Marc Branchaud]

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 <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))

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

[marcnarc]

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.

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

[marcnarc]

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

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

[marcnarc]

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

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

[marcnarc]

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

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

[Jonathan Nieder]

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

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

[Zbigniew Jędrzejewski-Szmek]

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

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

[Jonathan Nieder]

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

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

[Zbigniew Jędrzejewski-Szmek]

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

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

[Marc Branchaud]

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.

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

[Jonathan Nieder]

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

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

[Junio C Hamano]

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 <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

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

[Junio C Hamano]

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

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

[Marc Branchaud]

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.

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

[Jeff King]

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