threads / bug / 14171

bug related to branches using / in name

Subject: bug related to branches using / in name

## tl;dr

13 messages between Jun 26, 2008 and Jun 28, 2008.

replies: 12people: 4as markdown or json

Simon Holm Thøgersen· Jun 26, 2008, 19:42 UTC · lore
Hi git community

I have a bug report I think is most easily explained by a list of commands that illustrates the bug.

cd /tmp mkdir git-bug cd git-bug git init touch foo git add foo git commit -a -m '...' git branch sched git clone file:///tmp/git-bug /tmp/git-bug-clone git branch -d sched git branch sched/urgent git branch sched/devel cd /tmp/git-bug-clone git pull

The last command does not succeed, but produces the following output

error: unable to resolve reference refs/remotes/origin/sched/devel: Not a directory

>From file:///tmp/git-bug
 * [new branch]      sched/devel -> origin/sched/devel
error: unable to resolve reference refs/remotes/origin/sched/urgent: Not
a directory
 * [new branch]      sched/urgent -> origin/sched/urgent

I can work around it by rm /tmp/git-bug-clone/.git/{,logs/}refs/remotes/origin/sched

but git should take care of this automatically, right?
I'm on git version 1.5.6.
Simon Holm Thøgersen
Jeff King· Jun 27, 2008, 03:02 UTC · re: Simon Holm Thøgersen · lore

Re: bug related to branches using / in name

On Thu, Jun 26, 2008 at 09:42:30PM +0200, Simon Holm Thøgersen wrote:
Show 9 quoted lines
> The last command does not succeed, but produces the following output
> 
> error: unable to resolve reference refs/remotes/origin/sched/devel: Not
> a directory
> >From file:///tmp/git-bug
>  * [new branch]      sched/devel -> origin/sched/devel
> error: unable to resolve reference refs/remotes/origin/sched/urgent: Not
> a directory
>  * [new branch]      sched/urgent -> origin/sched/urgent

So to summarize, the problem is that you have an old tracking branch "refs/remotes/sched" that exists on the client, but upstream has since removed it in favor of "sched/devel", which you are now trying to fetch. And of course there is a conflict, because of the ref naming rules.

This doesn't work seamlessly because git-fetch never removes old tracking branches, even if they have been deleted upstream. And normally there is no conflict; leaving the old branches means they don't disappear from under you.

In this case, of course, it's inconvenient. The "right" way to solve it is to tell git to clean up your tracking branches for "origin":

  git remote prune origin

which should delete the old "sched" tracking branch (since it no longer exists on the remote), and then you are free to fetch.

It might be nicer if this were handled automatically, but it would violate git-fetch's rule about never deleting branches. And presumably this comes up infrequently enough that it isn't a problem. Perhaps a better error message suggesting git-prune might make sense?

-Peff
Jeff King· Jun 27, 2008, 03:04 UTC · re: Jeff King · lore

Re: bug related to branches using / in name

On Thu, Jun 26, 2008 at 11:02:46PM -0400, Jeff King wrote:
Show 9 quoted lines
> So to summarize, the problem is that you have an old tracking branch
> "refs/remotes/sched" that exists on the client, but upstream has since
> removed it in favor of "sched/devel", which you are now trying to fetch.
> And of course there is a conflict, because of the ref naming rules.
> 
> This doesn't work seamlessly because git-fetch never removes old
> tracking branches, even if they have been deleted upstream. And normally
> there is no conflict; leaving the old branches means they don't
> disappear from under you.

BTW, you can get the opposite problem, too. If you have "refs/remotes/origin/foo/bar", and then the remote removes "foo/bar" in favor of "foo", you will have a conflict on fetch.

-Peff
Simon Holm Thøgersen· Jun 27, 2008, 08:32 UTC · re: Jeff King · lore

Re: bug related to branches using / in name

Jeff King wrote:
Show 12 quoted lines
> Jeff King wrote:
> 
> > So to summarize, the problem is that you have an old tracking branch
> > "refs/remotes/sched" that exists on the client, but upstream has since
> > removed it in favor of "sched/devel", which you are now trying to fetch.
> > And of course there is a conflict, because of the ref naming rules.
> > 
> > This doesn't work seamlessly because git-fetch never removes old
> > tracking branches, even if they have been deleted upstream. And normally
> > there is no conflict; leaving the old branches means they don't
> > disappear from under you.
> 

Your summary is correct, but I cannot entirely convince myself that leaving old branches available is valid in any work flow. But what do I know.

> BTW, you can get the opposite problem, too. If you have
> "refs/remotes/origin/foo/bar", and then the remote removes "foo/bar" in
> favor of "foo", you will have a conflict on fetch.
> 

Yes, and you can also hit a similar problem using git-push, but I guess that the user is a bit more aware about what is going on in that case and is able resolve the problem without hints.

I tested the two patches and they did indeed seem to do what you intended them to for my test case. The solution is not exactly pretty, but it is better than nothing and it is admittedly a corner case.

Thank you for your time on this Jeff.
Simon Holm Thøgersen
Jeff King· Jun 27, 2008, 03:57 UTC · re: Jeff King · lore

Re: bug related to branches using / in name

On Thu, Jun 26, 2008 at 11:02:46PM -0400, Jeff King wrote:
> It might be nicer if this were handled automatically, but it would
> violate git-fetch's rule about never deleting branches. And presumably
> this comes up infrequently enough that it isn't a problem. Perhaps a
> better error message suggesting git-prune might make sense?

And here is a 2-patch series improving the error reporting for this case (and it also helps with some other cases). I'm a little iffy on 2/2, since we don't actually _know_ that remote pruning will help. But propagating specific errors up through the call chain would require reasonably major surgery.

  1/2: fetch: give a hint to the user when local refs fail to update
  2/2: fetch: report local storage errors in status table
-Peff
Jeff King· Jun 27, 2008, 03:59 UTC · re: Jeff King · lore

[PATCH] fetch: report local storage errors in status table

Previously, if there was an error while storing a local tracking ref, the low-level functions would report an error, but fetch's status output wouldn't indicate any problem. E.g., imagine you have an old "refs/remotes/origin/foo/bar" but upstream has deleted "foo/bar" in favor of a new branch "foo". You would get output like this:

  error: there are still refs under 'refs/remotes/origin/foo'
  From $url_of_repo
   * [new branch]      foo        -> origin/foo

With this patch, the output takes into account the status of updating the local ref:

  error: there are still refs under 'refs/remotes/origin/foo'
  From $url_of_repo
   ! [new branch]      foo        -> origin/foo  (unable to update local ref)
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-fetch.c |   36 ++++++++++++++++++++++++------------
 1 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/builtin-fetch.c b/builtin-fetch.c
index e81ee2d..7c16d38 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -233,10 +233,12 @@ static int update_local_ref(struct ref *ref,
 
 	if (!is_null_sha1(ref->old_sha1) &&
 	    !prefixcmp(ref->name, "refs/tags/")) {
-		sprintf(display, "- %-*s %-*s -> %s",
+		int r;
+		r = s_update_ref("updating tag", ref, 0);
+		sprintf(display, "%c %-*s %-*s -> %s%s", r ? '!' : '-',
 			SUMMARY_WIDTH, "[tag update]", REFCOL_WIDTH, remote,
-			pretty_ref);
-		return s_update_ref("updating tag", ref, 0);
+			pretty_ref, r ? "  (unable to update local ref)" : "");
+		return r;
 	}
 
 	current = lookup_commit_reference_gently(ref->old_sha1, 1);
@@ -244,6 +246,7 @@ static int update_local_ref(struct ref *ref,
 	if (!current || !updated) {
 		const char *msg;
 		const char *what;
+		int r;
 		if (!strncmp(ref->name, "refs/tags/", 10)) {
 			msg = "storing tag";
 			what = "[new tag]";
@@ -253,27 +256,36 @@ static int update_local_ref(struct ref *ref,
 			what = "[new branch]";
 		}
 
-		sprintf(display, "* %-*s %-*s -> %s", SUMMARY_WIDTH, what,
-			REFCOL_WIDTH, remote, pretty_ref);
-		return s_update_ref(msg, ref, 0);
+		r = s_update_ref(msg, ref, 0);
+		sprintf(display, "%c %-*s %-*s -> %s%s", r ? '!' : '*',
+			SUMMARY_WIDTH, what, REFCOL_WIDTH, remote, pretty_ref,
+			r ? "  (unable to update local ref)" : "");
+		return r;
 	}
 
 	if (in_merge_bases(current, &updated, 1)) {
 		char quickref[83];
+		int r;
 		strcpy(quickref, find_unique_abbrev(current->object.sha1, DEFAULT_ABBREV));
 		strcat(quickref, "..");
 		strcat(quickref, find_unique_abbrev(ref->new_sha1, DEFAULT_ABBREV));
-		sprintf(display, "  %-*s %-*s -> %s", SUMMARY_WIDTH, quickref,
-			REFCOL_WIDTH, remote, pretty_ref);
-		return s_update_ref("fast forward", ref, 1);
+		r = s_update_ref("fast forward", ref, 1);
+		sprintf(display, "%c %-*s %-*s -> %s%s", r ? '!' : ' ',
+			SUMMARY_WIDTH, quickref, REFCOL_WIDTH, remote,
+			pretty_ref, r ? "  (unable to update local ref)" : "");
+		return r;
 	} else if (force || ref->force) {
 		char quickref[84];
+		int r;
 		strcpy(quickref, find_unique_abbrev(current->object.sha1, DEFAULT_ABBREV));
 		strcat(quickref, "...");
 		strcat(quickref, find_unique_abbrev(ref->new_sha1, DEFAULT_ABBREV));
-		sprintf(display, "+ %-*s %-*s -> %s  (forced update)",
-			SUMMARY_WIDTH, quickref, REFCOL_WIDTH, remote, pretty_ref);
-		return s_update_ref("forced-update", ref, 1);
+		r = s_update_ref("forced-update", ref, 1);
+		sprintf(display, "%c %-*s %-*s -> %s  (%s)", r ? '!' : '+',
+			SUMMARY_WIDTH, quickref, REFCOL_WIDTH, remote,
+			pretty_ref,
+			r ? "unable to update local ref" : "forced update");
+		return r;
 	} else {
 		sprintf(display, "! %-*s %-*s -> %s  (non fast forward)",
 			SUMMARY_WIDTH, "[rejected]", REFCOL_WIDTH, remote,
-- 
1.5.6.1.79.g7b3a7.dirty
Junio C Hamano· Jun 27, 2008, 23:37 UTC · re: Jeff King · lore

Re: [PATCH] fetch: report local storage errors in status table

Jeff King <peff@peff.net> writes:
Show 19 quoted lines
> Previously, if there was an error while storing a local
> tracking ref, the low-level functions would report an error,
> but fetch's status output wouldn't indicate any problem.
> E.g., imagine you have an old "refs/remotes/origin/foo/bar" but
> upstream has deleted "foo/bar" in favor of a new branch
> "foo". You would get output like this:
>
>   error: there are still refs under 'refs/remotes/origin/foo'
>   From $url_of_repo
>    * [new branch]      foo        -> origin/foo
>
> With this patch, the output takes into account the status of
> updating the local ref:
>
>   error: there are still refs under 'refs/remotes/origin/foo'
>   From $url_of_repo
>    ! [new branch]      foo        -> origin/foo  (unable to update local ref)
>
> Signed-off-by: Jeff King <peff@peff.net>

Makes sense --- thanks. This is something we can have automated tests, isn't it?

Jeff King· Jun 28, 2008, 04:21 UTC · re: Junio C Hamano · lore

Re: [PATCH] fetch: report local storage errors in status table

On Fri, Jun 27, 2008 at 04:37:33PM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> > With this patch, the output takes into account the status of
> > updating the local ref:
> >
> >   error: there are still refs under 'refs/remotes/origin/foo'
> >   From $url_of_repo
> >    ! [new branch]      foo        -> origin/foo  (unable to update local ref)
>
> Makes sense --- thanks.  This is something we can have automated tests,
> isn't it?

We don't currently have any tests for either the fetch output or the push output. Note that we aren't changing the output _status_. Fetch always knew that this condition was a failure, and exited appropriately. So it really would just be testing the expected human-readable output in these situations, something I thought we usually didn't include in the tests.

But if you think it is worth doing, I can whip up a few tests.
-Peff
Jeff King· Jun 27, 2008, 04:01 UTC · re: Jeff King · lore

[PATCH 2/2] fetch: give a hint to the user when local refs fail to update

There are basically two categories of update failures for local refs:

  1. problems outside of git, like disk full, bad
     permissions, etc.
  2. D/F conflicts on tracking branch ref names

In either case, there should already have been an error message. In case '1', hopefully enough information has already been given that the user can fix it. In the case of '2', we can hint that the user can clean up their tracking branch area by using 'git remote prune'.

Note that we don't actually know _which_ case we have, so the user will receive the hint in case 1, as well. In this case the suggestion won't do any good, but hopefully the user is smart enough to figure out that it's just a hint.

Signed-off-by: Jeff King <peff@peff.net>
---
Actually, I think there might be a third category, "bad ref format".
But that would only come from a malicious remote trying to send you a
badly formatted ref, so I think that is rare enough not to worry about
showing the extra hint there.
 builtin-fetch.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/builtin-fetch.c b/builtin-fetch.c
index 7c16d38..97fdc51 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -181,9 +181,9 @@ static int s_update_ref(const char *action,
 	lock = lock_any_ref_for_update(ref->name,
 				       check_old ? ref->old_sha1 : NULL, 0);
 	if (!lock)
-		return 1;
+		return 2;
 	if (write_ref_sha1(lock, ref->new_sha1, msg) < 0)
-		return 1;
+		return 2;
 	return 0;
 }
 
@@ -294,7 +294,8 @@ static int update_local_ref(struct ref *ref,
 	}
 }
 
-static int store_updated_refs(const char *url, struct ref *ref_map)
+static int store_updated_refs(const char *url, const char *remote_name,
+		struct ref *ref_map)
 {
 	FILE *fp;
 	struct commit *commit;
@@ -380,6 +381,10 @@ static int store_updated_refs(const char *url, struct ref *ref_map)
 		}
 	}
 	fclose(fp);
+	if (rc & 2)
+		error("some local refs could not be updated; try running\n"
+		      " 'git remote prune %s' to remove any old, conflicting "
+		      "branches", remote_name);
 	return rc;
 }
 
@@ -450,7 +455,9 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)
 	if (ret)
 		ret = transport_fetch_refs(transport, ref_map);
 	if (!ret)
-		ret |= store_updated_refs(transport->url, ref_map);
+		ret |= store_updated_refs(transport->url,
+				transport->remote->name,
+				ref_map);
 	transport_unlock_pack(transport);
 	return ret;
 }
-- 
1.5.6.1.79.g7b3a7.dirty
Junio C Hamano· Jun 27, 2008, 23:31 UTC · re: Jeff King · lore

Re: bug related to branches using / in name

Jeff King <peff@peff.net> writes:
> On Thu, Jun 26, 2008 at 11:02:46PM -0400, Jeff King wrote:
>
>> It might be nicer if this were handled automatically, but it would
>> violate git-fetch's rule about never deleting branches.
Hmm.  Is there actually such a rule?

I was wondering if it might make more sense to do the equivalent of what checkout_entry() does (i.e. remove_subtree()) when there is such a conflict. After all, tracking branches are meant to accept rewinds and anything that happens on the remote end, and having to run "git remote prune" is not a feature but is a lack of feature in the "git fetch", which may make it look like deletion is somewhat special.

Jeff King· Jun 28, 2008, 04:18 UTC · re: Junio C Hamano · lore

Re: bug related to branches using / in name

On Fri, Jun 27, 2008 at 04:31:30PM -0700, Junio C Hamano wrote:
> >> It might be nicer if this were handled automatically, but it would
> >> violate git-fetch's rule about never deleting branches.
> 
> Hmm.  Is there actually such a rule?

I thought so, though I don't necessarily agree with it. But I seem to recall this being touted as a feature in the past; a remote deleting some work will not cause it to be deleted locally.

> I was wondering if it might make more sense to do the equivalent of what
> checkout_entry() does (i.e. remove_subtree()) when there is such a

As as long your "equivalent of" means "branch -d"; we need to kill off both the ref and its reflogs. And therefore...

> conflict.  After all, tracking branches are meant to accept rewinds and
> anything that happens on the remote end, and having to run "git remote
> prune" is not a feature but is a lack of feature in the "git fetch", which
> may make it look like deletion is somewhat special.

The one key difference between rewinds and branch deletion is that the latter will kill off the reflog, making the history inaccessible. You can always still access rewound or rebased work via the reflog.

If we don't care about this "safety feature", then I definitely agree that we should fix the problem rather than hint to the user.

-Peff
Junio C Hamano· Jun 28, 2008, 04:57 UTC · re: Jeff King · lore

Re: bug related to branches using / in name

Jeff King <peff@peff.net> writes:
Show 6 quoted lines
> As as long your "equivalent of" means "branch -d"; we need to kill off
> both the ref and its reflogs. And therefore...
> ...
> The one key difference between rewinds and branch deletion is that the
> latter will kill off the reflog, making the history inaccessible. You
> can always still access rewound or rebased work via the reflog.
Hmm, you are right.

I somehow still had a vague distant memory from late 2006 when we did not have reflogs for remotes/ hierarchy.

Jakub Narebski· Jun 28, 2008, 11:42 UTC · re: Jeff King · lore

Re: bug related to branches using / in name

Jeff King <peff@peff.net> writes:
Show 13 quoted lines
> On Fri, Jun 27, 2008 at 04:31:30PM -0700, Junio C Hamano wrote:
>> 
>> [...]  After all, tracking branches are meant to accept rewinds and
>> anything that happens on the remote end, and having to run "git remote
>> prune" is not a feature but is a lack of feature in the "git fetch", which
>> may make it look like deletion is somewhat special.
> 
> The one key difference between rewinds and branch deletion is that the
> latter will kill off the reflog, making the history inaccessible. You
> can always still access rewound or rebased work via the reflog.
> 
> If we don't care about this "safety feature", then I definitely agree
> that we should fix the problem rather than hint to the user.

There were some attempts to add some kind of "Attic" to save reflogs for deleted branches, but IIRC the discussion petered out after few initial patches because there were some conflict over details of implementation (the problem being how to deal with D/F conflicts).

If this makes into git, this trouble will disappear... perhaps with some stronger marker that branches can dissapear, not only rewind

        +!refs/heads/*:refs/remotes/origin/*
or
        +refs/heads/*?:refs/remotes/origin/*?
-- 
Jakub Narebski
Poland
ShadeHawk on #git

← back to recent threads