Jeff King <peff@xxxxxxxx> writes:
>> + 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.
>> - 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
