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.
> 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)?
> @@ -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
