Re: [PATCH RFC 05/20] cat-file: remove split_on_whitespace
- From
Jeff King <peff@peff.net>
- Date
- Feb 28, 2019, 21:22 UTC
- Message-ID
- <20190228212222.GE12723@sigill.intra.peff.net>
- In-Reply-To
- <0102016915f49a4b-b346412b-752e-4068-8a25-62cac2a1f555-000000@eu-west-1.amazonses.com>
On Fri, Feb 22, 2019 at 04:05:45PM +0000, Olga Telezhnaya wrote:
> Get rid of split_on_whitespace field in struct expand_data. > expand_data may be global further as we use it in ref-filter also, > so we need to remove cat-file specific fields from it.
OK, that makes some sense.
Show 14 quoted lines
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index e5de596611800..60f3839b06f8c 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -203,13 +203,6 @@ struct expand_data {
> */
> int mark_query;
>
> - /*
> - * Whether to split the input on whitespace before feeding it to
> - * get_sha1; this is decided during the mark_query phase based on
> - * whether we have a %(rest) token in our format.
> - */
> - int split_on_whitespace;It looks like we lose this name and comment in the movement, though (it's now "is_rest"). If it's just a local variable inside batch_objects(), I don't know that we need the comment. But I think it's more than is_rest, isn't it?
It looks like we auto-enable it when --textconv or --filters is given. Can we stick with the split_on_whitespace name (which I think is also more descriptive about how we intend it to be used)?
Show 5 quoted lines
> @@ -491,6 +482,7 @@ static int batch_objects(struct batch_options *opt) > struct expand_data data; > int save_warning; > int retval = 0; > + int is_rest = strstr(opt->format.format, "%(rest)") != NULL || opt->cmdmode;
I'm not excited by this loose parsing. It would do the wrong thing in some funny corner cases (e.g., "%%(rest)").
We should be able to ask the format parser whether the "rest" placeholder was used. That's what the initial strbuf_expand() call is doing. I see that it's hard for us to pass something to its callback outside of expand_data (since after all, expand_data takes up the void-pointer data slot).
But doesn't that point to this being the wrong change (or perhaps the wrong time to make it)? I think we'd want to keep using our own local expand_data as long as we are not using ref-filter. And then ref-filter would grow its own struct to hold the data that _it_ needs. Some of that would be duplicates of what we have here, but that's OK. When we cut over to ref-filter, that's when we'd drop the fields here.
And eventually we'd drop that strbuf_expand(), too, as ref-filter would do the parsing. But at that point we wouldn't want this strstr() either: we'd have ref-filter parse the format, and then check the parsed atoms to see if one of them is "rest".
-Peff