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

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 1, 2026, 05:36 UTC
Message-ID: <ar3xSurCd0-yDruA@pks.im>
In-Reply-To: <20261001012347.3998801-1-dev@grantmoyer.com>

On Wed, Sep 30, 2026 at 09:23:47PM -0400, Grant Moyer wrote:

Show 7 quoted lines

> The commit map dir is populated from the state branch assuming a
> "to_commit:from_commit" format, but the state branch is written with a
> "from_commit:to_commit" format, resulting in an inverted mapping when the
> map is populated from the state branch. This is especially evident when
> --prune-empty is used and creates commits which map to nothing; when the
> map dir is populated from this state on subsequent runs, git-filter-branch
> outputs many errors while trying to create files with empty names, like:

This still doesn't mention the commit that introduced the regression.

Show 5 quoted lines

> > /usr/lib/git-core/git-filter-branch: line 305: ../map/: Is a directory
> 
> This change corrects the population of the commit map dir to match the
> "from_commit:to_commit" format and adds/updates tests to check that the
> state branch is written correctly.

Nit: we typically write commit messages in imperative mood, as if you
were instructing the code to change.

> Signed-off-by: Grant Moyer <dev@grantmoyer.com>
> Tested-by: Michele Locati <michele@locati.it>
> Co-authored-by: Michele Locati <michele@locati.it>

Your Signed-off-by should always go last because you're signing off on everything that comes before it.

How about this message:

  The "--state-branch" option asks git-filter-branch(1) to write a
  mapping from old to new objects into the branch. This map is a
  simple blob stored in "$state_branch:filter.map" with the format
  "$to_commit:$from_commit".

  In f6d855091e (filter-branch: stop depending on Perl, 2025-04-16), we
  have refactored git-filter-branch(1) to no longer require Perl. But as
  part of that change we accidentally started to interpret the above
  format in reverse when populating the map directory. This of course
  breaks incremental rewrites that use the commit map multiple times.
  But we don't seem to  have any tests for this feature, and as a
  consequence we didn't notice the regression.

  Fix this bug by interpreting the mappings in the correct order again.

  Tested-by: Michele Locati <michele@locati.it>
  Co-authored-by: Michele Locati <michele@locati.it>
  Signed-off-by: Grant Moyer <dev@grantmoyer.com>

Show 14 quoted lines

> diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
> index 86011e7b1f..cf225b0f0f 100755
> --- a/t/t7003-filter-branch.sh
> +++ b/t/t7003-filter-branch.sh
> @@ -121,10 +121,32 @@ W=$(git rev-parse HEAD)
>  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
>  '
>  
> +test_expect_success '--state-branch incremental rewrite uses the rewritten parents' '

We could add `test_when_finished rm -rf incremental` here.

Show 20 quoted lines

> +	git init incremental &&
> +	(
> +		cd incremental &&
> +		mkdir sub &&
> +		test_commit first sub/file &&
> +		test_commit outside root-file &&
> +		git filter-branch --state-branch refs/state \
> +			--prune-empty --subdirectory-filter sub -- HEAD &&
> +		rewritten_first=$(git rev-parse HEAD) &&
> +		git reset --hard outside &&
> +		test_commit second sub/file &&
> +		git filter-branch -f --state-branch refs/state \
> +			--prune-empty --subdirectory-filter sub -- outside..HEAD &&
> +		test_cmp_rev $rewritten_first HEAD^ &&
> +		git show refs/state:filter.map >map &&
> +		echo "$(git rev-parse second):$(git rev-parse HEAD)" >expect &&
> +		grep "^$(git rev-parse second):" map >actual &&
> +		test_cmp expect actual
> +	)
> +'

Thanks!

Patrick