git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH 0/3] a small branch API clean-up
@ 2017-10-13  5:11 Junio C Hamano
  2017-10-13  5:11 ` [PATCH 1/3] branch: streamline "attr_only" handling in validate_new_branchname() Junio C Hamano
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: Junio C Hamano @ 2017-10-13  5:11 UTC (permalink / raw)
  To: git

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/3] branch: streamline "attr_only" handling in validate_new_branchname()
  2017-10-13  5:11 [PATCH 0/3] a small branch API clean-up Junio C Hamano
@ 2017-10-13  5:11 ` 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
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2017-10-13  5:11 UTC (permalink / raw)
  To: git

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 <gitster@pobox.com>
---
 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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 2/3] branch: split validate_new_branchname() into two
  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  5:11 ` Junio C Hamano
  2017-10-21  4:58   ` Kaartic Sivaraam
  2017-10-13  5:11 ` [PATCH 3/3] branch: forbid refs/heads/HEAD Junio C Hamano
  2017-10-21  3:07 ` [PATCH 0/3] a small branch API clean-up Kaartic Sivaraam
  3 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2017-10-13  5:11 UTC (permalink / raw)
  To: git

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 <gitster@pobox.com>
---
 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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 3/3] branch: forbid refs/heads/HEAD
  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  5:11 ` [PATCH 2/3] branch: split validate_new_branchname() into two Junio C Hamano
@ 2017-10-13  5:11 ` Junio C Hamano
  2017-10-13 13:15   ` Jeff King
  2017-10-21  3:07 ` [PATCH 0/3] a small branch API clean-up Kaartic Sivaraam
  3 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2017-10-13  5:11 UTC (permalink / raw)
  To: git

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 <gitster@pobox.com>
---
 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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/3] branch: streamline "attr_only" handling in validate_new_branchname()
  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
  0 siblings, 0 replies; 17+ messages in thread
From: Eric Sunshine @ 2017-10-13  7:05 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git List

On Fri, Oct 13, 2017 at 1:11 AM, Junio C Hamano <gitster@pobox.com> 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 <gitster@pobox.com>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] branch: forbid refs/heads/HEAD
  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
  0 siblings, 1 reply; 17+ messages in thread
From: Jeff King @ 2017-10-13 13:15 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

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.

> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] branch: forbid refs/heads/HEAD
  2017-10-13 13:15   ` Jeff King
@ 2017-10-14  2:11     ` Junio C Hamano
  2017-10-14  2:20       ` Junio C Hamano
  0 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2017-10-14  2:11 UTC (permalink / raw)
  To: Jeff King; +Cc: git

Jeff King <peff@peff.net> writes:

> 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. :)

Didn't notice /remember it; sorry about that.  I can retract it if
you want, but perhaps they cannot unsee it ;-)

>> 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?

Perhaps.  Also we may want to make sure that "git branch -D HEAD"
still works as a way to recover from historical mistakes.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] branch: forbid refs/heads/HEAD
  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
  0 siblings, 2 replies; 17+ messages in thread
From: Junio C Hamano @ 2017-10-14  2:20 UTC (permalink / raw)
  To: Jeff King; +Cc: git

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

>> Should we test that:
>>
>>   git update-ref refs/heads/HEAD HEAD^
>>
>> continues to work?
>
> Perhaps.  Also we may want to make sure that "git branch -D HEAD"
> still works as a way to recover from historical mistakes.

The only difference is improved tests where we use show-ref to make
sure refs/heads/HEAD does not exist when it shouldn't, exercise
update-ref to create and delete refs/heads/HEAD, and also make sure
it can be deleted with "git branch -d".

-- >8 --
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 <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1_name.c             |  2 +-
 t/t1430-bad-ref-name.sh | 23 +++++++++++++++++++++++
 2 files changed, 24 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..4556aa66b8 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -331,4 +331,27 @@ 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_done
-- 
2.15.0-rc1-158-gbd035ae683


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] branch: forbid refs/heads/HEAD
  2017-10-14  2:20       ` Junio C Hamano
@ 2017-10-16 21:38         ` Jeff King
  2017-10-21  4:50         ` Kaartic Sivaraam
  1 sibling, 0 replies; 17+ messages in thread
From: Jeff King @ 2017-10-16 21:38 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Sat, Oct 14, 2017 at 11:20:11AM +0900, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> Should we test that:
> >>
> >>   git update-ref refs/heads/HEAD HEAD^
> >>
> >> continues to work?
> >
> > Perhaps.  Also we may want to make sure that "git branch -D HEAD"
> > still works as a way to recover from historical mistakes.
> 
> The only difference is improved tests where we use show-ref to make
> sure refs/heads/HEAD does not exist when it shouldn't, exercise
> update-ref to create and delete refs/heads/HEAD, and also make sure
> it can be deleted with "git branch -d".

Thanks, this looks good to me.

-Peff

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/3] a small branch API clean-up
  2017-10-13  5:11 [PATCH 0/3] a small branch API clean-up Junio C Hamano
                   ` (2 preceding siblings ...)
  2017-10-13  5:11 ` [PATCH 3/3] branch: forbid refs/heads/HEAD Junio C Hamano
@ 2017-10-21  3:07 ` Kaartic Sivaraam
  2017-10-21  8:52   ` Junio C Hamano
  3 siblings, 1 reply; 17+ messages in thread
From: Kaartic Sivaraam @ 2017-10-21  3:07 UTC (permalink / raw)
  To: Junio C Hamano, git

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] branch: forbid refs/heads/HEAD
  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
  1 sibling, 1 reply; 17+ messages in thread
From: Kaartic Sivaraam @ 2017-10-21  4:50 UTC (permalink / raw)
  To: Junio C Hamano, Jeff King; +Cc: git

Junio C Hamano <gitster@pobox.com> writes:
> 
> > Perhaps.  Also we may want to make sure that "git branch -D HEAD"
> > still works as a way to recover from historical mistakes.
> 
> The only difference is improved tests where we use show-ref to make
> sure refs/heads/HEAD does not exist when it shouldn't, exercise
> update-ref to create and delete refs/heads/HEAD, and also make sure
> it can be deleted with "git branch -d".
> 

In which case you might also like to ensure that it's possible to
"rename" the branch with a name "HEAD" to recover from historical
mistakes.

> 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;

I guess this makes the check for "HEAD" in builtin/branch::cmd_branch()
 (line 796) redundant. May be it could be removed?

>  	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..4556aa66b8 100755
> --- a/t/t1430-bad-ref-name.sh
> +++ b/t/t1430-bad-ref-name.sh
> @@ -331,4 +331,27 @@ 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_done

So, may be the following test could also be added (untested yet),

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
'

-- 
Kaartic

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/3] branch: split validate_new_branchname() into two
  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
  0 siblings, 1 reply; 17+ messages in thread
From: Kaartic Sivaraam @ 2017-10-21  4:58 UTC (permalink / raw)
  To: Junio C Hamano, git

On Fri, 2017-10-13 at 14:11 +0900, Junio C Hamano wrote:
> 
> 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.

> +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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/3] a small branch API clean-up
  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
  0 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2017-10-21  8:52 UTC (permalink / raw)
  To: Kaartic Sivaraam; +Cc: git

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

> 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?

Sorry, but I am not sure what you are asking.  

I was looking at the code, noticed NEEDSWORK comment and worked on
cleaning things up---you seem to feel that I need a reason, or
perhaps even your permission, when I try improving the codebase?
That starts to sound a bit ridiculous.

As different developers are multiple people, it just happens that
areas somebody changes overlap an area somebody else wants to
change.  It's not like anybody owns a piece of source file when s/he
expresses an interest to work on something that may or may not
affect that area.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] branch: forbid refs/heads/HEAD
  2017-10-21  4:50         ` Kaartic Sivaraam
@ 2017-10-21  8:57           ` Junio C Hamano
  2017-10-22  5:00             ` Kaartic Sivaraam
  0 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2017-10-21  8:57 UTC (permalink / raw)
  To: Kaartic Sivaraam; +Cc: Jeff King, git

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

>> The only difference is improved tests where we use show-ref to make
>> sure refs/heads/HEAD does not exist when it shouldn't, exercise
>> update-ref to create and delete refs/heads/HEAD, and also make sure
>> it can be deleted with "git branch -d".
>
> In which case you might also like to ensure that it's possible to
> "rename" the branch with a name "HEAD" to recover from historical
> mistakes.

Perhaps.  I didn't think it was all that needed---as long as you can
delete, you can recreate at the same commit with a more desirable
name, and it is not like users have tons of repositories with
misnamed branches that they need to fix.  The code may already
handle it, or there may need even more code to support the rename; I
didn't check.

>>  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;
>
> I guess this makes the check for "HEAD" in builtin/branch::cmd_branch()
>  (line 796) redundant. May be it could be removed?

Perhaps.  But I think that is better done as a follow-up "now the
lower level consistently handles, let's remove the extra check that
has become unnecessary" separate patch.

> So, may be the following test could also be added (untested yet),
>
> 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
> '

Yeah, that would be a good material for that separate follow-up
patch.

Thanks.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/3] branch: split validate_new_branchname() into two
  2017-10-21  4:58   ` Kaartic Sivaraam
@ 2017-10-21  9:01     ` Junio C Hamano
  0 siblings, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2017-10-21  9:01 UTC (permalink / raw)
  To: Kaartic Sivaraam; +Cc: git

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

>> +/*
>> + * 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.

True.  I wrote this while designing the code, so the copy in .c file
came first, and then that was copied to .h file; the one in .c file
can go.

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

The confusion primarily was the way the parameter was named.
"forcing" does not have strong connection to marking "this is only
asking about attributes".  And removing that confusion, by
separating the caller and making it explicit that the caller needs
two separate behaviours depending on the condition, and naming the
functions more appropriately (i.e. is this about creating a new one
that must not exist already?), is the focus of this step.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/3] a small branch API clean-up
  2017-10-21  8:52   ` Junio C Hamano
@ 2017-10-22  4:36     ` Kaartic Sivaraam
  0 siblings, 0 replies; 17+ messages in thread
From: Kaartic Sivaraam @ 2017-10-22  4:36 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Sat, 2017-10-21 at 17:52 +0900, Junio C Hamano wrote:
> 
> Sorry, but I am not sure what you are asking.  
> 
> I was looking at the code, noticed NEEDSWORK comment and worked on
> cleaning things up---you seem to feel that I need a reason, or
> perhaps even your permission, when I try improving the codebase?
> That starts to sound a bit ridiculous.

Nothing like that, sorry. I was thinking that you would "remember" the
fact that I was trying to clean up the NEEDSWORK in the patch series
mentioned before as you have reviewed/commented on it. So, I thought
there would be something my patch series was doing wrong which made you
send another series that cleans up the NEEDSWORK. I just wanted to know
that specific reason (the reason which made you send a series cleaning
up the NEEDSWORK when you saw another series doing the same thing)?

Of course that's assuming that you remembered my series cleaning up the
NEEDSWORK, in the first place. If that's not the case then there's no
reason you could give :-)

-- 
Kaartic

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] branch: forbid refs/heads/HEAD
  2017-10-21  8:57           ` Junio C Hamano
@ 2017-10-22  5:00             ` Kaartic Sivaraam
  0 siblings, 0 replies; 17+ messages in thread
From: Kaartic Sivaraam @ 2017-10-22  5:00 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, git

On Sat, 2017-10-21 at 17:57 +0900, Junio C Hamano wrote:
> ... The code may already
> handle it, or there may need even more code to support the rename; I
> didn't check.
> 

As far as I could see there the code does seem to already handle the
rename. This part of builtin/branch.c is what seems to be ensuring
that,

        if (strbuf_check_branch_ref(&oldref, oldname)) {
                /*
                 * Bad name --- this could be an attempt to rename a
                 * ref that we used to allow to be created by accident.
                 */
                if (ref_exists(oldref.buf))
                        recovery = 1;
                else
                        die(_("Invalid branch name: '%s'"), oldname);
        }


-- 
Kaartic

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, back to index

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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

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