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

From: Johannes Sixt <j.sixt@viscovery.net>
Date: Nov 11, 2009, 08:30 UTC
Message-ID: <4AFA7624.5040400@viscovery.net>
In-Reply-To: <0280836a32983c848bbb0e3b441be256d3c8f4fa.1257885121.git.trast@student.ethz.ch>

Thomas Rast schrieb:

Show 10 quoted lines

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

Show 5 quoted lines

> 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