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

On Wed, Sep 30, 2026 at 11:48:25AM -0400, Grant Moyer wrote:

On September 30, 2026 10:40:06 AM EDT, Patrick Steinhardt [off-list ref] wrote:

quoted

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.

quoted

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

Yeah, the test for this case was broken.

quoted

quoted

 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.

The thing that I'm worried about is that the test seems to only
coincidentally exercise the code. I think it may be helpful to have a
test that explicitly tests for the scenarios that were reported as
broken by Michele. Let's maybe wait for them to respond -- if we're
lucky they already have a test we can reuse here that more directly
exercises a couple of the reportedly-broken scenarios.

quoted

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?

It depends. For a small fix like this the maintainer may make the change
themselves if nothing else needs to be changed. But Junio will often
tell you in that case, so if he stays quiet without the series getting
merged down, then I'd eventually just send a new version. Also helps to
bump up the patch series on the mailing list again :)

Thanks!

Patrick