Re: [PATCH 4/4] cat-file: support --textconv/--filters in batch mode

From: Jeff King <peff@peff.net>
Date: Aug 19, 2016, 13:22 UTC
Message-ID: <20160819132213.njhymlc5diigonlj@sigill.intra.peff.net>
In-Reply-To: <alpine.DEB.2.20.1608191500470.4924@virtualbox>

On Fri, Aug 19, 2016 at 03:09:19PM +0200, Johannes Schindelin wrote:

Show 17 quoted lines

> > The object name can have spaces in it, too. E.g.:
> > 
> >   HEAD:path with spaces
> > 
> > or even:
> > 
> >   :/grep for this
> > 
> > (as was pointed out to me when I tried to turn on %(rest) handling by
> > default, long ago). How do those work with your patch?
> 
> They don't ;-)
> 
> And quite frankly, the documentation should make it clear to users that
> batch mode with --filters or --textconv won't work when the object name
> contains white space: it says that the object name is split from the path
> at the first white space character.

I think that is an obvious implication of the new documentation. I was just concerned with a regression from the previous behavior.

Show 6 quoted lines

> > It looks like the extra split isn't enabled unless one of those options
> > is selected. Since --filters is new, that's OK for backwards
> > compatibility. But --textconv isn't.
> 
> Except that it is okay, because --textconv *was not even supported in
> batch mode*. So there is no backwards compatibility that could be broken.

Ah, OK. I thought we handled "HEAD:path with spaces" there, but I see that you cannot even specify "--textconv" with "--batch", because it complains of the cmdmode.

So the new rule becomes "we split if we see %(rest), or --textconv, or --filter", which is reasonable.

> Fixing %(rest) for object names containing spaces is distinctly outside
> the original intent, and certainly outside of my use case.

Yeah, I agree it is not necessary for this series (I was only considering it as an option to fix the regression I now see doesn't exist).

-Peff