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

[Junio C Hamano <gitster@pobox.com>]

Grant Moyer <dev@grantmoyer.com> writes:

> 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:
>
>> /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.
>
> Signed-off-by: Grant Moyer <dev@grantmoyer.com>
> Tested-by: Michele Locati <michele@locati.it>
> Co-authored-by: Michele Locati <michele@locati.it>
> ---
>  git-filter-branch.sh     |  4 +++-
>  t/t7003-filter-branch.sh | 24 +++++++++++++++++++++++-
>  2 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index 24fa317aaa..9aa07be6e1 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -302,7 +302,9 @@ then
>  		do
>  			case "$line" in
>  			*:*)
> -				echo "${line%:*}" >../map/"${line#*:}";;
> +				from_commit=${line%:*}
> +				to_commit=${line#*:}
> +				echo "$to_commit" >../map/"$from_commit";;
>  			*)
>  				die "Unable to load state from $state_branch:filter.map";;
>  			esac

I very much appreciate the clear description in the proposed log
message above that makes what this change corrects very easy to
understand.

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

I am not sure what this change is about.  Care to explain in the
commit log?

> +test_expect_success '--state-branch incremental rewrite uses the rewritten parents' '

It is a minor nit, but do we want to have

	test_when_finished "rm -fr incremental" &&

here, before creating a new repository that is used only for this
test, or do we expect that in the future we will add more pieces of
tests that work inside this new repository (in which case leaving it
there may indeed be a better choice)?

> +	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
> +	)
> +'
> +
>  git tag oldD HEAD~4
>  test_expect_success 'rewrite one branch, keeping a side branch' '
>  	git branch modD oldD &&

Thanks.