| Message ID | 245e48eb6835cae4e61f65af780b766d990d4b5f.1611954543.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Unify trailers formatting logic for pretty.c and ref-filter.c | expand |
On Fri, Jan 29, 2021 at 10:15 PM Hariom Verma via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Hariom Verma <hariom18599@gmail.com> > > As we would like to use this same logic in ref-filter, it's nice to > get invalid trailer argument. This will allow us to print precise > error message, while using `format_set_trailers_options()` in > ref-filter. Thanks for continuing to work on this! > { > for (;;) { > const char *argval; > size_t arglen; > > + if(**arg == ')') { > + break; > + } A space char is missing between "if" and "(". Also no need for "{" and "}". It could just be: > + if (**arg == ')') > + break;
"Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes: > + size_t invalid_arg_len = strcspn(*arg, ",)"); > + *invalid_arg = xstrndup(*arg, invalid_arg_len); > + return 1; How about doing this only when invalid_arg is not NULL, i.e. } else if (!match_placeholder_bool_arg(....) && ... !match_placeholder_bool_arg(....)) { if (invalid_arg) { size_t len = strcspn(*arg, ",)"); *invalid_arg = xstrndup(*arg, len); } return -1; } Note that I just used 'len'; when the scope of a variable is so short, it is clear the length of what thing it refers to from the context, and there is no point in using a variable name that long. In any case, by doing so, the callers that are not interested in the report can just pass NULL, which means ... > @@ -1464,12 +1472,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ > struct strbuf sepbuf = STRBUF_INIT; > struct strbuf kvsepbuf = STRBUF_INIT; > size_t ret = 0; > + char *unused = NULL; ... this will become unneeded, and ... > opts.no_divider = 1; > > if (*arg == ':') { > arg++; > - if (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg)) > + if (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg, &unused)) > goto trailer_out; ... this will pass NULL, and ... > } > if (*arg == ')') { > @@ -1479,6 +1488,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ > trailer_out: > string_list_clear(&filter_list, 0); > strbuf_release(&sepbuf); > + free((char *)unused); ... this will become unneeded. > return ret; > } > > diff --git a/pretty.h b/pretty.h > index 7369cf7e148..d902cdd70a9 100644 > --- a/pretty.h > +++ b/pretty.h > @@ -151,6 +151,7 @@ int format_set_trailers_options(struct process_trailer_options *opts, > struct string_list *filter_list, > struct strbuf *sepbuf, > struct strbuf *kvsepbuf, > - const char **arg); > + const char **arg, > + char **invalid_arg); > > #endif /* PRETTY_H */
"Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes: > } else if (!match_placeholder_bool_arg(*arg, "only", arg, &opts->only_trailers) && > !match_placeholder_bool_arg(*arg, "unfold", arg, &opts->unfold) && > !match_placeholder_bool_arg(*arg, "keyonly", arg, &opts->key_only) && > - !match_placeholder_bool_arg(*arg, "valueonly", arg, &opts->value_only)) > - break; > + !match_placeholder_bool_arg(*arg, "valueonly", arg, &opts->value_only)) { > + size_t invalid_arg_len = strcspn(*arg, ",)"); > + *invalid_arg = xstrndup(*arg, invalid_arg_len); > + return 1; > + } > } > return 0; > } Would the existing caller be OK with this change? It used to be that this parsing code simply _ignored_ unrecognised trailer keyword because the very original just did a "break" and fell though, but now because this returns non-zero, it causes the caller rewritten in the patch [v2 1/3] to "goto trailer_out". It is not clear from your proposed log message if this would result in behaviour change, and if so if that behaviour change was intended. I suspect that the behaviour change the code implements may be OK, but the log message needs to discuss why it is OK. Thanks.
Hi, On Sat, Jan 30, 2021 at 5:31 AM Junio C Hamano <gitster@pobox.com> wrote: > > "Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > + size_t invalid_arg_len = strcspn(*arg, ",)"); > > + *invalid_arg = xstrndup(*arg, invalid_arg_len); > > + return 1; > > How about doing this only when invalid_arg is not NULL, i.e. > > } else if (!match_placeholder_bool_arg(....) && > ... > !match_placeholder_bool_arg(....)) { > if (invalid_arg) { > size_t len = strcspn(*arg, ",)"); > *invalid_arg = xstrndup(*arg, len); > } > return -1; > } Sounds like an improvement to the current version. Will change. > Note that I just used 'len'; when the scope of a variable is so > short, it is clear the length of what thing it refers to from the > context, and there is no point in using a variable name that long. > > In any case, by doing so, the callers that are not interested in the > report can just pass NULL, which means ... > > > @@ -1464,12 +1472,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ > > struct strbuf sepbuf = STRBUF_INIT; > > struct strbuf kvsepbuf = STRBUF_INIT; > > size_t ret = 0; > > + char *unused = NULL; > > ... this will become unneeded, and ... > > > opts.no_divider = 1; > > > > if (*arg == ':') { > > arg++; > > - if (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg)) > > + if (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg, &unused)) > > goto trailer_out; > > ... this will pass NULL, and ... > > > } > > if (*arg == ')') { > > @@ -1479,6 +1488,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ > > trailer_out: > > string_list_clear(&filter_list, 0); > > strbuf_release(&sepbuf); > > + free((char *)unused); > > ... this will become unneeded. > Thanks, Hariom Verma
Hi, On Sat, Jan 30, 2021 at 5:37 AM Junio C Hamano <gitster@pobox.com> wrote: > > "Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > } else if (!match_placeholder_bool_arg(*arg, "only", arg, &opts->only_trailers) && > > !match_placeholder_bool_arg(*arg, "unfold", arg, &opts->unfold) && > > !match_placeholder_bool_arg(*arg, "keyonly", arg, &opts->key_only) && > > - !match_placeholder_bool_arg(*arg, "valueonly", arg, &opts->value_only)) > > - break; > > + !match_placeholder_bool_arg(*arg, "valueonly", arg, &opts->value_only)) { > > + size_t invalid_arg_len = strcspn(*arg, ",)"); > > + *invalid_arg = xstrndup(*arg, invalid_arg_len); > > + return 1; > > + } > > } > > return 0; > > } > > Would the existing caller be OK with this change? Yes, I made sure of that. > It used to be that this parsing code simply _ignored_ unrecognised > trailer keyword because the very original just did a "break" and > fell though, but now because this returns non-zero, it causes the > caller rewritten in the patch [v2 1/3] to "goto trailer_out". > > It is not clear from your proposed log message if this would result > in behaviour change, and if so if that behaviour change was intended. > > I suspect that the behaviour change the code implements may be OK, > but the log message needs to discuss why it is OK. I should have included that change in behaviour in the commit message. Will change that too. Thanks, Hariom Verma
Hi Christian, On Sat, Jan 30, 2021 at 3:58 AM Christian Couder <christian.couder@gmail.com> wrote: > > On Fri, Jan 29, 2021 at 10:15 PM Hariom Verma via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > > > From: Hariom Verma <hariom18599@gmail.com> > > > > As we would like to use this same logic in ref-filter, it's nice to > > get invalid trailer argument. This will allow us to print precise > > error message, while using `format_set_trailers_options()` in > > ref-filter. > > Thanks for continuing to work on this! > > > { > > for (;;) { > > const char *argval; > > size_t arglen; > > > > + if(**arg == ')') { > > + break; > > + } > > A space char is missing between "if" and "(". Also no need for "{" and > "}". It could just be: > > > + if (**arg == ')') > > + break; Thanks for pointing this out. Will fix it.
diff --git a/pretty.c b/pretty.c index bb6a3c634ac..b5fa7944389 100644 --- a/pretty.c +++ b/pretty.c @@ -1152,12 +1152,17 @@ int format_set_trailers_options(struct process_trailer_options *opts, struct string_list *filter_list, struct strbuf *sepbuf, struct strbuf *kvsepbuf, - const char **arg) + const char **arg, + char **invalid_arg) { for (;;) { const char *argval; size_t arglen; + if(**arg == ')') { + break; + } + if (match_placeholder_arg_value(*arg, "key", arg, &argval, &arglen)) { uintptr_t len = arglen; @@ -1186,8 +1191,11 @@ int format_set_trailers_options(struct process_trailer_options *opts, } else if (!match_placeholder_bool_arg(*arg, "only", arg, &opts->only_trailers) && !match_placeholder_bool_arg(*arg, "unfold", arg, &opts->unfold) && !match_placeholder_bool_arg(*arg, "keyonly", arg, &opts->key_only) && - !match_placeholder_bool_arg(*arg, "valueonly", arg, &opts->value_only)) - break; + !match_placeholder_bool_arg(*arg, "valueonly", arg, &opts->value_only)) { + size_t invalid_arg_len = strcspn(*arg, ",)"); + *invalid_arg = xstrndup(*arg, invalid_arg_len); + return 1; + } } return 0; } @@ -1464,12 +1472,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ struct strbuf sepbuf = STRBUF_INIT; struct strbuf kvsepbuf = STRBUF_INIT; size_t ret = 0; + char *unused = NULL; opts.no_divider = 1; if (*arg == ':') { arg++; - if (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg)) + if (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg, &unused)) goto trailer_out; } if (*arg == ')') { @@ -1479,6 +1488,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ trailer_out: string_list_clear(&filter_list, 0); strbuf_release(&sepbuf); + free((char *)unused); return ret; } diff --git a/pretty.h b/pretty.h index 7369cf7e148..d902cdd70a9 100644 --- a/pretty.h +++ b/pretty.h @@ -151,6 +151,7 @@ int format_set_trailers_options(struct process_trailer_options *opts, struct string_list *filter_list, struct strbuf *sepbuf, struct strbuf *kvsepbuf, - const char **arg); + const char **arg, + char **invalid_arg); #endif /* PRETTY_H */