Re: [PATCH v14 4/6] branch: add --prune-merged <branch>

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

Hi Harald

On 16/06/2026 20:15, Harald Nordgren wrote:
>>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
>>> index 4e7deddc04..27ea1319bb 100755
>>> --- a/t/t3200-branch.sh
>>> +++ b/t/t3200-branch.sh
>>> @@ -1809,4 +1809,205 @@ test_expect_success '--forked requires a value' '
>>>        test_grep "requires a value" err
>>>    '
>>>
>>> +test_expect_success '--prune-merged: setup' '
>>> +     test_create_repo pm-upstream &&
>>
>> The rest of this test would be easier to read if we did
>>
>>          (
>>                  cd pm-upstream &&
>>                  ...
>>          )
>>
>> rather than prefixing every command with "-C pm-upstream"
> 
> I feel like the discussion to nest or not to nest has come up many
> times in other topics as well. I don't feel strongly about either way,
> but I just want to flag that if I change it now, another reviewer
> might ask me to change it back later.
> 
> Should the rules be to nest inside of setup functions (and helpers?)
> but not inside the actual tests?

I think it depends on how many commands you're running in a row in the 
same directory. In this case we're running quite a few commands so it 
seems clearer to use a subshell. In the later tests we're switching 
between repositories and running fewer commands in each one so it is 
less clear that using subshells is clearer. Also later on we're using 
test_config() which I don't think works in a subshell because it relies 
on test_when_finished().

One thing I've just thought of related to this patch is whether we want 
to protect branches that are the upstreams of branches that are not 
slated for deletion. With stacked branches it is possible that a branch 
has been merged but has other branches stacked on top of it that have 
not been merged. If we build an strset of branches that we want to 
delete, then loop over all branches and if there are any that are not in 
the to be deleted set which have their upstream in that set we'd remove 
the upstream branch from the set. Once we've done that we can convert 
the set to an strvec to pass to delete_branches()

Thanks

Phillip

> 
>>> +     test_commit -C pm-upstream base &&
>>> +     git -C pm-upstream checkout -b next &&
>>> +     test_commit -C pm-upstream one-commit &&
>>> +     test_commit -C pm-upstream two-commit &&
>>> +     git -C pm-upstream branch one HEAD~ &&
>>> +     git -C pm-upstream branch two HEAD &&
>>> +     git -C pm-upstream branch wip main &&
>>> +     git -C pm-upstream checkout main &&
>>> +     test_create_repo pm-fork
>>> +'
>>> +
>>> +test_expect_success '--prune-merged deletes branches integrated into upstream' '
>>> +     test_when_finished "rm -rf pm-merged" &&
>>> +     git clone pm-upstream pm-merged &&
>>> +     git -C pm-merged remote add fork ../pm-fork &&
>>> +     test_config -C pm-merged remote.pushDefault fork &&
>>> +     test_config -C pm-merged push.default current &&
>>
>> So we clone upstream and add fork as the default push remote. I find the
>> pm- prefixes rather distracting. It would be clearer to me if we just
>> called the repositories "upstream", "fork" and "repo"
> 
> Good point.
> 
>>> +     test_must_fail git -C pm-local rev-parse --verify refs/heads/one
>>> +'
>>> +
>>> +test_expect_success '--prune-merged warns instead of erroring on un-integrated commits' '
>>> +     test_when_finished "rm -rf pm-unmerged" &&
>>> +     git clone pm-upstream pm-unmerged &&
>>> +     git -C pm-unmerged remote add fork ../pm-fork &&
>>> +     test_config -C pm-unmerged remote.pushDefault fork &&
>>> +     test_config -C pm-unmerged push.default current &&
>>> +     git -C pm-unmerged checkout -b wip origin/wip &&
>>> +     git -C pm-unmerged branch --set-upstream-to=origin/next wip &&
>>> +     test_commit -C pm-unmerged local-only &&
>>> +     git -C pm-unmerged checkout - &&
>>> +
>>> +     git -C pm-unmerged branch --prune-merged "origin/*" 2>err &&
>>> +     test_grep "not fully merged" err &&
>>> +     test_grep ! "If you are sure you want to delete it" err &&
>>
>> I'm always suspicious of test_grep when we know what the output should
>> look like - it might be better to use test_cmp. This test does not check
>> that we also delete branches that are merged when we see one that isn't.
>>
>> I'm going to stop here - the tests I've read seem to me to be too much
>> like unit tests checking one aspect of the implementation in isolation
>> rather than checking that the whole feature works as expected.
> 
> I'll respond to the rest here: Excellent points regarding the testing
> aboce, I will take a look at doing this.
> 
> 
> Harald
>