Re: [PATCH v19 5/7] branch: add --delete-merged <branch>
From: Junio C Hamano <hidden>
Date: 2026-07-19 21:43:00
"Harald Nordgren via GitGitGadget" [off-list ref] writes:
quoted hunk ↗ jump to hunk
diff --git a/builtin/branch.c b/builtin/branch.c... +struct spare_data { + struct strset *deletable; + struct strset *spared; +};
Let me offer a brief comment on the data representation chosen for this design, which initially left me confused enough to suspect a bug or two. It turns out the confusion was entirely mine, and I have since convinced myself that the approach is sound.
+/*
+ * A surviving branch stacked on a deletion candidate would lose its
+ * upstream, so drop that candidate from the delete set and remember it
+ * in "spared" so its own upstream can be tidied up afterwards.
+ */
+static int spare_stacked_base(const struct reference *ref, void *cb_data)
+{
+ struct spare_data *data = cb_data;
+ struct branch *branch;
+ const char *upstream, *up_short;
+
+ if (strset_contains(data->deletable, ref->name))
+ return 0;
+ branch = branch_get(ref->name);Here, spare_stacked_base() is a callback triggered by the refs_for_each_branch_ref() iterator. I initially misremembered what the for-each-ref family of iterators passes to its callbacks. I thought 'ref->name' here would be a full refname, such as 'refs/heads/main', which does not match what 'branch_get' expects (which is a branch name). The same confusion led me to think the 'deletable' strset was indexed by full refnames, which would then ...
+ upstream = branch_get_upstream(branch, NULL); + if (!upstream || !skip_prefix(upstream, "refs/heads/", &up_short) || + !strset_contains(data->deletable, up_short)) + return 0;
... mean that this lookup using 'up_short' (the branch name obtained after stripping the 'refs/heads/' prefix) is buggy. But that is not the case. The 'deletable' strset stores branch names, so indexing with 'up_short' is correct, and ...
+ strset_remove(data->deletable, up_short); + strset_add(data->spared, up_short);
... adding 'up_short' to the 'deletable' strset is correct too. The same applies to the 'spared' strset. It is consistently indexed by branch names rather than full refnames. This design choice makes perfect sense for this application. We have no business touching the upstream of a branch unless it is a local branch. A remote-tracking branch, such as 'refs/remotes/origin/main', lives outside the 'refs/heads/' hierarchy. Such a branch has no need to interact with either the 'deletable' or 'spared' tables. I hope others will not be as easily confused as I was, but just in case it helps future readers ... Thanks.