bug related to branches using / in name

bug related to branches using / in name

[Simon Holm Thøgersen]

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

Re: bug related to branches using / in name

[Jeff King]

On Thu, Jun 26, 2008 at 09:42:30PM +0200, Simon Holm Thøgersen wrote:

> 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

Re: bug related to branches using / in name

[Jeff King]

On Thu, Jun 26, 2008 at 11:02:46PM -0400, 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.

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

Re: bug related to branches using / in name

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

Re: bug related to branches using / in name

[Simon Holm Thøgersen]

Jeff King wrote:
> 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

Re: bug related to branches using / in name

[Junio C Hamano]

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.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Re: bug related to branches using / in name

[Jeff King]

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

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

[Jeff King]

On Fri, Jun 27, 2008 at 04:37:33PM -0700, Junio C Hamano wrote:

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

Re: bug related to branches using / in name

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Re: bug related to branches using / in name

[Jakub Narebski]

Jeff King <peff@peff.net> writes:
> 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