Re: [PATCH] filter-branch: skip commits present on --state-branch

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

Michael Barabanov <michael.barabanov@gmail.com> writes:

> The commits in state:filter.map have already been processed, so don't
> filter them again. This makes incremental git filter-branch much faster.
>
> Also add tests for --state-branch option.
>
> Signed-off-by: Michael Barabanov <michael.barabanov@gmail.com>
> ---
>  git-filter-branch.sh     |  3 +++
>  t/t7003-filter-branch.sh | 15 +++++++++++++++
>  2 files changed, 18 insertions(+)
>
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index ccceaf19a..2df7ed107 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -372,6 +372,9 @@ while read commit parents; do
>  	git_filter_branch__commit_count=$(($git_filter_branch__commit_count+1))
>  
>  	report_progress
> +	if test -r "$workdir/../map/$commit"; then
> +		continue
> +	fi

The original script is so much of a mess that I needed quite some
time to find enough evidence to convince myself that this change is
in line with what is already happening in the program.  We have

	test -f "$workdir"/../map/$sha1 && continue

in the codepath for remap-to-ancestor prostprocessing to do
pretty-much the same skipping.

I think the new code should follow suit, i.e.

	if test -f "$workdir/../map/$commit"
	then
		continue
	fi

to check just the existence for consistency.

It would have been reviewer friendly if the proposed commit log
message said how this change does *not* break the progress output
and count.  A possible alternative optimization could be not to add
these already mapped commits in ../revs file in the first place (so
they are not even counted as part of $commits), and such a change
would give different meaning to the progress output (which may or
may not be a good change).  Instead, the posted patch counts the
commits to be filtered the same way as before, and merely pretends
that it filtered those commits to their mapped counterparts without
spending any cycle (simply because we already _know_ what they are
mapped to), so the meaning of the numbers in the progress display
does not change at all---just they appear to progress much faster,
which is a welcome change ;-)