quoted

      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.

Makes sense, I updated it now and it seems to work fine.

quoted

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.

I want to avoid comments because of the maintenance burden they
introduce, I think 'create_merged_branch' will be a very clear name.
Thanks for that suggestion!

quoted

+     (
+             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.

That's a very good point. I wanted to keep it "vanilla" since I worked
a lot on upstreams and tracking branches in my other topics and didn't
want it to bleed over, but yes it's much nicer.

quoted

+             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?

I'll hardcode for now.

quoted

+             git fetch origin &&

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

quoted

+             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?

Nope, let's rip it out.

quoted

+             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?

Yes, but it's also a pattern that I copy-pasted a few too many times,
see my other comment where some of these can be removed.

quoted

+     ) &&
+     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.

Agreed, but other reviewers were against subshells before. I updated
it now and looks much cleaner.

quoted

+     (
+             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.

It's to have an indirection of a branch that is the same as main but
will be protected. I tried to delete it now and replace it with just
main, but then main was deleted and subsequent tests failed.

quoted

+             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

quoted

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

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

Deleted.

quoted

+             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?

That's a good point and a good regression test later on.

quoted

+
+     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.

I'll rename to 'local-to-delete' to clarify and to avoid the comment.

quoted

+'
+
+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")

Yup, should be fixed now. And I usually run "git rebase --keep-base
--exec" a lot!

quoted

+     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.

Yes, it's better.

quoted

+     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.

Nice.

quoted

+'
+
+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
first was numbered 1, rather than the tip of the stack.

quoted

+     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.

Deleting this one, which also removes the question of b1, b2 and b3 naming.

quoted

+
+     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

Very good idea! Created the helper!

quoted

+     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.

Will update! Thanks again!


Harald