[PATCH] Disallow creating ambiguous branch names by default

[PATCH] Disallow creating ambiguous branch names by default

[Conrad Irwin]

Before this change, it was comparatively easy to create a confusingly
named branch (like "origin/master" or "tag.1"). The former case is
particularly biting to newcomers, who suddenly find themselves needing
to handle nuances of the refs namespaces. The latter is something you're
not likely to want to do, the tag will take precedence when the name is
resolved meaning that your branch will be hard to use.

In both cases, git commands would omit a warning about "ambiguous refs"
if they noticed that this had occurred, however it feels nicer to help
users avoid getting into that situation to start with!

After this patch, git branch <foo>, git checkout -b <foo> and git branch
-m bar <foo> will all fail if <foo> is already a commit-ish; this
safety-net can be removed by passing the -f, -B or -M flags.

Signed-off-by: Conrad Irwin <conrad.irwin@gmail.com>

---

I considered adding a separate configuration variable to disable this
check permanently, but I couldn't find a convincing use-case. Perhaps
the closest is in `git branch $(git describe)` as a quick way to
bookmark a commit; but it seems like creating a tag might be a more
sensible option in that case. Would anyone want such a flag?

Conrad

 Documentation/git-branch.txt |    5 +++--
 branch.c                     |    2 ++
 builtin/branch.c             |    3 +++
 t/t2018-checkout-branch.sh   |   16 ++++++++++++++--
 t/t3200-branch.sh            |   33 +++++++++++++++++++++++++++++++++
 t/t6300-for-each-ref.sh      |    2 +-
 t/t7201-co.sh                |    4 ++--
 7 files changed, 58 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index c50f189..415eae3 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -80,8 +80,9 @@ OPTIONS
 
 -f::
 --force::
-	Reset <branchname> to <startpoint> if <branchname> exists
-	already. Without `-f` 'git branch' refuses to change an existing branch.
+	Create the branch even when it might be confusing to do so (for
+	example a tag with that name already exists). If a branch with
+	the same name already exists, it will be reset to <start-point>.
 
 -m::
 	Move/rename a branch and the corresponding reflog.
diff --git a/branch.c b/branch.c
index c0c865a..f26154e 100644
--- a/branch.c
+++ b/branch.c
@@ -162,6 +162,8 @@ void create_branch(const char *head,
 		else if (!is_bare_repository() && head && !strcmp(head, name))
 			die("Cannot force update the current branch.");
 		forcing = 1;
+	} else if (!force && !get_sha1(name, sha1)) {
+		die("A branch named '%s' would be ambiguous.", name);
 	}
 
 	real_ref = NULL;
diff --git a/builtin/branch.c b/builtin/branch.c
index 3142daa..6af1718 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -572,6 +572,9 @@ static void rename_branch(const char *oldname, const char *newname, int force)
 	if (resolve_ref(newref.buf, sha1, 1, NULL) && !force)
 		die(_("A branch named '%s' already exists."), newref.buf + 11);
 
+	if (!get_sha1(newname, sha1) && !force)
+		die(_("A branch named '%s' would be ambiguous."), newname);
+
 	strbuf_addf(&logmsg, "Branch: renamed %s to %s",
 		 oldref.buf, newref.buf);
 
diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index a42e039..26cc066 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -169,15 +169,27 @@ 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>' '
+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 checkout -B $name &&
 	git diff --exit-code change1 &&
 	echo "refs/heads/$name" >expect &&
 	git symbolic-ref HEAD >actual &&
 	test_cmp expect actual
 '
 
+test_expect_success 'checkout -b <tag-name> fails' '
+	git tag -f -m "First commit" initial initial &&
+	git checkout -f change1 &&
+	test_must_fail git checkout -b initial
+'
+
+test_expect_success 'checkout -B <tag-name> fails' '
+	git tag -f -m "First commit" initial initial &&
+	git checkout -f change1 &&
+	git checkout -B initial
+'
+
 test_done
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 9e69c8c..f3f4542 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -230,6 +230,39 @@ test_expect_success \
     'git tag foobar &&
      test_must_fail git branch --track my11 foobar'
 
+test_expect_success 'creating a branch called HEAD fails' '
+    test_must_fail git branch HEAD
+'
+
+test_expect_success 'creating a branch with an ambiguous name fails' '
+    git tag t/g &&
+    test_must_fail git branch t/g &&
+    git tag -d t/g
+'
+
+test_expect_success 'creating a branch with an ambigious name with -f' '
+    git tag t/g &&
+    git branch -f t/g &&
+    git branch -d t/g &&
+    git tag -d t/g
+'
+
+test_expect_success 'moving a branch to an ambiguous name with -m fails' '
+    git tag t/g &&
+    git branch bar &&
+    test_must_fail git branch -m bar t/g &&
+    git branch -d bar &&
+    git tag -d t/g
+'
+
+test_expect_success 'moving a branch to an ambiguous name with -M' '
+    git tag t/g &&
+    git branch b/h &&
+    git branch -M b/h t/g &&
+    git branch -d t/g &&
+    git tag -d t/g
+'
+
 # Keep this test last, as it changes the current branch
 cat >expect <<EOF
 $_z40 $HEAD $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000	branch: Created from master
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 7dc8a51..37f0394 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -344,7 +344,7 @@ EOF
 test_expect_success 'Check ambiguous head and tag refs II (loose)' '
 	git checkout master &&
 	git tag ambiguous testtag^0 &&
-	git branch ambiguous testtag^0 &&
+	git branch -f ambiguous testtag^0 &&
 	git for-each-ref --format "%(refname:short)" refs/heads/ambiguous refs/tags/ambiguous >actual &&
 	test_cmp expected actual
 '
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 07fb53a..a488c5b 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -311,7 +311,7 @@ test_expect_success 'checkout to detach HEAD with HEAD^0' '
 test_expect_success 'checkout with ambiguous tag/branch names' '
 
 	git tag both side &&
-	git branch both master &&
+	git branch -f both master &&
 	git reset --hard &&
 	git checkout master &&
 
@@ -330,7 +330,7 @@ test_expect_success 'checkout with ambiguous tag/branch names' '
 	git checkout master &&
 
 	git tag frotz side &&
-	git branch frotz master &&
+	git branch -f frotz master &&
 	git reset --hard &&
 	git checkout master &&
 
-- 
1.7.6.448.gc60f1

Re: [PATCH] Disallow creating ambiguous branch names by default

[Junio C Hamano]

Conrad Irwin <conrad.irwin@gmail.com> writes:

> Before this change, it was comparatively easy to create a confusingly

Drop everything before the ", ".

> named branch (like "origin/master" or "tag.1"). The former case is
> particularly biting to newcomers, who suddenly find themselves needing
> to handle nuances of the refs namespaces.

If you start forbidding certain names, newcomers will need to be exposed
the same nuances to understand why what they wanted to do is not allowed,
so that is not an argument.

My preferences (take them as "the ground rules" if you want) are:

 - We don't disallow what we have long allowed, without a good reason;
 - We make sure new people will get a warning with useful advice.

I would be happy to see the end result that warns when the end user
creates a branch (or a tag) that is ambiguous _when_ it is created (not
"much later, when we noticed there are ambiguous refs"), and offers an
advice message to use "branch -m" to rename it away (control the message
with a new "advice.*" configuration and unless explicitly declined with
it, always give the advice).

> In both cases, git commands would omit a warning about "ambiguous refs"
> if they noticed that this had occurred,

Assuming that you meant s/omit/emit/, I do agree that what we do right now
is suboptimal. I just tried these two:

	$ git branch v1.0.0
        $ git checkout v1.0.0
        warning: refname 'v1.0.0' is ambigous.
	$ git branch -m v1.0.0-branch

        $ git checkout -b v1.0.0
        Switched to a new branch 'v1.0.0'
	$ git checkout v1.0.0
        warning: refname 'v1.0.0' is ambigous.
	Already on 'v1.0.0'
	$ git branch -m v1.0.0-branch-2

We should be giving these warning messages immediately after creating
potentially problematic refs, i.e. just after "git branch v1.0.0" and
"git checkout -b v1.0.0". The user experience should look like this
instead:

	$ git branch v1.0.0
        warning: refname 'v1.0.0' is ambiguous.
        advice: you may want to rename it to an unambigous name with
        advice: git branch -m v1.0.0 v1.0.0-branch
	$ git branch -m v1.0.0 v1.0.0-branch ;# thanks for an advice

        $ git checkout -b v1.0.0
        warning: refname 'v1.0.0' is ambiguous.
        advice: you may want to rename it to an unambigous name with
        advice: git branch -m v1.0.0-branch-2
	$ git branch -m v1.0.0-branch-2 ;# thanks for an advice

Re: [PATCH] Disallow creating ambiguous branch names by default

[Vijay Lakshminarayanan]

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

> We should be giving these warning messages immediately after creating
> potentially problematic refs, i.e. just after "git branch v1.0.0" and
> "git checkout -b v1.0.0". The user experience should look like this
> instead:
>
> 	$ git branch v1.0.0
>         warning: refname 'v1.0.0' is ambiguous.
>         advice: you may want to rename it to an unambigous name with
>         advice: git branch -m v1.0.0 v1.0.0-branch
> 	$ git branch -m v1.0.0 v1.0.0-branch ;# thanks for an advice
>
>         $ git checkout -b v1.0.0
>         warning: refname 'v1.0.0' is ambiguous.
>         advice: you may want to rename it to an unambigous name with
>         advice: git branch -m v1.0.0-branch-2
> 	$ git branch -m v1.0.0-branch-2 ;# thanks for an advice

I'm not familiar with the git codebase, but I'm guessing this is
ambiguous because there's already a tag by name v1.0.0.  /If/ that's the
case, wouldn't be be prudent to explain why the branch name is
ambiguous?

Just my 2c.

-- 
Cheers
~vijay

Gnus should be more complicated.

Re: [PATCH] Disallow creating ambiguous branch names by default

[Stephen Bash]

----- Original Message -----
> From: "Junio C Hamano" <gitster@pobox.com>
> To: "Conrad Irwin" <conrad.irwin@gmail.com>
> Sent: Wednesday, August 17, 2011 2:41:41 PM
> Subject: Re: [PATCH] Disallow creating ambiguous branch names by default
>
> We should be giving these warning messages immediately after creating
> potentially problematic refs, i.e. just after "git branch v1.0.0" and
> "git checkout -b v1.0.0". The user experience should look like this
> instead:
> 
> $ git branch v1.0.0
> warning: refname 'v1.0.0' is ambiguous.
> advice: you may want to rename it to an unambigous name with
> advice: git branch -m v1.0.0 v1.0.0-branch
> $ git branch -m v1.0.0 v1.0.0-branch ;# thanks for an advice
> 
> $ git checkout -b v1.0.0
> warning: refname 'v1.0.0' is ambiguous.
> advice: you may want to rename it to an unambigous name with
> advice: git branch -m v1.0.0-branch-2
> $ git branch -m v1.0.0-branch-2 ;# thanks for an advice

Should case insensitive matches be added to the tests?  This morning I discovered coworkers working on branches foo and Foo thinking they were on the same branch...  Rather trivial to clean up, but certainly caused some confusion in the office.

Thanks,
Stephen

Re: [PATCH] Disallow creating ambiguous branch names by default

[Conrad Irwin]

On Thu, Aug 18, 2011 at 6:53 AM, Stephen Bash <bash@genarts.com> wrote:
>
> Should case insensitive matches be added to the tests?  This morning I discovered coworkers working on branches foo and Foo thinking they were on the same branch...  Rather trivial to clean up, but certainly caused some confusion in the office.
>

I can certainly see the use-case, but there's definitely a step-change
between "this branch has the same name as something else", and "this
branch is going to confuse you". When trying to change the code to be
a warning as Junio suggested, I did think about expanding the
definition of ambiguous to include things that are merely confusing;
however it's not clear where to stop (i.e. should we warn about
<remotename>/<anything>, foo and f00, a branch called " " [the
non-breaking space]). There's probably an argument for more general
warning, but I don't think I understand when it should be shown
well-enough.

Conrad

Re: [PATCH] Disallow creating ambiguous branch names by default

[Conrad Irwin]

On Wed, Aug 17, 2011 at 11:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Conrad Irwin <conrad.irwin@gmail.com> writes:
>
>> Before this change, it was comparatively easy to create a confusingly
>
> Drop everything before the ", ".
>
>> named branch (like "origin/master" or "tag.1"). The former case is
>> particularly biting to newcomers, who suddenly find themselves needing
>> to handle nuances of the refs namespaces.
>
> If you start forbidding certain names, newcomers will need to be exposed
> the same nuances to understand why what they wanted to do is not allowed,
> so that is not an argument.
>
> My preferences (take them as "the ground rules" if you want) are:
>
>  - We don't disallow what we have long allowed, without a good reason;
>  - We make sure new people will get a warning with useful advice.
>
> I would be happy to see the end result that warns when the end user
> creates a branch (or a tag) that is ambiguous _when_ it is created (not
> "much later, when we noticed there are ambiguous refs"), and offers an
> advice message to use "branch -m" to rename it away (control the message
> with a new "advice.*" configuration and unless explicitly declined with
> it, always give the advice).
>

In the process of changing things around to do this, I noticed that

git checkout -M <foo> <current-branch>

surprisingly works, and does confusing things, in that you will get a:

$ git rev-parse HEAD@{1}
warning: Log .git/logs/HEAD has gap after Fri, 19 Aug 2011 02:00:09 -0700

Presumably this is the reason that git branch -f forbids you from
changing the current branch?

If so is this a reasonable case where the current behaviour should be
forbidden (with the same error message "fatal: Cannot force update the
current branch.") — or should I just make it output a warning?

Conrad

Re: [PATCH] Disallow creating ambiguous branch names by default

[Stephen Bash]

----- Original Message -----
> From: "Conrad Irwin" <conrad.irwin@gmail.com>
> To: "Stephen Bash" <bash@genarts.com>
> Cc: "Junio C Hamano" <gitster@pobox.com>, git@vger.kernel.org
> Sent: Friday, August 19, 2011 2:07:53 PM
> Subject: Re: [PATCH] Disallow creating ambiguous branch names by default
>
> > Should case insensitive matches be added to the tests? This morning
> > I discovered coworkers working on branches foo and Foo thinking they
> > were on the same branch... Rather trivial to clean up, but certainly
> > caused some confusion in the office.
> 
> I can certainly see the use-case, but there's definitely a step-change
> between "this branch has the same name as something else", and "this
> branch is going to confuse you".

Good point.  I'd be curious if any of the msys/cygwin guys can comment on if/when capitalization in branch names becomes technically ambiguous?  I would think unpacked refs on a Windows machine could get complicated...  And I guess factory Macs are all formated case-insensitive as well, so the same problem might apply there.

> When trying to change the code to be
> a warning as Junio suggested, I did think about expanding the
> definition of ambiguous to include things that are merely confusing;
> however it's not clear where to stop (i.e. should we warn about
> <remotename>/<anything>, foo and f00, a branch called " " [the
> non-breaking space]). There's probably an argument for more general
> warning, but I don't think I understand when it should be shown
> well-enough.

Thanks for putting the thought into it, I agree it is a slippery slope.

Stephen

Re: [PATCH] Disallow creating ambiguous branch names by default

[Junio C Hamano]

Conrad Irwin <conrad.irwin@gmail.com> writes:

> In the process of changing things around to do this, I noticed that
>
> git checkout -M <foo> <current-branch>
>
> surprisingly works,...

What is "-M" supposed to do???

If you meant "-B", that should work. When I want to rewrite a topic in a
non-trivial way, I would often do:

	$ git checkout HEAD^^^
        work to redo what the few commits at the tip should have done,
        creating commits.
        $ git diff @{-1} HEAD
        $ git checkout -B @{-1}

which often happens to be simpler and more flexible than the canned
rewriting options "rebase -i" can offer me.

> ... in that you will get a:
>
> $ git rev-parse HEAD@{1}
> warning: Log .git/logs/HEAD has gap after Fri, 19 Aug 2011 02:00:09 -0700

If that is the case, then the codepath to update the reflog is
broken. That is not a reason to forbid -B, though.

But because I do not know what you meant by "checkout -M", ...

Re: [PATCH] Disallow creating ambiguous branch names by default

[Conrad Irwin]

On Fri, Aug 19, 2011 at 1:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> $ git rev-parse HEAD@{1}
>> warning: Log .git/logs/HEAD has gap after Fri, 19 Aug 2011 02:00:09 -0700
>
> If that is the case, then the codepath to update the reflog is
> broken. That is not a reason to forbid -B, though.
>
> But because I do not know what you meant by "checkout -M", ...

Sorry, I meant git branch -M <foo> <current-branch>

Conrad

Re: [PATCH] Disallow creating ambiguous branch names by default

[Junio C Hamano]

Conrad Irwin <conrad.irwin@gmail.com> writes:

> In the process of changing things around to do this, I noticed that
>
> git branch -M <foo> <current-branch>
>
> surprisingly works, and does confusing things, in that you will get a:
>
> $ git rev-parse HEAD@{1}
> warning: Log .git/logs/HEAD has gap after Fri, 19 Aug 2011 02:00:09 -0700
>
> Presumably this is the reason that git branch -f forbids you from
> changing the current branch?

[jc: edited typo in the original command exhibition]

I also suspect that "git status" will become nonsense at that point, as
the working tree and the index were still the original state while the
commit pointed by HEAD have changed underneath you.

And you are correct to point out that it is why "git branch -f" shouldn't
touch the current branch. We should notice the situation and error out.

Patches welcome.

I initially thought that such a patch can optionally as a bonus suggest an
alternative way to confuse yourself, e.g. "git reset --soft <foo>", which
is what is happening, but I do not think it makes sense, especially with
"--soft", either.

The user is saying "I know the <current-branch> exists already, and I want
it to match the tip of <foo> branch", without saying what should happen to
what is in the working tree, so depending on what s/he wants, either "git
reset --hard <foo>" or "git reset --keep <foo>" followed by "git branch -d
foo" would be the right thing to do, and I would imagine that one could
even argue that "git branch -M <foo> <current-branch>" should do exactly
that under the hood, but the <current-branch> may be a typo and the user
may have meant to affect some other branch, so in order to play it safe,
just an error message without any advice based on a vague second-guess of
the user's intention, would be the most appropriate, I would think.

Thanks.