Re: [PATCH] C: use skip-prefix to avoid hardcoded string length

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 28, 2020, 00:45 UTC
Message-ID: <xmqq36c09zo0.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20200127232004.GE19360@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 10 quoted lines

>> +		else if (skip_prefix(arg, "--expire=", &valptr)) {
>> +			if (parse_expiry_date(valptr, &cb.cmd.expire_total))
>>  				die(_("'%s' is not a valid timestamp"), arg);
>>  			explicit_expiry |= EXPIRE_TOTAL;
>>  		}
>
> In this case, I think the die message in the context could be improved
> to show "valptr". At which point you might as well do:
>
>   else if (skip_prefix(arg, "--expire=", &arg)) {

The version that loses "to which parameter did I give malformed timestamp?" information was what I originally have written, and then I added a new valptr variable to avoid the information loss and message change.

But thinking about it again,

	git frotz --expire=tea --expire-unreachable=tee

would say "I don't know 'tee'" and then the user can go back and see to which one the misspelt version went, and if the user did

	git frotz --expire=tee --expire-unreachable=tee

and got "I don't know 'tee'", then it also is OK to give that without saying it is about --expire or --expire-unreachable; they are both wrong ;-)

So, I guess it probably is a good idea to skip the option name in the error message (we might have to adjust some tests, though).

Thanks.

Show 11 quoted lines

>> -		else if (starts_with(arg, "--expire-unreachable=")) {
>> -			if (parse_expiry_date(arg + 21, &cb.cmd.expire_unreachable))
>> +		else if (skip_prefix(arg, "--expire-unreachable=", &valptr)) {
>> +			if (parse_expiry_date(valptr, &cb.cmd.expire_unreachable))
>>  				die(_("'%s' is not a valid timestamp"), arg);
>>  			explicit_expiry |= EXPIRE_UNREACH;
>>  		}
>
> Ditto here.
>
> -Peff