Re: [PATCH v4] builtin/reflog.c: use parse-options api for expire, delete subcommands

[Junio C Hamano <gitster@pobox.com>]

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +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.

> +	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.

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

> +	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),

> +	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.

> -	for ( ; i < argc; i++) {
> +	for (i = 0; i < argc; i++) {
>  		const char *spec = strstr(argv[i], "@{");
>  		char *ep, *ref;
>  		int recno;

Nice, too.

Thanks.