[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH RFC 05/20] cat-file: remove split_on_whitespace
From:       Jeff King <peff () peff ! net>
Date:       2019-02-28 21:22:23
Message-ID: 20190228212222.GE12723 () sigill ! intra ! peff ! net
[Download RAW message or body]

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
[prev in list] [next in list] [prev in thread] [next in thread]