On Mon, Jan 27, 2020 at 02:21:03PM -0800, Junio C Hamano wrote:
> 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...
> 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)) {
> - 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
