[BUG] "git checkout -b" erronously thinks a branch already exists

[BUG] "git checkout -b" erronously thinks a branch already exists

[Stefano Lattarini]

[-- Attachment #1: Type: Text/Plain, Size: 1238 bytes --]

First of all, sorry if this is the wrong address for bug reporting,
but I couldn't find any `git-bug' mailing list, nor a bug tracker.

Apparently, "git checkout -b NAME" fails if NAME contains the output
from "git-describe".  This happens for me both with git 1.7.2.3
installed from debian packages, and with the latest developement
version of git.

The attached script demonstrate the failures; here is what it outputs
on my system:

 $ sh bug.sh 
 + GIT=git
 + mkdir foo.dir
 + cd foo.dir
 + git init
 Initialized empty Git repository in /tmp/foo.dir/.git/
 + :
 + git add f1
 + git commit -m none
 [master (root-commit) 9b06795] none
  0 files changed, 0 insertions(+), 0 deletions(-)
  create mode 100644 f1
 + git tag -a -m 'first commit' v0
 + :
 + git add f2
 + git commit -m none
 [master ef5d975] none
  0 files changed, 0 insertions(+), 0 deletions(-)
  create mode 100644 f2
 ++ git describe
 + desc=v0-1-gef5d975
 + git checkout -b fix-v0-1-gef5d975
 fatal: git checkout: branch fix-v0-1-gef5d975 already exists

If you need more details, or if you can explain why this is an user error,
please keep me in CC:, as I'm not subscribed to this list.  In case it is
indeed an user error, sorry for the noise.

Regards,
  Stefano

[-- Attachment #2: bug.sh --]
[-- Type: application/x-shellscript, Size: 233 bytes --]

Re* [BUG] "git checkout -b" erronously thinks a branch already exists

[Junio C Hamano]

Interesting.

This is because "git checkout -b <name>" tries to make sure that
"refs/heads/<name>" does not exist, and the way it does it to pass it
to get_sha1() and make sure it fails.

It almost always works, except when that string is passed from get_sha1()
to get_describe_name(), which takes "ANYTHING-g<hexstring>" and as long as
hexstring names an existing object, it says "Yup, I found one."

I think the right fix is to make sure that "refs/heads/<name>" does not
exist by checking exactly that.

Perhaps something like this.  I am not sure if we want to use the "yield
non zero to signal not an error but an early return" trick like I did in
this patch, though.

---

 builtin/checkout.c |    2 +-
 refs.c             |   14 ++++++++++++++
 refs.h             |    1 +
 3 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 28cdc51..af1e7b5 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1071,7 +1071,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		if (strbuf_check_branch_ref(&buf, opts.new_branch))
 			die(_("git checkout: we do not like '%s' as a branch name."),
 			    opts.new_branch);
-		if (!get_sha1(buf.buf, rev)) {
+		if (ref_exists(buf.buf)) {
 			opts.branch_exists = 1;
 			if (!opts.new_branch_force)
 				die(_("git checkout: branch %s already exists"),
diff --git a/refs.c b/refs.c
index e3c0511..138b1a0 100644
--- a/refs.c
+++ b/refs.c
@@ -1826,6 +1826,20 @@ int update_ref(const char *action, const char *refname,
 	return 0;
 }
 
+static int compare_ref_name(const char *refname,
+			    const unsigned char *sha1, int flag,
+			    void *cb_data)
+{
+	if (!strcmp(cb_data, refname))
+		return -1; /* early return */
+	return 0;
+}
+
+int ref_exists(char *refname)
+{
+	return for_each_ref(compare_ref_name, refname) != 0;
+}
+
 struct ref *find_ref_by_name(const struct ref *list, const char *name)
 {
 	for ( ; list; list = list->next)
diff --git a/refs.h b/refs.h
index 5e7a9a5..5839487 100644
--- a/refs.h
+++ b/refs.h
@@ -35,6 +35,7 @@ extern int for_each_ref_in_submodule(const char *submodule, const char *prefix,
 extern int for_each_tag_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);
 extern int for_each_branch_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);
 extern int for_each_remote_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);
+extern int ref_exists(char *);
 
 static inline const char *has_glob_specials(const char *pattern)
 {

Re: Re* [BUG] "git checkout -b" erronously thinks a branch already exists

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I think the right fix is to make sure that "refs/heads/<name>" does not
> exist by checking exactly that.
>
> Perhaps something like this.  I am not sure if we want to use the "yield
> non zero to signal not an error but an early return" trick like I did in
> this patch, though.

Let's do this instead. I don't know what I was thinking when I wrote that
inefficient "loop refs to see if there is that one" patch.

-- >8 --
Subject: [PATCH] checkout -b <name>: correctly detect existing branch

When create a new branch, we fed "refs/heads/<proposed name>" as a string
to get_sha1() and expected it to fail when a branch already exists.

The right way to check if a ref exists is to check with resolve_ref().

A naïve solution that might appear attractive but does not work is to
forbid slashes in get_describe_name() but that will not work. A describe
name is is in the form of "ANYTHING-g<short sha1>", and that ANYTHING part
comes from a original tag name used in the repository the user ran the
describe command. A sick user could have a confusing hierarchical tag
whose name is "refs/heads/foobar" (stored as refs/tags/refs/heads/foobar")
to generate a describe name "refs/heads/foobar-6-g02ac983", and we should
be able to use that name to refer to the object whose name is 02ac983.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/checkout.c         |    2 +-
 refs.c                     |    6 ++++++
 refs.h                     |    1 +
 t/t2018-checkout-branch.sh |   11 +++++++++++
 4 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 4ad7427..88708d4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -874,7 +874,7 @@ no_reference:
 		if (strbuf_check_branch_ref(&buf, opts.new_branch))
 			die("git checkout: we do not like '%s' as a branch name.",
 			    opts.new_branch);
-		if (!get_sha1(buf.buf, rev)) {
+		if (ref_exists(buf.buf)) {
 			opts.branch_exists = 1;
 			if (!opts.new_branch_force)
 				die("git checkout: branch %s already exists",
diff --git a/refs.c b/refs.c
index 6f486ae..92cd0d1 100644
--- a/refs.c
+++ b/refs.c
@@ -1732,6 +1732,12 @@ int update_ref(const char *action, const char *refname,
 	return 0;
 }
 
+int ref_exists(char *refname)
+{
+	unsigned char sha1[20];
+	return !!resolve_ref(refname, sha1, 1, NULL);
+}
+
 struct ref *find_ref_by_name(const struct ref *list, const char *name)
 {
 	for ( ; list; list = list->next)
diff --git a/refs.h b/refs.h
index 762ce50..070a7d9 100644
--- a/refs.h
+++ b/refs.h
@@ -46,6 +46,7 @@ extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refn
  */
 extern void add_extra_ref(const char *refname, const unsigned char *sha1, int flags);
 extern void clear_extra_refs(void);
+extern int ref_exists(char *);
 
 extern int peel_ref(const char *, unsigned char *);
 
diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 1caffea..741d842 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -163,4 +163,15 @@ test_expect_success 'checkout -f -B to an existing branch with mergeable changes
 	test_must_fail test_dirty_mergeable
 '
 
+test_expect_success 'checkout -b <describe>' '
+	git tag -f -m "First commit" initial initial &&
+	git checkout -f change1 &&
+	name=$(git describe) &&
+	git checkout -b $name &&
+	git diff --exit-code change1 &&
+	echo "refs/heads/$name" >expect &&
+	git symbolic-ref HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.6.rc0.106.g68174

Re: Re* [BUG] "git checkout -b" erronously thinks a branch already exists

[Stefano Lattarini]

Hello Junio, and thanks for your quick answer.

On Monday 06 June 2011, Junio C wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I think the right fix is to make sure that "refs/heads/<name>" does not
> > exist by checking exactly that.
> >
> > Perhaps something like this.  I am not sure if we want to use the "yield
> > non zero to signal not an error but an early return" trick like I did in
> > this patch, though.
> 
> Let's do this instead. I don't know what I was thinking when I wrote that
> inefficient "loop refs to see if there is that one" patch.
> 
> [CUT PATCH]
>
I can confirm this change fixes the problem for me.

Thanks,
  Stefano

Re: Re* [BUG] "git checkout -b" erronously thinks a branch already exists

[Jeff King]

On Sun, Jun 05, 2011 at 10:27:34PM -0700, Junio C Hamano wrote:

> Let's do this instead. I don't know what I was thinking when I wrote that
> inefficient "loop refs to see if there is that one" patch.
> 
> -- >8 --
> Subject: [PATCH] checkout -b <name>: correctly detect existing branch

Yeah, I did a double-take seeing your other patch and wondering why you
weren't just using resolve_ref.

So this patch looks good to me.

> +int ref_exists(char *refname)
> +{
> +	unsigned char sha1[20];
> +	return !!resolve_ref(refname, sha1, 1, NULL);
> +}

I was tempted to suggest that ref_exists could be used in lots of other
places to make the code slightly more readable. But in many cases, the
variable holding the dummy sha1 cannot go away (because it is used
elsewhere in the function), which means manually figuring out whether or
not the sha1 was actually a dummy or not[1]. So it's probably not worth
the effort for such minor gain.

-Peff

[1] For example, the one in create_branch is OK to change, but the one
in delete_branch is not.