Re: [PATCH v2 2/3] pretty.c: capture invalid trailer argument

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 30, 2021, 00:07 UTC
Message-ID: <xmqqo8h7qoci.fsf@gitster.c.googlers.com>
In-Reply-To: <245e48eb6835cae4e61f65af780b766d990d4b5f.1611954543.git.gitgitgadget@gmail.com>

"Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 13 quoted lines

>  		} 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.