{"thread":{"id":"28177","subject":"[PATCH] Prevent force updating of the current branch","startedAt":"2011-08-20T21:49:47Z","lastAt":"2011-08-23T19:20:23Z","messageCount":5,"participants":["Conrad Irwin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"173939","messageId":"1313876989-16328-1-git-send-email-conrad.irwin@gmail.com","threadId":"28177","inReplyTo":null,"subject":"[PATCH] Prevent force updating of the current branch","fromName":"Conrad Irwin","fromEmail":"conrad.irwin@gmail.com","sentAt":"2011-08-20T21:49:47Z","receivedAt":"2011-08-20T21:49:47Z","isPatch":true,"sender":{"key":"conrad.irwin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/94272?v=4"},"body":"I noticed when trying to improve the user-experience of creating\nambiguous branches [1] that there are insufficient checks to safeguard\ngit-branch -M and git-checkout -B from over-writing the current branch.\n\nConrad\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/179503\n\n---\n branch.c | 34 ++++++++++++++++++++++++----------\n branch.h | 8 ++++++++\n builtin/branch.c | 6 +-----\n builtin/checkout.c | 12 +++---------\n t/t2018-checkout-branch.sh | 17 +++++++++++++++++\n t/t3200-branch.sh | 12 ++++++++++++\n 6 files changed, 65 insertions(+), 24 deletions(-)\n"},{"id":"173941","messageId":"1313876989-16328-2-git-send-email-conrad.irwin@gmail.com","threadId":"28177","inReplyTo":"1313876989-16328-1-git-send-email-conrad.irwin@gmail.com","subject":"[PATCH 1/2] Prevent force-updating of the current branch","fromName":"Conrad Irwin","fromEmail":"conrad.irwin@gmail.com","sentAt":"2011-08-20T21:49:48Z","receivedAt":"2011-08-20T21:49:48Z","isPatch":true,"sender":{"key":"conrad.irwin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/94272?v=4"},"body":"git branch -M could be used to change the branch\nto which HEAD points, without the necessary house-keeping that git reset\nnormally does to make this operation sensible. It would leave the reflog\nin a confusing state (you would be warned when trying to read it) and\nhad an apparently side-effect of staging the diff between \nand .\n\ngit checkout -B was also partly vulnerable to\nthis bug; due to inconsistent pre-flight checks it would perform half of\nits task and then abort just before rewriting the branch. Again this\nmanifested itself as the index file getting out-of-sync with HEAD.\n\ngit checkout -f already guarded against this problem, and aborted with\na fatal error.\n\ngit branch -M, git checkout -B and git branch -f now use the same checks\nbefore allowing a branch to be created. These prevent you from updating\nthe current branch.\n\nWe considered suggesting the use of \"git reset\" in the failure message\nbut concluded that it was not possible to discern what the user was\nactually trying to do.\n\nSigned-off-by: Conrad Irwin \n---\n branch.c | 34 ++++++++++++++++++++++++----------\n branch.h | 8 ++++++++\n builtin/branch.c | 6 +-----\n builtin/checkout.c | 12 +++---------\n t/t2018-checkout-branch.sh | 8 ++++++++\n t/t3200-branch.sh | 12 ++++++++++++\n 6 files changed, 56 insertions(+), 24 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex c0c865a..ff84b5b 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -135,6 +135,26 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,\n \treturn 0;\n }\n \n+int validate_new_branchname(const char *name, struct strbuf *ref, int force)\n+{\n+\tconst char *head;\n+\tunsigned char sha1[20];\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)\n+\t\tdie(\"A branch named '%s' already exists.\", name);\n+\n+\thead = resolve_ref(\"HEAD\", sha1, 0, NULL);\n+\tif (!is_bare_repository() && head && !strcmp(head, ref->buf))\n+\t\tdie(\"Cannot force update the current branch.\");\n+\n+\treturn 1;\n+}\n+\n void create_branch(const char *head,\n \t\t const char *name, const char *start_name,\n \t\t int force, int reflog, enum branch_track track)\n@@ -151,17 +171,11 @@ void create_branch(const char *head,\n \tif (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)\n \t\texplicit_tracking = 1;\n \n-\tif (strbuf_check_branch_ref(&ref, name))\n-\t\tdie(\"'%s' is not a valid branch name.\", name);\n-\n-\tif (resolve_ref(ref.buf, sha1, 1, NULL)) {\n-\t\tif (!force && track == BRANCH_TRACK_OVERRIDE)\n+\tif (validate_new_branchname(name, &ref, force || track == BRANCH_TRACK_OVERRIDE)) {\n+\t\tif (!force)\n \t\t\tdont_change_ref = 1;\n-\t\telse if (!force)\n-\t\t\tdie(\"A branch named '%s' already exists.\", name);\n-\t\telse if (!is_bare_repository() && head && !strcmp(head, name))\n-\t\t\tdie(\"Cannot force update the current branch.\");\n-\t\tforcing = 1;\n+\t\telse\n+\t\t\tforcing = 1;\n \t}\n \n \treal_ref = NULL;\ndiff --git a/branch.h b/branch.h\nindex 4026e38..01544e2 100644\n--- a/branch.h\n+++ b/branch.h\n@@ -16,6 +16,14 @@ void create_branch(const char *head, const char *name, const char *start_name,\n \t\t int force, int reflog, 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+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\n * branch. (E.g., MERGE_HEAD)\n */\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 3142daa..40f885c 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -566,11 +566,7 @@ static void rename_branch(const char *oldname, const char *newname, int force)\n \t\t\tdie(_(\"Invalid branch name: '%s'\"), oldname);\n \t}\n \n-\tif (strbuf_check_branch_ref(&newref, newname))\n-\t\tdie(_(\"Invalid branch name: '%s'\"), newname);\n-\n-\tif (resolve_ref(newref.buf, sha1, 1, NULL) && !force)\n-\t\tdie(_(\"A branch named '%s' already exists.\"), newref.buf + 11);\n+\tvalidate_new_branchname(newname, &newref, force);\n \n \tstrbuf_addf(&logmsg, \"Branch: renamed %s to %s\",\n \t\t oldref.buf, newref.buf);\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex d647a31..fc4e008 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1072,15 +1072,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \n \tif (opts.new_branch) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n-\t\tif (strbuf_check_branch_ref(&buf, opts.new_branch))\n-\t\t\tdie(_(\"git checkout: we do not like '%s' as a branch name.\"),\n-\t\t\t opts.new_branch);\n-\t\tif (ref_exists(buf.buf)) {\n-\t\t\topts.branch_exists = 1;\n-\t\t\tif (!opts.new_branch_force)\n-\t\t\t\tdie(_(\"git checkout: branch %s already exists\"),\n-\t\t\t\t opts.new_branch);\n-\t\t}\n+\n+\t\topts.branch_exists = validate_new_branchname(opts.new_branch, &buf, !!opts.new_branch_force);\n+\n \t\tstrbuf_release(&buf);\n \t}\n \ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex a42e039..b66db2b 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -180,4 +180,12 @@ test_expect_success 'checkout -b ' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'checkout -B to the current branch fails before merging' '\n+\tgit checkout branch1 &&\n+\tsetup_dirty_mergeable &&\n+\tgit commit -mfooble &&\n+\ttest_must_fail git checkout -B branch1 initial &&\n+\ttest_must_fail test_dirty_mergeable\n+'\n+\n test_done\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 9e69c8c..cb6458d 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -98,6 +98,18 @@ test_expect_success 'git branch -m q r/q should fail when r exists' '\n \ttest_must_fail git branch -m q r/q\n '\n \n+test_expect_success 'git branch -M foo bar should fail when bar is checked out' '\n+\tgit branch bar &&\n+\tgit checkout -b foo &&\n+\ttest_must_fail git branch -M bar foo\n+'\n+\n+test_expect_success 'git branch -M baz bam should succeed when baz is checked out' '\n+\tgit checkout -b baz &&\n+\tgit branch bam &&\n+\tgit branch -M baz bam\n+'\n+\n mv .git/config .git/config-saved\n \n test_expect_success 'git branch -m q q2 without config should succeed' '\n-- \n1.7.6.562.g0b2d4\n"},{"id":"173940","messageId":"1313876989-16328-3-git-send-email-conrad.irwin@gmail.com","threadId":"28177","inReplyTo":"1313876989-16328-1-git-send-email-conrad.irwin@gmail.com","subject":"[PATCH 2/2] Show interpreted branch name in error messages","fromName":"Conrad Irwin","fromEmail":"conrad.irwin@gmail.com","sentAt":"2011-08-20T21:49:49Z","receivedAt":"2011-08-20T21:49:49Z","isPatch":true,"sender":{"key":"conrad.irwin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/94272?v=4"},"body":"Change the error message when doing: \"git branch @{-1}\",\n\"git checkout -b @{-1}\", or \"git branch -m foo @{-1}\"\n\n * was: A branch named '@{-1}' already exists.\n * now: A branch named 'bar' already exists.\n\nSigned-off-by: Conrad Irwin \n---\n branch.c | 2 +-\n t/t2018-checkout-branch.sh | 9 +++++++++\n 2 files changed, 10 insertions(+), 1 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex ff84b5b..1fe3078 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -146,7 +146,7 @@ int validate_new_branchname(const char *name, struct strbuf *ref, int force)\n \tif (!ref_exists(ref->buf))\n \t\treturn 0;\n \telse if (!force)\n-\t\tdie(\"A branch named '%s' already exists.\", name);\n+\t\tdie(\"A branch named '%s' already exists.\", ref->buf + strlen(\"refs/heads/\"));\n \n \thead = resolve_ref(\"HEAD\", sha1, 0, NULL);\n \tif (!is_bare_repository() && head && !strcmp(head, ref->buf))\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex b66db2b..75874e8 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -118,6 +118,15 @@ test_expect_success 'checkout -b to an existing branch fails' '\n \ttest_must_fail do_checkout branch2 $HEAD2\n '\n \n+test_expect_success 'checkout -b to @{-1} fails with the right branch name' '\n+\tgit reset --hard HEAD &&\n+\tgit checkout branch1 &&\n+\tgit checkout branch2 &&\n+\techo >expect \"fatal: A branch named '\\''branch1'\\'' already exists.\" &&\n+\ttest_must_fail git checkout -b @{-1} 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'checkout -B to an existing branch resets branch to HEAD' '\n \tgit checkout branch1 &&\n \n-- \n1.7.6.562.g0b2d4\n"},{"id":"174107","messageId":"7v39gsrnuc.fsf@alter.siamese.dyndns.org","threadId":"28177","inReplyTo":"1313876989-16328-2-git-send-email-conrad.irwin@gmail.com","subject":"Re: [PATCH 1/2] Prevent force-updating of the current branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-23T18:20:59Z","receivedAt":"2011-08-23T18:20:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Conrad Irwin writes:\n\n> git branch -M could be used to change the branch\n> to which HEAD points, without the necessary house-keeping that git reset\n> normally does to make this operation sensible. It would leave the reflog\n> in a confusing state (you would be warned when trying to read it) and\n> had an apparently side-effect of staging the diff between \n> and .\n\nThe last two lines are redundant (it is \"without the house-keeping of\nreset\"); I'll remove \"and had an apparently...\".\n\n> git checkout -B was also partly vulnerable to\n> this bug; due to inconsistent pre-flight checks it would perform half of\n> its task and then abort just before rewriting the branch. Again this\n> manifested itself as the index file getting out-of-sync with HEAD.\n>\n> git checkout -f already guarded against this problem, and aborted with\n> a fatal error.\n\nI assume you mean \"branch -f\". I'll rewrite it so, and in the present\ntense.\n\n> git branch -M, git checkout -B and git branch -f now use the same checks\n> before allowing a branch to be created. These prevent you from updating\n> the current branch.\n\nLooks good ;-). Also the patch looks good, too.\n\nThanks.\n"},{"id":"174112","messageId":"CAOTq_ptTsO01XvjMcBcWNaOQHvwm7cHP2AzSRpbSzox-NNj7Rg@mail.gmail.com","threadId":"28177","inReplyTo":"7v39gsrnuc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] Prevent force-updating of the current branch","fromName":"Conrad Irwin","fromEmail":"conrad.irwin@gmail.com","sentAt":"2011-08-23T19:20:23Z","receivedAt":"2011-08-23T19:20:23Z","isPatch":true,"sender":{"key":"conrad.irwin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/94272?v=4"},"body":"On Tue, Aug 23, 2011 at 11:20 AM, Junio C Hamano wrote:\n> Conrad Irwin writes:\n>\n>> git branch -M could be used to change the branch\n>> to which HEAD points, without the necessary house-keeping that git reset\n>> normally does to make this operation sensible. It would leave the reflog\n>> in a confusing state (you would be warned when trying to read it) and\n>> had an apparently side-effect of staging the diff between \n>> and .\n>\n> The last two lines are redundant (it is \"without the house-keeping of\n> reset\"); I'll remove \"and had an apparently...\".\n\nThat's fine by me.\n\n>> git checkout -f already guarded against this problem, and aborted with\n>> a fatal error.\n>\n> I assume you mean \"branch -f\". I'll rewrite it so, and in the present\n> tense.\n\nYes. Thank you.\n\n>\n>> git branch -M, git checkout -B and git branch -f now use the same checks\n>> before allowing a branch to be created. These prevent you from updating\n>> the current branch.\n>\n> Looks good ;-). Also the patch looks good, too.\n>\n\nGlad to hear :).\n\nConrad\n"}]}