diff mbox series

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

Message ID 9bd3c6672c4232e0a5c0905b834146ca02af8a4b.1640932151.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series reflog.c: switch to use parse-options API | expand

Commit Message

John Cai Dec. 31, 2021, 6:29 a.m. UTC 
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(-)

Comments

John Cai Jan. 1, 2022, 2:06 a.m. UTC | #1 
> 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?

> 		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
>
René Scharfe Jan. 1, 2022, 11:16 a.m. UTC | #2 
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
>>
>
John Cai Jan. 1, 2022, 7:09 p.m. UTC | #3 
> On Jan 1, 2022, at 3:16 AM, René Scharfe <l.s.r@web.de> wrote:
> 
> 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?

That’s a good point. Does it matter that the default value varies between reflogs?

Would something like this suffice?

-       for (i = 1; i < argc; i++) {
-               const char *arg = argv[i];
-               if (starts_with(arg, "--expire=")) {
-                       explicit_expiry |= EXPIRE_TOTAL;
-               } else if (starts_with(arg, "--expire-unreachable=")) {
-                       explicit_expiry |= EXPIRE_UNREACH;
-               }
-       }
-
        argc = parse_options(argc, argv, prefix, options, reflog_expire_usage, 0);

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


> 
> 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
René Scharfe Jan. 2, 2022, 9 a.m. UTC | #4 
Am 01.01.22 um 20:09 schrieb John Cai:
>
>
>> On Jan 1, 2022, at 3:16 AM, René Scharfe <l.s.r@web.de> wrote:
>>
>> 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?
>
> That’s a good point. Does it matter that the default value varies between reflogs?
>
> Would something like this suffice?
>
> -       for (i = 1; i < argc; i++) {
> -               const char *arg = argv[i];
> -               if (starts_with(arg, "--expire=")) {
> -                       explicit_expiry |= EXPIRE_TOTAL;
> -               } else if (starts_with(arg, "--expire-unreachable=")) {
> -                       explicit_expiry |= EXPIRE_UNREACH;
> -               }
> -       }
> -
>         argc = parse_options(argc, argv, prefix, options, reflog_expire_usage, 0);
>
> +       if (cb.cmd.expire_total != default_reflog_expire)
> +               explicit_expiry |= EXPIRE_TOTAL;
> +       if (cb.cmd.expire_unreachable != default_reflog_expire_unreachable)
> +               explicit_expiry |= EXPIRE_UNREACH;

This would ignore --expire and -expire-unreachable options with the
value of default_reflog_expire and default_reflog_expire_unreachable,
respectively.  E.g. "git reflog expire --expire=90.days.ago refs/stash"
would not expire anything.

René
diff mbox series

Patch

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