Re: [PATCH v4 1/2] filter-branch: stop special-casing $filter_subdir argument

[Johannes Sixt <j.sixt@viscovery.net>]

Thomas Rast schrieb:
> Johannes Sixt wrote:
>> # we need "--" only if there are no path arguments in $@
>> nonrevs=$(git rev-parse --no-revs "$@") || exit
>> dashdash=${nonrevs+"--"}
>>
>> (including the comment; you can drop the dquotes in the dashdash
>> assignment if you think the result is still readable.)
> 
> Ok.  I was briefly scared that this was bash-specific syntax, but dash
> seems happy so I'll trust your advice.

You shouldn't. I'm wrong ;-)

In the snippet above, dashdash will always be set to "--" because a mere
'+' in the variable expansion only tests whether the variable ('nonrevs')
is unset, but it is always set. Even ${nonrevs:+"--"} is wrong, and your
earlier 'test -z' invocation was the correct way to set dashdash.

But...

I did some testing, and now I have to wonder about the use-case that you
present in the commit message:

> Furthermore, the case for --subdirectory-filter supplies its own --,
> so the user cannot provide one himself, so the following was
> impossible:
>
>   git filter-branch --subdirectory-filter subdir -- --all -- subdir/file

If you try in git.git itself:

  $ git rev-list --simplify-merges --all -- \
	Documentation/git-commit.txt | wc -l
  84
  $ git rev-list --simplify-merges --all -- \
	Documentation/git-commit.txt Documentation | wc -l
  4624

I thought that the intention to give an extra path argument is to reduce
the number of commits that remain in the rewritten history. But by giving
--subdirectory-filter, the path filter actually loosened, and many more
commits are rewritten.

Since your intention to write this patch is actually to implement
--remap-to-ancestor, I suggest that we defer the question whether the
above use-case makes sense, and only rewrite this particular paragraph in
the commit message to point out the real bug:

-----
Furthermore, --subdirectory-filter supplies its own '--', and if the user
provided one himself, such as in

  git filter-branch --subdirectory-filter subdir -- --all -- subdir/file

an extra '--' was used as path filter in the call to git-rev-list that
determines the commits that shall be rewritten.
-----

I'll submit a replacement patch.

-- Hannes