| Message ID | 76bac570-147e-7c74-c18c-1da88bc3d342@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | bb07ee667c9ae809aac613a4745b28332302c479 |
| Headers | show |
| Series | branch: operations on orphan branches | expand |
Firstly, the subject could be more precise. Maybe "branch: check all worktrees for orphan branches" (47 characters) or something like that. Rubén Justo <rjusto@gmail.com> writes: > In bcfc82bd48 (branch: description for non-existent branch errors, > 2022-10-08) we checked the current HEAD Probably clearer to say "HEAD in the current worktree" instead of "current HEAD". > 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. Probably clearer to just say "But there might be orphan branches in other worktrees". > Let's extend the check we did in bcfc82bd48, to all HEADs in the > repository, using the helper introduced in 31ad6b61bd (branch: add > branch_checked_out() helper, 2022-06-15) s/HEADs/worktrees/ > @@ -493,8 +496,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int > 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(); > + struct worktree *oldref_wt = NULL; Better to have 2 variables (one for rebased, and one for bisected) to avoid the situation in which the last problematic worktree seen was a bisected one, but a prior one was a rebased one. > @@ -818,7 +835,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); > @@ -863,7 +880,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); > } What is the relevance of these changes?
Jonathan Tan <jonathantanmy@google.com> writes: > Firstly, the subject could be more precise. Maybe "branch: check all > worktrees for orphan branches" (47 characters) or something like that. > > Rubén Justo <rjusto@gmail.com> writes: >> In bcfc82bd48 (branch: description for non-existent branch errors, >> 2022-10-08) we checked the current HEAD > > Probably clearer to say "HEAD in the current worktree" instead of > "current HEAD". > >> 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. > > Probably clearer to just say "But there might be orphan branches in > other worktrees". > >> Let's extend the check we did in bcfc82bd48, to all HEADs in the >> repository, using the helper introduced in 31ad6b61bd (branch: add >> branch_checked_out() helper, 2022-06-15) > > s/HEADs/worktrees/ > >> @@ -493,8 +496,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int >> 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(); >> + struct worktree *oldref_wt = NULL; > > Better to have 2 variables (one for rebased, and one for bisected) to > avoid the situation in which the last problematic worktree seen was a > bisected one, but a prior one was a rebased one. All good suggestions. >> @@ -818,7 +835,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); >> @@ -863,7 +880,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); >> } > > What is the relevance of these changes? Thanks for reading very carefully and asking clarification for relevant parts of the changes.
Thank you for your review! On 27/2/23 20:38, Jonathan Tan wrote: > Firstly, the subject could be more precise. Maybe "branch: check all > worktrees for orphan branches" (47 characters) or something like that. The main intention in this series is to stop giving the user a confusing error "No branch named..." for a branch he may have just created. I think the current subject states that better. But I'm open to change it in that direction. > Rubén Justo <rjusto@gmail.com> writes: >> In bcfc82bd48 (branch: description for non-existent branch errors, >> 2022-10-08) we checked the current HEAD > > Probably clearer to say "HEAD in the current worktree" instead of > "current HEAD". OK. I'll reword with that. >> 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. > > Probably clearer to just say "But there might be orphan branches in > other worktrees". That loses important details IMHO, the intention: "avoid the confusing..", and the reasoning on why we need to check HEAD in all worktrees. >> Let's extend the check we did in bcfc82bd48, to all HEADs in the >> repository, using the helper introduced in 31ad6b61bd (branch: add >> branch_checked_out() helper, 2022-06-15) > > s/HEADs/worktrees/ I understand your suggestion, but my intention along the message is to maintain the reasoning on the "HEAD", due to an orphan branch is a HEAD pointing to a non-existing ref. Maybe "the HEADs in all worktrees" could be better? >> @@ -493,8 +496,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int >> 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(); >> + struct worktree *oldref_wt = NULL; > > Better to have 2 variables (one for rebased, and one for bisected) to > avoid the situation in which the last problematic worktree seen was a > bisected one, but a prior one was a rebased one. Well seen. Thanks for reading carefully. I'll re-roll with that. >> @@ -818,7 +835,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); >> @@ -863,7 +880,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); >> } > > What is the relevance of these changes? > This is the main intention in the patch: not showing the confusing error "No branch named..." for orphan branches. I'm not sure if I understand your question...
diff --git a/builtin/branch.c b/builtin/branch.c index a32ae64006..aecf009993 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -486,6 +486,9 @@ static void print_current_branch_name(void) die(_("HEAD (%s) points outside of refs/heads/"), refname); } +#define IS_BISECTED 1 +#define IS_REBASED 2 +#define IS_HEAD 4 static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force) { @@ -493,8 +496,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int 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(); + struct worktree *oldref_wt = NULL; if (strbuf_check_branch_ref(&oldref, oldname)) { /* @@ -507,8 +511,28 @@ 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; + + if (!wt->is_detached) + continue; + + if (is_worktree_being_rebased(wt, oldref.buf)) { + oldref_usage |= IS_REBASED; + oldref_wt = wt; + } + + if (is_worktree_being_bisected(wt, oldref.buf)) { + oldref_usage |= IS_BISECTED; + oldref_wt = wt; + } + } + + 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); @@ -523,20 +547,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int else validate_new_branchname(newname, &newref, force); - for (int i = 0; worktrees[i]; i++) { - struct worktree *wt = worktrees[i]; + if (oldref_usage & IS_BISECTED) + die(_("Branch %s is being rebased at %s"), + oldref.buf, oldref_wt->path); - if (!wt->is_detached) - continue; - - if (is_worktree_being_rebased(wt, oldref.buf)) - die(_("Branch %s is being rebased at %s"), - oldref.buf, wt->path); - - if (is_worktree_being_bisected(wt, oldref.buf)) - die(_("Branch %s is being bisected at %s"), - oldref.buf, wt->path); - } + if (oldref_usage & IS_REBASED) + die(_("Branch %s is being bisected at %s"), + oldref.buf, oldref_wt->path); if (!skip_prefix(oldref.buf, "refs/heads/", &interpreted_oldname) || !skip_prefix(newref.buf, "refs/heads/", &interpreted_newname)) { @@ -818,7 +835,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); @@ -863,7 +880,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
In bcfc82bd48 (branch: description for non-existent branch errors, 2022-10-08) we checked the current HEAD 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 all HEADs in the repository, using the helper introduced in 31ad6b61bd (branch: add branch_checked_out() helper, 2022-06-15) We are already traversing all worktrees in "copy_or_rename_branch()" checking if the branch to be copied or renamed is being bisected or rebased. Let's include also a check for being HEAD, and use this information within the function rather than the helper. This implies doing the worktrees iteration earlier in the function; to keep the user-visible behavior unchanged lets maintain the die("Branch foo is being rebased/bisected...") in the same position within the function. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- builtin/branch.c | 53 ++++++++++++++++++++++++++++-------------- t/t3202-show-branch.sh | 18 ++++++++++++++ 2 files changed, 53 insertions(+), 18 deletions(-)