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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 4, 2022, 22:04 UTC
Message-ID: <xmqq8rvvb1ja.fsf@gitster.g>
In-Reply-To: <pull.1175.v3.git.git.1641318061821.gitgitgadget@gmail.com>

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

Show 10 quoted lines

> From: John Cai <johncai86@gmail.com>
>
> Switching out manual arg parsing for the parse-options API for the
> expire and delete subcommands.
>
> Move explicit_expiry flag into cmd_reflog_expire_cb struct so callbacks
> can set both the value of the timestamp as well as the explicit_expiry
> flag.
>
> Signed-off-by: "John Cai" <johncai86@gmail.com>

Makes sense.

Show 6 quoted lines

> -/* NEEDSWORK: switch to using parse_options */
> -static const char reflog_expire_usage[] =
> -N_("git reflog expire [--expire=<time>] "
> -   "[--expire-unreachable=<time>] "
> -   "[--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] "
> -   "[--verbose] [--all] <refs>...");

...

Show 5 quoted lines

> +static const char * reflog_expire_usage[] = {
> +	N_("git reflog expire [--expire=<time>] "
> +   "[--expire-unreachable=<time>] "
> +   "[--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] "
> +   "[--verbose] [--all] <refs>..."),

Funny indentation? Let's align the opening double-quote of all the other lines to that of the first line on a fixed-column terminal, like was done in the original.

Show 10 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[] = {

This does not seem to apply. Please leave a note on how the 'base' was chosen when sending a patch that requires something not yet in 'master'.

I've based the topic for this patch by prepar ng a merge of 'ab/reflog-prep' into 'master', at least for now.

Other than that, looks nicely done. The new approach seems much simpler and sensible than the previous round.

Thanks.