git does the wrong thing with ambiguous names

git does the wrong thing with ambiguous names

[Brandon Casey]

When a branch and tag have the same name, a git-checkout using that name 
succeeds (exits zero without complaining), switches to the _branch_, but 
updates the working directory contents to that specified by the _tag_. 
git-status show modified files.

Looks like the ambiguity issue was brought up last year, and git is now 
*supposed* to warn when it encounters an ambiguous name. I agree with 
Petr, it should fail violently, preferably as Josef Weidendorfer 
suggests also printing out the ambiguous matches so the user can cut and 
paste.

http://article.gmane.org/gmane.comp.version-control.git/17720

But it doesn't, git-checkout.sh redirects stderr on git-rev-parse 
--verify so the message is lost. git-log complains, but most users will 
never see it since it is pushed off the screen so quickly.

In another portion of the same thread Junio describes the caveats and 
his experience:

http://article.gmane.org/gmane.comp.version-control.git/16996

The tag==branch name case is trivial to reproduce and a test script is 
provided below. It should fail, so it's not really a submission as a 
test script until this is fixed.

-brandon

ps. I was told a previous patch was ws munged. I'm using thunderbird, if 
it happens this time suggestions welcome.



Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
  t/t2006-checkout-name-clash.sh |   39 
+++++++++++++++++++++++++++++++++++++++
  1 files changed, 39 insertions(+), 0 deletions(-)
  create mode 100755 t/t2006-checkout-name-clash.sh

diff --git a/t/t2006-checkout-name-clash.sh b/t/t2006-checkout-name-clash.sh
new file mode 100755
index 0000000..2220578
--- /dev/null
+++ b/t/t2006-checkout-name-clash.sh
@@ -0,0 +1,39 @@
+#!/bin/sh
+
+test_description='git-checkout with tag/branch naming clash
+
+This tests whether git does something sane when a tag
+and a branch have the same name.'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+echo initial >file1 &&
+echo initial >file2 &&
+git-add file1 file2 &&
+git-commit -m initial
+'
+
+test_expect_success 'create modified branch named test' '
+git-branch test &&
+git-checkout test &&
+echo test >file1 &&
+git-commit -a -m test
+'
+
+test_expect_success 'create tag named test on master branch' '
+git-checkout master &&
+git-tag test
+'
+
+test_expect_success 'modify master and commit' '
+echo master >file2 &&
+git-commit -a -m master
+'
+
+test_expect_success 'checkout test and check consistency' '
+git-checkout test &&
+test "`git diff-index --name-only HEAD`" = ""
+'
+
+test_done
-- 
1.5.2.1.126.g6abd0

Re: git does the wrong thing with ambiguous names

[Alex Riesen]

Brandon Casey, Thu, Jun 07, 2007 00:13:48 +0200:
> 
> When a branch and tag have the same name, a git-checkout using that name 
> succeeds (exits zero without complaining), switches to the _branch_, but 
> updates the working directory contents to that specified by the _tag_. 
> git-status show modified files.

Bad. To reproduce:

mkdir a && cd a && git init && :> a && git add . && git commit -m1 &&
:>b && git add . && git commit -m2 && git tag master HEAD^ &&
find .git/refs/ && gco -b new && gco master && git status


> Looks like the ambiguity issue was brought up last year, and git is now 
> *supposed* to warn when it encounters an ambiguous name. I agree with 
> Petr, it should fail violently, preferably as Josef Weidendorfer 
> suggests also printing out the ambiguous matches so the user can cut and 
> paste.
> 

That'd be failing friendly :) Very good idea

Re: git does the wrong thing with ambiguous names

[Alex Riesen]

Alex Riesen, Thu, Jun 07, 2007 00:58:26 +0200:
> Brandon Casey, Thu, Jun 07, 2007 00:13:48 +0200:
> > 
> > When a branch and tag have the same name, a git-checkout using that name 
> > succeeds (exits zero without complaining), switches to the _branch_, but 
> > updates the working directory contents to that specified by the _tag_. 
> > git-status show modified files.
> 
> Bad. To reproduce:
> 
> mkdir a && cd a && git init && :> a && git add . && git commit -m1 &&
> :>b && git add . && git commit -m2 && git tag master HEAD^ &&
> find .git/refs/ && gco -b new && gco master && git status
> 

git-rev-parse actually warns about ambguities:

    $ git-rev-parse --verify master
    warning: refname 'master' is ambiguous.
    dd5cdc387f2160bf04d02ac08dfdaf952f769357

It's just that the warning is thrown away in git-checkout.sh

A quick and _very_ messy fix could like like that:

diff --git a/builtin-rev-parse.c b/builtin-rev-parse.c
index 37addb2..3c3bc4e 100644
--- a/builtin-rev-parse.c
+++ b/builtin-rev-parse.c
@@ -368,6 +368,10 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				show_datestring("--min-age=", arg+8);
 				continue;
 			}
+			if (!strcmp(arg, "--fail-ambiguous")) {
+				fail_ambiguous_refs = 1;
+				continue;
+			}
 			if (show_flag(arg) && verify)
 				die("Needed a single revision");
 			continue;
diff --git a/cache.h b/cache.h
index 8a9d1f3..4c532c6 100644
--- a/cache.h
+++ b/cache.h
@@ -279,6 +279,7 @@ extern int assume_unchanged;
 extern int prefer_symlink_refs;
 extern int log_all_ref_updates;
 extern int warn_ambiguous_refs;
+extern int fail_ambiguous_refs;
 extern int shared_repository;
 extern const char *apply_default_whitespace;
 extern int zlib_compression_level;
diff --git a/environment.c b/environment.c
index 9d3e5eb..872ab36 100644
--- a/environment.c
+++ b/environment.c
@@ -18,6 +18,7 @@ int prefer_symlink_refs;
 int is_bare_repository_cfg = -1; /* unspecified */
 int log_all_ref_updates = -1; /* unspecified */
 int warn_ambiguous_refs = 1;
+int fail_ambiguous_refs = 0;
 int repository_format_version;
 const char *git_commit_encoding;
 const char *git_log_output_encoding;
diff --git a/git-checkout.sh b/git-checkout.sh
index 6b6facf..4c07fe5 100755
--- a/git-checkout.sh
+++ b/git-checkout.sh
@@ -57,6 +57,13 @@ while [ "$#" != "0" ]; do
 		usage
 		;;
 	*)
+		if ! git-rev-parse --verify --fail-ambiguous "$arg^0" \
+		    2>/dev/null
+		then
+		    echo >&2 "$arg is ambiguous"
+		    find "$GIT_DIR/refs/" |grep -F "$arg"|sed -e 's|.git/|  |'
+		    exit 1
+		fi
 		if rev=$(git-rev-parse --verify "$arg^0" 2>/dev/null)
 		then
 			if [ -z "$rev" ]; then
diff --git a/sha1_name.c b/sha1_name.c
index 7df01af..d094d41 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -345,6 +345,10 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 
 	if (warn_ambiguous_refs && refs_found > 1)
 		fprintf(stderr, warning, len, str);
+	if (fail_ambiguous_refs && refs_found > 1) {
+		fprintf(stderr, "found %d refs\n", refs_found);
+		return -1;
+	}
 
 	if (reflog_len) {
 		int nth, i;

Re: git does the wrong thing with ambiguous names

[Alex Riesen]

Alex Riesen, Thu, Jun 07, 2007 01:33:27 +0200:
> Alex Riesen, Thu, Jun 07, 2007 00:58:26 +0200:
> > Brandon Casey, Thu, Jun 07, 2007 00:13:48 +0200:
> > > 
> > > When a branch and tag have the same name, a git-checkout using that name 
> > > succeeds (exits zero without complaining), switches to the _branch_, but 
> > > updates the working directory contents to that specified by the _tag_. 
> > > git-status show modified files.
> > 
> > Bad. To reproduce:
> > 
> > mkdir a && cd a && git init && :> a && git add . && git commit -m1 &&
> > :>b && git add . && git commit -m2 && git tag master HEAD^ &&
> > find .git/refs/ && gco -b new && gco master && git status
> > 
> 
> git-rev-parse actually warns about ambguities:
> 
>     $ git-rev-parse --verify master
>     warning: refname 'master' is ambiguous.
>     dd5cdc387f2160bf04d02ac08dfdaf952f769357
> 
> It's just that the warning is thrown away in git-checkout.sh
> 
> A quick and _very_ messy fix could like like that:
> 

This one is much shorter and less friendly. Suggested by Junio on irc.
It makes checkout always prefer a branch.

diff --git a/git-checkout.sh b/git-checkout.sh
index 6b6facf..282c84f 100755
--- a/git-checkout.sh
+++ b/git-checkout.sh
@@ -67,6 +67,8 @@ while [ "$#" != "0" ]; do
 			new_name="$arg"
 			if git-show-ref --verify --quiet -- "refs/heads/$arg"
 			then
+				rev=$(git-rev-parse --verify "refs/heads/$arg^0" 2>/dev/null)
+				new="$rev"
 				branch="$arg"
 			fi
 		elif rev=$(git-rev-parse --verify "$arg^{tree}" 2>/dev/null)

Re: git does the wrong thing with ambiguous names

[Brandon Casey]

Alex Riesen wrote:
> Alex Riesen, Thu, Jun 07, 2007 01:33:27 +0200:
>> Alex Riesen, Thu, Jun 07, 2007 00:58:26 +0200:
>>> Brandon Casey, Thu, Jun 07, 2007 00:13:48 +0200:
[snip]
> This one is much shorter and less friendly. Suggested by Junio on irc.
> It makes checkout always prefer a branch.
> 
> diff --git a/git-checkout.sh b/git-checkout.sh
> index 6b6facf..282c84f 100755
> --- a/git-checkout.sh
> +++ b/git-checkout.sh
> @@ -67,6 +67,8 @@ while [ "$#" != "0" ]; do
>  			new_name="$arg"
>  			if git-show-ref --verify --quiet -- "refs/heads/$arg"
>  			then
> +				rev=$(git-rev-parse --verify "refs/heads/$arg^0" 2>/dev/null)
> +				new="$rev"
>  				branch="$arg"
>  			fi
>  		elif rev=$(git-rev-parse --verify "$arg^{tree}" 2>/dev/null)
> 

This doesn't work.

Now the working directory contents are never updated when switching 
branches.

Run my test script, or add an 'echo "some data" > a' in your shell code 
so that the two branches have different contents.

-brandon

Re: git does the wrong thing with ambiguous names

[Brandon Casey]

Alex Riesen wrote:
> Alex Riesen, Thu, Jun 07, 2007 00:58:26 +0200:
>> Brandon Casey, Thu, Jun 07, 2007 00:13:48 +0200:
>>> When a branch and tag have the same name, a git-checkout using that name 
>>> succeeds (exits zero without complaining), switches to the _branch_, but 
>>> updates the working directory contents to that specified by the _tag_. 
>>> git-status show modified files.
>> Bad. To reproduce:
>>
>> mkdir a && cd a && git init && :> a && git add . && git commit -m1 &&
>> :>b && git add . && git commit -m2 && git tag master HEAD^ &&
>> find .git/refs/ && gco -b new && gco master && git status
>>
> 
> git-rev-parse actually warns about ambguities:
> 
>     $ git-rev-parse --verify master
>     warning: refname 'master' is ambiguous.
>     dd5cdc387f2160bf04d02ac08dfdaf952f769357
> 
> It's just that the warning is thrown away in git-checkout.sh
> 
> A quick and _very_ messy fix could like like that:

[snip patch]

This one suffers from what Junio described previously on the mailing 
list, when get_sha1_basic() fails, get_sha1_1() continues trying 
alternatives. The risk is that one of those alternatives could match, 
for example if the ambiguous branch and tag name is 'dead'.

-brandon

Re: git does the wrong thing with ambiguous names

[Brandon Casey]

Didn't hit reply-all...

Brandon Casey wrote:
> Alex Riesen wrote:
>> Alex Riesen, Thu, Jun 07, 2007 01:33:27 +0200:
>>> Alex Riesen, Thu, Jun 07, 2007 00:58:26 +0200:
>>>> Brandon Casey, Thu, Jun 07, 2007 00:13:48 +0200:
> [snip]
>> This one is much shorter and less friendly. Suggested by Junio on irc.
>> It makes checkout always prefer a branch.
>>
>> diff --git a/git-checkout.sh b/git-checkout.sh
>> index 6b6facf..282c84f 100755
>> --- a/git-checkout.sh
>> +++ b/git-checkout.sh
>> @@ -67,6 +67,8 @@ while [ "$#" != "0" ]; do
>>              new_name="$arg"
>>              if git-show-ref --verify --quiet -- "refs/heads/$arg"
>>              then
>> +                rev=$(git-rev-parse --verify "refs/heads/$arg^0" 
>> 2>/dev/null)
>> +                new="$rev"
>>                  branch="$arg"
>>              fi
>>          elif rev=$(git-rev-parse --verify "$arg^{tree}" 2>/dev/null)
>>
> 
> This doesn't work.

sorry, scratch that. It does work.

Thunderbird turned $arg _hat_ 0 into $arg _superscript_ 0, which became
$arg0 when I copy/pasted. And I didn't catch it. Again, apologies.

I think this is the intuitive behavior, prefer branch over tag. though I
think a warning or refusal to switch would be better (but more
complicated, and I don't know how to do it).

A quick run through of some other porcelain commands shows they prefer tag:
git log
git show
git diff

-brandon