Re: [PATCH 1/2] builtin/reflog.c: use parse-options for expire subcommand

["René Scharfe" <l.s.r@web.de>]

Am 01.01.22 um 03:06 schrieb John Cai:
>
>
>> On Dec 30, 2021, at 10:29 PM, John Cai via GitGitGadget <gitgitgadget@gmail.com> wrote:
>>
>> From: John Cai <johncai86@gmail.com>
>>
>> Switch out manual argv parsing for the reflog expire subcommand to use
>> the parse-options API.
>>
>> Signed-off-by: "John Cai" <johncai86@gmail.com>
>> ---
>> builtin/reflog.c | 72 ++++++++++++++++++++++++------------------------
>> 1 file changed, 36 insertions(+), 36 deletions(-)
>>
>> diff --git a/builtin/reflog.c b/builtin/reflog.c
>> index 175c83e7cc2..afaf5ba67e2 100644
>> --- a/builtin/reflog.c
>> +++ b/builtin/reflog.c
>> @@ -11,13 +11,8 @@
>> #include "revision.h"
>> #include "reachable.h"
>> #include "worktree.h"
>> +#include "parse-options.h"
>>
>> -/* 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>...");
>> static const char reflog_delete_usage[] =
>> N_("git reflog delete [--rewrite] [--updateref] "
>>    "[--dry-run | -n] [--verbose] <refs>...");
>> @@ -539,6 +534,14 @@ static void set_reflog_expiry_param(struct cmd_reflog_expire_cb *cb, int slot, c
>> 		cb->expire_unreachable = default_reflog_expire_unreachable;
>> }
>>
>> +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>..."),
>> +	NULL
>> +};
>> +
>> static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
>> {
>> 	struct expire_reflog_policy_cb cb;
>> @@ -547,6 +550,29 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
>> 	int explicit_expiry = 0;
>> 	unsigned int flags = 0;
>>
>> +	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_BIT(0, "verbose", &flags, N_("print extra information on screen."),
>> +				EXPIRE_REFLOGS_VERBOSE),
>> +		OPT_EXPIRY_DATE(0, "expire", &cb.cmd.expire_total,
>> +				N_("prune entries older than the specified time")),
>> +		OPT_EXPIRY_DATE(0, "expire-unreachable", &cb.cmd.expire_unreachable,
>> +			N_("prune entries older than <time> that are not reachable from the current tip of the branch")),
>> +		OPT_BOOL(0, "stale-fix", &cb.cmd.stalefix,
>> +				N_("prune any reflog entries that point to broken commits")),
>> +		OPT_BOOL(0, "all", &do_all, N_("process the reflogs of all references")),
>> +		OPT_BOOL(1, "single-worktree", &all_worktrees,
>> +				N_("limits processing to reflogs from the current worktree only.")),
>> +		OPT_END()
>> +	};
>> +
>> 	default_reflog_expire_unreachable = now - 30 * 24 * 3600;
>> 	default_reflog_expire = now - 90 * 24 * 3600;
>> 	git_config(reflog_expire_config, NULL);
>> @@ -560,41 +586,15 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
>>
>> 	for (i = 1; i < argc; i++) {
>
> I was hoping we could get rid of this for loop altogether, but I
> couldn’t figure out a clean way since --expire and
> expire-unreachable take a value __and__ set a flag bit. So I kept
> this for loop for the sole purpose of setting the explicit_expiry bit
> flag. Any suggestions?

The problem is that the default value can vary between reflogs and we
only know which ones are to be expired after option parsing, right?

The easiest way is probably to initialize the date variables to a
magic value that is unlikely to be specified explicitly.
parse_expiry_date() already uses two such magic values: 0 for "never"
and TIME_MAX for "now".  Perhaps 1 for "default"?

	cb.cmd.expire_total = cb.cmd.expire_unreachable = 1;

	argc = parse_options(...);

	if (cb.cmd.expire_total == 1)
		cb.cmd.expire_total = default_reflog_expire;
	else
		explicit_expiry |= EXPIRE_TOTAL;
	if (cb.cmd.expire_unreachable == 1)
		cb.cmd.expire_unreachable = default_reflog_expire_unreachable;
	else
		explicit_expiry |= EXPIRE_UNREACH;

A somewhat cleaner approach would be to store that bit separately:

	struct expire_date {
		unsigned is_explicitly_set:1;
		timestamp_t at;
	};

... and add a callback function that wraps parse_opt_expiry_date_cb(),
expects the new struct (instead of timestamp_t directlly) and sets
that bit.

>
>> 		const char *arg = argv[i];
>> -
>> -		if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
>> -			flags |= EXPIRE_REFLOGS_DRY_RUN;
>> -		else if (skip_prefix(arg, "--expire=", &arg)) {
>> -			if (parse_expiry_date(arg, &cb.cmd.expire_total))
>> -				die(_("'%s' is not a valid timestamp"), arg);
>> +		if (starts_with(arg, "--expire=")) {
>> 			explicit_expiry |= EXPIRE_TOTAL;
>> -		}
>> -		else if (skip_prefix(arg, "--expire-unreachable=", &arg)) {
>> -			if (parse_expiry_date(arg, &cb.cmd.expire_unreachable))
>> -				die(_("'%s' is not a valid timestamp"), arg);
>> +		} else if (starts_with(arg, "--expire-unreachable=")) {
>> 			explicit_expiry |= EXPIRE_UNREACH;
>> 		}
>> -		else if (!strcmp(arg, "--stale-fix"))
>> -			cb.cmd.stalefix = 1;
>> -		else if (!strcmp(arg, "--rewrite"))
>> -			flags |= EXPIRE_REFLOGS_REWRITE;
>> -		else if (!strcmp(arg, "--updateref"))
>> -			flags |= EXPIRE_REFLOGS_UPDATE_REF;
>> -		else if (!strcmp(arg, "--all"))
>> -			do_all = 1;
>> -		else if (!strcmp(arg, "--single-worktree"))
>> -			all_worktrees = 0;
>> -		else if (!strcmp(arg, "--verbose"))
>> -			flags |= EXPIRE_REFLOGS_VERBOSE;
>> -		else if (!strcmp(arg, "--")) {
>> -			i++;
>> -			break;
>> -		}
>> -		else if (arg[0] == '-')
>> -			usage(_(reflog_expire_usage));
>> -		else
>> -			break;
>> 	}
>>
>> +	argc = parse_options(argc, argv, prefix, options, reflog_expire_usage, 0);
>> +
>> 	/*
>> 	 * We can trust the commits and objects reachable from refs
>> 	 * even in older repository.  We cannot trust what's reachable
>> --
>> gitgitgadget
>>
>