{"thread":{"id":"46963","subject":"[PATCH 0/3] a small branch API clean-up","startedAt":"2017-10-13T05:11:40Z","lastAt":"2017-11-16T17:05:10Z","messageCount":26,"participants":["Junio C Hamano","Eric Sunshine","Jeff King","Kaartic Sivaraam"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"330333","messageId":"20171013051132.3973-1-gitster@pobox.com","threadId":"46963","inReplyTo":null,"subject":"[PATCH 0/3] a small branch API clean-up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-13T05:11:29Z","receivedAt":"2017-10-13T05:11:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This started as a little clean-up for a NEEDSWORK comment in\nbranch.h, but it ended up adding a rather useful safety feature.\n\nJunio C Hamano (3):\n branch: streamline \"attr_only\" handling in validate_new_branchname()\n branch: split validate_new_branchname() into two\n branch: forbid refs/heads/HEAD\n\n branch.c | 44 ++++++++++++++++++++++++++++++--------------\n branch.h | 27 ++++++++++++---------------\n builtin/branch.c | 8 ++++----\n builtin/checkout.c | 10 +++++-----\n sha1_name.c | 2 +-\n t/t1430-bad-ref-name.sh | 8 ++++++++\n 6 files changed, 60 insertions(+), 39 deletions(-)\n\n-- \n2.15.0-rc1-158-gbd035ae683\n\n"},{"id":"330334","messageId":"20171013051132.3973-3-gitster@pobox.com","threadId":"46963","inReplyTo":"20171013051132.3973-1-gitster@pobox.com","subject":"[PATCH 2/3] branch: split validate_new_branchname() into two","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-13T05:11:31Z","receivedAt":"2017-10-13T05:11:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Checking if a proposed name is appropriate for a branch is strictly\na subset of checking if we want to allow creating or updating a\nbranch with such a name. The mysterious sounding 'attr_only'\nparameter to validate_new_branchname() is used to switch the\nfunction between these two roles.\n\nInstead, split the function into two, and adjust the callers. A new\nhelper validate_branchname() only checks the name and reports if the\nbranch already exists.\n\nThis loses one NEEDSWORK from the branch API.\n\nSigned-off-by: Junio C Hamano \n---\n branch.c | 34 +++++++++++++++++++++++-----------\n branch.h | 27 ++++++++++++---------------\n builtin/branch.c | 8 ++++----\n builtin/checkout.c | 10 +++++-----\n 4 files changed, 44 insertions(+), 35 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 7404597678..2c3a364a0b 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -178,19 +178,31 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)\n \treturn 0;\n }\n \n-int validate_new_branchname(const char *name, struct strbuf *ref,\n-\t\t\t int force, int attr_only)\n+/*\n+ * Check if 'name' can be a valid name for a branch; die otherwise.\n+ * Return 1 if the named branch already exists; return 0 otherwise.\n+ * Fill ref with the full refname for the branch.\n+ */\n+int validate_branchname(const char *name, struct strbuf *ref)\n {\n-\tconst char *head;\n-\n \tif (strbuf_check_branch_ref(ref, name))\n \t\tdie(_(\"'%s' is not a valid branch name.\"), name);\n \n-\tif (!ref_exists(ref->buf))\n-\t\treturn 0;\n+\treturn ref_exists(ref->buf);\n+}\n \n-\tif (attr_only)\n-\t\treturn 1;\n+/*\n+ * Check if a branch 'name' can be created as a new branch; die otherwise.\n+ * 'force' can be used when it is OK for the named branch already exists.\n+ * Return 1 if the named branch already exists; return 0 otherwise.\n+ * Fill ref with the full refname for the branch.\n+ */\n+int validate_new_branchname(const char *name, struct strbuf *ref, int force)\n+{\n+\tconst char *head;\n+\n+\tif (!validate_branchname(name, ref))\n+\t\treturn 0;\n \n \tif (!force)\n \t\tdie(_(\"A branch named '%s' already exists.\"),\n@@ -246,9 +258,9 @@ void create_branch(const char *name, const char *start_name,\n \tif (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)\n \t\texplicit_tracking = 1;\n \n-\tif (validate_new_branchname(name, &ref, force,\n-\t\t\t\t track == BRANCH_TRACK_OVERRIDE ||\n-\t\t\t\t clobber_head)) {\n+\tif ((track == BRANCH_TRACK_OVERRIDE || clobber_head)\n+\t ? validate_branchname(name, &ref)\n+\t : validate_new_branchname(name, &ref, force)) {\n \t\tif (!force)\n \t\t\tdont_change_ref = 1;\n \t\telse\ndiff --git a/branch.h b/branch.h\nindex b07788558c..be5e5d1308 100644\n--- a/branch.h\n+++ b/branch.h\n@@ -23,22 +23,19 @@ void create_branch(const char *name, const char *start_name,\n \t\t int clobber_head, int quiet, enum branch_track track);\n \n /*\n- * Validates that the requested branch may be created, returning the\n- * interpreted ref in ref, force indicates whether (non-head) branches\n- * may be overwritten. A non-zero return value indicates that the force\n- * parameter was non-zero and the branch already exists.\n- *\n- * Contrary to all of the above, when attr_only is 1, the caller is\n- * not interested in verifying if it is Ok to update the named\n- * branch to point at a potentially different commit. It is merely\n- * asking if it is OK to change some attribute for the named branch\n- * (e.g. tracking upstream).\n- *\n- * NEEDSWORK: This needs to be split into two separate functions in the\n- * longer run for sanity.\n- *\n+ * Check if 'name' can be a valid name for a branch; die otherwise.\n+ * Return 1 if the named branch already exists; return 0 otherwise.\n+ * Fill ref with the full refname for the branch.\n+ */\n+extern int validate_branchname(const char *name, struct strbuf *ref);\n+\n+/*\n+ * Check if a branch 'name' can be created as a new branch; die otherwise.\n+ * 'force' can be used when it is OK for the named branch already exists.\n+ * Return 1 if the named branch already exists; return 0 otherwise.\n+ * Fill ref with the full refname for the branch.\n */\n-int validate_new_branchname(const char *name, struct strbuf *ref, int force, int attr_only);\n+extern int validate_new_branchname(const char *name, struct strbuf *ref, int force);\n \n /*\n * Remove information about the state of working on the current\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex b67593288c..e5bbfb4a17 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -463,7 +463,6 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \tstruct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;\n \tstruct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;\n \tint recovery = 0;\n-\tint clobber_head_ok;\n \n \tif (!oldname) {\n \t\tif (copy)\n@@ -487,9 +486,10 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t * A command like \"git branch -M currentbranch currentbranch\" cannot\n \t * cause the worktree to become inconsistent with HEAD, so allow it.\n \t */\n-\tclobber_head_ok = !strcmp(oldname, newname);\n-\n-\tvalidate_new_branchname(newname, &newref, force, clobber_head_ok);\n+\tif (!strcmp(oldname, newname))\n+\t\tvalidate_branchname(newname, &newref);\n+\telse\n+\t\tvalidate_new_branchname(newname, &newref, force);\n \n \treject_rebase_or_bisect_branch(oldref.buf);\n \ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex fc4f8fd2ea..697ac7dcaf 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1289,11 +1289,11 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \tif (opts.new_branch) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n-\t\topts.branch_exists =\n-\t\t\tvalidate_new_branchname(opts.new_branch, &buf,\n-\t\t\t\t\t\t!!opts.new_branch_force,\n-\t\t\t\t\t\t!!opts.new_branch_force);\n-\n+\t\tif (opts.new_branch_force)\n+\t\t\topts.branch_exists = validate_branchname(opts.new_branch, &buf);\n+\t\telse\n+\t\t\topts.branch_exists =\n+\t\t\t\tvalidate_new_branchname(opts.new_branch, &buf, 0);\n \t\tstrbuf_release(&buf);\n \t}\n \n-- \n2.15.0-rc1-158-gbd035ae683\n\n"},{"id":"330335","messageId":"20171013051132.3973-2-gitster@pobox.com","threadId":"46963","inReplyTo":"20171013051132.3973-1-gitster@pobox.com","subject":"[PATCH 1/3] branch: streamline \"attr_only\" handling in validate_new_branchname()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-13T05:11:30Z","receivedAt":"2017-10-13T05:11:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The function takes a parameter \"attr_only\" (which is a name that is\nhard to reason about, which will be corrected soon) and skips some\nchecks when it is set. Reorganize the conditionals to make it move\nobvious that some checks are never made when this parameter is set.\n\nSigned-off-by: Junio C Hamano \n---\n branch.c | 20 ++++++++++++--------\n 1 file changed, 12 insertions(+), 8 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 4377ce2fb1..7404597678 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -181,21 +181,25 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)\n int validate_new_branchname(const char *name, struct strbuf *ref,\n \t\t\t int force, int attr_only)\n {\n+\tconst char *head;\n+\n \tif (strbuf_check_branch_ref(ref, name))\n \t\tdie(_(\"'%s' is not a valid branch name.\"), name);\n \n \tif (!ref_exists(ref->buf))\n \t\treturn 0;\n-\telse if (!force && !attr_only)\n-\t\tdie(_(\"A branch named '%s' already exists.\"), ref->buf + strlen(\"refs/heads/\"));\n \n-\tif (!attr_only) {\n-\t\tconst char *head;\n+\tif (attr_only)\n+\t\treturn 1;\n+\n+\tif (!force)\n+\t\tdie(_(\"A branch named '%s' already exists.\"),\n+\t\t ref->buf + strlen(\"refs/heads/\"));\n+\n+\thead = resolve_ref_unsafe(\"HEAD\", 0, NULL, NULL);\n+\tif (!is_bare_repository() && head && !strcmp(head, ref->buf))\n+\t\tdie(_(\"Cannot force update the current branch.\"));\n \n-\t\thead = resolve_ref_unsafe(\"HEAD\", 0, NULL, NULL);\n-\t\tif (!is_bare_repository() && head && !strcmp(head, ref->buf))\n-\t\t\tdie(_(\"Cannot force update the current branch.\"));\n-\t}\n \treturn 1;\n }\n \n-- \n2.15.0-rc1-158-gbd035ae683\n\n"},{"id":"330336","messageId":"20171013051132.3973-4-gitster@pobox.com","threadId":"46963","inReplyTo":"20171013051132.3973-1-gitster@pobox.com","subject":"[PATCH 3/3] branch: forbid refs/heads/HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-13T05:11:32Z","receivedAt":"2017-10-13T05:12:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"strbuf_check_branch_ref() is the central place where many codepaths\nsee if a proposed name is suitable for the name of a branch. It was\ndesigned to allow us to get stricter than the check_refname_format()\ncheck used for refnames in general, and we already use it to reject\na branch whose name begins with a '-'.\n\nUse it to also reject \"HEAD\" as a branch name.\n\nSigned-off-by: Junio C Hamano \n---\n sha1_name.c | 2 +-\n t/t1430-bad-ref-name.sh | 8 ++++++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex c7c5ab376c..1b8c503095 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1332,7 +1332,7 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)\n int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n {\n \tstrbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);\n-\tif (name[0] == '-')\n+\tif (*name == '-' || !strcmp(name, \"HEAD\"))\n \t\treturn -1;\n \tstrbuf_splice(sb, 0, 0, \"refs/heads/\", 11);\n \treturn check_refname_format(sb->buf, 0);\ndiff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\nindex e88349c8a0..3ecb2eab0c 100755\n--- a/t/t1430-bad-ref-name.sh\n+++ b/t/t1430-bad-ref-name.sh\n@@ -331,4 +331,12 @@ test_expect_success 'update-ref --stdin -z fails delete with bad ref name' '\n \tgrep \"fatal: invalid ref format: ~a\" err\n '\n \n+test_expect_success 'branch rejects HEAD as a branch name' '\n+\ttest_must_fail git branch HEAD HEAD^\n+'\n+\n+test_expect_success 'checkout -b rejects HEAD as a branch name' '\n+\ttest_must_fail git checkout -B HEAD HEAD^\n+'\n+\n test_done\n-- \n2.15.0-rc1-158-gbd035ae683\n\n"},{"id":"330338","messageId":"CAPig+cSey2M4jUE-VbdqyQjDWZL1PHfvei5VL1VE+KDxd82v6g@mail.gmail.com","threadId":"46963","inReplyTo":"20171013051132.3973-2-gitster@pobox.com","subject":"Re: [PATCH 1/3] branch: streamline \"attr_only\" handling in validate_new_branchname()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-10-13T07:05:59Z","receivedAt":"2017-10-13T07:06:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Oct 13, 2017 at 1:11 AM, Junio C Hamano wrote:\n> The function takes a parameter \"attr_only\" (which is a name that is\n> hard to reason about, which will be corrected soon) and skips some\n> checks when it is set. Reorganize the conditionals to make it move\n\ns/move/more/\n\n> obvious that some checks are never made when this parameter is set.\n>\n> Signed-off-by: Junio C Hamano \n"},{"id":"330354","messageId":"20171013131501.3qarwhanktfvgjqd@sigill.intra.peff.net","threadId":"46963","inReplyTo":"20171013051132.3973-4-gitster@pobox.com","subject":"Re: [PATCH 3/3] branch: forbid refs/heads/HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-13T13:15:01Z","receivedAt":"2017-10-13T13:15:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 13, 2017 at 02:11:32PM +0900, Junio C Hamano wrote:\n\n> strbuf_check_branch_ref() is the central place where many codepaths\n> see if a proposed name is suitable for the name of a branch. It was\n> designed to allow us to get stricter than the check_refname_format()\n> check used for refnames in general, and we already use it to reject\n> a branch whose name begins with a '-'.\n> \n> Use it to also reject \"HEAD\" as a branch name.\n\nHeh, I just pointed somebody to this a day or two ago as #leftoverbit. I\nguess it's taken now. :)\n\nRelated to this: should we do a better job of confirming that the\nrefname is available for use?\n\nIf you do:\n\n git branch foo\n git checkout -b foo/bar\n\nthen \"foo/bar\" is not available. And for \"checkout -b\", we'd notice when\nwe tried to create the ref. But for:\n\n git checkout --orphan foo/bar\n\nwe'd update HEAD with a non-viable name, and only find out later during\n\"git commit\". That's not the end of the world, but it might be nice to\ncomplain when writing the symlink.\n\nLargely orthogonal to the problem you're solving here, but I suspect it\nmay touch the same code, so it might be worth thinking about while we're\nhere.\n\n> diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\n> index e88349c8a0..3ecb2eab0c 100755\n> --- a/t/t1430-bad-ref-name.sh\n> +++ b/t/t1430-bad-ref-name.sh\n> @@ -331,4 +331,12 @@ test_expect_success 'update-ref --stdin -z fails delete with bad ref name' '\n> \tgrep \"fatal: invalid ref format: ~a\" err\n> '\n> \n> +test_expect_success 'branch rejects HEAD as a branch name' '\n> +\ttest_must_fail git branch HEAD HEAD^\n> +'\n> +\n> +test_expect_success 'checkout -b rejects HEAD as a branch name' '\n> +\ttest_must_fail git checkout -B HEAD HEAD^\n> +'\n\nShould we test that:\n\n git update-ref refs/heads/HEAD HEAD^\n\ncontinues to work?\n\n-Peff\n"},{"id":"330387","messageId":"xmqqefq6v5qj.fsf@gitster.mtv.corp.google.com","threadId":"46963","inReplyTo":"20171013131501.3qarwhanktfvgjqd@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] branch: forbid refs/heads/HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-14T02:11:00Z","receivedAt":"2017-10-14T02:11:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Oct 13, 2017 at 02:11:32PM +0900, Junio C Hamano wrote:\n>\n>> strbuf_check_branch_ref() is the central place where many codepaths\n>> see if a proposed name is suitable for the name of a branch. It was\n>> designed to allow us to get stricter than the check_refname_format()\n>> check used for refnames in general, and we already use it to reject\n>> a branch whose name begins with a '-'.\n>> \n>> Use it to also reject \"HEAD\" as a branch name.\n>\n> Heh, I just pointed somebody to this a day or two ago as #leftoverbit. I\n> guess it's taken now. :)\n\nDidn't notice /remember it; sorry about that. I can retract it if\nyou want, but perhaps they cannot unsee it ;-)\n\n>> diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\n>> index e88349c8a0..3ecb2eab0c 100755\n>> --- a/t/t1430-bad-ref-name.sh\n>> +++ b/t/t1430-bad-ref-name.sh\n>> @@ -331,4 +331,12 @@ test_expect_success 'update-ref --stdin -z fails delete with bad ref name' '\n>> \tgrep \"fatal: invalid ref format: ~a\" err\n>> '\n>> \n>> +test_expect_success 'branch rejects HEAD as a branch name' '\n>> +\ttest_must_fail git branch HEAD HEAD^\n>> +'\n>> +\n>> +test_expect_success 'checkout -b rejects HEAD as a branch name' '\n>> +\ttest_must_fail git checkout -B HEAD HEAD^\n>> +'\n>\n> Should we test that:\n>\n> git update-ref refs/heads/HEAD HEAD^\n>\n> continues to work?\n\nPerhaps. Also we may want to make sure that \"git branch -D HEAD\"\nstill works as a way to recover from historical mistakes.\n"},{"id":"330388","messageId":"xmqqa80uv5b8.fsf@gitster.mtv.corp.google.com","threadId":"46963","inReplyTo":"xmqqefq6v5qj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] branch: forbid refs/heads/HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-14T02:20:11Z","receivedAt":"2017-10-14T02:20:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> Should we test that:\n>>\n>> git update-ref refs/heads/HEAD HEAD^\n>>\n>> continues to work?\n>\n> Perhaps. Also we may want to make sure that \"git branch -D HEAD\"\n> still works as a way to recover from historical mistakes.\n\nThe only difference is improved tests where we use show-ref to make\nsure refs/heads/HEAD does not exist when it shouldn't, exercise\nupdate-ref to create and delete refs/heads/HEAD, and also make sure\nit can be deleted with \"git branch -d\".\n\n-- >8 --\nstrbuf_check_branch_ref() is the central place where many codepaths\nsee if a proposed name is suitable for the name of a branch. It was\ndesigned to allow us to get stricter than the check_refname_format()\ncheck used for refnames in general, and we already use it to reject\na branch whose name begins with a '-'.\n\nUse it to also reject \"HEAD\" as a branch name.\n\nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n sha1_name.c | 2 +-\n t/t1430-bad-ref-name.sh | 23 +++++++++++++++++++++++\n 2 files changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex c7c5ab376c..1b8c503095 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1332,7 +1332,7 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)\n int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n {\n \tstrbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);\n-\tif (name[0] == '-')\n+\tif (*name == '-' || !strcmp(name, \"HEAD\"))\n \t\treturn -1;\n \tstrbuf_splice(sb, 0, 0, \"refs/heads/\", 11);\n \treturn check_refname_format(sb->buf, 0);\ndiff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\nindex e88349c8a0..4556aa66b8 100755\n--- a/t/t1430-bad-ref-name.sh\n+++ b/t/t1430-bad-ref-name.sh\n@@ -331,4 +331,27 @@ test_expect_success 'update-ref --stdin -z fails delete with bad ref name' '\n \tgrep \"fatal: invalid ref format: ~a\" err\n '\n \n+test_expect_success 'branch rejects HEAD as a branch name' '\n+\ttest_must_fail git branch HEAD HEAD^ &&\n+\ttest_must_fail git show-ref refs/heads/HEAD\n+'\n+\n+test_expect_success 'checkout -b rejects HEAD as a branch name' '\n+\ttest_must_fail git checkout -B HEAD HEAD^ &&\n+\ttest_must_fail git show-ref refs/heads/HEAD\n+'\n+\n+test_expect_success 'update-ref can operate on refs/heads/HEAD' '\n+\tgit update-ref refs/heads/HEAD HEAD^ &&\n+\tgit show-ref refs/heads/HEAD &&\n+\tgit update-ref -d refs/heads/HEAD &&\n+\ttest_must_fail git show-ref refs/heads/HEAD\n+'\n+\n+test_expect_success 'branch -d can remove refs/heads/HEAD' '\n+\tgit update-ref refs/heads/HEAD HEAD^ &&\n+\tgit branch -d HEAD &&\n+\ttest_must_fail git show-ref refs/heads/HEAD\n+'\n+\n test_done\n-- \n2.15.0-rc1-158-gbd035ae683\n\n"},{"id":"330481","messageId":"20171016213819.jgx5tihszqngvti2@sigill.intra.peff.net","threadId":"46963","inReplyTo":"xmqqa80uv5b8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] branch: forbid refs/heads/HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-16T21:38:20Z","receivedAt":"2017-10-16T21:38:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 14, 2017 at 11:20:11AM +0900, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> >> Should we test that:\n> >>\n> >> git update-ref refs/heads/HEAD HEAD^\n> >>\n> >> continues to work?\n> >\n> > Perhaps. Also we may want to make sure that \"git branch -D HEAD\"\n> > still works as a way to recover from historical mistakes.\n> \n> The only difference is improved tests where we use show-ref to make\n> sure refs/heads/HEAD does not exist when it shouldn't, exercise\n> update-ref to create and delete refs/heads/HEAD, and also make sure\n> it can be deleted with \"git branch -d\".\n\nThanks, this looks good to me.\n\n-Peff\n"},{"id":"330769","messageId":"1508555222.2516.11.camel@gmail.com","threadId":"46963","inReplyTo":"20171013051132.3973-1-gitster@pobox.com","subject":"Re: [PATCH 0/3] a small branch API clean-up","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-10-21T03:07:02Z","receivedAt":"2017-10-21T03:07:14Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Fri, 2017-10-13 at 14:11 +0900, Junio C Hamano wrote:\n> This started as a little clean-up for a NEEDSWORK comment in\n> branch.h, but it ended up adding a rather useful safety feature.\n> \n\nPart of this series seems to duplicate the work done in part of my\nseries that tries to give more useful error messages when renaming a\nbranch.\n\nhttps://public-inbox.org/git/%3C20170919071525.9404-1-kaarticsivaraam91196@gmail.com%3E/\n\nAny reasons for this?\n\n-- \nKaartic\n"},{"id":"330774","messageId":"1508561424.2516.19.camel@gmail.com","threadId":"46963","inReplyTo":"xmqqa80uv5b8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] branch: forbid refs/heads/HEAD","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-10-21T04:50:24Z","receivedAt":"2017-10-21T04:50:34Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"Junio C Hamano writes:\n> \n> > Perhaps. Also we may want to make sure that \"git branch -D HEAD\"\n> > still works as a way to recover from historical mistakes.\n> \n> The only difference is improved tests where we use show-ref to make\n> sure refs/heads/HEAD does not exist when it shouldn't, exercise\n> update-ref to create and delete refs/heads/HEAD, and also make sure\n> it can be deleted with \"git branch -d\".\n> \n\nIn which case you might also like to ensure that it's possible to\n\"rename\" the branch with a name \"HEAD\" to recover from historical\nmistakes.\n\n> diff --git a/sha1_name.c b/sha1_name.c\n> index c7c5ab376c..1b8c503095 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -1332,7 +1332,7 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)\n> int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n> {\n> \tstrbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);\n> -\tif (name[0] == '-')\n> +\tif (*name == '-' || !strcmp(name, \"HEAD\"))\n> \t\treturn -1;\n\nI guess this makes the check for \"HEAD\" in builtin/branch::cmd_branch()\n (line 796) redundant. May be it could be removed?\n\n> \tstrbuf_splice(sb, 0, 0, \"refs/heads/\", 11);\n> \treturn check_refname_format(sb->buf, 0);\n> diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\n> index e88349c8a0..4556aa66b8 100755\n> --- a/t/t1430-bad-ref-name.sh\n> +++ b/t/t1430-bad-ref-name.sh\n> @@ -331,4 +331,27 @@ test_expect_success 'update-ref --stdin -z fails delete with bad ref name' '\n> \tgrep \"fatal: invalid ref format: ~a\" err\n> '\n> \n> +test_expect_success 'branch rejects HEAD as a branch name' '\n> +\ttest_must_fail git branch HEAD HEAD^ &&\n> +\ttest_must_fail git show-ref refs/heads/HEAD\n> +'\n> +\n> +test_expect_success 'checkout -b rejects HEAD as a branch name' '\n> +\ttest_must_fail git checkout -B HEAD HEAD^ &&\n> +\ttest_must_fail git show-ref refs/heads/HEAD\n> +'\n> +\n> +test_expect_success 'update-ref can operate on refs/heads/HEAD' '\n> +\tgit update-ref refs/heads/HEAD HEAD^ &&\n> +\tgit show-ref refs/heads/HEAD &&\n> +\tgit update-ref -d refs/heads/HEAD &&\n> +\ttest_must_fail git show-ref refs/heads/HEAD\n> +'\n> +\n> +test_expect_success 'branch -d can remove refs/heads/HEAD' '\n> +\tgit update-ref refs/heads/HEAD HEAD^ &&\n> +\tgit branch -d HEAD &&\n> +\ttest_must_fail git show-ref refs/heads/HEAD\n> +'\n> +\n> test_done\n\nSo, may be the following test could also be added (untested yet),\n\ntest_expect_success 'branch -m can rename refs/heads/HEAD' '\n\tgit update-ref refs/heads/HEAD HEAD^ &&\n\tgit branch -m HEAD head &&\n\ttest_must_fail git show-ref refs/heads/HEAD\n'\n\n-- \nKaartic\n"},{"id":"330775","messageId":"1508561910.2516.22.camel@gmail.com","threadId":"46963","inReplyTo":"20171013051132.3973-3-gitster@pobox.com","subject":"Re: [PATCH 2/3] branch: split validate_new_branchname() into two","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-10-21T04:58:30Z","receivedAt":"2017-10-21T05:02:37Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Fri, 2017-10-13 at 14:11 +0900, Junio C Hamano wrote:\n> \n> diff --git a/branch.c b/branch.c\n> index 7404597678..2c3a364a0b 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -178,19 +178,31 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)\n> \treturn 0;\n> }\n> \n> -int validate_new_branchname(const char *name, struct strbuf *ref,\n> -\t\t\t int force, int attr_only)\n> +/*\n> + * Check if 'name' can be a valid name for a branch; die otherwise.\n> + * Return 1 if the named branch already exists; return 0 otherwise.\n> + * Fill ref with the full refname for the branch.\n> + */\n> +int validate_branchname(const char *name, struct strbuf *ref)\n> {\n> -\tconst char *head;\n> -\n> \tif (strbuf_check_branch_ref(ref, name))\n> \t\tdie(_(\"'%s' is not a valid branch name.\"), name);\n> \n> -\tif (!ref_exists(ref->buf))\n> -\t\treturn 0;\n> +\treturn ref_exists(ref->buf);\n> +}\n> \n> -\tif (attr_only)\n> -\t\treturn 1;\n> +/*\n> + * Check if a branch 'name' can be created as a new branch; die otherwise.\n> + * 'force' can be used when it is OK for the named branch already exists.\n> + * Return 1 if the named branch already exists; return 0 otherwise.\n> + * Fill ref with the full refname for the branch.\n> + */\n\nI guess it's better to avoid repeating the comments in both the .h and\n.c file as they might quite easily become stale. I would prefer keeping\nit in the header file alone.\n\n> +int validate_new_branchname(const char *name, struct strbuf *ref, int force)\n> +{\n> +\tconst char *head;\n> +\n> +\tif (!validate_branchname(name, ref))\n> +\t\treturn 0;\n> \n> \tif (!force)\n> \t\tdie(_(\"A branch named '%s' already exists.\"),\n> @@ -246,9 +258,9 @@ void create_branch(const char *name, const char *start_name,\n> \tif (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)\n> \t\texplicit_tracking = 1;\n> \n> -\tif (validate_new_branchname(name, &ref, force,\n> -\t\t\t\t track == BRANCH_TRACK_OVERRIDE ||\n> -\t\t\t\t clobber_head)) {\n> +\tif ((track == BRANCH_TRACK_OVERRIDE || clobber_head)\n> +\t ? validate_branchname(name, &ref)\n> +\t : validate_new_branchname(name, &ref, force)) {\n> \t\tif (!force)\n> \t\t\tdont_change_ref = 1;\n> \n\nThe change was simple by splitting the function into two and calling\nthe right function as required at every call site! As far as I could\nsee this doesn't seem to be reducing the confusion that the 'attr_only'\nparameter caused. That's because the new validate_branchname function\nseems to be called in some cases when the 'force' parameter is true and\nin other cases the 'force' parameter is sent to the\n'validate_new_branchname' function. So, I think consistency is lacking\nin this change. That's just my opinion, of course.\n\n-- \nKaartic\n"},{"id":"330782","messageId":"xmqqtvysg9w8.fsf@gitster.mtv.corp.google.com","threadId":"46963","inReplyTo":"1508555222.2516.11.camel@gmail.com","subject":"Re: [PATCH 0/3] a small branch API clean-up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-21T08:52:55Z","receivedAt":"2017-10-21T08:53:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kaartic Sivaraam writes:\n\n> On Fri, 2017-10-13 at 14:11 +0900, Junio C Hamano wrote:\n>> This started as a little clean-up for a NEEDSWORK comment in\n>> branch.h, but it ended up adding a rather useful safety feature.\n>> \n>\n> Part of this series seems to duplicate the work done in part of my\n> series that tries to give more useful error messages when renaming a\n> branch.\n>\n> https://public-inbox.org/git/%3C20170919071525.9404-1-kaarticsivaraam91196@gmail.com%3E/\n>\n> Any reasons for this?\n\nSorry, but I am not sure what you are asking. \n\nI was looking at the code, noticed NEEDSWORK comment and worked on\ncleaning things up---you seem to feel that I need a reason, or\nperhaps even your permission, when I try improving the codebase?\nThat starts to sound a bit ridiculous.\n\nAs different developers are multiple people, it just happens that\nareas somebody changes overlap an area somebody else wants to\nchange. It's not like anybody owns a piece of source file when s/he\nexpresses an interest to work on something that may or may not\naffect that area.\n"},{"id":"330783","messageId":"xmqqpo9gg9pa.fsf@gitster.mtv.corp.google.com","threadId":"46963","inReplyTo":"1508561424.2516.19.camel@gmail.com","subject":"Re: [PATCH 3/3] branch: forbid refs/heads/HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-21T08:57:05Z","receivedAt":"2017-10-21T08:57:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kaartic Sivaraam writes:\n\n>> The only difference is improved tests where we use show-ref to make\n>> sure refs/heads/HEAD does not exist when it shouldn't, exercise\n>> update-ref to create and delete refs/heads/HEAD, and also make sure\n>> it can be deleted with \"git branch -d\".\n>\n> In which case you might also like to ensure that it's possible to\n> \"rename\" the branch with a name \"HEAD\" to recover from historical\n> mistakes.\n\nPerhaps. I didn't think it was all that needed---as long as you can\ndelete, you can recreate at the same commit with a more desirable\nname, and it is not like users have tons of repositories with\nmisnamed branches that they need to fix. The code may already\nhandle it, or there may need even more code to support the rename; I\ndidn't check.\n\n>> int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n>> {\n>> \tstrbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);\n>> -\tif (name[0] == '-')\n>> +\tif (*name == '-' || !strcmp(name, \"HEAD\"))\n>> \t\treturn -1;\n>\n> I guess this makes the check for \"HEAD\" in builtin/branch::cmd_branch()\n> (line 796) redundant. May be it could be removed?\n\nPerhaps. But I think that is better done as a follow-up \"now the\nlower level consistently handles, let's remove the extra check that\nhas become unnecessary\" separate patch.\n\n> So, may be the following test could also be added (untested yet),\n>\n> test_expect_success 'branch -m can rename refs/heads/HEAD' '\n> \tgit update-ref refs/heads/HEAD HEAD^ &&\n> \tgit branch -m HEAD head &&\n> \ttest_must_fail git show-ref refs/heads/HEAD\n> '\n\nYeah, that would be a good material for that separate follow-up\npatch.\n\nThanks.\n\n"},{"id":"330784","messageId":"xmqqlgk4g9h7.fsf@gitster.mtv.corp.google.com","threadId":"46963","inReplyTo":"1508561910.2516.22.camel@gmail.com","subject":"Re: [PATCH 2/3] branch: split validate_new_branchname() into two","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-21T09:01:56Z","receivedAt":"2017-10-21T09:02:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kaartic Sivaraam writes:\n\n>> +/*\n>> + * Check if a branch 'name' can be created as a new branch; die otherwise.\n>> + * 'force' can be used when it is OK for the named branch already exists.\n>> + * Return 1 if the named branch already exists; return 0 otherwise.\n>> + * Fill ref with the full refname for the branch.\n>> + */\n>\n> I guess it's better to avoid repeating the comments in both the .h and\n> .c file as they might quite easily become stale. I would prefer keeping\n> it in the header file alone.\n\nTrue. I wrote this while designing the code, so the copy in .c file\ncame first, and then that was copied to .h file; the one in .c file\ncan go.\n\n> The change was simple by splitting the function into two and calling\n> the right function as required at every call site! As far as I could\n> see this doesn't seem to be reducing the confusion that the 'attr_only'\n> parameter caused.\n\nThe confusion primarily was the way the parameter was named.\n\"forcing\" does not have strong connection to marking \"this is only\nasking about attributes\". And removing that confusion, by\nseparating the caller and making it explicit that the caller needs\ntwo separate behaviours depending on the condition, and naming the\nfunctions more appropriately (i.e. is this about creating a new one\nthat must not exist already?), is the focus of this step.\n"},{"id":"330798","messageId":"1508646970.2152.2.camel@gmail.com","threadId":"46963","inReplyTo":"xmqqtvysg9w8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/3] a small branch API clean-up","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-10-22T04:36:10Z","receivedAt":"2017-10-22T04:36:22Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Sat, 2017-10-21 at 17:52 +0900, Junio C Hamano wrote:\n> \n> Sorry, but I am not sure what you are asking. \n> \n> I was looking at the code, noticed NEEDSWORK comment and worked on\n> cleaning things up---you seem to feel that I need a reason, or\n> perhaps even your permission, when I try improving the codebase?\n> That starts to sound a bit ridiculous.\n\nNothing like that, sorry. I was thinking that you would \"remember\" the\nfact that I was trying to clean up the NEEDSWORK in the patch series\nmentioned before as you have reviewed/commented on it. So, I thought\nthere would be something my patch series was doing wrong which made you\nsend another series that cleans up the NEEDSWORK. I just wanted to know\nthat specific reason (the reason which made you send a series cleaning\nup the NEEDSWORK when you saw another series doing the same thing)?\n\nOf course that's assuming that you remembered my series cleaning up the\nNEEDSWORK, in the first place. If that's not the case then there's no\nreason you could give :-)\n\n-- \nKaartic\n"},{"id":"330799","messageId":"1508648422.2152.7.camel@gmail.com","threadId":"46963","inReplyTo":"xmqqpo9gg9pa.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] branch: forbid refs/heads/HEAD","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-10-22T05:00:22Z","receivedAt":"2017-10-22T05:06:49Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Sat, 2017-10-21 at 17:57 +0900, Junio C Hamano wrote:\n> ... The code may already\n> handle it, or there may need even more code to support the rename; I\n> didn't check.\n> \n\nAs far as I could see there the code does seem to already handle the\nrename. This part of builtin/branch.c is what seems to be ensuring\nthat,\n\n if (strbuf_check_branch_ref(&oldref, oldname)) {\n /*\n * Bad name --- this could be an attempt to rename a\n * ref that we used to allow to be created by accident.\n */\n if (ref_exists(oldref.buf))\n recovery = 1;\n else\n die(_(\"Invalid branch name: '%s'\"), oldname);\n }\n\n\n-- \nKaartic\n"},{"id":"332532","messageId":"20171114114259.8937-1-kaartic.sivaraam@gmail.com","threadId":"46963","inReplyTo":"20171013051132.3973-1-gitster@pobox.com","subject":"[PATCH v2 1/2] branch: forbid refs/heads/HEAD","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-11-14T11:42:58Z","receivedAt":"2017-11-14T11:43:36Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"From: Junio C Hamano \n\nstrbuf_check_branch_ref() is the central place where many codepaths\nsee if a proposed name is suitable for the name of a branch. It was\ndesigned to allow us to get stricter than the check_refname_format()\ncheck used for refnames in general, and we already use it to reject\na branch whose name begins with a '-'.\n\nUse it to also reject \"HEAD\" as a branch name.\n\nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \nSigned-off-by: Kaartic Sivaraam \n---\n Changes in v2:\n\n * Fixed the issue of .git/HEAD being renamed when trying to do,\n\n $ git branch -m HEAD head\n\n This also allows to rename a branch named HEAD that was created by accident.\n\n cf. <1509209933.2256.4.camel@gmail.com>\n\n * Added a test to ensure that it is possible to rename a branch named HEAD.\n\n sha1_name.c | 8 +++++++-\n t/t1430-bad-ref-name.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 9a2d5caf3..657a060cb 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1438,9 +1438,15 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n \t\tstrbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);\n \telse\n \t\tstrbuf_addstr(sb, name);\n-\tif (name[0] == '-')\n+\tif (*name == '-')\n \t\treturn -1;\n+\n \tstrbuf_splice(sb, 0, 0, \"refs/heads/\", 11);\n+\n+\t/* HEAD is not to be used as a branch name */\n+\tif(!strcmp(sb->buf, \"refs/heads/HEAD\"))\n+\t\treturn -1;\n+\n \treturn check_refname_format(sb->buf, 0);\n }\n \ndiff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\nindex e88349c8a..421e80a7a 100755\n--- a/t/t1430-bad-ref-name.sh\n+++ b/t/t1430-bad-ref-name.sh\n@@ -331,4 +331,33 @@ test_expect_success 'update-ref --stdin -z fails delete with bad ref name' '\n \tgrep \"fatal: invalid ref format: ~a\" err\n '\n \n+test_expect_success 'branch rejects HEAD as a branch name' '\n+\ttest_must_fail git branch HEAD HEAD^ &&\n+\ttest_must_fail git show-ref refs/heads/HEAD\n+'\n+\n+test_expect_success 'checkout -b rejects HEAD as a branch name' '\n+\ttest_must_fail git checkout -B HEAD HEAD^ &&\n+\ttest_must_fail git show-ref refs/heads/HEAD\n+'\n+\n+test_expect_success 'update-ref can operate on refs/heads/HEAD' '\n+\tgit update-ref refs/heads/HEAD HEAD^ &&\n+\tgit show-ref refs/heads/HEAD &&\n+\tgit update-ref -d refs/heads/HEAD &&\n+\ttest_must_fail git show-ref refs/heads/HEAD\n+'\n+\n+test_expect_success 'branch -d can remove refs/heads/HEAD' '\n+\tgit update-ref refs/heads/HEAD HEAD^ &&\n+\tgit branch -d HEAD &&\n+\ttest_must_fail git show-ref refs/heads/HEAD\n+'\n+\n+test_expect_success 'branch -m can rename refs/heads/HEAD' '\n+\tgit update-ref refs/heads/HEAD HEAD^ &&\n+\tgit branch -m HEAD head &&\n+\ttest_must_fail git show-ref refs/heads/HEAD\n+'\n+\n test_done\n-- \n2.15.0.rc2.397.geff0134c7.dirty\n\n"},{"id":"332533","messageId":"20171114114259.8937-2-kaartic.sivaraam@gmail.com","threadId":"46963","inReplyTo":"20171114114259.8937-1-kaartic.sivaraam@gmail.com","subject":"[PATCH v2 2/2] builtin/branch: remove redundant check for HEAD","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-11-14T11:42:59Z","receivedAt":"2017-11-14T11:43:46Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"The lower level code has been made to handle this case for the\nsake of consistency. This has made this check redundant.\n\nSo, remove the redundant check.\n\nSigned-off-by: Kaartic Sivaraam \n---\n builtin/branch.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex ffd39333a..5fc57cdc9 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -793,9 +793,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t} else if (argc > 0 && argc <= 2) {\n \t\tstruct branch *branch = branch_get(argv[0]);\n \n-\t\tif (!strcmp(argv[0], \"HEAD\"))\n-\t\t\tdie(_(\"it does not make sense to create 'HEAD' manually\"));\n-\n \t\tif (!branch)\n \t\t\tdie(_(\"no such branch '%s'\"), argv[0]);\n \n-- \n2.15.0.rc2.397.geff0134c7.dirty\n\n"},{"id":"332534","messageId":"0f8166ab-7955-aa78-7d32-0ec80944f007@gmail.com","threadId":"46963","inReplyTo":"20171114114259.8937-1-kaartic.sivaraam@gmail.com","subject":"Re: [PATCH v2 1/2] branch: forbid refs/heads/HEAD","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-11-14T12:00:11Z","receivedAt":"2017-11-14T12:00:40Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"I should have been a little more clear with the numbering, sorry. The \ncorrect prefix should have been as follows,\n\n * [PATCH v2 1/2] --> [PATCH v2 3/3]\n\n * [PATCH v2 1/2] --> [PATCH v2 4/3]\n\nSorry for the inconvenience.\n\n\n---\nKaartic\n"},{"id":"332543","messageId":"xmqq375g3nkf.fsf@gitster.mtv.corp.google.com","threadId":"46963","inReplyTo":"0f8166ab-7955-aa78-7d32-0ec80944f007@gmail.com","subject":"Re: [PATCH v2 1/2] branch: forbid refs/heads/HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-14T15:08:32Z","receivedAt":"2017-11-14T15:08:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kaartic Sivaraam writes:\n\n> I should have been a little more clear with the numbering, sorry. The\n> correct prefix should have been as follows,\n>\n> * [PATCH v2 1/2] --> [PATCH v2 3/3]\n>\n> * [PATCH v2 1/2] --> [PATCH v2 4/3]\n>\n> Sorry for the inconvenience.\n\nI assume that the second one above actually talks about what was\nsent as \"v2 2/2\" (not \"v2 1/2\") being \"4/3\"?\n\nAre these two patches follow-up fixes (replacement of 3/3 plus an\nextra patch) to jc/branch-name-sanity topic?\n\nThanks for working on these.\n"},{"id":"332607","messageId":"bb3485d0-71bc-452e-e4b9-8a7d767753a5@gmail.com","threadId":"46963","inReplyTo":"xmqq375g3nkf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/2] branch: forbid refs/heads/HEAD","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-11-15T16:59:38Z","receivedAt":"2017-11-15T16:59:50Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Tuesday 14 November 2017 08:38 PM, Junio C Hamano wrote:\n> Kaartic Sivaraam writes:\n> \n>> I should have been a little more clear with the numbering, sorry. The\n>> correct prefix should have been as follows,\n>>\n>> * [PATCH v2 1/2] --> [PATCH v2 3/3]\n>>\n>> * [PATCH v2 1/2] --> [PATCH v2 4/3]\n>>\n>> Sorry for the inconvenience.\n> \n> I assume that the second one above actually talks about what was\n> sent as \"v2 2/2\" (not \"v2 1/2\") being \"4/3\"?\n>\n\nYeah. Copy paste error, sorry.\n\n\n> Are these two patches follow-up fixes (replacement of 3/3 plus an\n> extra patch) to jc/branch-name-sanity topic?\n> \n\nYes, that's right.\n\n\n> Thanks for working on these.\n> \n\nYou're welcome. Please do be sure I haven't broken anything in v2. These \npatches should cleanly apply on 'next', if they don't let me know.\n\n\nThanks,\nKaartic\n\n"},{"id":"332624","messageId":"xmqqpo8jyyti.fsf_-_@gitster.mtv.corp.google.com","threadId":"46963","inReplyTo":"bb3485d0-71bc-452e-e4b9-8a7d767753a5@gmail.com","subject":"[PATCH 3/4] branch: correctly reject refs/heads/{-dash,HEAD}","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-15T22:14:17Z","receivedAt":"2017-11-15T22:14:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"strbuf_check_branch_ref() is the central place where many codepaths\nsee if a proposed name is suitable for the name of a branch. It was\ndesigned to allow us to get stricter than the check_refname_format()\ncheck used for refnames in general, and we already use it to reject\na branch whose name begins with a '-'. The function gets a strbuf\nand a string \"name\", and returns non-zero if the name is not\nappropriate as the name for a branch. When the name is good, it\nplaces the full refname for the branch with the proposed name in the\nstrbuf before it returns.\n\nHowever, it turns out that one caller looks at what is in the strbuf\neven when the function returns an error. Make the function populate\nthe strbuf even when it returns an error. That way, when \"-dash\" is\ngiven as name, \"refs/heads/-dash\" is placed in the strbuf when\nreturning an error to copy_or_rename_branch(), which notices that\nthe user is trying to recover with \"git branch -m -- -dash dash\" to\nrename \"-dash\" to \"dash\".\n\nWhile at it, use the same mechanism to also reject \"HEAD\" as a\nbranch name.\n\nHelped-by: Jeff King \nHelped-by: Kaartic Sivaraam \nSigned-off-by: Junio C Hamano \n---\n\n Kaartic Sivaraam writes:\n\n >> Are these two patches follow-up fixes (replacement of 3/3 plus an\n >> extra patch) to jc/branch-name-sanity topic?\n >\n > Yes, that's right.\n >\n >> Thanks for working on these.\n >\n > You're welcome. Please do be sure I haven't broken anything in\n > v2. These patches should cleanly apply on 'next', if they don't let me\n > know.\n\n OK, so here is a replacement for your replacement, based on an\n additional analysis I did while I was reviewing your changes.\n The final 4/4 is what you sent as [v2 2/2] (which was meant to\n be [v2 4/3]). I think with these updates, the resulting 4-patch\n series is good for 'next'.\n\n Thanks again.\n\n sha1_name.c | 14 ++++++++++++--\n t/t1430-bad-ref-name.sh | 43 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 55 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex c7c5ab376c..67961d6e47 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1332,9 +1332,19 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)\n int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n {\n \tstrbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);\n-\tif (name[0] == '-')\n-\t\treturn -1;\n+\n+\t/*\n+\t * This splice must be done even if we end up rejecting the\n+\t * name; builtin/branch.c::copy_or_rename_branch() still wants\n+\t * to see what the name expanded to so that \"branch -m\" can be\n+\t * used as a tool to correct earlier mistakes.\n+\t */\n \tstrbuf_splice(sb, 0, 0, \"refs/heads/\", 11);\n+\n+\tif (*name == '-' ||\n+\t !strcmp(sb->buf, \"refs/heads/HEAD\"))\n+\t\treturn -1;\n+\n \treturn check_refname_format(sb->buf, 0);\n }\n \ndiff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\nindex e88349c8a0..c7878a60ed 100755\n--- a/t/t1430-bad-ref-name.sh\n+++ b/t/t1430-bad-ref-name.sh\n@@ -331,4 +331,47 @@ test_expect_success 'update-ref --stdin -z fails delete with bad ref name' '\n \tgrep \"fatal: invalid ref format: ~a\" err\n '\n \n+test_expect_success 'branch rejects HEAD as a branch name' '\n+\ttest_must_fail git branch HEAD HEAD^ &&\n+\ttest_must_fail git show-ref refs/heads/HEAD\n+'\n+\n+test_expect_success 'checkout -b rejects HEAD as a branch name' '\n+\ttest_must_fail git checkout -B HEAD HEAD^ &&\n+\ttest_must_fail git show-ref refs/heads/HEAD\n+'\n+\n+test_expect_success 'update-ref can operate on refs/heads/HEAD' '\n+\tgit update-ref refs/heads/HEAD HEAD^ &&\n+\tgit show-ref refs/heads/HEAD &&\n+\tgit update-ref -d refs/heads/HEAD &&\n+\ttest_must_fail git show-ref refs/heads/HEAD\n+'\n+\n+test_expect_success 'branch -d can remove refs/heads/HEAD' '\n+\tgit update-ref refs/heads/HEAD HEAD^ &&\n+\tgit branch -d HEAD &&\n+\ttest_must_fail git show-ref refs/heads/HEAD\n+'\n+\n+test_expect_success 'branch -m can rename refs/heads/HEAD' '\n+\tgit update-ref refs/heads/HEAD HEAD^ &&\n+\tgit branch -m HEAD tail &&\n+\ttest_must_fail git show-ref refs/heads/HEAD &&\n+\tgit show-ref refs/heads/tail\n+'\n+\n+test_expect_success 'branch -d can remove refs/heads/-dash' '\n+\tgit update-ref refs/heads/-dash HEAD^ &&\n+\tgit branch -d -- -dash &&\n+\ttest_must_fail git show-ref refs/heads/-dash\n+'\n+\n+test_expect_success 'branch -m can rename refs/heads/-dash' '\n+\tgit update-ref refs/heads/-dash HEAD^ &&\n+\tgit branch -m -- -dash dash &&\n+\ttest_must_fail git show-ref refs/heads/-dash &&\n+\tgit show-ref refs/heads/dash\n+'\n+\n test_done\n-- \n2.15.0-358-g6c105002b3\n\n"},{"id":"332662","messageId":"471c7b6e-d3a0-4aee-8f80-282ff0b8d972@gmail.com","threadId":"46963","inReplyTo":"xmqqpo8jyyti.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/4] branch: correctly reject refs/heads/{-dash,HEAD}","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-11-16T13:11:10Z","receivedAt":"2017-11-16T13:11:27Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Thursday 16 November 2017 03:44 AM, Junio C Hamano wrote:\n> Kaartic Sivaraam writes:\n> \n> >> Are these two patches follow-up fixes (replacement of 3/3 plus an\n> >> extra patch) to jc/branch-name-sanity topic?\n> >\n> > Yes, that's right.\n> >\n> >> Thanks for working on these.\n> >\n> > You're welcome. Please do be sure I haven't broken anything in\n> > v2. These patches should cleanly apply on 'next', if they don't let me\n> > know.\n> \n> OK, so here is a replacement for your replacement, based on an\n> additional analysis I did while I was reviewing your changes.\n> The final 4/4 is what you sent as [v2 2/2] (which was meant to\n> be [v2 4/3]). I think with these updates, the resulting 4-patch\n> series is good for 'next'.\n> \n\nI guess this series is not yet ready for 'next'. When I tried to apply \nthis patch it doesn't seem to be applying cleanly. I get some conflicts \nin 'sha1_name.c' possibly as a consequence of the changes to the file \nthat aren't accounted by the patch. As to which change,\n\n$ git whatchanged jch/jc/branch-name-sanity..origin/next sha1_name.c\n\nlists at least 5 of them, so there's possibly a lot of change that \nhasn't been taken into account by this patch. Particularly, the function \n'strbuf_check_branch_ref' itself is found at line 1435 in the version \nfound in 'next' though this patch expects it to be near line 1332, I guess.\n\nFurther comment inline.\n\n> sha1_name.c | 14 ++++++++++++--\n> t/t1430-bad-ref-name.sh | 43 +++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 55 insertions(+), 2 deletions(-)\n> \n> diff --git a/sha1_name.c b/sha1_name.c\n> index c7c5ab376c..67961d6e47 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -1332,9 +1332,19 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)\n> int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n> {\n> \tstrbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);\n> -\tif (name[0] == '-')\n> -\t\treturn -1;\n> +\n> +\t/*\n> +\t * This splice must be done even if we end up rejecting the\n> +\t * name; builtin/branch.c::copy_or_rename_branch() still wants\n> +\t * to see what the name expanded to so that \"branch -m\" can be\n> +\t * used as a tool to correct earlier mistakes.\n> +\t */\n> \tstrbuf_splice(sb, 0, 0, \"refs/heads/\", 11);\n> +\n> +\tif (*name == '-' ||\n> +\t !strcmp(sb->buf, \"refs/heads/HEAD\"))\n\nI guess this check should be made more consistent. Possibly either of,\n\n\tif (starts_with(sb->buf, \"refs/heads/-\") ||\n\t !strcmp(sb->buf, \"refs/heads/HEAD\"))\n\nor,\n\n\tif (*name == '-' ||\n\t !strcmp(name, \"HEAD\"))\n\n\nmight make them consistent (at least from my perspective).\n\n\nI tried to reproduce this patch manually and other than the above this \none LGTM. Though I can't be very sure as I couldn't apply it (I did it \n\"manually\" to some extent, you see ;-)\n\n\n> +\t\treturn -1;\n> +\n> \treturn check_refname_format(sb->buf, 0);\n> }\n> \n> diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\n> index e88349c8a0..c7878a60ed 100755\n> --- a/t/t1430-bad-ref-name.sh\n> +++ b/t/t1430-bad-ref-name.sh\n> @@ -331,4 +331,47 @@ test_expect_success 'update-ref --stdin -z fails delete with bad ref name' '\n> \tgrep \"fatal: invalid ref format: ~a\" err\n> '\n> \n> +test_expect_success 'branch rejects HEAD as a branch name' '\n> +\ttest_must_fail git branch HEAD HEAD^ &&\n> +\ttest_must_fail git show-ref refs/heads/HEAD\n> +'\n> +\n> +test_expect_success 'checkout -b rejects HEAD as a branch name' '\n> +\ttest_must_fail git checkout -B HEAD HEAD^ &&\n> +\ttest_must_fail git show-ref refs/heads/HEAD\n> +'\n> +\n> +test_expect_success 'update-ref can operate on refs/heads/HEAD' '\n> +\tgit update-ref refs/heads/HEAD HEAD^ &&\n> +\tgit show-ref refs/heads/HEAD &&\n> +\tgit update-ref -d refs/heads/HEAD &&\n> +\ttest_must_fail git show-ref refs/heads/HEAD\n> +'\n> +\n> +test_expect_success 'branch -d can remove refs/heads/HEAD' '\n> +\tgit update-ref refs/heads/HEAD HEAD^ &&\n> +\tgit branch -d HEAD &&\n> +\ttest_must_fail git show-ref refs/heads/HEAD\n> +'\n> +\n> +test_expect_success 'branch -m can rename refs/heads/HEAD' '\n> +\tgit update-ref refs/heads/HEAD HEAD^ &&\n> +\tgit branch -m HEAD tail &&\n> +\ttest_must_fail git show-ref refs/heads/HEAD &&\n> +\tgit show-ref refs/heads/tail\n> +'\n> +\n> +test_expect_success 'branch -d can remove refs/heads/-dash' '\n> +\tgit update-ref refs/heads/-dash HEAD^ &&\n> +\tgit branch -d -- -dash &&\n> +\ttest_must_fail git show-ref refs/heads/-dash\n> +'\n> +\n> +test_expect_success 'branch -m can rename refs/heads/-dash' '\n> +\tgit update-ref refs/heads/-dash HEAD^ &&\n> +\tgit branch -m -- -dash dash &&\n> +\ttest_must_fail git show-ref refs/heads/-dash &&\n> +\tgit show-ref refs/heads/dash\n> +'\n> +\n> test_done\n> \n\n"},{"id":"332665","messageId":"xmqqlgj6uv8k.fsf@gitster.mtv.corp.google.com","threadId":"46963","inReplyTo":"471c7b6e-d3a0-4aee-8f80-282ff0b8d972@gmail.com","subject":"Re: [PATCH 3/4] branch: correctly reject refs/heads/{-dash,HEAD}","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-16T14:57:31Z","receivedAt":"2017-11-16T14:57:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kaartic Sivaraam writes:\n\n> I guess this series is not yet ready for 'next'. When I tried to apply\n> this patch it doesn't seem to be applying cleanly. I get some\n> conflicts in 'sha1_name.c' possibly as a consequence of the changes to\n> the file that aren't accounted by the patch.\n\nOh, it is totally expected that this patch (and others) may not\napply on 'next' without conflict resolution, as this topic, as all\nthe other topics, are designed to apply cleanly to either 'master'\nor 'maint' or one of the older 'maint-*' series, if it is a bugfix\ntopic. A patch series that only applies cleanly to 'next' would be\nuseless---it would mean all the topics that are already in 'next'\nthat interacts with it must graduate first before it can go in.\nMake it a habit to build on 'master' or older and then merge to\nhigher integration branches to make sure it fits with others.\n\nWhat you could do is to inspect origin/pu branch after you fetch,\nand look at the commit that merges this topic to learn how the\nconflicts are resolved (the contrib/rerere-train.sh script may help\nthis process).\n\nYour inability to resolve merge conflicts does not have much to do\nwith the readiness of a topic, as long as somebody else can resolve\nthem ;-)\n\n>> +\tif (*name == '-' ||\n>> +\t !strcmp(sb->buf, \"refs/heads/HEAD\"))\n>\n> I guess this check should be made more consistent. Possibly either of,\n\nAmong these two, this one\n\n> \tif (starts_with(sb->buf, \"refs/heads/-\") ||\n> \t !strcmp(sb->buf, \"refs/heads/HEAD\"))\n\nhas more chance to be correct. Also, if we were to check the result\nof expansion in sb->buf[], I would think that we should keep a\nseparate variable that points at &sb->buf[11] and compare the\nremainder against fixed strings, as we already know sb->buf[] starts\nwith \"refs/heads/\" due to our splicing in the fixed string.\n\nBecause the point of using strbuf_branchname() is to allow us expand\nfunny notations like @{-1} to refs/heads/foo, and the result of\nexpansion is what eventually matters, checking name[] is wrong, I\nwould think, even though I haven't thought things through. \n\nIn any case, I would say thinking this part through should be left\nas a theme for a follow-on patch, and not within the scope of this\none. After all, checking *name against '-' was part of the original\ncode, so it is safer to keep the thing we are not touching bug-to-bug\ncompatible and fixing things one step at a time (the one fix we made\nwith this patch is to make sure we store refs/heads/-dash in sb when\nwe reject name==\"-dash\").\n\n"},{"id":"332674","messageId":"d2bd10c1-be6a-25e2-47cf-613f34ed9513@gmail.com","threadId":"46963","inReplyTo":"xmqqlgj6uv8k.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/4] branch: correctly reject refs/heads/{-dash,HEAD}","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-11-16T17:02:42Z","receivedAt":"2017-11-16T17:05:10Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Thursday 16 November 2017 08:27 PM, Junio C Hamano wrote:\n> Kaartic Sivaraam writes:\n> \n>> I guess this series is not yet ready for 'next'. When I tried to apply\n>> this patch it doesn't seem to be applying cleanly. I get some\n>> conflicts in 'sha1_name.c' possibly as a consequence of the changes to\n>> the file that aren't accounted by the patch.\n> \n> Oh, it is totally expected that this patch (and others) may not\n> apply on 'next' without conflict resolution, as this topic, as all\n> the other topics, are designed to apply cleanly to either 'master'\n> or 'maint' or one of the older 'maint-*' series, if it is a bugfix\n> topic. A patch series that only applies cleanly to 'next' would be\n> useless---it would mean all the topics that are already in 'next'\n> that interacts with it must graduate first before it can go in.\n\nThanks for the explanation. Seems I still have to become accustomed to \nthe workflow.\n\n\n> Make it a habit to build on 'master' or older and then merge to\n> higher integration branches to make sure it fits with others.\n>\n\nThough I don't clearly understand how to do that, I'll let experience \nteach me the same (if possible). :-)\n\n\n> What you could do is to inspect origin/pu branch after you fetch,\n> and look at the commit that merges this topic to learn how the\n> conflicts are resolved (the contrib/rerere-train.sh script may help\n> this process).\n> \n\nSure thing.\n\n\n>>> +\tif (*name == '-' ||\n>>> +\t !strcmp(sb->buf, \"refs/heads/HEAD\"))\n>>\n>> I guess this check should be made more consistent. Possibly either of,\n> \n> Among these two, this one\n> \n>> \tif (starts_with(sb->buf, \"refs/heads/-\") ||\n>> \t !strcmp(sb->buf, \"refs/heads/HEAD\"))\n> \n> has more chance to be correct. Also, if we were to check the result\n> of expansion in sb->buf[], I would think that we should keep a\n> separate variable that points at &sb->buf[11] and compare the\n> remainder against fixed strings, as we already know sb->buf[] starts\n> with \"refs/heads/\" due to our splicing in the fixed string.\n> \n> Because the point of using strbuf_branchname() is to allow us expand\n> funny notations like @{-1} to refs/heads/foo, and the result of\n> expansion is what eventually matters, checking name[] is wrong, I\n> would think, even though I haven't thought things through.\n> \n> In any case, I would say thinking this part through should be left\n> as a theme for a follow-on patch, and not within the scope of this\n> one. After all, checking *name against '-' was part of the original\n> code, so it is safer to keep the thing we are not touching bug-to-bug\n> compatible and fixing things one step at a time (the one fix we made\n> with this patch is to make sure we store refs/heads/-dash in sb when\n> we reject name==\"-dash\").\n> \n\nGood point. This is better for a follow-up patch as there's a \npossibility that we might be introducing new bugs which weren't present \npreviously as a consequence of changing that conditional (bug-to-bug \ncompatibility, good term that (possibly) summarizes my long-winded \nexplanation ;-)\n"}]}