{"thread":{"id":"29020","subject":"git branch -M\" regression in 1.7.7?","startedAt":"2011-11-26T00:36:08Z","lastAt":"2011-11-28T19:38:57Z","messageCount":9,"participants":["☂Josh Chia (谢任中)","Jonathan Nieder","Conrad Irwin","Junio C Hamano","Andreas Schwab"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"179977","messageId":"CALxtSbRbwkVDKJcXiKY9rHYCjA3XGgCytbXQnRhQvbEnY8SpjA@mail.gmail.com","threadId":"29020","inReplyTo":null,"subject":"git branch -M\" regression in 1.7.7?","fromName":"☂Josh Chia (谢任中)","fromEmail":"joshchia@gmail.com","sentAt":"2011-11-26T00:36:08Z","receivedAt":"2011-11-26T00:36:08Z","isPatch":false,"sender":{"key":"joshchia@gmail.com","avatar":null},"body":"On git 1.7.7.3, when I try to \"git branch -M master\" when I'm already\non a branch 'master', I get this error message:\nCannot force update the current branch\n\nOn 1.7.6.4, the command succeeds.\n\nIs this intended?\n"},{"id":"179978","messageId":"20111126023002.GA17652@elie.hsd1.il.comcast.net","threadId":"29020","inReplyTo":"CALxtSbRbwkVDKJcXiKY9rHYCjA3XGgCytbXQnRhQvbEnY8SpjA@mail.gmail.com","subject":"Re: git branch -M\" regression in 1.7.7?","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-26T02:30:02Z","receivedAt":"2011-11-26T02:30:02Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJosh Chia (谢任中) wrote:\n\n> On git 1.7.7.3, when I try to \"git branch -M master\" when I'm already\n> on a branch 'master', I get this error message:\n> Cannot force update the current branch\n>\n> On 1.7.6.4, the command succeeds.\n>\n> Is this intended?\n\nYes, but it probably wasn't a good idea. How about this patch?\n\nA reproduction recipe (preferrably in the form of a patch to\nt/t3200-branch.sh would be welcome.\n\n-- >8 --\nSubject: treat \"git branch -M master master\" as a no-op again\n\nBefore v1.7.7-rc2~1^2~2 (Prevent force-updating of the current branch,\n2011-08-20), commands like \"git branch -M topic master\" could be used\neven when \"master\" was the current branch, with the somewhat\ncounterintuitive result that HEAD would point to some place new while\nthe index and worktree kept the content of the old commit. This is\nnot a very sensible operation and the result is what almost nobody\nwould expect, so erroring out in this case was a good change.\n\nHowever, there is one exception to the \"it's usually not obvious what\nit would mean to overwrite the current branch by another one\" rule.\nNamely:\n\n\tgit branch -M master master\n\nis clearly meant to be a no-op, even if you are on the master branch.\nAnd in the latter case, it can be abbreviated:\n\n\tgit branch -M master\n\nThis seems like a valuable exception to allow, because then \"git\nbranch -M foo\" would _always_ be allowed --- either 'foo' is not the\ncurrent branch, and it does the obvious thing, or 'foo' is the current\nbranch, and nothing happens.\n\nBuildbot uses this idiom and was broken in 1.7.7 (it would emit the\nmessage \"Cannot force update the current branch\").\n\nReported-by: Soeren Sonnenburg \nReported-by: Josh Chia (谢任中)\nSigned-off-by: Jonathan Nieder \n---\n builtin/branch.c | 9 ++++++++-\n 1 files changed, 8 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 51ca6a02..24f33b24 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -568,6 +568,7 @@ static void rename_branch(const char *oldname, const char *newname, int force)\n \tunsigned char sha1[20];\n \tstruct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;\n \tint recovery = 0;\n+\tint clobber_head_ok;\n \n \tif (!oldname)\n \t\tdie(_(\"cannot rename the current branch while not on any.\"));\n@@ -583,7 +584,13 @@ 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-\tvalidate_new_branchname(newname, &newref, force, 0);\n+\t/*\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 \n \tstrbuf_addf(&logmsg, \"Branch: renamed %s to %s\",\n \t\t oldref.buf, newref.buf);\n-- \n1.7.8.rc3\n"},{"id":"179979","messageId":"1322290364-16207-1-git-send-email-conrad.irwin@gmail.com","threadId":"29020","inReplyTo":"20111126023002.GA17652@elie.hsd1.il.comcast.net","subject":"[PATCH] Test renaming a branch to itself","fromName":"Conrad Irwin","fromEmail":"conrad.irwin@gmail.com","sentAt":"2011-11-26T06:52:44Z","receivedAt":"2011-11-26T06:52:44Z","isPatch":true,"sender":{"key":"conrad.irwin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/94272?v=4"},"body":"Test for a regression introduced in v1.7.7-rc2~1^2~2.\n\nRequested-by: Jonathan Nieder \nSigned-off-by: Conrad Irwin \n---\n t/t3200-branch.sh | 16 ++++++++++++++++\n 1 files changed, 16 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex bc73c20..7690332 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -115,6 +115,22 @@ test_expect_success 'git branch -M baz bam should succeed when baz is checked ou\n \tgit branch -M baz bam\n '\n \n+test_expect_success 'git branch -M master should work when master is checked out' '\n+\tgit checkout master &&\n+\tgit branch -M master\n+'\n+\n+test_expect_success 'git branch -M master master should work when master is checked out' '\n+\tgit checkout master &&\n+\tgit branch -M master master\n+'\n+\n+test_expect_success 'git branch -M master2 master2 should work when master is checked out' '\n+\tgit checkout master &&\n+\tgit branch master2 &&\n+\tgit branch -M master2 master2\n+'\n+\n test_expect_success 'git branch -v -d t should work' '\n \tgit branch t &&\n \ttest_path_is_file .git/refs/heads/t &&\n-- \n1.7.7.1.433.ga2d04a\n"},{"id":"179980","messageId":"20111126065901.GB20923@elie.hsd1.il.comcast.net","threadId":"29020","inReplyTo":"1322290364-16207-1-git-send-email-conrad.irwin@gmail.com","subject":"Re: [PATCH] Test renaming a branch to itself","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-26T06:59:01Z","receivedAt":"2011-11-26T06:59:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Conrad Irwin wrote:\n\n> Test for a regression introduced in v1.7.7-rc2~1^2~2.\n\nThanks! I take it that means you like the patch. :)\n\nThe tests look sane fwiw (and it looks like the tests you wrote before\ncover the \"branch -M\" safety valve pretty well).\n"},{"id":"179981","messageId":"CAOTq_pv4dyAkbqye+diK9mTTsrTg9OKg0tExKcfDgs8RfiTwTQ@mail.gmail.com","threadId":"29020","inReplyTo":"20111126023002.GA17652@elie.hsd1.il.comcast.net","subject":"Re: git branch -M\" regression in 1.7.7?","fromName":"Conrad Irwin","fromEmail":"conrad.irwin@gmail.com","sentAt":"2011-11-26T07:05:26Z","receivedAt":"2011-11-26T07:05:26Z","isPatch":false,"sender":{"key":"conrad.irwin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/94272?v=4"},"body":"On Fri, Nov 25, 2011 at 6:30 PM, Jonathan Nieder wrote:\n> A reproduction recipe (preferrably in the form of a patch to\n> t/t3200-branch.sh would be welcome.\n\nSent in a separate email. Feel free to add a \"Tested-by:\" header to\nyour patch if you want :).\n\n>\n> -- >8 --\n> Subject: treat \"git branch -M master master\" as a no-op again\n>\n> Before v1.7.7-rc2~1^2~2 (Prevent force-updating of the current branch,\n> 2011-08-20), commands like \"git branch -M topic master\" could be used\n> even when \"master\" was the current branch, with the somewhat\n> counterintuitive result that HEAD would point to some place new while\n> the index and worktree kept the content of the old commit.  This is\n> not a very sensible operation and the result is what almost nobody\n> would expect, so erroring out in this case was a good change.\n>\n> However, there is one exception to the \"it's usually not obvious what\n> it would mean to overwrite the current branch by another one\" rule.\n> Namely:\n>\n>        git branch -M master master\n>\n> is clearly meant to be a no-op, even if you are on the master branch.\n\nAgreed. I thought after reading your patch about making it just do:\n\n if (!strcmp(oldname, newname))\n exit(0);\n\nbut I guess it would then not mark an entry in the reflog that people\ncould be relying on...\n\n> +       clobber_head_ok = !strcmp(oldname, newname);\n> +\n> +       validate_new_branchname(newname, &newref, force, clobber_head_ok);\n\nThis looks ok, and will be improvable if the NEEDSWORK in branch.h is done.\n\nThe other thing I wonder is whether \"git checkout -B master HEAD\" or\n\"git branch -f master master\" should have the same short-cut?\n\nConrad\n"},{"id":"179983","messageId":"20111126085455.GB22656@elie.hsd1.il.comcast.net","threadId":"29020","inReplyTo":"CAOTq_pv4dyAkbqye+diK9mTTsrTg9OKg0tExKcfDgs8RfiTwTQ@mail.gmail.com","subject":"Re: git branch -M\" regression in 1.7.7?","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-26T08:54:55Z","receivedAt":"2011-11-26T08:54:55Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Conrad Irwin wrote:\n\n> I thought after reading your patch about making it just do:\n>\n> if (!strcmp(oldname, newname))\n> exit(0);\n>\n> but I guess it would then not mark an entry in the reflog that people\n> could be relying on...\n\nAh, this deserves a comment.\n\nI thought about doing the same thing, and then didn't do it because I\nwanted to make sure that\n\n\tgit branch -M nonexistent nonexistent\n\ndoes not succeed. Which presumably warrants another test:\n\n test_expect_success \"rename-to-self dwimery doesn't hide nonexistent ref\" '\n\ttest_must_fail git branch -M nonexistent nonexistent &&\n\ttest_must_fail git rev-parse --verify nonexistent\n '\n\nSloppy of me.\n\n>> +       clobber_head_ok = !strcmp(oldname, newname);\n>> +\n>> +       validate_new_branchname(newname, &newref, force, clobber_head_ok);\n>\n> This looks ok, and will be improvable if the NEEDSWORK in branch.h is done.\n\nThanks for looking it over.\n\n> The other thing I wonder is whether \"git checkout -B master HEAD\" or\n> \"git branch -f master master\" should have the same short-cut?\n\nI think \"git branch -M\" is the only one buildbot was relying on.\n\nAs an aside, I'm not convinced the check is needed for checkout -B at\nall. In an ideal world, the order of operations would be:\n\n\t1. look up commit argument\n\t2. merge working tree\n\t3. update branch to match commit\n\t4. update HEAD symref to point to branch\n\nIn other words, when on master, \"git checkout -B master \"\nwould be another way to say \"git reset --keep \", except that\nit also sets up tracking.\n\nSurprisingly, switch_branches() seems to match that pretty well already.\n\nSigned-off-by: Jonathan Nieder \n---\n branch.c | 6 ++++--\n branch.h | 3 ++-\n builtin/branch.c | 2 +-\n builtin/checkout.c | 15 +++++++++++----\n t/t2018-checkout-branch.sh | 9 +++++----\n 5 files changed, 23 insertions(+), 12 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 025a97be..f85c4382 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -160,7 +160,8 @@ int validate_new_branchname(const char *name, struct strbuf *ref,\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+\t\t int force, int reflog, int clobber_head,\n+\t\t enum branch_track track)\n {\n \tstruct ref_lock *lock = NULL;\n \tstruct commit *commit;\n@@ -175,7 +176,8 @@ void create_branch(const char *head,\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 track == BRANCH_TRACK_OVERRIDE ||\n+\t\t\t\t clobber_head)) {\n \t\tif (!force)\n \t\t\tdont_change_ref = 1;\n \t\telse\ndiff --git a/branch.h b/branch.h\nindex 1285158d..e125ff4c 100644\n--- a/branch.h\n+++ b/branch.h\n@@ -13,7 +13,8 @@\n * branch for (if any).\n */\n 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+\t\t int force, int reflog,\n+\t\t int clobber_head, enum branch_track track);\n \n /*\n * Validates that the requested branch may be created, returning the\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 51ca6a02..730f9139 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -730,7 +730,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tif (kinds != REF_LOCAL_BRANCH)\n \t\t\tdie(_(\"-a and -r options to 'git branch' do not make sense with a branch name\"));\n \t\tcreate_branch(head, argv[0], (argc == 2) ? argv[1] : head,\n-\t\t\t force_create, reflog, track);\n+\t\t\t force_create, reflog, 0, track);\n \t} else\n \t\tusage_with_options(builtin_branch_usage, options);\n \ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 2a807724..ca00a853 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -540,7 +540,9 @@ static void update_refs_for_switch(struct checkout_opts *opts,\n \t\telse\n \t\t\tcreate_branch(old->name, opts->new_branch, new->name,\n \t\t\t\t opts->new_branch_force ? 1 : 0,\n-\t\t\t\t opts->new_branch_log, opts->track);\n+\t\t\t\t opts->new_branch_log,\n+\t\t\t\t opts->new_branch_force ? 1 : 0,\n+\t\t\t\t opts->track);\n \t\tnew->name = opts->new_branch;\n \t\tsetup_branch_path(new);\n \t}\n@@ -565,8 +567,12 @@ static void update_refs_for_switch(struct checkout_opts *opts,\n \t\tcreate_symref(\"HEAD\", new->path, msg.buf);\n \t\tif (!opts->quiet) {\n \t\t\tif (old->path && !strcmp(new->path, old->path)) {\n-\t\t\t\tfprintf(stderr, _(\"Already on '%s'\\n\"),\n-\t\t\t\t\tnew->name);\n+\t\t\t\tif (opts->new_branch_force)\n+\t\t\t\t\tfprintf(stderr, _(\"Reset branch '%s'\\n\"),\n+\t\t\t\t\t\tnew->name);\n+\t\t\t\telse\n+\t\t\t\t\tfprintf(stderr, _(\"Already on '%s'\\n\"),\n+\t\t\t\t\t\tnew->name);\n \t\t\t} else if (opts->new_branch) {\n \t\t\t\tif (opts->branch_exists)\n \t\t\t\t\tfprintf(stderr, _(\"Switched to and reset branch '%s'\\n\"), new->name);\n@@ -1057,7 +1063,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n \t\topts.branch_exists = validate_new_branchname(opts.new_branch, &buf,\n-\t\t\t\t\t\t\t !!opts.new_branch_force, 0);\n+\t\t\t\t\t\t\t !!opts.new_branch_force,\n+\t\t\t\t\t\t\t !!opts.new_branch_force);\n \n \t\tstrbuf_release(&buf);\n \t}\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 75874e85..27412623 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -189,12 +189,13 @@ 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+test_expect_success 'checkout -B to the current branch works' '\n \tgit checkout branch1 &&\n+\tgit checkout -B branch1-scratch &&\n+\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+\tgit checkout -B branch1-scratch initial &&\n+\ttest_dirty_mergeable\n '\n \n test_done\n-- \n1.7.8.rc3\n"},{"id":"179989","messageId":"7v39day0fb.fsf@alter.siamese.dyndns.org","threadId":"29020","inReplyTo":"20111126085455.GB22656@elie.hsd1.il.comcast.net","subject":"Re: git branch -M\" regression in 1.7.7?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-26T22:38:16Z","receivedAt":"2011-11-26T22:38:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> In other words, when on master, \"git checkout -B master \"\n> would be another way to say \"git reset --keep \", except that\n> it also sets up tracking.\n\nI havn't look at the patch (not a regression between 1.7.7 and 1.7.8 so\nnot a candidate for the remainder of this cycle), but I like the above\ndescription quite a lot. I think Linus's \"git reset --sane\" which was\ninitially called \"git reset --merge\" but ended up as \"git reset --keep\"\nshould have been spelled as \"checkout -B \" from the\nbeginning.\n"},{"id":"179993","messageId":"m2bory5vmm.fsf@igel.home","threadId":"29020","inReplyTo":"7v39day0fb.fsf@alter.siamese.dyndns.org","subject":"Re: git branch -M\" regression in 1.7.7?","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2011-11-26T23:09:21Z","receivedAt":"2011-11-26T23:09:21Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Junio C Hamano writes:\n\n> I havn't look at the patch (not a regression between 1.7.7 and 1.7.8 so\n> not a candidate for the remainder of this cycle), but I like the above\n> description quite a lot. I think Linus's \"git reset --sane\" which was\n> initially called \"git reset --merge\" but ended up as \"git reset --keep\"\n> should have been spelled as \"checkout -B \" from the\n> beginning.\n\nIt is more convenient if you don't have to spell out the name of the\ncurrent branch (which fails if you aren't on a branch).\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"180062","messageId":"7vk46kuje6.fsf@alter.siamese.dyndns.org","threadId":"29020","inReplyTo":"20111126023002.GA17652@elie.hsd1.il.comcast.net","subject":"Re: git branch -M\" regression in 1.7.7?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-28T19:38:57Z","receivedAt":"2011-11-28T19:38:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> \tgit branch -M master\n>\n> This seems like a valuable exception to allow, because then \"git\n> branch -M foo\" would _always_ be allowed --- either 'foo' is not the\n> current branch, and it does the obvious thing, or 'foo' is the current\n> branch, and nothing happens.\n>\n> Buildbot uses this idiom and was broken in 1.7.7 (it would emit the\n> message \"Cannot force update the current branch\").\n\nAlthough I am not sure the practice deserves to be called \"idiom\", I agree\nthat there is no reason to forbid renaming the current branch to the tip\ncommit of itself.\n\nWill queue.\n"}]}