[PATCH 3/4] branch: correctly reject refs/heads/{-dash,HEAD}

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 15, 2017, 22:14 UTC
Message-ID: <xmqqpo8jyyti.fsf_-_@gitster.mtv.corp.google.com>
In-Reply-To: <bb3485d0-71bc-452e-e4b9-8a7d767753a5@gmail.com>

strbuf_check_branch_ref() is the central place where many codepaths see if a proposed name is suitable for the name of a branch. It was designed to allow us to get stricter than the check_refname_format() check used for refnames in general, and we already use it to reject a branch whose name begins with a '-'. The function gets a strbuf and a string "name", and returns non-zero if the name is not appropriate as the name for a branch. When the name is good, it places the full refname for the branch with the proposed name in the strbuf before it returns.

However, it turns out that one caller looks at what is in the strbuf even when the function returns an error. Make the function populate the strbuf even when it returns an error. That way, when "-dash" is given as name, "refs/heads/-dash" is placed in the strbuf when returning an error to copy_or_rename_branch(), which notices that the user is trying to recover with "git branch -m -- -dash dash" to rename "-dash" to "dash".

While at it, use the same mechanism to also reject "HEAD" as a branch name.

Helped-by: Jeff King <peff@peff.net>
Helped-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

    Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

    >> Are these two patches follow-up fixes (replacement of 3/3 plus an
    >> extra patch) to jc/branch-name-sanity topic?
    >
    > Yes, that's right.
    >
    >> Thanks for working on these.
    >
    > You're welcome. Please do be sure I haven't broken anything in
    > v2. These patches should cleanly apply on 'next', if they don't let me
    > know.

    OK, so here is a replacement for your replacement, based on an
    additional analysis I did while I was reviewing your changes.
    The final 4/4 is what you sent as [v2 2/2] (which was meant to
    be [v2 4/3]).  I think with these updates, the resulting 4-patch
    series is good for 'next'.

    Thanks again.

 sha1_name.c             | 14 ++++++++++++--
 t/t1430-bad-ref-name.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index c7c5ab376c..67961d6e47 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1332,9 +1332,19 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)
 int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
 {
 	strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
-	if (name[0] == '-')
-		return -1;
+
+	/*
+	 * This splice must be done even if we end up rejecting the
+	 * name; builtin/branch.c::copy_or_rename_branch() still wants
+	 * to see what the name expanded to so that "branch -m" can be
+	 * used as a tool to correct earlier mistakes.
+	 */
 	strbuf_splice(sb, 0, 0, "refs/heads/", 11);
+
+	if (*name == '-' ||
+	    !strcmp(sb->buf, "refs/heads/HEAD"))
+		return -1;
+
 	return check_refname_format(sb->buf, 0);
 }
 
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index e88349c8a0..c7878a60ed 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -331,4 +331,47 @@ test_expect_success 'update-ref --stdin -z fails delete with bad ref name' '
 	grep "fatal: invalid ref format: ~a" err
 '
 
+test_expect_success 'branch rejects HEAD as a branch name' '
+	test_must_fail git branch HEAD HEAD^ &&
+	test_must_fail git show-ref refs/heads/HEAD
+'
+
+test_expect_success 'checkout -b rejects HEAD as a branch name' '
+	test_must_fail git checkout -B HEAD HEAD^ &&
+	test_must_fail git show-ref refs/heads/HEAD
+'
+
+test_expect_success 'update-ref can operate on refs/heads/HEAD' '
+	git update-ref refs/heads/HEAD HEAD^ &&
+	git show-ref refs/heads/HEAD &&
+	git update-ref -d refs/heads/HEAD &&
+	test_must_fail git show-ref refs/heads/HEAD
+'
+
+test_expect_success 'branch -d can remove refs/heads/HEAD' '
+	git update-ref refs/heads/HEAD HEAD^ &&
+	git branch -d HEAD &&
+	test_must_fail git show-ref refs/heads/HEAD
+'
+
+test_expect_success 'branch -m can rename refs/heads/HEAD' '
+	git update-ref refs/heads/HEAD HEAD^ &&
+	git branch -m HEAD tail &&
+	test_must_fail git show-ref refs/heads/HEAD &&
+	git show-ref refs/heads/tail
+'
+
+test_expect_success 'branch -d can remove refs/heads/-dash' '
+	git update-ref refs/heads/-dash HEAD^ &&
+	git branch -d -- -dash &&
+	test_must_fail git show-ref refs/heads/-dash
+'
+
+test_expect_success 'branch -m can rename refs/heads/-dash' '
+	git update-ref refs/heads/-dash HEAD^ &&
+	git branch -m -- -dash dash &&
+	test_must_fail git show-ref refs/heads/-dash &&
+	git show-ref refs/heads/dash
+'
+
 test_done

-- 
2.15.0-358-g6c105002b3