git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/2] ref-filter: 'contents:trailers' show error if `:` is missing

From
Junio C Hamano <gitster@pobox.com>
Date
Aug 19, 2020, 19:07 UTC
Message-ID
<xmqqmu2qtpxp.fsf@gitster.c.googlers.com>
In-Reply-To
<xmqqv9hettag.fsf@gitster.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 32 quoted lines
> "Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Hariom Verma <hariom18599@gmail.com>
>>
>> The 'contents' atom does not show any error if used with 'trailers'
>> atom and semicolon is missing before trailers arguments.
>>
>> e.g %(contents:trailersonly) works, while it shouldn't.
>>
>> It is definitely not an expected behavior.
>>
>> Let's fix this bug.
>>
>> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
>> Mentored-by: Heba Waly <heba.waly@gmail.com>
>> Signed-off-by: Hariom Verma <hariom18599@gmail.com>
>> ---
>
> Nice spotting.  7a5edbdb (ref-filter.c: parse trailers arguments
> with %(contents) atom, 2017-10-01) talks about being deliberate
> about the case where skip_prefix(":") does not find a colon after
> the "trailers" token, but from the message it is clear that it
> expected that the case happens only when "trailers" is at the end of
> the string.
>
> The new helper that is overly verbose and may be overkill.
>
> Shouldn't this be clear enough, equivalent and sufficient?
>
> 	else if (skip_prefix(arg, "trailers", &arg) &&
> 		 (!*arg || *arg == ':'))) {
> 		if (trailers_atom_parser(...);

Ah, no, even with "*arg++ == ':'. This moves arg past "trailers" if given "trailersandsomegarbage" and the next one in "else if" cascade would look at "andsomegarbage"---which is not what we want.

Show 15 quoted lines
>> +static int check_format_field(const char *arg, const char *field, const char **option)
>> +{
>> +	const char *opt;
>> +	if (skip_prefix(arg, field, &opt)) {
>> +		if (*opt == '\0') {
>> +			*option = NULL;
>> +			return 1;
>> +		}
>> +		else if (*opt == ':') {
>> +			*option = ++opt;
>> +			return 1;
>> +		}
>> +	}
>> +	return 0;
>> +}
And the helper does not have such a breakage.  It looks good.
Thanks.
Previous: Junio C HamanoNext: Eric Sunshine
Message 7 of 31 in “Fix trailers atom bug and improved tests”
  1. 0/2 Fix trailers atom bug and improved testsHariom Verma via GitGitGadget, Aug 19, 2020
  2. 1/2 t6300: unify %(trailers) and %(contents:trailers) testsHariom Verma via GitGitGadget, Aug 19, 2020
  3. Junio C HamanoAug 19, 2020
  4. Hariom vermaAug 21, 2020
  5. 2/2 ref-filter: 'contents:trailers' show error if `:` is missingHariom Verma via GitGitGadget, Aug 19, 2020
  6. Junio C HamanoAug 19, 2020
  7. Junio C HamanoAug 19, 2020
  8. Eric SunshineAug 19, 2020
  9. Junio C HamanoAug 19, 2020
  10. Hariom vermaAug 20, 2020
  11. 0/2 Fix trailers atom bug and improved testsHariom Verma via GitGitGadget, Aug 21, 2020
  12. 1/2 t6300: unify %(trailers) and %(contents:trailers) testsHariom Verma via GitGitGadget, Aug 21, 2020
  13. 2/2 ref-filter: 'contents:trailers' show error if `:` is missingHariom Verma via GitGitGadget, Aug 21, 2020
  14. Eric SunshineAug 21, 2020
  15. Junio C HamanoAug 21, 2020
  16. Hariom vermaAug 23, 2020
  17. Eric SunshineAug 24, 2020
  18. Hariom vermaAug 24, 2020
  19. Christian CouderAug 26, 2020
  20. Christian CouderAug 26, 2020
  21. Hariom vermaAug 26, 2020
  22. 0/4 [GSoC] Fix trailers atom bug and improved testsHariom Verma via GitGitGadget, Aug 21, 2020
  23. 1/4 t6300: unify %(trailers) and %(contents:trailers) testsHariom Verma via GitGitGadget, Aug 21, 2020
  24. 2/4 ref-filter: 'contents:trailers' show error if `:` is missingHariom Verma via GitGitGadget, Aug 21, 2020
  25. Eric SunshineAug 21, 2020
  26. Hariom vermaAug 21, 2020
  27. Junio C HamanoAug 21, 2020
  28. 4/4 ref-filter: using pretty.c logic for trailersHariom Verma via GitGitGadget, Aug 21, 2020
  29. 3/4 pretty.c: refactor trailer logic to `format_set_trailers_options()`Hariom Verma via GitGitGadget, Aug 21, 2020
  30. Junio C HamanoAug 21, 2020
  31. Hariom vermaAug 22, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.