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

From: Jeff King <peff@peff.net>
Date: Jan 27, 2020, 23:20 UTC
Message-ID: <20200127232004.GE19360@coredump.intra.peff.net>
In-Reply-To: <xmqqv9owa6cw.fsf@gitster-ct.c.googlers.com>

On Mon, Jan 27, 2020 at 02:21:03PM -0800, Junio C Hamano wrote:

Show 9 quoted lines

> We often skip an optional prefix in a string with a hardcoded
> constant, e.g.
> 
> 	if (starts_with(string, "prefix"))
> 		string += 6;
> 
> which is less error prone when written
> 
> 	skip_prefix(string, "prefix", &string);

All of these look obviously correct, because you've introduced new temporary variables to replace the computed values. But...

Show 19 quoted lines

> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 4d3430900d..51ffd74945 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -560,15 +560,17 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
>  
>  	for (i = 1; i < argc; i++) {
>  		const char *arg = argv[i];
> +		const char *valptr;
> +
>  		if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
>  			flags |= EXPIRE_REFLOGS_DRY_RUN;
> -		else if (starts_with(arg, "--expire=")) {
> -			if (parse_expiry_date(arg + 9, &cb.cmd.expire_total))
> +		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)) {

Show 7 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