Re: [PATCH v18 5/7] branch: add --delete-merged <branch>

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Harald

On 24/06/2026 22:55, Harald Nordgren via GitGitGadget wrote:
> From: Harald Nordgren <haraldnordgren@gmail.com>
> 
> 	git branch --delete-merged <branch>...

This design means that unlike --forked there is no way to limit the 
branches considered for deletion. I wonder if we'd be better to have 
--delete-merged take an argument like --forked so that the user can 
limit the branches that might be deleted without resorting to the config 
setting added in the next patch.

> deletes the local branches that "--forked <branch>" would list,
> keeping only those whose tip is reachable from their configured
> upstream. The work has already landed on the upstream they track,
> so the local copy is no longer needed.
> 
> A branch is not deleted when:
> 
>    * it is checked out in any worktree
>    * its upstream remote-tracking branch no longer exists, since a
>      missing upstream is not by itself a sign of integration
>    * its push destination equals its upstream (<branch>@{push} is
>      the same as <branch>@{upstream}), such as a local "main" that
>      tracks and pushes to "origin/main". Right after a pull it just
>      looks "fully merged", so it is kept. Only branches that push
>      somewhere other than their upstream, typically topics in a fork
>      workflow, are candidates.
> 
> A branch whose work is not yet merged into its upstream is silently
> skipped, so one unmerged topic does not abort the whole sweep.
> 
> A branch that another, surviving branch tracks as its upstream is
> also kept, so a branch is never deleted out from under one stacked
> on top of it. Such a kept branch is itself merged, so when its own
> upstream is being deleted, clear its now-stale upstream config.

The commit message explains the new feature really well. The 
implementation looks good, I've left a few questions and comments on the 
tests

> +static void spare_stacked_bases(struct ref_store *refs, struct strset *deletable)
> +{
> [...]
> +		strbuf_addf(&key, "branch.%s.merge", branch->name);
> +		repo_config_set_gently(the_repository, key.buf, NULL);
> +		strbuf_reset(&key);
> +		strbuf_addf(&key, "branch.%s.remote", branch->name);
> +		repo_config_set_gently(the_repository, key.buf, NULL);

If there are any errors updating the config then the config code will 
print a message and we continue. As clearing the config is really a 
convenience feature I think it is fine to ignore errors here.

> +static int delete_merged_branches(int argc, const char **argv,
> +				 unsigned int flags)
> [...]
> +		if (check_branch_commit(short_name, short_name,
> +					&candidates.items[i]->objectname, NULL,
> +					FILTER_REFS_BRANCHES, DELETE_BRANCH_SKIP_UNMERGED))

This check is performed again when we call delete_branches() but we need 
to do it here to prune the branch in order to stop delete_branches() 
printing an error message. The check involves finding a merge-base so it 
is not necessarily cheap - if that becomes a problem in the future we 
can add a flag to delete_branches() to skip the check there.

> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 3104c555f6..047ba54778 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -1839,4 +1839,189 @@ test_expect_success '--forked narrows a <pattern> argument' '
>   	test_cmp expect actual
>   '
>   
> +test_expect_success '--delete-merged: setup' '
> +	git init -b main upstream &&
> +	(
> +		cd upstream &&
> +		test_commit base &&
> +		git checkout -b next &&
> +		test_commit next-work &&
> +		git checkout main
> +	) &&
> +	git init -b main other &&
> +	test_commit -C other other-base &&
> +	git init -b main fork
> +'
> +
> +setup_repo_for_delete_merged () {
> +	rm -rf repo &&
> +	git clone upstream repo &&
> +	(
> +		cd repo &&
> +		git remote add fork ../fork &&
> +		git remote add other ../other &&
> +		git config remote.pushDefault fork &&
> +		git config push.default current &&
> +		git fetch other
> +	)
> +}
> +
> +merged_branch () {

A comment would have helped to explain what this helper does. Also 
calling it create_merged_branch() would be clearer too I think.

> +	(
> +		cd repo &&
> +		git checkout -b "$1" "$2" &&

If we add '--track' we can avoid having to run "git branch 
--set-upstream-to" below. The same goes for many if not all of the 
branches created by "git checkout -b" and "git branch" in these tests.

> +		git commit --allow-empty -m "$1 work" &&
> +		git push origin "$1:next" &&

We let the caller specify the upstream branch, but then always push to 
origin/next - should be be using 'git push ${2%%/*} "$1:${2#*/}"', or if 
we don't need that flexibility hard coding the upstream branch?

> +		git fetch origin &&

We've just pushed, what are we fetching here?

> +		git branch --set-upstream-to="$2" "$1"
> +	)
> +}
> +
> +test_expect_success '--delete-merged deletes merged branches and spares the rest' '
> +	test_when_finished "rm -rf repo" &&

The first thing setup_repo_for_delete does is delete repo so do we need 
this as well?

> +	setup_repo_for_delete_merged &&
> +	merged_branch merged origin/next &&
> +	(
> +		cd repo &&
> +		git checkout -b unmerged origin/next &&
> +		git commit --allow-empty -m "unmerged work" &&

good - we have a branch with upstream origin/next that isn't merged and 
one that is.

> +		git branch --set-upstream-to=origin/next unmerged &&
> +		git checkout -b tracks-other other/main &&
> +		git branch --set-upstream-to=other/main tracks-other &&
> +		git checkout --detach

I assume this is to ensure we don't spare a branch because it is checked 
out?

> +	) &&
> +	sha=$(git -C repo rev-parse --short merged) &&
> +
> +	git -C repo branch --delete-merged origin/next >actual 2>&1 &&
> +
> +	echo "Deleted branch merged (was $sha)." >expect &&

There doesn't seem to be any reason for these command or the ones below 
to be outside the subshell - they're all running commands in "repo". 
That seems to be a common pattern in these tests.

> +	test_cmp expect actual &&

This is good we have two branches with an upstream of origin/next, but 
only one of them is merged. We also check no other branches are deleted.

> +	git -C repo for-each-ref --format="%(refname:short)" refs/heads/ >actual &&
> +	cat >expect <<-\EOF &&
> +	main
> +	tracks-other
> +	unmerged
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success '--delete-merged deletes merged branches and spares protected ones' '
> +	test_when_finished "rm -rf repo" &&
> +	setup_repo_for_delete_merged &&
> +	merged_branch on-next origin/next &&
> +	merged_branch checked-out origin/next &&
> +	merged_branch upstream-gone origin/next &&

Right, we create three branches that are all merged into origin/next

> +	(
> +		cd repo &&
> +		git checkout -b mainline main &&
> +		git checkout -b on-local mainline &&
> +		git branch --set-upstream-to=mainline on-local &&

Why do we need on-local to track mainline rather than main? I'm a bit 
confused what the point of mainline is.

> +		git update-ref refs/remotes/origin/topic refs/remotes/origin/next &&
> +		git branch --set-upstream-to=origin/topic upstream-gone &&
> +		git update-ref -d refs/remotes/origin/topic &&

These three lines can be replaced by

	git config branch.gone.merge does-not-exist

> +		git branch --set-upstream-to=origin/main main &&
> +		git config branch.main.pushRemote origin &&

What does this do? Isn't its pushRemote already origin?

> +		git checkout -b tracks-other other/main &&
> +		git branch --set-upstream-to=other/main tracks-other &&
> +		git checkout checked-out
> +	) &&
> +
> +	git -C repo branch --delete-merged origin/next mainline &&

Do we want to use "origin/*" here instead so that we check that main is 
not deleted because its push destination matches its upstream?

> +
> +	git -C repo for-each-ref --format="%(refname:short)" refs/heads/ >actual &&
> +	cat >expect <<-\EOF &&
> +	checked-out
> +	main
> +	mainline
> +	tracks-other
> +	upstream-gone
> +	EOF
> +	test_cmp expect actual

This checks we delete on-local - good. I wonder if we should add a 
comment about the expected outcome so it is clear to the casual reader 
what is happening.

> +'
> +
> +test_expect_success '--delete-merged requires at least one <branch>' '
> +	test_must_fail git -C forked branch --delete-merged 2>err &&
> +	test_grep "requires at least one <branch>" err
> +'
> +
> +test_expect_success '--delete-merged keeps a branch that is an upstream' '
> +	test_when_finished "rm -rf repo" &&
> +	setup_repo_for_delete_merged &&
> +	merged_branch feature origin/next &&
> +	(
> +		cd repo &&
> +		git checkout -b topic feature &&
> +		git commit --allow-empty -m "topic work" &&
> +		git branch --set-upstream-to=feature topic &&
> +		git checkout --detach
> +	) &&
> +
> +	git -C repo branch --dry-run --delete-merged origin/next >out &&

This belongs in a later patch and shows that the patches in this series 
have not been individually tested (c.f. my previous mail about running 
"git rebase --keep-base --exec")

> +	test_grep ! "feature" out &&
> +
> +	git -C repo branch --delete-merged origin/next 2>err &&
> +
> +	test_must_be_empty err &&

So we don't delete anything because feature is needed by topic

> +	git -C repo rev-parse --verify refs/heads/feature &&
> +	git -C repo rev-parse --verify refs/heads/topic &&

I preferred the way this as checked in the previous tests with 
for-each-ref and test_cmp as that shows everything that was kept.

> +	echo origin/next >expect &&
> +	git -C repo rev-parse --abbrev-ref feature@{upstream} >actual &&
> +	test_cmp expect actual &&
> +	echo feature >expect &&
> +	git -C repo rev-parse --abbrev-ref topic@{upstream} >actual &&
> +	test_cmp expect actual

This is a bit of a faff. Perhaps

     git config --local --get-regexp 
"branch.(feature|topic).(merge|remote)" >actual

followed by test_cmp would be more concise and more clearly show that 
we're interested in checking that the config settings still exist.

> +'
> +
> +test_expect_success '--delete-merged keeps a chain of upstreams of a kept branch' '
> +	test_when_finished "rm -rf repo" &&
> +	setup_repo_for_delete_merged &&
> +	(
> +		cd repo &&
> +		git branch b3 origin/next &&
> +		git branch --set-upstream-to=origin/next b3 &&
> +		git branch b2 origin/next &&
> +		git branch --set-upstream-to=b3 b2 &&
> +		git checkout -b b1 b2 &&
> +		git commit --allow-empty -m "b1 work" &&
> +		git branch --set-upstream-to=b2 b1 &&
> +		git checkout --detach
> +	) &&

I'd find this easier to follow if the base branch which is created 
firest was numbered 1, rather than the tip of the stack.


> +	git -C repo branch --delete-merged origin/next &&

b3 is merged but cannot be deleted because it is the upstream for b2 
which although it is merged into b3 isn't a candidate for deletion 
because its upstream is b3.

I'm not quite sure what this test demonstrates that the next one does not.

> +
> +	git -C repo for-each-ref --format="%(refname:short)" refs/heads/ >actual &&
> +	cat >expect <<-\EOF &&
> +	b1
> +	b2
> +	b3
> +	main
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success '--delete-merged clears the upstream of a kept base whose own base is deleted' '
> +	test_when_finished "rm -rf repo" &&
> +	setup_repo_for_delete_merged &&
> +	(
> +		cd repo &&
> +		git branch lower origin/next &&
> +		git branch --set-upstream-to=origin/next lower &&
> +		git branch mid origin/next &&
> +		git branch --set-upstream-to=lower mid &&
> +		git checkout -b tip mid &&
> +		git commit --allow-empty -m "tip work" &&
> +		git branch --set-upstream-to=mid tip &&
> +		git checkout --detach
> +	) &&
> +
> +	git -C repo branch --delete-merged origin/next lower &&

We expect lower to be deleted, but not mid because although it is merged 
it is the upstream of an unmerged branch. Again it would be nice to 
check that with for-each-ref (maybe that is a common enough pattern to 
justify a helper that takes the expected output on stdin

	check_branches <<-\EOF
	main
	mid
	tip
	EOF

> +	test_must_fail git -C repo rev-parse --verify refs/heads/lower &&
> +	git -C repo rev-parse --verify refs/heads/mid &&
> +	test_must_fail git -C repo rev-parse mid@{upstream} &&
> +	echo mid >expect &&
> +	git -C repo rev-parse --abbrev-ref tip@{upstream} >actual &&
> +	test_cmp expect actual

I'd check the config settings here as suggested for the test above. The 
test coverage looks good, there are just a few places where a comment 
would help explain what's going on and some places where we can save a 
few commands.

Thanks

Phillip

> +'
> +
>   test_done