diff mbox series

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

Message ID pull.1175.v4.git.git.1641355561700.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v4] builtin/reflog.c: use parse-options api for expire, delete subcommands | expand

Commit Message

John Cai Jan. 5, 2022, 4:06 a.m. UTC 
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>
---
    reflog.c: switch to use parse-options API
    
    Switch out manual argv parsing for the reflog expire subcommand to use
    the parse-options API. This reduces code redundancy by consolidating
    option parsing logic.
    
    NOTE: this patch was based off of next. I know now that I should base it
    off of master. But practically, nothing in master touched reflog.c
    except for ab/reflog-prep
    
    changes since v3:
    
     * fixed indentation
    
    changes since v2:
    
     * got rid of parse_opt_expiry_date helper based on feedback from Junio.
       Just call parse_expiry_date directly in the callbacks, and don't
       worry about unset, as we can just set the PARSE_OPT_NONEG flag for
       --expire and --expire-unreachable since the original code didn't
       support a negated version of those flags anyway. The documentation
       also explicitly states to use "never" as the argument.
     * fix places where we set int i inside a for loop construct.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1175%2Fjohn-cai%2Fjc%2Freflog-switch-to-option-parse-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1175/john-cai/jc/reflog-switch-to-option-parse-v4
Pull-Request: https://github.com/git/git/pull/1175

Range-diff vs v3:

 1:  99a97e6ee52 ! 1:  0efdf3d2cb1 builtin/reflog.c: use parse-options api for expire, delete subcommands
     @@ builtin/reflog.c: static void set_reflog_expiry_param(struct cmd_reflog_expire_c
       
      +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>..."),
     ++	   "[--expire-unreachable=<time>] "
     ++	   "[--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] "
     ++	   "[--verbose] [--all] <refs>..."),
      +	NULL
      +};
      +


 builtin/reflog.c | 174 ++++++++++++++++++++++++++---------------------
 1 file changed, 95 insertions(+), 79 deletions(-)


base-commit: 194610f4cf2df839ebfd040c5da187c8c0162620

Comments

Junio C Hamano Jan. 5, 2022, 8:34 p.m. UTC | #1 
"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.
diff mbox series

Patch

diff --git a/builtin/reflog.c b/builtin/reflog.c
index a4b1dd27e13..1b3b298eec4 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -12,15 +12,6 @@ 
 #include "reachable.h"
 #include "worktree.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>...");
 static const char reflog_exists_usage[] =
 N_("git reflog exists <ref>");
 
@@ -29,6 +20,7 @@  static timestamp_t default_reflog_expire_unreachable;
 
 struct cmd_reflog_expire_cb {
 	int stalefix;
+	int explicit_expiry;
 	timestamp_t expire_total;
 	timestamp_t expire_unreachable;
 	int recno;
@@ -520,18 +512,18 @@  static int reflog_expire_config(const char *var, const char *value, void *cb)
 	return 0;
 }
 
-static void set_reflog_expiry_param(struct cmd_reflog_expire_cb *cb, int slot, const char *ref)
+static void set_reflog_expiry_param(struct cmd_reflog_expire_cb *cb, const char *ref)
 {
 	struct reflog_expire_cfg *ent;
 
-	if (slot == (EXPIRE_TOTAL|EXPIRE_UNREACH))
+	if (cb->explicit_expiry == (EXPIRE_TOTAL|EXPIRE_UNREACH))
 		return; /* both given explicitly -- nothing to tweak */
 
 	for (ent = reflog_expire_cfg; ent; ent = ent->next) {
 		if (!wildmatch(ent->pattern, ref, 0)) {
-			if (!(slot & EXPIRE_TOTAL))
+			if (!(cb->explicit_expiry & EXPIRE_TOTAL))
 				cb->expire_total = ent->expire_total;
-			if (!(slot & EXPIRE_UNREACH))
+			if (!(cb->explicit_expiry & EXPIRE_UNREACH))
 				cb->expire_unreachable = ent->expire_unreachable;
 			return;
 		}
@@ -541,29 +533,87 @@  static void set_reflog_expiry_param(struct cmd_reflog_expire_cb *cb, int slot, c
 	 * If unconfigured, make stash never expire
 	 */
 	if (!strcmp(ref, "refs/stash")) {
-		if (!(slot & EXPIRE_TOTAL))
+		if (!(cb->explicit_expiry & EXPIRE_TOTAL))
 			cb->expire_total = 0;
-		if (!(slot & EXPIRE_UNREACH))
+		if (!(cb->explicit_expiry & EXPIRE_UNREACH))
 			cb->expire_unreachable = 0;
 		return;
 	}
 
 	/* Nothing matched -- use the default value */
-	if (!(slot & EXPIRE_TOTAL))
+	if (!(cb->explicit_expiry & EXPIRE_TOTAL))
 		cb->expire_total = default_reflog_expire;
-	if (!(slot & EXPIRE_UNREACH))
+	if (!(cb->explicit_expiry & EXPIRE_UNREACH))
 		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 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);
+
+	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);
+
+	cmd->explicit_expiry |= EXPIRE_TOTAL;
+	return 0;
+}
+
 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.")),
+		OPT_CALLBACK_F(0, "expire", &cmd, N_("timestamp"),
+			N_("prune entries older than the specified time"),
+			PARSE_OPT_NONEG,
+			expire_total_callback),
+		OPT_CALLBACK_F(0, "expire-unreachable", &cmd, N_("timestamp"),
+			N_("prune entries older than <time> that are not reachable from the current tip of the branch"),
+			PARSE_OPT_NONEG,
+			expire_unreachable_callback),
+		OPT_BOOL(0, "stale-fix", &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;
@@ -572,45 +622,11 @@  static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 	save_commit_buffer = 0;
 	do_all = status = 0;
 
+	cmd.explicit_expiry = 0;
 	cmd.expire_total = default_reflog_expire;
 	cmd.expire_unreachable = default_reflog_expire_unreachable;
 
-	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, &cmd.expire_total))
-				die(_("'%s' is not a valid timestamp"), arg);
-			explicit_expiry |= EXPIRE_TOTAL;
-		}
-		else if (skip_prefix(arg, "--expire-unreachable=", &arg)) {
-			if (parse_expiry_date(arg, &cmd.expire_unreachable))
-				die(_("'%s' is not a valid timestamp"), arg);
-			explicit_expiry |= EXPIRE_UNREACH;
-		}
-		else if (!strcmp(arg, "--stale-fix"))
-			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"))
-			verbose = 1;
-		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);
 
 	if (verbose)
 		should_prune_fn = should_expire_reflog_ent_verbose;
@@ -657,7 +673,7 @@  static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 				.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
 			};
 
-			set_reflog_expiry_param(&cb.cmd, explicit_expiry, item->string);
+			set_reflog_expiry_param(&cb.cmd,  item->string);
 			status |= reflog_expire(item->string, flags,
 						reflog_expiry_prepare,
 						should_prune_fn,
@@ -667,7 +683,7 @@  static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		string_list_clear(&collected.reflogs, 0);
 	}
 
-	for (; i < argc; i++) {
+	for (i = 0; i < argc; i++) {
 		char *ref;
 		struct expire_reflog_policy_cb cb = { .cmd = cmd };
 
@@ -675,7 +691,7 @@  static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 			status |= error(_("%s points nowhere!"), argv[i]);
 			continue;
 		}
-		set_reflog_expiry_param(&cb.cmd, explicit_expiry, ref);
+		set_reflog_expiry_param(&cb.cmd, ref);
 		status |= reflog_expire(ref, flags,
 					reflog_expiry_prepare,
 					should_prune_fn,
@@ -696,6 +712,12 @@  static int count_reflog_ent(struct object_id *ooid, struct object_id *noid,
 	return 0;
 }
 
+static const char * reflog_delete_usage[] = {
+	N_("git reflog delete [--rewrite] [--updateref] "
+   "[--dry-run | -n] [--verbose] <refs>..."),
+	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;
-
-	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 (!strcmp(arg, "--rewrite"))
-			flags |= EXPIRE_REFLOGS_REWRITE;
-		else if (!strcmp(arg, "--updateref"))
-			flags |= EXPIRE_REFLOGS_UPDATE_REF;
-		else if (!strcmp(arg, "--verbose"))
-			verbose = 1;
-		else if (!strcmp(arg, "--")) {
-			i++;
-			break;
-		}
-		else if (arg[0] == '-')
-			usage(_(reflog_delete_usage));
-		else
-			break;
-	}
+	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.")),
+		OPT_END()
+	};
+
+	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"));
 
-	for ( ; i < argc; i++) {
+	for (i = 0; i < argc; i++) {
 		const char *spec = strstr(argv[i], "@{");
 		char *ep, *ref;
 		int recno;