# git branch -M" regression in 1.7.7? 9 messages from 2011-11-26 to 2011-11-28. Participants: ☂Josh Chia (谢任中), Jonathan Nieder, Conrad Irwin, Junio C Hamano, Andreas Schwab. Thread: https://gitlist.dev/t/29020 ## ☂Josh Chia (谢任中), 2011-11-26 00:36 Subject: git branch -M" regression in 1.7.7? Message-ID: URL: https://gitlist.dev/e/CALxtSbRbwkVDKJcXiKY9rHYCjA3XGgCytbXQnRhQvbEnY8SpjA%40mail.gmail.com ``` On git 1.7.7.3, when I try to "git branch -M master" when I'm already on a branch 'master', I get this error message: Cannot force update the current branch On 1.7.6.4, the command succeeds. Is this intended? ``` ## Jonathan Nieder, 2011-11-26 02:30 Subject: Re: git branch -M" regression in 1.7.7? Message-ID: <20111126023002.GA17652@elie.hsd1.il.comcast.net> URL: https://gitlist.dev/e/20111126023002.GA17652%40elie.hsd1.il.comcast.net In-Reply-To: ``` Hi, Josh Chia (谢任中) wrote: > On git 1.7.7.3, when I try to "git branch -M master" when I'm already > on a branch 'master', I get this error message: > Cannot force update the current branch > > On 1.7.6.4, the command succeeds. > > Is this intended? Yes, but it probably wasn't a good idea. How about this patch? A reproduction recipe (preferrably in the form of a patch to t/t3200-branch.sh would be welcome. -- >8 -- Subject: treat "git branch -M master master" as a no-op again Before v1.7.7-rc2~1^2~2 (Prevent force-updating of the current branch, 2011-08-20), commands like "git branch -M topic master" could be used even when "master" was the current branch, with the somewhat counterintuitive result that HEAD would point to some place new while the index and worktree kept the content of the old commit. This is not a very sensible operation and the result is what almost nobody would expect, so erroring out in this case was a good change. However, there is one exception to the "it's usually not obvious what it would mean to overwrite the current branch by another one" rule. Namely: git branch -M master master is clearly meant to be a no-op, even if you are on the master branch. And in the latter case, it can be abbreviated: git branch -M master This seems like a valuable exception to allow, because then "git branch -M foo" would _always_ be allowed --- either 'foo' is not the current branch, and it does the obvious thing, or 'foo' is the current branch, and nothing happens. Buildbot uses this idiom and was broken in 1.7.7 (it would emit the message "Cannot force update the current branch"). Reported-by: Soeren Sonnenburg Reported-by: Josh Chia (谢任中) Signed-off-by: Jonathan Nieder --- builtin/branch.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index 51ca6a02..24f33b24 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -568,6 +568,7 @@ static void rename_branch(const char *oldname, const char *newname, int force) unsigned char sha1[20]; struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT; int recovery = 0; + int clobber_head_ok; if (!oldname) die(_("cannot rename the current branch while not on any.")); @@ -583,7 +584,13 @@ static void rename_branch(const char *oldname, const char *newname, int force) die(_("Invalid branch name: '%s'"), oldname); } - validate_new_branchname(newname, &newref, force, 0); + /* + * A command like "git branch -M currentbranch currentbranch" cannot + * cause the worktree to become inconsistent with HEAD, so allow it. + */ + clobber_head_ok = !strcmp(oldname, newname); + + validate_new_branchname(newname, &newref, force, clobber_head_ok); strbuf_addf(&logmsg, "Branch: renamed %s to %s", oldref.buf, newref.buf); -- 1.7.8.rc3 ``` ## Conrad Irwin, 2011-11-26 06:52 Subject: [PATCH] Test renaming a branch to itself Message-ID: <1322290364-16207-1-git-send-email-conrad.irwin@gmail.com> URL: https://gitlist.dev/e/1322290364-16207-1-git-send-email-conrad.irwin%40gmail.com In-Reply-To: <20111126023002.GA17652@elie.hsd1.il.comcast.net> ``` Test for a regression introduced in v1.7.7-rc2~1^2~2. Requested-by: Jonathan Nieder Signed-off-by: Conrad Irwin --- t/t3200-branch.sh | 16 ++++++++++++++++ 1 files changed, 16 insertions(+), 0 deletions(-) diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index bc73c20..7690332 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -115,6 +115,22 @@ test_expect_success 'git branch -M baz bam should succeed when baz is checked ou git branch -M baz bam ' +test_expect_success 'git branch -M master should work when master is checked out' ' + git checkout master && + git branch -M master +' + +test_expect_success 'git branch -M master master should work when master is checked out' ' + git checkout master && + git branch -M master master +' + +test_expect_success 'git branch -M master2 master2 should work when master is checked out' ' + git checkout master && + git branch master2 && + git branch -M master2 master2 +' + test_expect_success 'git branch -v -d t should work' ' git branch t && test_path_is_file .git/refs/heads/t && -- 1.7.7.1.433.ga2d04a ``` ## Jonathan Nieder, 2011-11-26 06:59 Subject: Re: [PATCH] Test renaming a branch to itself Message-ID: <20111126065901.GB20923@elie.hsd1.il.comcast.net> URL: https://gitlist.dev/e/20111126065901.GB20923%40elie.hsd1.il.comcast.net In-Reply-To: <1322290364-16207-1-git-send-email-conrad.irwin@gmail.com> ``` Conrad Irwin wrote: > Test for a regression introduced in v1.7.7-rc2~1^2~2. Thanks! I take it that means you like the patch. :) The tests look sane fwiw (and it looks like the tests you wrote before cover the "branch -M" safety valve pretty well). ``` ## Conrad Irwin, 2011-11-26 07:05 Subject: Re: git branch -M" regression in 1.7.7? Message-ID: URL: https://gitlist.dev/e/CAOTq_pv4dyAkbqye%2BdiK9mTTsrTg9OKg0tExKcfDgs8RfiTwTQ%40mail.gmail.com In-Reply-To: <20111126023002.GA17652@elie.hsd1.il.comcast.net> ``` On Fri, Nov 25, 2011 at 6:30 PM, Jonathan Nieder wrote: > A reproduction recipe (preferrably in the form of a patch to > t/t3200-branch.sh would be welcome. Sent in a separate email. Feel free to add a "Tested-by:" header to your patch if you want :). > > -- >8 -- > Subject: treat "git branch -M master master" as a no-op again > > Before v1.7.7-rc2~1^2~2 (Prevent force-updating of the current branch, > 2011-08-20), commands like "git branch -M topic master" could be used > even when "master" was the current branch, with the somewhat > counterintuitive result that HEAD would point to some place new while > the index and worktree kept the content of the old commit.  This is > not a very sensible operation and the result is what almost nobody > would expect, so erroring out in this case was a good change. > > However, there is one exception to the "it's usually not obvious what > it would mean to overwrite the current branch by another one" rule. > Namely: > >        git branch -M master master > > is clearly meant to be a no-op, even if you are on the master branch. Agreed. I thought after reading your patch about making it just do: if (!strcmp(oldname, newname)) exit(0); but I guess it would then not mark an entry in the reflog that people could be relying on... > +       clobber_head_ok = !strcmp(oldname, newname); > + > +       validate_new_branchname(newname, &newref, force, clobber_head_ok); This looks ok, and will be improvable if the NEEDSWORK in branch.h is done. The other thing I wonder is whether "git checkout -B master HEAD" or "git branch -f master master" should have the same short-cut? Conrad ``` ## Jonathan Nieder, 2011-11-26 08:54 Subject: Re: git branch -M" regression in 1.7.7? Message-ID: <20111126085455.GB22656@elie.hsd1.il.comcast.net> URL: https://gitlist.dev/e/20111126085455.GB22656%40elie.hsd1.il.comcast.net In-Reply-To: ``` Conrad Irwin wrote: > I thought after reading your patch about making it just do: > > if (!strcmp(oldname, newname)) > exit(0); > > but I guess it would then not mark an entry in the reflog that people > could be relying on... Ah, this deserves a comment. I thought about doing the same thing, and then didn't do it because I wanted to make sure that git branch -M nonexistent nonexistent does not succeed. Which presumably warrants another test: test_expect_success "rename-to-self dwimery doesn't hide nonexistent ref" ' test_must_fail git branch -M nonexistent nonexistent && test_must_fail git rev-parse --verify nonexistent ' Sloppy of me. >> +       clobber_head_ok = !strcmp(oldname, newname); >> + >> +       validate_new_branchname(newname, &newref, force, clobber_head_ok); > > This looks ok, and will be improvable if the NEEDSWORK in branch.h is done. Thanks for looking it over. > The other thing I wonder is whether "git checkout -B master HEAD" or > "git branch -f master master" should have the same short-cut? I think "git branch -M" is the only one buildbot was relying on. As an aside, I'm not convinced the check is needed for checkout -B at all. In an ideal world, the order of operations would be: 1. look up commit argument 2. merge working tree 3. update branch to match commit 4. update HEAD symref to point to branch In other words, when on master, "git checkout -B master " would be another way to say "git reset --keep ", except that it also sets up tracking. Surprisingly, switch_branches() seems to match that pretty well already. Signed-off-by: Jonathan Nieder --- branch.c | 6 ++++-- branch.h | 3 ++- builtin/branch.c | 2 +- builtin/checkout.c | 15 +++++++++++---- t/t2018-checkout-branch.sh | 9 +++++---- 5 files changed, 23 insertions(+), 12 deletions(-) diff --git a/branch.c b/branch.c index 025a97be..f85c4382 100644 --- a/branch.c +++ b/branch.c @@ -160,7 +160,8 @@ int validate_new_branchname(const char *name, struct strbuf *ref, void create_branch(const char *head, const char *name, const char *start_name, - int force, int reflog, enum branch_track track) + int force, int reflog, int clobber_head, + enum branch_track track) { struct ref_lock *lock = NULL; struct commit *commit; @@ -175,7 +176,8 @@ void create_branch(const char *head, explicit_tracking = 1; if (validate_new_branchname(name, &ref, force, - track == BRANCH_TRACK_OVERRIDE)) { + track == BRANCH_TRACK_OVERRIDE || + clobber_head)) { if (!force) dont_change_ref = 1; else diff --git a/branch.h b/branch.h index 1285158d..e125ff4c 100644 --- a/branch.h +++ b/branch.h @@ -13,7 +13,8 @@ * branch for (if any). */ void create_branch(const char *head, const char *name, const char *start_name, - int force, int reflog, enum branch_track track); + int force, int reflog, + int clobber_head, enum branch_track track); /* * Validates that the requested branch may be created, returning the diff --git a/builtin/branch.c b/builtin/branch.c index 51ca6a02..730f9139 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -730,7 +730,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) if (kinds != REF_LOCAL_BRANCH) die(_("-a and -r options to 'git branch' do not make sense with a branch name")); create_branch(head, argv[0], (argc == 2) ? argv[1] : head, - force_create, reflog, track); + force_create, reflog, 0, track); } else usage_with_options(builtin_branch_usage, options); diff --git a/builtin/checkout.c b/builtin/checkout.c index 2a807724..ca00a853 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -540,7 +540,9 @@ static void update_refs_for_switch(struct checkout_opts *opts, else create_branch(old->name, opts->new_branch, new->name, opts->new_branch_force ? 1 : 0, - opts->new_branch_log, opts->track); + opts->new_branch_log, + opts->new_branch_force ? 1 : 0, + opts->track); new->name = opts->new_branch; setup_branch_path(new); } @@ -565,8 +567,12 @@ static void update_refs_for_switch(struct checkout_opts *opts, create_symref("HEAD", new->path, msg.buf); if (!opts->quiet) { if (old->path && !strcmp(new->path, old->path)) { - fprintf(stderr, _("Already on '%s'\n"), - new->name); + if (opts->new_branch_force) + fprintf(stderr, _("Reset branch '%s'\n"), + new->name); + else + fprintf(stderr, _("Already on '%s'\n"), + new->name); } else if (opts->new_branch) { if (opts->branch_exists) fprintf(stderr, _("Switched to and reset branch '%s'\n"), new->name); @@ -1057,7 +1063,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) struct strbuf buf = STRBUF_INIT; opts.branch_exists = validate_new_branchname(opts.new_branch, &buf, - !!opts.new_branch_force, 0); + !!opts.new_branch_force, + !!opts.new_branch_force); strbuf_release(&buf); } diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh index 75874e85..27412623 100755 --- a/t/t2018-checkout-branch.sh +++ b/t/t2018-checkout-branch.sh @@ -189,12 +189,13 @@ test_expect_success 'checkout -b ' ' test_cmp expect actual ' -test_expect_success 'checkout -B to the current branch fails before merging' ' +test_expect_success 'checkout -B to the current branch works' ' git checkout branch1 && + git checkout -B branch1-scratch && + setup_dirty_mergeable && - git commit -mfooble && - test_must_fail git checkout -B branch1 initial && - test_must_fail test_dirty_mergeable + git checkout -B branch1-scratch initial && + test_dirty_mergeable ' test_done -- 1.7.8.rc3 ``` ## Junio C Hamano, 2011-11-26 22:38 Subject: Re: git branch -M" regression in 1.7.7? Message-ID: <7v39day0fb.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v39day0fb.fsf%40alter.siamese.dyndns.org In-Reply-To: <20111126085455.GB22656@elie.hsd1.il.comcast.net> ``` Jonathan Nieder writes: > In other words, when on master, "git checkout -B master " > would be another way to say "git reset --keep ", except that > it also sets up tracking. I havn't look at the patch (not a regression between 1.7.7 and 1.7.8 so not a candidate for the remainder of this cycle), but I like the above description quite a lot. I think Linus's "git reset --sane" which was initially called "git reset --merge" but ended up as "git reset --keep" should have been spelled as "checkout -B " from the beginning. ``` ## Andreas Schwab, 2011-11-26 23:09 Subject: Re: git branch -M" regression in 1.7.7? Message-ID: URL: https://gitlist.dev/e/m2bory5vmm.fsf%40igel.home In-Reply-To: <7v39day0fb.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > I havn't look at the patch (not a regression between 1.7.7 and 1.7.8 so > not a candidate for the remainder of this cycle), but I like the above > description quite a lot. I think Linus's "git reset --sane" which was > initially called "git reset --merge" but ended up as "git reset --keep" > should have been spelled as "checkout -B " from the > beginning. It is more convenient if you don't have to spell out the name of the current branch (which fails if you aren't on a branch). Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ``` ## Junio C Hamano, 2011-11-28 19:38 Subject: Re: git branch -M" regression in 1.7.7? Message-ID: <7vk46kuje6.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vk46kuje6.fsf%40alter.siamese.dyndns.org In-Reply-To: <20111126023002.GA17652@elie.hsd1.il.comcast.net> ``` Jonathan Nieder writes: > git branch -M master > > This seems like a valuable exception to allow, because then "git > branch -M foo" would _always_ be allowed --- either 'foo' is not the > current branch, and it does the obvious thing, or 'foo' is the current > branch, and nothing happens. > > Buildbot uses this idiom and was broken in 1.7.7 (it would emit the > message "Cannot force update the current branch"). Although I am not sure the practice deserves to be called "idiom", I agree that there is no reason to forbid renaming the current branch to the tip commit of itself. Will queue. ```