Re: [PATCH 1/4] branch: add branch_checked_out() helper

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Jun 14 2022, Derrick Stolee wrote:

> On 6/13/2022 7:59 PM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Wed, Jun 08 2022, Derrick Stolee via GitGitGadget wrote:
>> 
>>> From: Derrick Stolee <derrickstolee@github.com>
>>> [...]
>>> +	path_in_set = strmap_get(&current_checked_out_branches, refname);
>>> +	if (path_in_set && path)
>>> +		*path = xstrdup(path_in_set);
>> 
>> Looking at the end state of this series there is nothing that passes a
>> null "path" to this function, i.e. it's all &some_variable.
>> 
>> So just dropping that && seems better from a code understanding &
>> analysis perspective.
>
> I don't see the value in making this method more prone to error in
> the future, or less flexible to a possible caller that doesn't want
> to give a 'path'.

I think creating wrappers for exsiting APIs when they're not really
needed is what's more error prone. I.e. you need to learn to use the new
thing, whereas if it returns an existing type like "struct strmap",
"struct strbuf" etc. there's no learning curve.

It doesn't matter *that much* in this case, but I think it really adds
up.

>> More generally, can't we just expose an API that gives us the strmap
>> itself, so that callers don't need to keep any special-behavior in mind?
>> I.e. just "make me a strmap of checked out branches".
>> 
>> Then you can just use strmap_get(), or xstrdup(strmap_get()), and if the
>> pattern of "get me the item but duped" is found elsewhere maybe we
>> should eventually have a strmap_get_dup() or whatever.
>> 
>> I.e. just making it: xstrdup(strmap_get(checked_out_branches_strmap(), refname)).
>
> This seems unnecessarily complicated. It also risks someone inserting
> key-value pairs in an improper place instead of being contained to
> prepare_checked_out_branches().

I really don't think the internal APIs in git need to be this paranoid,
just document it as "don't touch this".

I think in practice we have (and can expect) pretty mmuch zero bugs due
to misuse of APIs that are clearly meant to be global singletons,
whereas as this series & the fix-up shows it's more likely that we'll
need to deal with subtle memory leaks etc. due to over-wrapping in new
APIS :)

Or, if it does need paranoia simply structure the API to have each
caller get its own copy, here all the callers are one-shot built-ins, so
isn't this a premature optimization?

And speaking of risking things, probably overly fragile as an underlying
"general" API, if that's what you're aiming for.

I.e. you're making the hard assumption that the set of checked out
branches aren't changing during the lifetime of the process. So
e.g. builtin/checkout.c couldn't call this at the end of its run if
something in it had needed to get the map earlier (before we switched
branches)...

> If your concern is about creating the static current_checked_out_branches,
> keep in mind that the callers that call branch_checked_out() in a loop
> would need to keep track of that strmap across several other methods.
> That additional complexity is much worse than asking for a simple answer
> through the black box of branch_checked_out().

...that being said I was not suggesting to do away with the static
pattern, but to keep it, and to just return the map.

As far as hypothetical hot loops are concerned that would be marginally
cheaper, as you could move the "get map" outside a loop in such a
caller.

I tried this fix-up on top of "seen", which passes your test, and as
noted allocates less memory:

diff --git a/branch.c b/branch.c
index c7896f69a7c..efe1f6ce865 100644
--- a/branch.c
+++ b/branch.c
@@ -373,14 +373,13 @@ int validate_branchname(const char *name, struct strbuf *ref)
 static int initialized_checked_out_branches;
 static struct strmap current_checked_out_branches = STRMAP_INIT;
 
-static void prepare_checked_out_branches(void)
+struct strmap *checked_out_branches_map(void)
 {
 	int i = 0;
 	struct worktree **worktrees;
 
 	if (initialized_checked_out_branches)
-		return;
-	initialized_checked_out_branches = 1;
+		goto done;
 
 	worktrees = get_worktrees();
 
@@ -426,18 +425,10 @@ static void prepare_checked_out_branches(void)
 	}
 
 	free_worktrees(worktrees);
-}
 
-int branch_checked_out(const char *refname, char **path)
-{
-	const char *path_in_set;
-	prepare_checked_out_branches();
-
-	path_in_set = strmap_get(&current_checked_out_branches, refname);
-	if (path_in_set && path)
-		*path = xstrdup(path_in_set);
-
-	return !!path_in_set;
+done:
+	initialized_checked_out_branches = 1;
+	return &current_checked_out_branches;
 }
 
 /*
@@ -456,7 +447,8 @@ int validate_new_branchname(const char *name, struct strbuf *ref, int force)
 		die(_("a branch named '%s' already exists"),
 		    ref->buf + strlen("refs/heads/"));
 
-	if (branch_checked_out(ref->buf, &path))
+	path = strmap_get(checked_out_branches_map() , ref->buf);
+	if (path)
 		die(_("cannot force update the branch '%s' "
 		      "checked out at '%s'"),
 		    ref->buf + strlen("refs/heads/"), path);
diff --git a/branch.h b/branch.h
index 5ea93d217b1..63cb0f7adc4 100644
--- a/branch.h
+++ b/branch.h
@@ -103,11 +103,12 @@ void create_branches_recursively(struct repository *r, const char *name,
 				 int dry_run);
 
 /*
- * Returns true if the branch at 'refname' is checked out at any
- * non-bare worktree. The path of the worktree is stored in the
- * given 'path', if provided.
+ * Lazily returns a "struct strmap" of checked out branches. The
+ * returned value is a global that'll be populated on the first
+ * call. You should only call read-only strmap API functions on this,
+ * such as strmap_get(), strmap_contains() etc.
  */
-int branch_checked_out(const char *refname, char **path);
+struct strmap *checked_out_branches_map(void);
 
 /*
  * Check if 'name' can be a valid name for a branch; die otherwise.
diff --git a/builtin/branch.c b/builtin/branch.c
index 8e11e433840..2913b2e75e4 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -24,6 +24,7 @@
 #include "worktree.h"
 #include "help.h"
 #include "commit-reach.h"
+#include "strmap.h"
 
 static const char * const builtin_branch_usage[] = {
 	N_("git branch [<options>] [-r | -a] [--merged] [--no-merged]"),
@@ -253,12 +254,12 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		name = mkpathdup(fmt, bname.buf);
 
 		if (kinds == FILTER_REFS_BRANCHES) {
-			char *path;
-			if (branch_checked_out(name, &path)) {
+			const char *path = strmap_get(checked_out_branches_map(),
+						      name);
+			if (path) {
 				error(_("Cannot delete branch '%s' "
 					"checked out at '%s'"),
 				      bname.buf, path);
-				free(path);
 				ret = 1;
 				continue;
 			}
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 9ee7f2241ad..a57506a4003 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -30,6 +30,7 @@
 #include "shallow.h"
 #include "worktree.h"
 #include "bundle-uri.h"
+#include "strmap.h"
 
 #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)
 
@@ -890,7 +891,6 @@ static int update_local_ref(struct ref *ref,
 			    struct worktree **worktrees)
 {
 	struct commit *current = NULL, *updated;
-	char *path = NULL;
 	const char *pretty_ref = prettify_refname(ref->name);
 	int fast_forward = 0;
 
@@ -905,17 +905,17 @@ static int update_local_ref(struct ref *ref,
 	}
 
 	if (!update_head_ok &&
-	    !is_null_oid(&ref->old_oid) &&
-	    branch_checked_out(ref->name, &path)) {
+	    !is_null_oid(&ref->old_oid)) {
+		int in_set = strmap_contains(checked_out_branches_map(),
+					     ref->name);
 		/*
 		 * If this is the head, and it's not okay to update
 		 * the head, and the old value of the head isn't empty...
 		 */
 		format_display(display, '!', _("[rejected]"),
-			       path ? _("can't fetch in current branch") :
+			       in_set ? _("can't fetch in current branch") :
 				      _("checked out in another worktree"),
 			       remote, pretty_ref, summary_width);
-		free(path);
 		return 1;
 	}
 
@@ -1445,7 +1445,8 @@ static void check_not_current_branch(struct ref *ref_map)
 	for (; ref_map; ref_map = ref_map->next)
 		if (ref_map->peer_ref &&
 		    starts_with(ref_map->peer_ref->name, "refs/heads/") &&
-		    branch_checked_out(ref_map->peer_ref->name, &path))
+		    (path = strmap_get(checked_out_branches_map(),
+				       ref_map->peer_ref->name)))
 			die(_("refusing to fetch into branch '%s' "
 			      "checked out at '%s'"),
 			    ref_map->peer_ref->name, path);