[PATCH 0/3] a small branch API clean-up

STALE3185d

11 messages, 4 authors, 2017-11-14 · open the first message on its own page

[PATCH 0/3] a small branch API clean-up

From: Junio C Hamano <hidden>
Date: 2017-10-13 05:11:40

This started as a little clean-up for a NEEDSWORK comment in
branch.h, but it ended up adding a rather useful safety feature.

Junio C Hamano (3):
  branch: streamline "attr_only" handling in validate_new_branchname()
  branch: split validate_new_branchname() into two
  branch: forbid refs/heads/HEAD

 branch.c                | 44 ++++++++++++++++++++++++++++++--------------
 branch.h                | 27 ++++++++++++---------------
 builtin/branch.c        |  8 ++++----
 builtin/checkout.c      | 10 +++++-----
 sha1_name.c             |  2 +-
 t/t1430-bad-ref-name.sh |  8 ++++++++
 6 files changed, 60 insertions(+), 39 deletions(-)

-- 
2.15.0-rc1-158-gbd035ae683

[PATCH 2/3] branch: split validate_new_branchname() into two

From: Junio C Hamano <hidden>
Date: 2017-10-13 05:11:43

Checking if a proposed name is appropriate for a branch is strictly
a subset of checking if we want to allow creating or updating a
branch with such a name.  The mysterious sounding 'attr_only'
parameter to validate_new_branchname() is used to switch the
function between these two roles.

Instead, split the function into two, and adjust the callers.  A new
helper validate_branchname() only checks the name and reports if the
branch already exists.

This loses one NEEDSWORK from the branch API.

Signed-off-by: Junio C Hamano <redacted>
---
 branch.c           | 34 +++++++++++++++++++++++-----------
 branch.h           | 27 ++++++++++++---------------
 builtin/branch.c   |  8 ++++----
 builtin/checkout.c | 10 +++++-----
 4 files changed, 44 insertions(+), 35 deletions(-)
diff --git a/branch.c b/branch.c
index 7404597678..2c3a364a0b 100644
--- a/branch.c
+++ b/branch.c
@@ -178,19 +178,31 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)
 	return 0;
 }
 
-int validate_new_branchname(const char *name, struct strbuf *ref,
-			    int force, int attr_only)
+/*
+ * Check if 'name' can be a valid name for a branch; die otherwise.
+ * Return 1 if the named branch already exists; return 0 otherwise.
+ * Fill ref with the full refname for the branch.
+ */
+int validate_branchname(const char *name, struct strbuf *ref)
 {
-	const char *head;
-
 	if (strbuf_check_branch_ref(ref, name))
 		die(_("'%s' is not a valid branch name."), name);
 
-	if (!ref_exists(ref->buf))
-		return 0;
+	return ref_exists(ref->buf);
+}
 
-	if (attr_only)
-		return 1;
+/*
+ * Check if a branch 'name' can be created as a new branch; die otherwise.
+ * 'force' can be used when it is OK for the named branch already exists.
+ * Return 1 if the named branch already exists; return 0 otherwise.
+ * Fill ref with the full refname for the branch.
+ */
+int validate_new_branchname(const char *name, struct strbuf *ref, int force)
+{
+	const char *head;
+
+	if (!validate_branchname(name, ref))
+		return 0;
 
 	if (!force)
 		die(_("A branch named '%s' already exists."),
@@ -246,9 +258,9 @@ void create_branch(const char *name, const char *start_name,
 	if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
 		explicit_tracking = 1;
 
-	if (validate_new_branchname(name, &ref, force,
-				    track == BRANCH_TRACK_OVERRIDE ||
-				    clobber_head)) {
+	if ((track == BRANCH_TRACK_OVERRIDE || clobber_head)
+	    ? validate_branchname(name, &ref)
+	    : validate_new_branchname(name, &ref, force)) {
 		if (!force)
 			dont_change_ref = 1;
 		else
diff --git a/branch.h b/branch.h
index b07788558c..be5e5d1308 100644
--- a/branch.h
+++ b/branch.h
@@ -23,22 +23,19 @@ void create_branch(const char *name, const char *start_name,
 		   int clobber_head, int quiet, enum branch_track track);
 
 /*
- * Validates that the requested branch may be created, returning the
- * interpreted ref in ref, force indicates whether (non-head) branches
- * may be overwritten. A non-zero return value indicates that the force
- * parameter was non-zero and the branch already exists.
- *
- * Contrary to all of the above, when attr_only is 1, the caller is
- * not interested in verifying if it is Ok to update the named
- * branch to point at a potentially different commit. It is merely
- * asking if it is OK to change some attribute for the named branch
- * (e.g. tracking upstream).
- *
- * NEEDSWORK: This needs to be split into two separate functions in the
- * longer run for sanity.
- *
+ * Check if 'name' can be a valid name for a branch; die otherwise.
+ * Return 1 if the named branch already exists; return 0 otherwise.
+ * Fill ref with the full refname for the branch.
+ */
+extern int validate_branchname(const char *name, struct strbuf *ref);
+
+/*
+ * Check if a branch 'name' can be created as a new branch; die otherwise.
+ * 'force' can be used when it is OK for the named branch already exists.
+ * Return 1 if the named branch already exists; return 0 otherwise.
+ * Fill ref with the full refname for the branch.
  */
-int validate_new_branchname(const char *name, struct strbuf *ref, int force, int attr_only);
+extern int validate_new_branchname(const char *name, struct strbuf *ref, int force);
 
 /*
  * Remove information about the state of working on the current
diff --git a/builtin/branch.c b/builtin/branch.c
index b67593288c..e5bbfb4a17 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -463,7 +463,6 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;
 	struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
 	int recovery = 0;
-	int clobber_head_ok;
 
 	if (!oldname) {
 		if (copy)
@@ -487,9 +486,10 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	 * A command like "git branch -M currentbranch currentbranch" cannot
 	 * cause the worktree to become inconsistent with HEAD, so allow it.
 	 */
-	clobber_head_ok = !strcmp(oldname, newname);
-
-	validate_new_branchname(newname, &newref, force, clobber_head_ok);
+	if (!strcmp(oldname, newname))
+		validate_branchname(newname, &newref);
+	else
+		validate_new_branchname(newname, &newref, force);
 
 	reject_rebase_or_bisect_branch(oldref.buf);
 
diff --git a/builtin/checkout.c b/builtin/checkout.c
index fc4f8fd2ea..697ac7dcaf 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1289,11 +1289,11 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	if (opts.new_branch) {
 		struct strbuf buf = STRBUF_INIT;
 
-		opts.branch_exists =
-			validate_new_branchname(opts.new_branch, &buf,
-						!!opts.new_branch_force,
-						!!opts.new_branch_force);
-
+		if (opts.new_branch_force)
+			opts.branch_exists = validate_branchname(opts.new_branch, &buf);
+		else
+			opts.branch_exists =
+				validate_new_branchname(opts.new_branch, &buf, 0);
 		strbuf_release(&buf);
 	}
 
-- 
2.15.0-rc1-158-gbd035ae683

[PATCH 1/3] branch: streamline "attr_only" handling in validate_new_branchname()

From: Junio C Hamano <hidden>
Date: 2017-10-13 05:11:45

The function takes a parameter "attr_only" (which is a name that is
hard to reason about, which will be corrected soon) and skips some
checks when it is set.  Reorganize the conditionals to make it move
obvious that some checks are never made when this parameter is set.

Signed-off-by: Junio C Hamano <redacted>
---
 branch.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/branch.c b/branch.c
index 4377ce2fb1..7404597678 100644
--- a/branch.c
+++ b/branch.c
@@ -181,21 +181,25 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)
 int validate_new_branchname(const char *name, struct strbuf *ref,
 			    int force, int attr_only)
 {
+	const char *head;
+
 	if (strbuf_check_branch_ref(ref, name))
 		die(_("'%s' is not a valid branch name."), name);
 
 	if (!ref_exists(ref->buf))
 		return 0;
-	else if (!force && !attr_only)
-		die(_("A branch named '%s' already exists."), ref->buf + strlen("refs/heads/"));
 
-	if (!attr_only) {
-		const char *head;
+	if (attr_only)
+		return 1;
+
+	if (!force)
+		die(_("A branch named '%s' already exists."),
+		    ref->buf + strlen("refs/heads/"));
+
+	head = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
+	if (!is_bare_repository() && head && !strcmp(head, ref->buf))
+		die(_("Cannot force update the current branch."));
 
-		head = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
-		if (!is_bare_repository() && head && !strcmp(head, ref->buf))
-			die(_("Cannot force update the current branch."));
-	}
 	return 1;
 }
 
-- 
2.15.0-rc1-158-gbd035ae683

[PATCH 3/3] branch: forbid refs/heads/HEAD

From: Junio C Hamano <hidden>
Date: 2017-10-13 05:12:07

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 '-'.

Use it to also reject "HEAD" as a branch name.

Signed-off-by: Junio C Hamano <redacted>
---
 sha1_name.c             | 2 +-
 t/t1430-bad-ref-name.sh | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/sha1_name.c b/sha1_name.c
index c7c5ab376c..1b8c503095 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1332,7 +1332,7 @@ 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] == '-')
+	if (*name == '-' || !strcmp(name, "HEAD"))
 		return -1;
 	strbuf_splice(sb, 0, 0, "refs/heads/", 11);
 	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..3ecb2eab0c 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -331,4 +331,12 @@ 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_expect_success 'checkout -b rejects HEAD as a branch name' '
+	test_must_fail git checkout -B HEAD HEAD^
+'
+
 test_done
-- 
2.15.0-rc1-158-gbd035ae683

Re: [PATCH 1/3] branch: streamline "attr_only" handling in validate_new_branchname()

From: Eric Sunshine <hidden>
Date: 2017-10-13 07:06:04

On Fri, Oct 13, 2017 at 1:11 AM, Junio C Hamano [off-list ref] wrote:
The function takes a parameter "attr_only" (which is a name that is
hard to reason about, which will be corrected soon) and skips some
checks when it is set.  Reorganize the conditionals to make it move
s/move/more/
obvious that some checks are never made when this parameter is set.

Signed-off-by: Junio C Hamano <redacted>

Re: [PATCH 3/3] branch: forbid refs/heads/HEAD

From: Jeff King <hidden>
Date: 2017-10-13 13:15:08

On Fri, Oct 13, 2017 at 02:11:32PM +0900, Junio C Hamano wrote:
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 '-'.

Use it to also reject "HEAD" as a branch name.
Heh, I just pointed somebody to this a day or two ago as #leftoverbit. I
guess it's taken now. :)

Related to this: should we do a better job of confirming that the
refname is available for use?

If you do:

  git branch foo
  git checkout -b foo/bar

then "foo/bar" is not available. And for "checkout -b", we'd notice when
we tried to create the ref. But for:

  git checkout --orphan foo/bar

we'd update HEAD with a non-viable name, and only find out later during
"git commit". That's not the end of the world, but it might be nice to
complain when writing the symlink.

Largely orthogonal to the problem you're solving here, but I suspect it
may touch the same code, so it might be worth thinking about while we're
here.
quoted hunk
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index e88349c8a0..3ecb2eab0c 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -331,4 +331,12 @@ 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_expect_success 'checkout -b rejects HEAD as a branch name' '
+	test_must_fail git checkout -B HEAD HEAD^
+'
Should we test that:

  git update-ref refs/heads/HEAD HEAD^

continues to work?

-Peff

Re: [PATCH 0/3] a small branch API clean-up

From: Kaartic Sivaraam <hidden>
Date: 2017-10-21 03:07:14

On Fri, 2017-10-13 at 14:11 +0900, Junio C Hamano wrote:
This started as a little clean-up for a NEEDSWORK comment in
branch.h, but it ended up adding a rather useful safety feature.
Part of this series seems to duplicate the work done in part of my
series that tries to give more useful error messages when renaming a
branch.

https://public-inbox.org/git/%3C20170919071525.9404-1-kaarticsivaraam91196@gmail.com%3E/

Any reasons for this?

-- 
Kaartic

Re: [PATCH 2/3] branch: split validate_new_branchname() into two

From: Kaartic Sivaraam <hidden>
Date: 2017-10-21 05:02:37

On Fri, 2017-10-13 at 14:11 +0900, Junio C Hamano wrote:
quoted hunk
diff --git a/branch.c b/branch.c
index 7404597678..2c3a364a0b 100644
--- a/branch.c
+++ b/branch.c
@@ -178,19 +178,31 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)
 	return 0;
 }
 
-int validate_new_branchname(const char *name, struct strbuf *ref,
-			    int force, int attr_only)
+/*
+ * Check if 'name' can be a valid name for a branch; die otherwise.
+ * Return 1 if the named branch already exists; return 0 otherwise.
+ * Fill ref with the full refname for the branch.
+ */
+int validate_branchname(const char *name, struct strbuf *ref)
 {
-	const char *head;
-
 	if (strbuf_check_branch_ref(ref, name))
 		die(_("'%s' is not a valid branch name."), name);
 
-	if (!ref_exists(ref->buf))
-		return 0;
+	return ref_exists(ref->buf);
+}
 
-	if (attr_only)
-		return 1;
+/*
+ * Check if a branch 'name' can be created as a new branch; die otherwise.
+ * 'force' can be used when it is OK for the named branch already exists.
+ * Return 1 if the named branch already exists; return 0 otherwise.
+ * Fill ref with the full refname for the branch.
+ */
I guess it's better to avoid repeating the comments in both the .h and
.c file as they might quite easily become stale. I would prefer keeping
it in the header file alone.
quoted hunk
+int validate_new_branchname(const char *name, struct strbuf *ref, int force)
+{
+	const char *head;
+
+	if (!validate_branchname(name, ref))
+		return 0;
 
 	if (!force)
 		die(_("A branch named '%s' already exists."),
@@ -246,9 +258,9 @@ void create_branch(const char *name, const char *start_name,
 	if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
 		explicit_tracking = 1;
 
-	if (validate_new_branchname(name, &ref, force,
-				    track == BRANCH_TRACK_OVERRIDE ||
-				    clobber_head)) {
+	if ((track == BRANCH_TRACK_OVERRIDE || clobber_head)
+	    ? validate_branchname(name, &ref)
+	    : validate_new_branchname(name, &ref, force)) {
 		if (!force)
 			dont_change_ref = 1;
The change was simple by splitting the function into two and calling
the right function as required at every call site! As far as I could
see this doesn't seem to be reducing the confusion that the 'attr_only'
parameter caused. That's because the new validate_branchname function
seems to be called in some cases when the 'force' parameter is true and
in other cases the 'force' parameter is sent to the
'validate_new_branchname' function. So, I think consistency is lacking
in this change. That's just my opinion, of course.

-- 
Kaartic

[PATCH v2 1/2] branch: forbid refs/heads/HEAD

From: Kaartic Sivaraam <hidden>
Date: 2017-11-14 11:43:36

From: Junio C Hamano <redacted>

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 '-'.

Use it to also reject "HEAD" as a branch name.

Helped-by: Jeff King [off-list ref]
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Kaartic Sivaraam <redacted>
---
  Changes in v2:

  * Fixed the issue of .git/HEAD being renamed when trying to do,

      $ git branch -m HEAD head

   This also allows to rename a branch named HEAD that was created by accident.

   cf. [off-list ref]

  * Added a test to ensure that it is possible to rename a branch named HEAD.

 sha1_name.c             |  8 +++++++-
 t/t1430-bad-ref-name.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/sha1_name.c b/sha1_name.c
index 9a2d5caf3..657a060cb 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1438,9 +1438,15 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
 		strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
 	else
 		strbuf_addstr(sb, name);
-	if (name[0] == '-')
+	if (*name == '-')
 		return -1;
+
 	strbuf_splice(sb, 0, 0, "refs/heads/", 11);
+
+	/* HEAD is not to be used as a branch name */
+	if(!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 e88349c8a..421e80a7a 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -331,4 +331,33 @@ 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 head &&
+	test_must_fail git show-ref refs/heads/HEAD
+'
+
 test_done
-- 
2.15.0.rc2.397.geff0134c7.dirty

[PATCH v2 2/2] builtin/branch: remove redundant check for HEAD

From: Kaartic Sivaraam <hidden>
Date: 2017-11-14 11:43:46

The lower level code has been made to handle this case for the
sake of consistency. This has made this check redundant.

So, remove the redundant check.

Signed-off-by: Kaartic Sivaraam <redacted>
---
 builtin/branch.c | 3 ---
 1 file changed, 3 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index ffd39333a..5fc57cdc9 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -793,9 +793,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	} else if (argc > 0 && argc <= 2) {
 		struct branch *branch = branch_get(argv[0]);
 
-		if (!strcmp(argv[0], "HEAD"))
-			die(_("it does not make sense to create 'HEAD' manually"));
-
 		if (!branch)
 			die(_("no such branch '%s'"), argv[0]);
 
-- 
2.15.0.rc2.397.geff0134c7.dirty

Re: [PATCH v2 1/2] branch: forbid refs/heads/HEAD

From: Kaartic Sivaraam <hidden>
Date: 2017-11-14 12:00:40

I should have been a little more clear with the numbering, sorry. The 
correct prefix should have been as follows,

    * [PATCH v2 1/2] --> [PATCH v2 3/3]

    * [PATCH v2 1/2] --> [PATCH v2 4/3]

Sorry for the inconvenience.


---
Kaartic
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help