Re: [PATCH v2 2/4] branch -m: allow renaming a yet-unborn branch

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,


On Mon, 23 Nov 2020, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > @@ -538,7 +538,8 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
> >  		strbuf_addf(&logmsg, "Branch: renamed %s to %s",
> >  			    oldref.buf, newref.buf);
> >
> > -	if (!copy && rename_ref(oldref.buf, newref.buf, logmsg.buf))
> > +	if (!copy && (oldname != head || !is_null_oid(&head_oid)) &&
>
> It always makes readers uneasy to see pointer comparison of two
> strings.

Even if it was on purpose ;-)

> Does this mean, after "git -c init.defaultbranch=master init",
>
> 	git branch -m master main
>
> would not work while
>
> 	git branch -m main
>
> would?  It would be easy to see with the attached patch to the test,
> I guess.

At first, I thought that it would be inappropriate to do that because it
would not work with unborn branches in a worktree other than the current
one. Like,

	git worktree add --no-checkout --detach other
	git -C other switch --orphan start-over-again
	git branch -m start-over-again fresh-new-start

On second thought, that's a really obscure use case, anyway. It is not
even possible to create a secondary worktree! I started down that rabbit
hole, but think I'd better let it be. This is where I stopped:

-- snip --
diff --git a/builtin/branch.c b/builtin/branch.c
index 200da319f1d..c84bffe9632 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -489,6 +489,16 @@ static void reject_rebase_or_bisect_branch(const char *target)
 	free_worktrees(worktrees);
 }

+static int is_unborn_branch(const char *branch_name, const char *full_ref_name)
+{
+	int flags;
+
+	return (head && !strcmp(branch_name, head) && is_null_oid(&head_oid)) ||
+		(!resolve_ref_unsafe(full_ref_name, RESOLVE_REF_READING, NULL,
+				     &flags) &&
+		 find_shared_symref("HEAD", full_ref));
+}
+
 static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)
 {
 	struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;
@@ -538,8 +548,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 		strbuf_addf(&logmsg, "Branch: renamed %s to %s",
 			    oldref.buf, newref.buf);

-	if (!copy &&
-	    (!head || strcmp(oldname, head) || !is_null_oid(&head_oid)) &&
+	if (!copy && !is_unborn_branch(oldname, oldref.buf) &&
 	    rename_ref(oldref.buf, newref.buf, logmsg.buf))
 		die(_("Branch rename failed"));
 	if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 84047ac64e6..124abeedf19 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -586,4 +586,12 @@ test_expect_success 'branch -m with the initial branch' '
 	test again = $(git -C rename-initial symbolic-ref --short HEAD)
 '

+test_expect_success 'branch -m with the initial branch in another worktree' '
+	git -c init.defaultBranch=initial init rename-two &&
+	test_commit -C rename-two initial &&
+	git -C rename-two worktree add --no-checkout ../rename-worktree &&
+	git -C rename-worktree switch --orphan brand-new-day &&
+	git -C rename-two branch -m brand-new-day renamed
+'
+
 test_done
-- snap --

Having said that, I fixed the `git branch -m <current-unborn> <new-name>`
use case; the fix will be part of v3.

Ciao,
Dscho