| Message ID | 0102016915f49a4b-b346412b-752e-4068-8a25-62cac2a1f555-000000@eu-west-1.amazonses.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,01/20] cat-file: reuse struct ref_format | expand |
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
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; - /* * After a mark_query run, this object_info is set up to be * passed to oid_object_info_extended. It will point to the data @@ -255,9 +248,7 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len, else strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)data->disk_size); } else if (is_atom("rest", atom, len)) { - if (data->mark_query) - data->split_on_whitespace = 1; - else if (data->rest) + if (data->rest) strbuf_addstr(sb, data->rest); } else if (is_atom("deltabase", atom, len)) { if (data->mark_query) @@ -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; /* * Expand once with our special mark_query flag, which will prime the @@ -502,8 +494,6 @@ static int batch_objects(struct batch_options *opt) strbuf_expand(&output, opt->format.format, expand_format, &data); data.mark_query = 0; strbuf_release(&output); - if (opt->cmdmode) - data.split_on_whitespace = 1; if (opt->all_objects) { struct object_info empty = OBJECT_INFO_INIT; @@ -564,7 +554,7 @@ static int batch_objects(struct batch_options *opt) warn_on_object_refname_ambiguity = 0; while (strbuf_getline(&input, stdin) != EOF) { - if (data.split_on_whitespace) { + if (is_rest) { /* * Split at first whitespace, tying off the beginning * of the string and saving the remainder (or NULL) in
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. Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com> --- builtin/cat-file.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) -- https://github.com/git/git/pull/568