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

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

[Junio C Hamano]

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 1/3] branch: streamline "attr_only" handling in validate_new_branchname()

[Junio C Hamano]

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

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

[Eric Sunshine]

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>

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

[Junio C Hamano]

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

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

[Kaartic Sivaraam]

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

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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

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

[Jeff King]

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

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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

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

[Jeff King]

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

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

[Kaartic Sivaraam]

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

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

[Junio C Hamano]

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.

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

[Kaartic Sivaraam]

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

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

[Kaartic Sivaraam]

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 0/3] a small branch API clean-up

[Junio C Hamano]

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.

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

[Kaartic Sivaraam]

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

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

[Kaartic Sivaraam]

From: Junio C Hamano <gitster@pobox.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 '-'.

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>
Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
  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. <1509209933.2256.4.camel@gmail.com>

  * 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

[Kaartic Sivaraam]

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 <kaartic.sivaraam@gmail.com>
---
 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

[Kaartic Sivaraam]

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

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

[Junio C Hamano]

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

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

I assume that the second one above actually talks about what was
sent as "v2 2/2" (not "v2 1/2") being "4/3"?

Are these two patches follow-up fixes (replacement of 3/3 plus an
extra patch) to jc/branch-name-sanity topic?

Thanks for working on these.

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

[Kaartic Sivaraam]

On Tuesday 14 November 2017 08:38 PM, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
>> 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.
> 
> I assume that the second one above actually talks about what was
> sent as "v2 2/2" (not "v2 1/2") being "4/3"?
>

Yeah. Copy paste error, sorry.


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


Thanks,
Kaartic

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

[Junio C Hamano]

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

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

[Kaartic Sivaraam]

On Thursday 16 November 2017 03:44 AM, Junio C Hamano wrote:
>      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'.
> 

I guess this series is not yet ready for 'next'. When I tried to apply 
this patch it doesn't seem to be applying cleanly. I get some conflicts 
in 'sha1_name.c' possibly as a consequence of the changes to the file 
that aren't accounted by the patch. As to which change,

$ git whatchanged  jch/jc/branch-name-sanity..origin/next sha1_name.c

lists at least 5 of them, so there's possibly a lot of change that 
hasn't been taken into account by this patch. Particularly, the function 
'strbuf_check_branch_ref' itself is found at line 1435 in the version 
found in 'next' though this patch expects it to be near line 1332, I guess.

Further comment inline.

>   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"))

I guess this check should be made more consistent. Possibly either of,

	if (starts_with(sb->buf, "refs/heads/-") ||
	    !strcmp(sb->buf, "refs/heads/HEAD"))

or,

	if (*name == '-' ||
	    !strcmp(name, "HEAD"))


might make them consistent (at least from my perspective).


I tried to reproduce this patch manually and other than the above this 
one LGTM. Though I can't be very sure as I couldn't apply it (I did it 
"manually" to some extent, you see ;-)


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

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

[Junio C Hamano]

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

> I guess this series is not yet ready for 'next'. When I tried to apply
> this patch it doesn't seem to be applying cleanly. I get some
> conflicts in 'sha1_name.c' possibly as a consequence of the changes to
> the file that aren't accounted by the patch.

Oh, it is totally expected that this patch (and others) may not
apply on 'next' without conflict resolution, as this topic, as all
the other topics, are designed to apply cleanly to either 'master'
or 'maint' or one of the older 'maint-*' series, if it is a bugfix
topic.  A patch series that only applies cleanly to 'next' would be
useless---it would mean all the topics that are already in 'next'
that interacts with it must graduate first before it can go in.
Make it a habit to build on 'master' or older and then merge to
higher integration branches to make sure it fits with others.

What you could do is to inspect origin/pu branch after you fetch,
and look at the commit that merges this topic to learn how the
conflicts are resolved (the contrib/rerere-train.sh script may help
this process).

Your inability to resolve merge conflicts does not have much to do
with the readiness of a topic, as long as somebody else can resolve
them ;-)

>> +	if (*name == '-' ||
>> +	    !strcmp(sb->buf, "refs/heads/HEAD"))
>
> I guess this check should be made more consistent. Possibly either of,

Among these two, this one

> 	if (starts_with(sb->buf, "refs/heads/-") ||
> 	    !strcmp(sb->buf, "refs/heads/HEAD"))

has more chance to be correct.  Also, if we were to check the result
of expansion in sb->buf[], I would think that we should keep a
separate variable that points at &sb->buf[11] and compare the
remainder against fixed strings, as we already know sb->buf[] starts
with "refs/heads/" due to our splicing in the fixed string.

Because the point of using strbuf_branchname() is to allow us expand
funny notations like @{-1} to refs/heads/foo, and the result of
expansion is what eventually matters, checking name[] is wrong, I
would think, even though I haven't thought things through.  

In any case, I would say thinking this part through should be left
as a theme for a follow-on patch, and not within the scope of this
one.  After all, checking *name against '-' was part of the original
code, so it is safer to keep the thing we are not touching bug-to-bug
compatible and fixing things one step at a time (the one fix we made
with this patch is to make sure we store refs/heads/-dash in sb when
we reject name=="-dash").

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

[Kaartic Sivaraam]

On Thursday 16 November 2017 08:27 PM, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
>> I guess this series is not yet ready for 'next'. When I tried to apply
>> this patch it doesn't seem to be applying cleanly. I get some
>> conflicts in 'sha1_name.c' possibly as a consequence of the changes to
>> the file that aren't accounted by the patch.
> 
> Oh, it is totally expected that this patch (and others) may not
> apply on 'next' without conflict resolution, as this topic, as all
> the other topics, are designed to apply cleanly to either 'master'
> or 'maint' or one of the older 'maint-*' series, if it is a bugfix
> topic.  A patch series that only applies cleanly to 'next' would be
> useless---it would mean all the topics that are already in 'next'
> that interacts with it must graduate first before it can go in.

Thanks for the explanation. Seems I still have to become accustomed to 
the workflow.


> Make it a habit to build on 'master' or older and then merge to
> higher integration branches to make sure it fits with others.
>

Though I don't clearly understand how to do that, I'll let experience 
teach me the same (if possible). :-)


> What you could do is to inspect origin/pu branch after you fetch,
> and look at the commit that merges this topic to learn how the
> conflicts are resolved (the contrib/rerere-train.sh script may help
> this process).
> 

Sure thing.


>>> +	if (*name == '-' ||
>>> +	    !strcmp(sb->buf, "refs/heads/HEAD"))
>>
>> I guess this check should be made more consistent. Possibly either of,
> 
> Among these two, this one
> 
>> 	if (starts_with(sb->buf, "refs/heads/-") ||
>> 	    !strcmp(sb->buf, "refs/heads/HEAD"))
> 
> has more chance to be correct.  Also, if we were to check the result
> of expansion in sb->buf[], I would think that we should keep a
> separate variable that points at &sb->buf[11] and compare the
> remainder against fixed strings, as we already know sb->buf[] starts
> with "refs/heads/" due to our splicing in the fixed string.
> 
> Because the point of using strbuf_branchname() is to allow us expand
> funny notations like @{-1} to refs/heads/foo, and the result of
> expansion is what eventually matters, checking name[] is wrong, I
> would think, even though I haven't thought things through.
> 
> In any case, I would say thinking this part through should be left
> as a theme for a follow-on patch, and not within the scope of this
> one.  After all, checking *name against '-' was part of the original
> code, so it is safer to keep the thing we are not touching bug-to-bug
> compatible and fixing things one step at a time (the one fix we made
> with this patch is to make sure we store refs/heads/-dash in sb when
> we reject name=="-dash").
> 

Good point. This is better for a follow-up patch as there's a 
possibility that we might be introducing new bugs which weren't present 
previously as a consequence of changing that conditional (bug-to-bug 
compatibility, good term that (possibly) summarizes my long-winded 
explanation ;-)