rebase from ambiguous ref discards changes

rebase from ambiguous ref discards changes

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1768 bytes --]

So, I started with a very simple repository

---*--- master
    \
     -- origin/master

From master, I did

$ git-rebase origin/master
warning: refname 'master' is ambiguous.
First, rewinding head to replay your work on top of it...
HEAD is now at 2a8592f... Fix G33 GTT stolen mem range
Fast-forwarded master to origin/master.
$

Note the lack of the usual 'Applying <patch>' messages.

checking the tree, I now had

---*
    \
     -- origin/master
        master

with my patch lost.

recovering my patch (having the ID in my terminal window from the
commit), I named it 'master-with-fix'

---*--- master-with-fix
    \
     -- origin/master
        master

Now the rebase from 'master-with-fix worked as expected:

$ git-rebase origin/master
First, rewinding head to replay your work on top of it...
HEAD is now at 2a8592f... Fix G33 GTT stolen mem range

Applying Switch to pci_device_map_range/pci_device_unmap_range APIs.

Adds trailing whitespace.
.dotest/patch:225:      
Adds trailing whitespace.
.dotest/patch:226:      if (IS_I965G(pI830)) 
Adds trailing whitespace.
.dotest/patch:446:
dev->regions[mmio_bar].size, 
Adds trailing whitespace.
.dotest/patch:449:    
warning: 4 lines add whitespace errors.
Wrote tree cd373666254d56a137d282deeb15a2ccaf8da22b
Committed: 286f5df0b62f571cbb4dbf120679d3af029b8775
$ 

And the tree looks right too:

--- origin/master --- master-with-fix
    master
       
Seems like there's something going on when 'master' is ambiguous, or
perhaps some other problem.

This is all from version 1.5.3, but I think I've seen this on 1.5.2 as
well.

Git made me sad today; I'm not sure it's ever disappointed like this
before.

-- 
keith.packard@intel.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: rebase from ambiguous ref discards changes

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 622 bytes --]

On Thu, Sep 06, 2007 at 09:48:28PM +0000, Keith Packard wrote:
> recovering my patch (having the ID in my terminal window from the
> commit), I named it 'master-with-fix'

  Note that your patch was not lost, even if you had pruned, because it
would still be accessible from the reflog of your master branch. Not
that it's supposed to soften your disappointment but well, at least it
should make you a bit less uncomfortable, maybe :)

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: rebase from ambiguous ref discards changes

[Junio C Hamano]

Keith Packard <keithp@keithp.com> writes:

> So, I started with a very simple repository
>
> ---*--- master
>     \
>      -- origin/master
>
> From master, I did
>
> $ git-rebase origin/master
> warning: refname 'master' is ambiguous.
> First, rewinding head to replay your work on top of it...
> HEAD is now at 2a8592f... Fix G33 GTT stolen mem range
> Fast-forwarded master to origin/master.
> ...
> Seems like there's something going on when 'master' is ambiguous, or
> perhaps some other problem.
>
> This is all from version 1.5.3, but I think I've seen this on 1.5.2 as
> well.

We haven't touched this area for a long time (like "v1.3.0" or
"since March 2006").  I wish you told us about this earlier.

I'd like to reproduce this, but I need to be sure what your
"ambiguous" situation is really like.  What ambiguous "master"s
do you have?  IOW, what does:

	git show-ref | grep master

say?  I have 11 lines of output from the above and I have never
seen a problem like this.

Perhaps you have ".git/master" by mistake?

Re: rebase from ambiguous ref discards changes

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1713 bytes --]

On Thu, 2007-09-06 at 16:26 -0700, Junio C Hamano wrote:

> We haven't touched this area for a long time (like "v1.3.0" or
> "since March 2006").  I wish you told us about this earlier.

I would have were I certain that it wasn't user-error last time. I took
a few minutes this time to verify precisely what I saw and reproduce it.

> I'd like to reproduce this, but I need to be sure what your
> "ambiguous" situation is really like.  What ambiguous "master"s
> do you have?  IOW, what does:
> 
> 	git show-ref | grep master

$ git show-ref | grep master
286f5df0b62f571cbb4dbf120679d3af029b8775 refs/heads/master
1feb733eb8b09a8b07b7a6987add5149c53b0157 refs/heads/master-guitar
d957c6b8e1dde8e11c1db3431e0ff58c5d984880 refs/heads/master-i830
0fd3ba0518b3cde9ca0e4e2fc1854c00d8a43d5c refs/remotes/kyle/master
e25f8e145f4f73b62c32389b922291fd561af9d2 refs/remotes/origin/lg3d-master
286f5df0b62f571cbb4dbf120679d3af029b8775 refs/remotes/origin/master
e25f8e145f4f73b62c32389b922291fd561af9d2 refs/remotes/otc/lg3d-master
6781575f734f05547d7d5ceef4116fc157bba44d refs/remotes/otc/master

so, nothing obviously amiss here.

> Perhaps you have ".git/master" by mistake?

oops.

$ find .git -name master
.git/master
.git/refs/heads/master
.git/refs/remotes/kyle/master
.git/refs/remotes/origin/master
.git/refs/remotes/otc/master
.git/logs/refs/heads/master
.git/logs/refs/remotes/fdo/master
.git/logs/refs/remotes/kyle/master
.git/logs/refs/remotes/origin/master
.git/logs/refs/remotes/otc/master

So, I think that explains where the ambiguous master came from.  Seems
like rebase should be able to bail out before breaking things though.

-- 
keith.packard@intel.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: rebase from ambiguous ref discards changes

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1043 bytes --]

On Fri, Sep 07, 2007 at 02:58:18AM +0000, Keith Packard wrote:
> On Thu, 2007-09-06 at 16:26 -0700, Junio C Hamano wrote:
>
> > Perhaps you have ".git/master" by mistake?
>
> oops.
>
> $ find .git -name master
> ..git/master
> ..git/refs/heads/master
> ..git/refs/remotes/kyle/master
> ..git/refs/remotes/origin/master
> ..git/refs/remotes/otc/master
> ..git/logs/refs/heads/master
> ..git/logs/refs/remotes/fdo/master
> ..git/logs/refs/remotes/kyle/master
> ..git/logs/refs/remotes/origin/master
> ..git/logs/refs/remotes/otc/master
> 
> So, I think that explains where the ambiguous master came from.  Seems
> like rebase should be able to bail out before breaking things though.

  Actually if I get this right, it didn't broke anything, it just
rebased your ".git/master" :) It just chose the wrong desambiguation for
some reason.



-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH] HEAD, ORIG_HEAD and FETCH_HEAD are really special.

[Junio C Hamano]

Keith Packard <keithp@keithp.com> writes:

> On Thu, 2007-09-06 at 16:26 -0700, Junio C Hamano wrote:
> ...
>> Perhaps you have ".git/master" by mistake?
>
> oops.

Ok, that explains it; git is doing exactly what the user asked
it to do.

              5---6 side
             /
	1---2---3---4 master

The user has this history.  But he has a stray .git/master file,
perhaps created by hand by mistake (it would be very interesting
to find how that file got there in the first place), that points
at commit "3".  From a side branch, he says "git rebase master".

The first parameter to "git rebase" in a single parameter form
is "the commit on which to replay the changes my current branch
has".  It is not limited to a branch name.  IOW, it can be an
arbitrary object name, and one of the rules to translate a user
string that is supposed to mean an arbitrary object name goes
through this table:

        "%s", "refs/%s", "refs/tags/%s", "refs/heads/%s",
        "refs/remotes/%s", "refs/remotes/%s/HEAD"

For each entry in the above table, "%s" part of the entry is
replaced by the user string, and resulting string is used to see
if there is such a file under .git/ directory, and the first
match is used (this explanation is simplifying things a bit).

This rule has been there almost from the beginning.  The first
entry allowed you to have a tag A and a branch A at the same
time, while giving you a way to disambiguate them by spelling
them out as "refs/tags/A" and "refs/heads/A", respectively.
Also the first rule covered special "ref" names such as HEAD and
ORIG_HEAD.

Alas, there is one drawback of this rule.  His .git/master hides
refs/heads/master.  So essentially his command line said "I've
built a few commits since I forked from the history that leads
to commit 3; I want to replay my changes on top of that commit".
He meant to say 4, but said 3, and as a unfortunate consequence,
commit 4 is lost.

The above explanation is solely for understanding the situation
and, not meant to defend the current behaviour.  I think the
current behaviour is inviting mistakes and confusion.

This patch brings in a new world order by introducing a backward
incompatible change.  When the string the user gave us does not
contain any slash, we do not apply the first entry (i.e.
directly underneath .git/ without any "refs/***") unless the
name consists solely of uppercase letters or an underscore,
thereby ignoring .git/master.  The ones we often use, such as
HEAD and ORIG_HEAD are not affected by this change.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1_name.c                |   35 ++++++++++++++++++++++++++++++++++
 t/t3407-rebase-confused.sh |   45 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+), 0 deletions(-)
 create mode 100755 t/t3407-rebase-confused.sh

diff --git a/sha1_name.c b/sha1_name.c
index 2d727d5..1b980ca 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -239,6 +239,35 @@ static int ambiguous_path(const char *path, int len)
 	return slash;
 }
 
+static int confused_ref(const char *str)
+{
+	char ch;
+	int seen_non_uppercase = 0;
+
+	/*
+	 * People create .git/master by mistake using hand-rolled
+	 * scripts and confuse themselves utterly.  Make sure this
+	 * does not match such a path.
+	 */
+	while ((ch = *str++) != '\0') {
+		if (ch == '/')
+			return 0;
+		if (!((ch == '_') ||
+		      ('A' <= ch && ch <= 'Z') ||
+		      ('0' <= ch && ch <= '9')))
+			seen_non_uppercase = 1;
+	}
+
+	/*
+	 * str did not have any slash and we are checking when
+	 * it hangs directly underneath .git/; the only valid
+	 * cases we currently have are HEAD, ORIG_HEAD, MERGE_HEAD and
+	 * FETCH_HEAD.  If we saw any non uppercase, non underscore,
+	 * we should ignore this string.
+	 */
+	return seen_non_uppercase;
+}
+
 static const char *ref_fmt[] = {
 	"%.*s",
 	"refs/%.*s",
@@ -259,6 +288,9 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
 		unsigned char sha1_from_ref[20];
 		unsigned char *this_result;
 
+		if ((p == ref_fmt) && confused_ref(str))
+			continue;
+
 		this_result = refs_found ? sha1_from_ref : sha1;
 		r = resolve_ref(mkpath(*p, len, str), this_result, 1, NULL);
 		if (r) {
@@ -283,6 +315,9 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
 		char path[PATH_MAX];
 		const char *ref, *it;
 
+		if (p == ref_fmt && confused_ref(str))
+			continue;
+
 		strcpy(path, mkpath(*p, len, str));
 		ref = resolve_ref(path, hash, 0, NULL);
 		if (!ref)
diff --git a/t/t3407-rebase-confused.sh b/t/t3407-rebase-confused.sh
new file mode 100755
index 0000000..5254da6
--- /dev/null
+++ b/t/t3407-rebase-confused.sh
@@ -0,0 +1,45 @@
+#!/bin/sh
+
+test_description="Keithp's .git/master problem
+
+              5---6 side
+             /
+	1---2---3---4 master
+
+"
+
+. ./test-lib.sh
+
+build () {
+	echo "$1" >"$1" && git add "$1" && test_tick && git commit -m "$1"
+}
+
+test_expect_success setup '
+
+	build one &&
+	build two &&
+	git branch side &&
+	build three &&
+	git tag anchor &&
+	build four &&
+	git-checkout side &&
+	build five &&
+	build six &&
+	git update-ref master anchor &&
+	git tag -d anchor
+
+'
+
+test_expect_success rebase '
+
+	git rebase master
+
+'
+
+test_expect_success 'everybody is still there' '
+
+	test 6 = $(git log --pretty=oneline HEAD | wc -l)
+
+'
+
+test_done
-- 
1.5.3.1.879.g4d83f

Re: [PATCH] HEAD, ORIG_HEAD and FETCH_HEAD are really special.

[Johannes Sixt]

Junio C Hamano schrieb:
> But he has a stray .git/master file,
> perhaps created by hand by mistake (it would be very interesting
> to find how that file got there in the first place),

It is easy to get one there if, in a brave moment, you try

    git update-ref master $some_other_ref

instead of the correct

    git update-ref refs/heads/master $some_other_ref

-- Hannes

Re: [PATCH] HEAD, ORIG_HEAD and FETCH_HEAD are really special.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1235 bytes --]

On Fri, Sep 07, 2007 at 12:36:15PM +0000, Johannes Sixt wrote:
> Junio C Hamano schrieb:
> >But he has a stray .git/master file,
> >perhaps created by hand by mistake (it would be very interesting
> >to find how that file got there in the first place),
> 
> It is easy to get one there if, in a brave moment, you try
> 
>    git update-ref master $some_other_ref
> 
> instead of the correct
> 
>    git update-ref refs/heads/master $some_other_ref

  I was about to say the same :)
  I'd have added though that maybe update-ref should print a warning for
the references that do not match the restriction Junio added. This could
be done using the function Junio proposed un update_ref() in refs.c

  note that it's a sane thing to do anyways, I would not bet a lot of
money on what happens if you ask git to:

  git update-ref $foo $sha

  for foo in (completely random :P): index config packed-refs ...

  If a tool needs a new reference namespace, it can create a
subdirectory under refs/ so it does not really causes harm IMHO.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] HEAD, ORIG_HEAD and FETCH_HEAD are really special.

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> I'd have added though that maybe update-ref should print a warning for
> the references that do not match the restriction Junio added. This could
> be done using the function Junio proposed un update_ref() in refs.c

I would even suggest making it into an error, even if we do not
error out on the reading side (being liberal when reading but
more strict when creating, that is).

That confused_ref() needs to be tightened further, by the way.
It is called only when we are considering to tack the user
string immediately below $GIT_DIR/ so the only valid cases are
(1) the string begins with "refs/", or (2) the string is all
uppercase (or underscore), especially without slash.  The one in
the proposed patch is not strict enough and does not enforce the
former.

Re: [PATCH] HEAD, ORIG_HEAD and FETCH_HEAD are really special.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]

On Fri, Sep 07, 2007 at 08:39:43PM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > I'd have added though that maybe update-ref should print a warning for
> > the references that do not match the restriction Junio added. This could
> > be done using the function Junio proposed un update_ref() in refs.c
> 
> I would even suggest making it into an error, even if we do not
> error out on the reading side (being liberal when reading but
> more strict when creating, that is).
> 
> That confused_ref() needs to be tightened further, by the way.
> It is called only when we are considering to tack the user
> string immediately below $GIT_DIR/ so the only valid cases are
> (1) the string begins with "refs/", or (2) the string is all
> uppercase (or underscore), especially without slash.  The one in
> the proposed patch is not strict enough and does not enforce the
> former.

  I reckon I didn't checked what the function did in detail, just the
code layout :) And I agree an error is event better, I just don't have
enough knowledge of the scripts that used refs in git for a long time
that such a change could break. I mean, I only use git since the 1.2
(maybe even 1.3) series :)

  I'm always all for refusing dangerous layouts rather than trying too
hard to support cumbersome things that are 99% of the times issues :)

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] HEAD, ORIG_HEAD and FETCH_HEAD are really special.

[Alex Riesen]

Junio C Hamano, Fri, Sep 07, 2007 22:39:43 +0200:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > I'd have added though that maybe update-ref should print a warning for
> > the references that do not match the restriction Junio added. This could
> > be done using the function Junio proposed un update_ref() in refs.c
> 
> I would even suggest making it into an error, even if we do not
> error out on the reading side (being liberal when reading but
> more strict when creating, that is).

I agree (and suggest failing even on reading), but see below

> That confused_ref() needs to be tightened further, by the way.
> It is called only when we are considering to tack the user
> string immediately below $GIT_DIR/ so the only valid cases are
> (1) the string begins with "refs/",

If that will be the case git-p4-import.bat (yes, just a script of
mine) will break because it has its namespace directly in $GIT_DIR
(i.e. .git/p4/*) and stores there backup references. It is just a
someones (ok, it is mine) script, but maybe there are others, who
expect that plumbing level git-update-ref just do what its told.

> or (2) the string is all uppercase (or underscore), especially
> without slash.

I'd suggest just check for uppercase+underscore _or_ slash. It is
plumbing after all.

Re: [PATCH] HEAD, ORIG_HEAD and FETCH_HEAD are really special.

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 311 bytes --]

On Fri, 2007-09-07 at 14:36 +0200, Johannes Sixt wrote:

>     git update-ref master $some_other_ref

I'm sure that's how it was created; I was using update-ref on a regular
basis for a few weeks with this repository before the 1.5 new world
order for remotes came about.

-- 
keith.packard@intel.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] HEAD, ORIG_HEAD and FETCH_HEAD are really special.

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 818 bytes --]

On Fri, 2007-09-07 at 04:21 -0700, Junio C Hamano wrote:

> This patch brings in a new world order by introducing a backward
> incompatible change.  When the string the user gave us does not
> contain any slash, we do not apply the first entry (i.e.
> directly underneath .git/ without any "refs/***") unless the
> name consists solely of uppercase letters or an underscore,
> thereby ignoring .git/master.  The ones we often use, such as
> HEAD and ORIG_HEAD are not affected by this change.

It seems to me that instead of introducing an incompatible (but probably
useful) change, a sensible option would be to have the ambiguous
reference be an error instead of a warning. One shouldn't be encouraged
to use names in .git that conflict with stuff in refs/heads anyway.

-- 
keith.packard@intel.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] HEAD, ORIG_HEAD and FETCH_HEAD are really special.

[Nicolas Pitre]

On Fri, 7 Sep 2007, Keith Packard wrote:

> On Fri, 2007-09-07 at 04:21 -0700, Junio C Hamano wrote:
> 
> > This patch brings in a new world order by introducing a backward
> > incompatible change.  When the string the user gave us does not
> > contain any slash, we do not apply the first entry (i.e.
> > directly underneath .git/ without any "refs/***") unless the
> > name consists solely of uppercase letters or an underscore,
> > thereby ignoring .git/master.  The ones we often use, such as
> > HEAD and ORIG_HEAD are not affected by this change.
> 
> It seems to me that instead of introducing an incompatible (but probably
> useful) change, a sensible option would be to have the ambiguous
> reference be an error instead of a warning. One shouldn't be encouraged
> to use names in .git that conflict with stuff in refs/heads anyway.

I agree.  IMHO the sensible thing to do is to always warn, and error out 
by default.  I see no advantage for core.warnAmbiguousRefs=false other 
than allow the user to shoot himself in the foot someday.  Instead, we 
should have core.allowAmbiguousRefs set to off by default.


Nicolas

Re: [PATCH] HEAD, ORIG_HEAD and FETCH_HEAD are really special.

[Junio C Hamano]

Nicolas Pitre <nico@cam.org> writes:

>> It seems to me that instead of introducing an incompatible (but probably
>> useful) change, a sensible option would be to have the ambiguous
>> reference be an error instead of a warning. One shouldn't be encouraged
>> to use names in .git that conflict with stuff in refs/heads anyway.
>
> I agree.  IMHO the sensible thing to do is to always warn, and error out 
> by default.  I see no advantage for core.warnAmbiguousRefs=false other 
> than allow the user to shoot himself in the foot someday.  Instead, we 
> should have core.allowAmbiguousRefs set to off by default.

Well, for about three weeks late November to early December
2005, we did make this an error.  Since mid December 2005, we
reverted that change to the original "take first match, without
even attempting to detect ambiguity".  I do not recall what the
discussion that led to that change was about, but it could have
been the issue Len had that confused "git merge" with a tag and
a branch named after bugzilla bug number.  In any case, this
change most likely was because some people were actually using
the same name and the change to make it an error hurted them.

We then reintroduced the ambiguity detection late March 2006,
but only as a warning, again fearing that erroring out would
break people's existing setups.  I think we also rewrote
examples in our documentation that said "create your own branch
v2.6.13 that fork from v2.6.13 tag" to read "create your own
brancy my-2.6.13..." to avoid encouraging the use of same name
to people.

I think the warning has been with us for a long time and by now
people know better not to confuse themselves.

So I am all for making an ambiguous refname an error in 1.5.4.

At the same time, I think it makes sense to forbid update-ref
outside refs/ if the refname is not special (say, with any
lowercase letters), and ignore names immediately below .git that
are not all-uppercase+underscore (e.g. "FETCH_HEAD" we read,
"description" we ignore).

Please make it so.

Re: [PATCH] HEAD, ORIG_HEAD and FETCH_HEAD are really special.

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 468 bytes --]

On Fri, 07 Sep 2007 13:32:30 -0700, Junio C Hamano wrote:
> So I am all for making an ambiguous refname an error in 1.5.4.

If you do, then please also make it an error to create an ambiguous
refname as well.

For example, look at how late the warning comes out in this case, and
how changing it to an error at that point would not help anything:

	$ git branch tmp
	$ ...
	$ git tag tmp	# No warning here!
	$ git show tmp
	warning: refname 'tmp' is ambiguous.

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] HEAD, ORIG_HEAD and FETCH_HEAD are really special.

[Junio C Hamano]

Carl Worth <cworth@cworth.org> writes:

> On Fri, 07 Sep 2007 13:32:30 -0700, Junio C Hamano wrote:
>> So I am all for making an ambiguous refname an error in 1.5.4.
>
> If you do, then please also make it an error to create an ambiguous
> refname as well.

Yeah, didn't I also suggest that already?