git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>
Subject: [PATCH 3/4] branch: correctly reject refs/heads/{-dash,HEAD}
Date: Thu, 16 Nov 2017 07:14:17 +0900
Message-ID: <xmqqpo8jyyti.fsf_-_@gitster.mtv.corp.google.com> (raw)
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


  reply index

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-13  5:11 [PATCH 0/3] a small branch API clean-up Junio C Hamano
2017-10-13  5:11 ` [PATCH 1/3] branch: streamline "attr_only" handling in validate_new_branchname() Junio C Hamano
2017-10-13  7:05   ` Eric Sunshine
2017-10-13  5:11 ` [PATCH 2/3] branch: split validate_new_branchname() into two Junio C Hamano
2017-10-21  4:58   ` Kaartic Sivaraam
2017-10-21  9:01     ` Junio C Hamano
2017-10-13  5:11 ` [PATCH 3/3] branch: forbid refs/heads/HEAD Junio C Hamano
2017-10-13 13:15   ` Jeff King
2017-10-14  2:11     ` Junio C Hamano
2017-10-14  2:20       ` Junio C Hamano
2017-10-16 21:38         ` Jeff King
2017-10-21  4:50         ` Kaartic Sivaraam
2017-10-21  8:57           ` Junio C Hamano
2017-10-22  5:00             ` Kaartic Sivaraam
2017-10-21  3:07 ` [PATCH 0/3] a small branch API clean-up Kaartic Sivaraam
2017-10-21  8:52   ` Junio C Hamano
2017-10-22  4:36     ` Kaartic Sivaraam
2017-11-14 11:42 ` [PATCH v2 1/2] branch: forbid refs/heads/HEAD Kaartic Sivaraam
2017-11-14 11:42   ` [PATCH v2 2/2] builtin/branch: remove redundant check for HEAD Kaartic Sivaraam
2017-11-14 12:00   ` [PATCH v2 1/2] branch: forbid refs/heads/HEAD Kaartic Sivaraam
2017-11-14 15:08     ` Junio C Hamano
2017-11-15 16:59       ` Kaartic Sivaraam
2017-11-15 22:14         ` Junio C Hamano [this message]
2017-11-16 13:11           ` [PATCH 3/4] branch: correctly reject refs/heads/{-dash,HEAD} Kaartic Sivaraam
2017-11-16 14:57             ` Junio C Hamano
2017-11-16 17:02               ` Kaartic Sivaraam

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqpo8jyyti.fsf_-_@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=kaartic.sivaraam@gmail.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox