Thread (224 messages) 224 messages, 6 authors, 5h ago

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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help