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

From: Thomas Rast <trast@student.ethz.ch>
Date: Oct 22, 2009, 08:05 UTC
Message-ID: <200910221005.11813.trast@student.ethz.ch>
In-Reply-To: <4ADFF66F.1080005@viscovery.net>

Johannes Sixt wrote:

Show 11 quoted lines

> Thomas Rast schrieb:
> > Handling $filter_subdir in the usual way requires a separate case at
> > every use, because the variable is empty when unused.  Furthermore,
> > the case for --subdirectory-filter supplies its own --, so the user
> > cannot provide one himself (though there is also very little point in
> > doing so).
> 
> I understand that this is a preparatory patch, but you seem to argue that
> even without the follow-up patch there is a problem. But from your
> explanation I do not understand what it is. An example invocation that
> shows the problem would be very helpful.

Well, I just observed while writing the patch that you cannot say

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

because the --subdirectory-filter supplies its own -- to the rev-list invocation, i.e., it calls

  git rev-list --all -- subdir/file -- subdir

which filters for a file called --.

I doubt anyone ever needed this operation though, and it can easily be done in two separate filtering steps.

Show 12 quoted lines

> > @@ -257,15 +257,29 @@ git read-tree || die "Could not seed the index"
> >  # map old->new commit ids for rewriting parents
> >  mkdir ../map || die "Could not create map/ directory"
> >  
> > +non_ref_args=$(git rev-parse --no-revs --sq "$@")
> > +dashdash=--
> > +for arg in "$non_ref_args"; do
> 
> At this point $non_ref_args might contain one or more IFS-separatable
> words, but if you say "$non_ref_args" here, this loop will be entered
> exactly once. But even if you drop the dquotes, the --sq quoting that you
> requested from rev-parse bought you nothing.

Hrm. Ok, so the ".." were clearly in mistake, but why could I remove the --sq? Doesn't the shell expand the arguments provided by $non_ref_args if I use it without quotes nor --sq, so that it might accidentally expand paths or such?

Show 5 quoted lines

> > +	if test arg = --; then
> 
> Did you mean $arg here? Even then this test will succeed only if
> $non_ref_args contains exactly one word and that word is '--'. Is that
> what you mean?

No, it should find a -- argument to see if we need to supply our own before the $filter_subdir.

> This looks so convoluted; there must be a simpler way to achieve your goal.

I'll try with more 'case's later... maybe I can at least avoid the eval.

Thanks for your comments!

-- 
Thomas Rast
trast@{inf,student}.ethz.ch