From 2e8af499ff6e166a5f54f18cf3d1fa8f5d9bde38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Justo?= Date: Mon, 27 Mar 2023 00:33:02 +0200 Subject: [PATCH 1/5] branch: test for failures while renaming branches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When we introduced replace_each_worktree_head_symref() in 70999e9cec (branch -m: update all per-worktree HEADs, 2016-03-27), we implemented a best effort approach. If we are asked to rename a branch that is simultaneously checked out in multiple worktrees, we try to update all of those worktrees. If we fail updating any of them, we die() as a signal that something has gone wrong. However, at this point, the branch ref has already been renamed and also updated the HEADs of the successfully updated worktrees. Despite returning an error, we do not try to rollback those changes. Let's add a test to notice if we change this behavior in the future. In next commits we will change replace_each_worktree_head_symref() to work more closely with its only caller, copy_or_rename_branch(). Let's move the former closer to its caller, to facilitate those changes. Signed-off-by: Rubén Justo Signed-off-by: Junio C Hamano --- branch.c | 27 --------------------------- branch.h | 8 -------- builtin/branch.c | 32 ++++++++++++++++++++++++++++++++ t/t3200-branch.sh | 15 +++++++++++++++ 4 files changed, 47 insertions(+), 35 deletions(-) diff --git a/branch.c b/branch.c index e5614b53b3..f64062be71 100644 --- a/branch.c +++ b/branch.c @@ -830,30 +830,3 @@ void die_if_checked_out(const char *branch, int ignore_current_worktree) free_worktrees(worktrees); } - -int replace_each_worktree_head_symref(const char *oldref, const char *newref, - const char *logmsg) -{ - int ret = 0; - struct worktree **worktrees = get_worktrees(); - int i; - - for (i = 0; worktrees[i]; i++) { - struct ref_store *refs; - - if (worktrees[i]->is_detached) - continue; - if (!worktrees[i]->head_ref) - continue; - if (strcmp(oldref, worktrees[i]->head_ref)) - continue; - - refs = get_worktree_ref_store(worktrees[i]); - if (refs_create_symref(refs, "HEAD", newref, logmsg)) - ret = error(_("HEAD of working tree %s is not updated"), - worktrees[i]->path); - } - - free_worktrees(worktrees); - return ret; -} diff --git a/branch.h b/branch.h index ef56103c05..30c01aed73 100644 --- a/branch.h +++ b/branch.h @@ -155,12 +155,4 @@ int read_branch_desc(struct strbuf *, const char *branch_name); */ void die_if_checked_out(const char *branch, int ignore_current_worktree); -/* - * Update all per-worktree HEADs pointing at the old ref to point the new ref. - * This will be used when renaming a branch. Returns 0 if successful, non-zero - * otherwise. - */ -int replace_each_worktree_head_symref(const char *oldref, const char *newref, - const char *logmsg); - #endif diff --git a/builtin/branch.c b/builtin/branch.c index f63fd45edb..c7ace2f2da 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -509,6 +509,38 @@ static void reject_rebase_or_bisect_branch(const char *target) free_worktrees(worktrees); } +/* + * Update all per-worktree HEADs pointing at the old ref to point the new ref. + * This will be used when renaming a branch. Returns 0 if successful, non-zero + * otherwise. + */ +static int replace_each_worktree_head_symref(const char *oldref, const char *newref, + const char *logmsg) +{ + int ret = 0; + struct worktree **worktrees = get_worktrees(); + int i; + + for (i = 0; worktrees[i]; i++) { + struct ref_store *refs; + + if (worktrees[i]->is_detached) + continue; + if (!worktrees[i]->head_ref) + continue; + if (strcmp(oldref, worktrees[i]->head_ref)) + continue; + + refs = get_worktree_ref_store(worktrees[i]); + if (refs_create_symref(refs, "HEAD", newref, logmsg)) + ret = error(_("HEAD of working tree %s is not updated"), + worktrees[i]->path); + } + + free_worktrees(worktrees); + return ret; +} + 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; diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 5a169b68d6..6b8b59f15e 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -239,6 +239,21 @@ test_expect_success 'git branch -M baz bam should succeed when baz is checked ou git worktree prune ' +test_expect_success 'git branch -M fails if updating any linked working tree fails' ' + git worktree add -b baz bazdir1 && + git worktree add -f bazdir2 baz && + touch .git/worktrees/bazdir1/HEAD.lock && + test_must_fail git branch -M baz bam && + test $(git -C bazdir2 rev-parse --abbrev-ref HEAD) = bam && + git branch -M bam baz && + rm .git/worktrees/bazdir1/HEAD.lock && + touch .git/worktrees/bazdir2/HEAD.lock && + test_must_fail git branch -M baz bam && + test $(git -C bazdir1 rev-parse --abbrev-ref HEAD) = bam && + rm -rf bazdir1 bazdir2 && + git worktree prune +' + test_expect_success 'git branch -M baz bam should succeed within a worktree in which baz is checked out' ' git checkout -b baz && git worktree add -f bazdir baz && From d7f4ca61b51da5655df09277309380794ba1bd19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Justo?= Date: Mon, 27 Mar 2023 00:33:09 +0200 Subject: [PATCH 2/5] branch: use get_worktrees() in copy_or_rename_branch() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Obtaining the list of worktrees, using get_worktrees(), is not a lightweight operation, because it involves reading from disk. Let's stop calling get_worktrees() in reject_rebase_or_bisect_branch() and in replace_each_worktree_head_symref(). Make them receive the list of worktrees from their only caller, copy_or_rename_branch(). Signed-off-by: Rubén Justo Signed-off-by: Junio C Hamano --- builtin/branch.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index c7ace2f2da..bac67c27d5 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -486,9 +486,9 @@ static void print_current_branch_name(void) die(_("HEAD (%s) points outside of refs/heads/"), refname); } -static void reject_rebase_or_bisect_branch(const char *target) +static void reject_rebase_or_bisect_branch(struct worktree **worktrees, + const char *target) { - struct worktree **worktrees = get_worktrees(); int i; for (i = 0; worktrees[i]; i++) { @@ -505,8 +505,6 @@ static void reject_rebase_or_bisect_branch(const char *target) die(_("Branch %s is being bisected at %s"), target, wt->path); } - - free_worktrees(worktrees); } /* @@ -514,11 +512,11 @@ static void reject_rebase_or_bisect_branch(const char *target) * This will be used when renaming a branch. Returns 0 if successful, non-zero * otherwise. */ -static int replace_each_worktree_head_symref(const char *oldref, const char *newref, +static int replace_each_worktree_head_symref(struct worktree **worktrees, + const char *oldref, const char *newref, const char *logmsg) { int ret = 0; - struct worktree **worktrees = get_worktrees(); int i; for (i = 0; worktrees[i]; i++) { @@ -537,7 +535,6 @@ static int replace_each_worktree_head_symref(const char *oldref, const char *new worktrees[i]->path); } - free_worktrees(worktrees); return ret; } @@ -548,6 +545,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int const char *interpreted_oldname = NULL; const char *interpreted_newname = NULL; int recovery = 0; + struct worktree **worktrees = get_worktrees(); if (strbuf_check_branch_ref(&oldref, oldname)) { /* @@ -576,7 +574,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int else validate_new_branchname(newname, &newref, force); - reject_rebase_or_bisect_branch(oldref.buf); + reject_rebase_or_bisect_branch(worktrees, oldref.buf); if (!skip_prefix(oldref.buf, "refs/heads/", &interpreted_oldname) || !skip_prefix(newref.buf, "refs/heads/", &interpreted_newname)) { @@ -607,7 +605,8 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int } if (!copy && - replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf)) + replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf, + logmsg.buf)) die(_("Branch renamed to %s, but HEAD is not updated!"), newname); strbuf_release(&logmsg); @@ -622,6 +621,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int strbuf_release(&newref); strbuf_release(&oldsection); strbuf_release(&newsection); + free_worktrees(worktrees); } static GIT_PATH_FUNC(edit_description, "EDIT_DESCRIPTION") From 7a6ccdfb4eeb9d55893eae9b2c7e573b92f3d01d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Justo?= Date: Mon, 27 Mar 2023 00:33:17 +0200 Subject: [PATCH 3/5] branch: description for orphan branch errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In bcfc82bd48 (branch: description for non-existent branch errors, 2022-10-08) we checked the HEAD in the current worktree to detect if the branch to operate with is an orphan branch, so as to avoid the confusing error: "No branch named...". If we are asked to operate with an orphan branch in a different working tree than the current one, we need to check the HEAD in that different working tree. Let's extend the check we did in bcfc82bd48, to check the HEADs in all worktrees linked to the current repository, using the helper introduced in 31ad6b61bd (branch: add branch_checked_out() helper, 2022-06-15). The helper, branch_checked_out(), does its work obtaining internally a list of worktrees linked to the current repository. Obtaining that list is not a lightweight work because it implies disk access. In copy_or_rename_branch() we already have a list of worktrees. Let's use that already obtained list, and avoid using here the helper. Signed-off-by: Rubén Justo Signed-off-by: Junio C Hamano --- builtin/branch.c | 21 ++++++++++++++++----- t/t3202-show-branch.sh | 18 ++++++++++++++++++ 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index bac67c27d5..90dcbb0c6e 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -538,13 +538,15 @@ static int replace_each_worktree_head_symref(struct worktree **worktrees, return ret; } +#define IS_HEAD 1 + 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; struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT; const char *interpreted_oldname = NULL; const char *interpreted_newname = NULL; - int recovery = 0; + int recovery = 0, oldref_usage = 0; struct worktree **worktrees = get_worktrees(); if (strbuf_check_branch_ref(&oldref, oldname)) { @@ -558,8 +560,17 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int die(_("Invalid branch name: '%s'"), oldname); } - if ((copy || strcmp(head, oldname)) && !ref_exists(oldref.buf)) { - if (copy && !strcmp(head, oldname)) + for (int i = 0; worktrees[i]; i++) { + struct worktree *wt = worktrees[i]; + + if (wt->head_ref && !strcmp(oldref.buf, wt->head_ref)) { + oldref_usage |= IS_HEAD; + break; + } + } + + if ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) { + if (oldref_usage & IS_HEAD) die(_("No commit on branch '%s' yet."), oldname); else die(_("No branch named '%s'."), oldname); @@ -838,7 +849,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) strbuf_addf(&branch_ref, "refs/heads/%s", branch_name); if (!ref_exists(branch_ref.buf)) - error((!argc || !strcmp(head, branch_name)) + error((!argc || branch_checked_out(branch_ref.buf)) ? _("No commit on branch '%s' yet.") : _("No branch named '%s'."), branch_name); @@ -883,7 +894,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) } if (!ref_exists(branch->refname)) { - if (!argc || !strcmp(head, branch->name)) + if (!argc || branch_checked_out(branch->refname)) die(_("No commit on branch '%s' yet."), branch->name); die(_("branch '%s' does not exist"), branch->name); } diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh index ea7cfd1951..be20ebe1d5 100755 --- a/t/t3202-show-branch.sh +++ b/t/t3202-show-branch.sh @@ -221,4 +221,22 @@ test_expect_success 'fatal descriptions on non-existent branch' ' test_cmp expect actual ' +test_expect_success 'error descriptions on orphan branch' ' + test_when_finished git worktree remove -f wt && + git worktree add wt --detach && + git -C wt checkout --orphan orphan-branch && + test_branch_op_in_wt() { + test_orphan_error() { + test_must_fail git $* 2>actual && + test_i18ngrep "No commit on branch .orphan-branch. yet.$" actual + } && + test_orphan_error -C wt branch $1 $2 && # implicit branch + test_orphan_error -C wt branch $1 orphan-branch $2 && # explicit branch + test_orphan_error branch $1 orphan-branch $2 # different worktree + } && + test_branch_op_in_wt --edit-description && + test_branch_op_in_wt --set-upstream-to=ne && + test_branch_op_in_wt -c new-branch +' + test_done From a675ad1708383f47883d0bb0b2dada827a295acd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Justo?= Date: Mon, 27 Mar 2023 00:33:27 +0200 Subject: [PATCH 4/5] branch: rename orphan branches in any worktree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In cfaff3aac (branch -m: allow renaming a yet-unborn branch, 2020-12-13) we added support for renaming an orphan branch when that branch is checked out in the current worktree. Let's also allow renaming an orphan branch checked out in a worktree different than the current one. Signed-off-by: Rubén Justo Signed-off-by: Junio C Hamano --- builtin/branch.c | 6 ++++-- t/t3200-branch.sh | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index 90dcbb0c6e..a93b9fc0ab 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -539,6 +539,7 @@ static int replace_each_worktree_head_symref(struct worktree **worktrees, } #define IS_HEAD 1 +#define IS_ORPHAN 2 static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force) { @@ -565,6 +566,8 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int if (wt->head_ref && !strcmp(oldref.buf, wt->head_ref)) { oldref_usage |= IS_HEAD; + if (is_null_oid(&wt->head_oid)) + oldref_usage |= IS_ORPHAN; break; } } @@ -599,8 +602,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 && !(oldref_usage & IS_ORPHAN) && 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/t3200-branch.sh b/t/t3200-branch.sh index 6b8b59f15e..a0a3d0cb24 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -294,6 +294,20 @@ test_expect_success 'git branch -M and -C fail on detached HEAD' ' test_cmp expect err ' +test_expect_success 'git branch -m should work with orphan branches' ' + test_when_finished git checkout - && + test_when_finished git worktree remove -f wt && + git worktree add wt --detach && + # rename orphan in another worktreee + git -C wt checkout --orphan orphan-foo-wt && + git branch -m orphan-foo-wt orphan-bar-wt && + test orphan-bar-wt=$(git -C orphan-worktree branch --show-current) && + # rename orphan in the current worktree + git checkout --orphan orphan-foo && + git branch -m orphan-foo orphan-bar && + test orphan-bar=$(git branch --show-current) +' + test_expect_success 'git branch -d on orphan HEAD (merged)' ' test_when_finished git checkout main && git checkout --orphan orphan && From 3521c6321350cf1432f4772efa09db8bec0aa331 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Justo?= Date: Mon, 27 Mar 2023 00:33:40 +0200 Subject: [PATCH 5/5] branch: avoid unnecessary worktrees traversals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When we rename a branch ref, we need to update any worktree that have its HEAD pointing to the branch ref being renamed, so to make it use the new ref name. If we know in advance that we're renaming a branch that is not currently checked out in any worktree, we can skip this step entirely. Let's do it so. Signed-off-by: Rubén Justo Signed-off-by: Junio C Hamano --- builtin/branch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/branch.c b/builtin/branch.c index a93b9fc0ab..6a564b22b0 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -617,7 +617,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int interpreted_oldname); } - if (!copy && + if (!copy && (oldref_usage & IS_HEAD) && replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf, logmsg.buf)) die(_("Branch renamed to %s, but HEAD is not updated!"), newname);