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

Commit Message

Johannes Schindelin Nov. 23, 2020, 11:20 p.m. UTC

From: Johannes Schindelin <[e-mail]>

In one of the next commits, we would like to give users some advice
regarding the initial branch name, and how to modify it.

To that end, it would be good if `git branch -m <name>` worked in a
freshly initialized repository without any commits. Let's make it so.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 builtin/branch.c | 3 ++-
 t/t0001-init.sh  | 6 ++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

Comments

Junio C Hamano Nov. 23, 2020, 11:45 p.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
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.  

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.

> +	    rename_ref(oldref.buf, newref.buf, logmsg.buf))
>  		die(_("Branch rename failed"));
>  	if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
>  		die(_("Branch copy failed"));
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> index 69a320489f..69c5ad179c 100755
> --- a/t/t0001-init.sh
> +++ b/t/t0001-init.sh
> @@ -571,4 +571,10 @@ test_expect_success 'invalid default branch name' '
>  	test_i18ngrep "invalid branch name" err
>  '
>  
> +test_expect_success 'branch -m with the initial branch' '
> +	git init rename-initial-branch &&
> +	git -C rename-initial-branch branch -m renamed &&
> +	test renamed = $(git -C rename-initial-branch symbolic-ref --short HEAD)
> +'
> +
>  test_done

Thanks.


 t/t0001-init.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git c/t/t0001-init.sh w/t/t0001-init.sh
index 69c5ad179c..07c34431d9 100755
--- c/t/t0001-init.sh
+++ w/t/t0001-init.sh
@@ -577,4 +577,10 @@ test_expect_success 'branch -m with the initial branch' '
 	test renamed = $(git -C rename-initial-branch symbolic-ref --short HEAD)
 '
 
+test_expect_success 'branch -m with the initial branch' '
+	git -c init.defaultBranch=initial init rename-unborn-branch &&
+	git -C rename-unborn-branch branch -m initial renamed &&
+	test renamed = $(git -C rename-unborn-branch symbolic-ref --short HEAD)
+'
+
 test_done

Johannes Schindelin Nov. 24, 2020, 5:47 a.m. UTC | #2

Hi Junio,


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

> "Johannes Schindelin via GitGitGadget" <[e-mail]>
> 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

Junio C Hamano Nov. 24, 2020, 8:14 p.m. UTC | #3

Johannes Schindelin <[e-mail]> writes:

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

FWIW, for fun I dropped "oldname != head ||" and tried to run the
whole testsuite (plus the "branch -m src dst" form test I gave
earlier), and it almost passed all of them except one, which was
surprising.

> +	if (!copy && !is_unborn_branch(oldname, oldref.buf) &&
>  	    rename_ref(oldref.buf, newref.buf, logmsg.buf))

Yeah, a helper function makes it much more clear what is going on.

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

;-)

Patch

diff --git a/builtin/branch.c b/builtin/branch.c
index efb30b8820..10609981e6 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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)) &&
+	    rename_ref(oldref.buf, newref.buf, logmsg.buf))
 		die(_("Branch rename failed"));
 	if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
 		die(_("Branch copy failed"));
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 69a320489f..69c5ad179c 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -571,4 +571,10 @@  test_expect_success 'invalid default branch name' '
 	test_i18ngrep "invalid branch name" err
 '
 
+test_expect_success 'branch -m with the initial branch' '
+	git init rename-initial-branch &&
+	git -C rename-initial-branch branch -m renamed &&
+	test renamed = $(git -C rename-initial-branch symbolic-ref --short HEAD)
+'
+
 test_done