Re: [PATCH] fiter-branch: fix commit map init from state branch

[Grant Moyer <dev@grantmoyer.com>]

On September 30, 2026 10:40:06 AM EDT, Patrick Steinhardt <ps@pks.im> wrote:
>Okay. A simpler fix could've been the following diff:
>
>- echo "${line%:*}" >../map/"${line#*:}";;
>+ echo "${line#*:}" >../map/"${line%:*}";;

Yep, that's equivalent and was my first version, but I decided to err on 
the side of clearer intent. Plus, the more verbose version mirrors how 
the state branch is constructed further down in the script.

>So... does this mean that we don't have test coverage for this case at
>all?

Yeah, the test for this case was broken.

>>  test_expect_success 'using --state-branch to skip already rewritten commits' '
>>  test_when_finished git reset --hard $V &&
>>  git reset --hard $V &&
>> - git filter-branch --state-branch state -f --tree-filter "touch file || :" HEAD &&
>> + git filter-branch --state-branch state -f --tree-filter "exit 1" HEAD &&
>>  test_cmp_rev $W HEAD
>>  '
>
>So does this now detect the issue? If so, it feels somewhat roundabout.

The current test seemingly intends to check if the --tree-filter filter 
was run on already processed commits, but it only checks that the filter 
produces the same final result. Since the filter is deterministic, the 
final result is the same whether or not the filter is re-run on already 
processed commits.

The proposed change makes the test fail immediately if the tree-filter
is re-run. I looked around other tests for a test_* command or
conventions to fail with a message, but I didn't find anything. I've
checked that the test fails without the filter-branch change, and passes
with it.

>Nit: pointed out by Michele: the subject has a typo in "fiter-branch".

I'm new here. Is this something I should fix by submitting a new version 
of the patch, or will the maintainer fix it  up if/when they merge the 
patch?


Grant