Re: git filter-branch not removing commits when it should in 2.7.0

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 19, 2016, 21:35 UTC
Message-ID: <xmqq7fj59rs2.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqbn8h9squ.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 43 quoted lines

> John Fultz <jfultz@wolfram.com> writes:
>
>> This seems to be a 2.7.0 regression in filter-branch.  The bug is reproducible on Mac/Windows (haven't tried Linux) in the 2.7.0 production releases.
>>
>> Make an empty repo and put an empty commit in the history.  E.g.,
>>
>> echo > foo && git add . && git commit -m "commit 1" && git commit --allow-empty -m "commit 2"
>>
>> Now try to use filter-branch to remove the empty commit.  Both of the following methods leave master unchanged, but both worked in 2.6.4:
>>
>> git filter-branch --prune-empty
>> git filter-branch --commit-filter 'git_commit_non_empty_tree "$@"'
>
> Thanks.
>
> Since there were only 5 changes to git-filter-branch.sh between
> v2.6.0 and v2.7.0, it was fairly easy to pinpoint.
>
> Reverting the following commit from v2.7.0 seems to give the same
> result as v2.6.0 for "--prune-empty" experiment.
>
> commit 348d4f2fc5d3c4f7ba47079b96676b4e2dd831fc
> Author: Jeff King <peff@peff.net>
> Date:   Fri Nov 6 01:24:29 2015 -0500
>
>     filter-branch: skip index read/write when possible
>     
>     If the user specifies an index filter but not a tree filter,
>     filter-branch cleverly avoids checking out the tree
>     entirely. But we don't do the next level of optimization: if
>     you have no index or tree filter, we do not need to read the
>     index at all.
>     
>     This can greatly speed up cases where we are only changing
>     the commit objects (e.g., cementing a graft into place).
>     Here are numbers from the newly-added perf test:
>     
>       Test                  HEAD^              HEAD
>       ---------------------------------------------------------------
>       7000.2: noop filter   13.81(4.95+0.83)   5.43(0.42+0.43) -60.7%
>     
>     Signed-off-by: Jeff King <peff@peff.net>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>

OK, is this because git_commit_non_empty_tree() does this to decide that it should skip the commit:

git_commit_non_empty_tree()
{
        if test $# = 3 && test "$1" = $(git rev-parse "$3^{tree}");
        then
                map "$3"
        else
                git commit-tree "$@"
        fi
}

where its parameters when --prune-empty is in use (or when the function is used as the commit-filter), $1 is "$tree", $2 and $3 are "-p" and its sole commit object name $commit (which is read from the revs file, which is an output from rev-list, so it is known to be 40-hex) and tree after the said patch is computed like so:

        if test -n "$need_index"
        then
                tree=$(git write-tree)
        else
                tree="$commit^{tree}"
        fi

i.e. the helper does textual comparison between "$FOURTY_HEX^{tree}" and 40-hex from rev-parse "$3^{tree}"?

In other words, would the fix be a one-liner like this?

 git-filter-branch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 98f1779..86b2ff1 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -404,7 +404,7 @@ while read commit parents; do
 	then
 		tree=$(git write-tree)
 	else
-		tree="$commit^{tree}"
+		tree=$(git rev-parse "$commit^{tree}")
 	fi
 	workdir=$workdir @SHELL_PATH@ -c "$filter_commit" "git commit-tree" \
 		"$tree" $parentstr < ../message > ../map/$commit ||