Re: [PATCH v3 0/3] branch: operations on orphan branches

["Rubén Justo" <rjusto@gmail.com>]

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