git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Feb 7, 2023, 08:33 UTC
Message-ID
<230207.86cz6l501v.gmgdl@evledraar.gmail.com>
In-Reply-To
<2193a4ed-b263-068e-92f8-847dcb053f8c@gmail.com>
On Tue, Feb 07 2023, Rubén Justo wrote:
Show 7 quoted lines
> 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).

>    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/?:".

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;
Previous: Junio C HamanoNext: Rubén Justo
Message 27 of 49 in “branch: operations on orphan branches”
  1. 0/2 branch: operations on orphan branchesRubén Justo, Dec 30, 2022
  2. 1/2 branch: description for orphan branch errorsRubén Justo, Dec 30, 2022
  3. Junio C HamanoJan 1, 2023
  4. Rubén JustoJan 3, 2023
  5. Junio C HamanoJan 4, 2023
  6. Rubén JustoJan 6, 2023
  7. Junio C HamanoJan 6, 2023
  8. Rubén JustoJan 7, 2023
  9. Junio C HamanoJan 7, 2023
  10. 2/2 branch: rename orphan branches in any worktreeRubén Justo, Dec 30, 2022
  11. 0/3 branch: operations on orphan branchesRubén Justo, Jan 15, 2023
  12. 1/3 avoid unnecessary worktrees traversingRubén Justo, Jan 16, 2023
  13. Junio C HamanoJan 19, 2023
  14. Rubén JustoJan 19, 2023
  15. 2/3 branch: description for orphan branch errorsRubén Justo, Jan 16, 2023
  16. 3/3 branch: rename orphan branches in any worktreeRubén Justo, Jan 16, 2023
  17. Junio C HamanoJan 19, 2023
  18. Rubén JustoJan 19, 2023
  19. 3/3 branch: rename orphan branches in any worktreeRubén Justo, Jan 16, 2023
  20. 0/3 branch: operations on orphan branchesRubén Justo, Feb 6, 2023
  21. 1/3 branch: avoid unnecessary worktrees traversalsRubén Justo, Feb 6, 2023
  22. Jonathan TanFeb 11, 2023
  23. Rubén JustoFeb 15, 2023
  24. 2/3 branch: description for orphan branch errorsRubén Justo, Feb 6, 2023
  25. 3/3 branch: rename orphan branches in any worktreeRubén Justo, Feb 6, 2023
  26. Junio C HamanoFeb 7, 2023
  27. Ævar Arnfjörð BjarmasonFeb 7, 2023
  28. Rubén JustoFeb 8, 2023
  29. Junio C HamanoFeb 8, 2023
  30. 0/3 branch: operations on orphan branchesRubén Justo, Feb 22, 2023
  31. 1/3 branch: avoid unnecessary worktrees traversalsRubén Justo, Feb 22, 2023
  32. Rubén JustoFeb 25, 2023
  33. Jonathan TanFeb 27, 2023
  34. Rubén JustoFeb 28, 2023
  35. 2/3 branch: description for orphan branch errorsRubén Justo, Feb 22, 2023
  36. Jonathan TanFeb 27, 2023
  37. Junio C HamanoFeb 27, 2023
  38. Rubén JustoFeb 28, 2023
  39. 3/3 branch: rename orphan branches in any worktreeRubén Justo, Feb 22, 2023
  40. Jonathan TanFeb 27, 2023
  41. Rubén JustoFeb 28, 2023
  42. 0/5 branch: operations on orphan branchesRubén Justo, Mar 26, 2023
  43. 1/5 branch: test for failures while renaming branchesRubén Justo, Mar 26, 2023
  44. 2/5 branch: use get_worktrees() in copy_or_rename_branch()Rubén Justo, Mar 26, 2023
  45. 3/5 branch: description for orphan branch errorsRubén Justo, Mar 26, 2023
  46. 4/5 branch: rename orphan branches in any worktreeRubén Justo, Mar 26, 2023
  47. 5/5 branch: avoid unnecessary worktrees traversalsRubén Justo, Mar 26, 2023
  48. Junio C HamanoMar 27, 2023
  49. Junio C HamanoMay 1, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.