On 07-feb-2023 09:33:39, Ævar Arnfjörð Bjarmason wrote:
Thank you reviewing this.
>
> On Tue, Feb 07 2023, Rubén Justo wrote:
>
> > Avoid some confusing errors operating with orphan branches when
> > working with worktrees.
> >
> > Changes from v2:
> >
> > - Renamed "ishead_and_reject_rebase_or_bisect_branch()" to
> > "die_if_branch_is_being_rebased_or_bisected()"
>
> Looking this over holistically, I think this is a great example of where
> factoring something out into a function is just making readbility
> worse. This function is only used in copy_or_rename_branch(), and the
> overloaded name & semantics are making things quite confusing.
>
> Whereas if we just start by pulling it into its only caller I think this
> gets much better, at the end of this message is a diff-on-top these
> three patches where I do that (I kept the "target" variable to minimize
> the diff with the move detection, but we probalby want the strbuf
> directly instead).
I'm sorry, but I don't agree. copy_or_rename_branch() already does some
heterogeneous things. IMHO, making it also iterate worktrees makes
things worse, in terms of readability. I could agree with maybe the
function die_if_branch_is_being_rebased_or_bisected() could be part of a
more general use, maybe something in worktree.h.
> > A proposed name "die_if_branch_is_is_use()" has not been used because
> > it could lead to confusion. We don't yet support copying or renaming
> > a branch being rebased or bisected, but we do under other uses.
>
> Another thing that I think could be improved in this series is if you
> skip the refactoring-while-at-it of changing the existing
> "if/if/die/die" into a "if/die/?:".
I'm sorry, but I don't agree with this reasoning either. The ternary op
here allows the code to be more clear, IMHO, in the intention: there is
no conditional die() or error(), the conditional is for the message.
>
> In the below diff I have that proposed change on top, but this snippet
> here shows the diff to "origin/master":
>
> @@ -806,7 +806,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);
> @@ -851,10 +851,11 @@ 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);
> }
>
> I.e. your refactoring of this in 2/3 turns out to in the end have just
> been inflating the code change, for no functional benefit.
>
> I wouldn't mind if this were in some pre-cleanup, or if it actually made
> the code easier to read, but IMO this pattern of using a ternary to
> select the format to "error" or "die" makes things worse for
> readability. It's a few bytes less code, but makes things harder to follow overall.
>
> And even if you disagree with that as far as the end state is concerned,
> I think it's unarguable that it makes the 2/3 harder to follow, since
> it's sticking a refactoring that's not neede dfor the end-goal here into
> an otherwise functional change.
>
> I'm aware that some of the code in the context uses this pattern, and
> you probably changed the "if" block you modified to be consistent with
> the code above, but I think in this case it's better not to follow the
> existing style (which is used in that function, but is a rare exception
> overall in this codebase).
>
> The diff-on-top, mentioned above:
>
> == BEGIN
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 7efda622241..dc7a3e3dde1 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -486,45 +486,16 @@ static void print_current_branch_name(void)
> die(_("HEAD (%s) points outside of refs/heads/"), refname);
> }
>
> -/*
> - * Dies if the specified branch is being rebased or bisected. Otherwise returns
> - * 0 or, if the branch is HEAD in any worktree, returns 1. If the branch is HEAD
> - * and also orphan, returns 2.
> - */
> -static int die_if_branch_is_being_rebased_or_bisected(const char *target)
> -{
> - struct worktree **worktrees = get_worktrees();
> - int i, ret = 0;
> -
> - for (i = 0; worktrees[i]; i++) {
> - struct worktree *wt = worktrees[i];
> -
> - if (wt->head_ref && !strcmp(target, wt->head_ref))
> - ret = is_null_oid(&wt->head_oid) ? 2 : 1;
> -
> - if (!wt->is_detached)
> - continue;
> -
> - if (is_worktree_being_rebased(wt, target))
> - die(_("Branch %s is being rebased at %s"),
> - target, wt->path);
> -
> - if (is_worktree_being_bisected(wt, target))
> - die(_("Branch %s is being bisected at %s"),
> - target, wt->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;
> struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
> const char *interpreted_oldname = NULL;
> const char *interpreted_newname = NULL;
> - int recovery = 0, oldref_is_head, oldref_is_orphan;
> + int recovery = 0, oldref_is_head = 0, oldref_is_orphan = 0;
> + struct worktree **worktrees;
> + int i;
> + const char *target;
>
> if (strbuf_check_branch_ref(&oldref, oldname)) {
> /*
> @@ -537,8 +508,29 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
> die(_("Invalid branch name: '%s'"), oldname);
> }
>
> - oldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);
> - oldref_is_orphan = (oldref_is_head > 1);
> + worktrees = get_worktrees();
> + target = oldref.buf;
> + for (i = 0; worktrees[i]; i++) {
> + struct worktree *wt = worktrees[i];
> +
> + if (wt->head_ref && !strcmp(target, wt->head_ref)) {
> + oldref_is_head = 1;
> + if (is_null_oid(&wt->head_oid))
> + oldref_is_orphan = 1;
> + }
> +
> + if (!wt->is_detached)
> + continue;
> +
> + if (is_worktree_being_rebased(wt, target))
> + die(_("Branch %s is being rebased at %s"),
> + target, wt->path);
> +
> + if (is_worktree_being_bisected(wt, target))
> + die(_("Branch %s is being bisected at %s"),
> + target, wt->path);
> + }
> + free_worktrees(worktrees);
>
> if ((copy || !oldref_is_head) &&
> (oldref_is_orphan || !ref_exists(oldref.buf)))
> @@ -858,10 +850,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> die(_("no such branch '%s'"), argv[0]);
> }
>
> - if (!ref_exists(branch->refname))
> - die((!argc || branch_checked_out(branch->refname))
> - ? _("No commit on branch '%s' yet.")
> - : _("branch '%s' does not exist"), branch->name);
> + if (!ref_exists(branch->refname)) {
> + if (!argc || branch_checked_out(branch->refname))
> + die(_("No commit on branch '%s' yet."), branch->name);
> + die(_("branch '%s' does not exist"), branch->name);
> + }
> +
>
> dwim_and_setup_tracking(the_repository, branch->name,
> new_upstream, BRANCH_TRACK_OVERRIDE,
>
> == END
>
> P.S. if I were refactoring those ?: for style in that function I'd
> probably go for this on-top. The N_() followed by _() pattern is
> probably overdoing it, but included to show that one way out of this
> sort of thing with i18n is that you can pre-mark the string with N_(),
> then use it with _() to emit the message (right now the code uses
> "copy?" over "copy ?" instead to align them):
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index dc7a3e3dde1..e42f9bc4900 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -805,31 +805,35 @@ 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 || branch_checked_out(branch_ref.buf))
> - ? _("No commit on branch '%s' yet.")
> - : _("No branch named '%s'."),
> - branch_name);
> - else if (!edit_branch_description(branch_name))
> + if (!ref_exists(branch_ref.buf)) {
> + if (!argc || branch_checked_out(branch_ref.buf))
> + error(_("No commit on branch '%s' yet."),
> + branch_name);
> + else
> + error(_("No branch named '%s'."), branch_name);
> + } else if (!edit_branch_description(branch_name)) {
> ret = 0; /* happy */
> + }
>
> strbuf_release(&branch_ref);
> strbuf_release(&buf);
>
> return ret;
> } else if (copy || rename) {
> + static const char *cannot_copy = N_("cannot copy the current branch while not on any.");
> + static const char *cannot_rename = N_("cannot rename the current branch while not on any.");
> if (!argc)
> die(_("branch name required"));
> else if ((argc == 1) && filter.detached)
> - die(copy? _("cannot copy the current branch while not on any.")
> - : _("cannot rename the current branch while not on any."));
> + die("%s", copy ? _(cannot_copy) : _(cannot_rename));
> else if (argc == 1)
> copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
> else if (argc == 2)
> copy_or_rename_branch(argv[0], argv[1], copy, copy + rename > 1);
> + else if (copy)
> + die(_("too many branches for a copy operation"));
> else
> - die(copy? _("too many branches for a copy operation")
> - : _("too many arguments for a rename operation"));
> + die(_("too many arguments for a rename operation"));
> } else if (new_upstream) {
> struct branch *branch;
> struct strbuf buf = STRBUF_INIT;
>
