git branch -M" regression in 1.7.7?

git branch -M" regression in 1.7.7?

[☂Josh Chia (谢任中)]

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?

Re: git branch -M" regression in 1.7.7?

[Jonathan Nieder]

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 <sonne@debian.org>
Reported-by: Josh Chia (谢任中)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 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

[PATCH] Test renaming a branch to itself

[Conrad Irwin]

Test for a regression introduced in v1.7.7-rc2~1^2~2.

Requested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Conrad Irwin <conrad.irwin@gmail.com>
---
 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

Re: [PATCH] Test renaming a branch to itself

[Jonathan Nieder]

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

Re: git branch -M" regression in 1.7.7?

[Conrad Irwin]

On Fri, Nov 25, 2011 at 6:30 PM, Jonathan Nieder <jrnieder@gmail.com> 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

Re: git branch -M" regression in 1.7.7?

[Jonathan Nieder]

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 <commit>"
would be another way to say "git reset --keep <commit>", except that
it also sets up tracking.

Surprisingly, switch_branches() seems to match that pretty well already.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 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 <describe>' '
 	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

Re: git branch -M" regression in 1.7.7?

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> In other words, when on master, "git checkout -B master <commit>"
> would be another way to say "git reset --keep <commit>", 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 <current-branch>" from the
beginning.

Re: git branch -M" regression in 1.7.7?

[Andreas Schwab]

Junio C Hamano <gitster@pobox.com> 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 <current-branch>" 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."

Re: git branch -M" regression in 1.7.7?

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> 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.