Re: [PATCH v4] builtin/reflog.c: use parse-options api for expire, delete subcommands
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 5, 2022, 20:34 UTC
- Message-ID
- <xmqqy23thqgi.fsf@gitster.g>
- In-Reply-To
- <pull.1175.v4.git.git.1641355561700.gitgitgadget@gmail.com>
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 8 quoted lines
> +static int expire_unreachable_callback(const struct option *opt,
> + const char *arg,
> + int unset)
> +{
> + struct cmd_reflog_expire_cb *cmd = opt->value;
> +
> + if (parse_expiry_date(arg, &cmd->expire_unreachable))
> + die(_("malformed expiration date '%s'"), arg);Was there a reason to indent this overly deep? The same for the other die() we can see below.
Show 12 quoted lines
> + cmd->explicit_expiry |= EXPIRE_UNREACH;
> + return 0;
> +}
> +
> +static int expire_total_callback(const struct option *opt,
> + const char *arg,
> + int unset)
> +{
> + struct cmd_reflog_expire_cb *cmd = opt->value;
> +
> + if (parse_expiry_date(arg, &cmd->expire_total))
> + die(_("malformed expiration date '%s'"), arg);We used to say "'%s' is not a valid timestamp". The same for the other die() we saw earlier.
"expiration date" is more specific to "timestamp" and in that sense, the new message might be an improvement, but if we were to improve messages better, perhaps we should go one step further and tell the user which option got an invalid value, e.g.
die(_("invalid timestamp '%s' given to '--%s'"),
arg, opt->long_name);perhaps.
Show 19 quoted lines
> static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
> {
> struct cmd_reflog_expire_cb cmd = { 0 };
> timestamp_t now = time(NULL);
> int i, status, do_all, all_worktrees = 1;
> - int explicit_expiry = 0;
> unsigned int flags = 0;
> int verbose = 0;
> reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
> + const struct option options[] = {
> + OPT_BIT(0, "dry-run", &flags, N_("do not actually prune any entries"),
> + EXPIRE_REFLOGS_DRY_RUN),
> + OPT_BIT(0, "rewrite", &flags,
> + N_("rewrite the old SHA1 with the new SHA1 of the entry that now precedes it"),
> + EXPIRE_REFLOGS_REWRITE),
> + OPT_BIT(0, "updateref", &flags,
> + N_("update the reference to the value of the top reflog entry"),
> + EXPIRE_REFLOGS_UPDATE_REF),
> + OPT_BOOL(0, "verbose", &verbose, N_("print extra information on screen.")),Somebody else might suggest OPT__VERBOSE() or OPT__VERBOSITY(), but for the purpose of this "use parse-options" topic, a simple BOOL is sufficient. When we start supporting different levels of verbosity, we can switch to more featureful variant (and change the behaviour).
> +static const char * reflog_delete_usage[] = {
> + N_("git reflog delete [--rewrite] [--updateref] "
> + "[--dry-run | -n] [--verbose] <refs>..."),Funny indentation still here?
Show 13 quoted lines
> + NULL
> +};
> +
> static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
> {
> struct cmd_reflog_expire_cb cmd = { 0 };
> @@ -703,34 +725,28 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
> unsigned int flags = 0;
> int verbose = 0;
> reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
> + const struct option options[] = {
> + OPT_BIT(0, "dry-run", &flags, N_("do not actually prune any entries"),
> + EXPIRE_REFLOGS_DRY_RUN),This is formatted quite differently from the one in reflog_expire(). Be consistent and make sure each line of what's inside "OPT_BIT(...)" begins at the same column, e.g.
OPT_BIT(0, "dry-run", &flags, N_("do not actually prune any entries"),
EXPIRE_REFLOGS_DRY_RUN),Show 8 quoted lines
> + argc = parse_options(argc, argv, prefix, options, reflog_delete_usage, 0);
>
> if (verbose)
> should_prune_fn = should_expire_reflog_ent_verbose;
>
> - if (argc - i < 1)
> + if (argc < 1)
> return error(_("no reflog specified to delete"));Nice.
Show 5 quoted lines
> - for ( ; i < argc; i++) {
> + for (i = 0; i < argc; i++) {
> const char *spec = strstr(argv[i], "@{");
> char *ep, *ref;
> int recno;Nice, too.
Thanks.