Re: [PATCH v2 11/12] builtin/rerere: use `OPT_SUBCOMMAND()`

[Patrick Steinhardt <ps@pks.im>]

On Fri, Nov 06, 2026 at 03:44:38AM -0800, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/builtin/rerere.c b/builtin/rerere.c
> > index 3162b074ec6..97c791e420b 100644
> > --- a/builtin/rerere.c
> > +++ b/builtin/rerere.c
> > @@ -211,41 +237,43 @@ int cmd_rerere(int argc,
> >  	       const char *prefix,
> >  	       struct repository *repo)
> >  {
> > -	int autoupdate = -1, flags = 0;
> > -
> > +	parse_opt_subcommand_fn *fn = NULL;
> >  	struct option options[] = {
> > -		OPT_SET_INT(0, "rerere-autoupdate", &autoupdate,
> > -			N_("register clean resolutions in index"), 1),
> > +		OPT_SUBCOMMAND("clear", &fn, cmd_rerere_clear),
> > +		OPT_SUBCOMMAND("diff", &fn, cmd_rerere_diff),
> > +		OPT_SUBCOMMAND("forget", &fn, cmd_rerere_forget),
> > +		OPT_SUBCOMMAND("gc", &fn, cmd_rerere_gc),
> > +		OPT_SUBCOMMAND("remaining", &fn, cmd_rerere_remaining),
> > +		OPT_SUBCOMMAND("status", &fn, cmd_rerere_status),
> >  		OPT_END(),
> >  	};
> >
> >  	argc = parse_options(argc, argv, prefix, options, rerere_usage,
> > -			     PARSE_OPT_KEEP_UNKNOWN_OPT);
> > -
> > +			     PARSE_OPT_KEEP_UNKNOWN_OPT | PARSE_OPT_SUBCOMMAND_OPTIONAL);
> 
> So this matches what I thought.
> 
> >  	repo_config(repo, git_xmerge_config, NULL);
> >
> > -	if (autoupdate == 1)
> > -		flags = RERERE_AUTOUPDATE;
> > -	if (autoupdate == 0)
> > -		flags = RERERE_NOAUTOUPDATE;
> > +	if (!fn) {
> 
> Okay if there's no function we call the default case. I wonder if we
> could've done the inverse to reduce code block. if (fn) reutrn fn(...).
> Doesn't matter much anyway.

I think that the result is actually harder to read in that case, even
though that might just be my personal opinion. But having a return in
the middle of the function is awkward, and I like that the variables
that are specific to the default subcommand are nicely scoped to the
block so that they obviously cannot be used by any other subcommand.

So I prefer to keep this as-is.

> > +		struct strvec args = STRVEC_INIT;
> > +		const char **args_copy;
> > +		int ret;
> >
> > -	if (argc < 1)
> > -		return rerere(repo, flags);
> > -
> > -	if (!strcmp(argv[0], "forget"))
> > -		return cmd_rerere_forget(argc, argv, prefix, repo);
> > -	else if (!strcmp(argv[0], "clear"))
> > -		return cmd_rerere_clear(argc, argv, prefix, repo);
> > -	else if (!strcmp(argv[0], "gc"))
> > -		return cmd_rerere_gc(argc, argv, prefix, repo);
> > -	else if (!strcmp(argv[0], "status"))
> > -		return cmd_rerere_status(argc, argv, prefix, repo);
> > -	else if (!strcmp(argv[0], "remaining"))
> > -		return cmd_rerere_remaining(argc, argv, prefix, repo);
> > -	else if (!strcmp(argv[0], "diff"))
> > -		return cmd_rerere_diff(argc, argv, prefix, repo);
> > -	else
> > -		usage_with_options(rerere_usage, options);
> > +		strvec_push(&args, "perform");
> 
> Worthwhile adding a comment that this is a dummy placeholder subcommand?

Hmm. I could add something like this:

diff --git a/builtin/rerere.c b/builtin/rerere.c
index 97c791e420b..f88bc43e363 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -257,6 +257,7 @@ int cmd_rerere(int argc,
 		const char **args_copy;
 		int ret;
 
+		/* This is a dummy subcommand name. */
 		strvec_push(&args, "perform");
 		strvec_pushv(&args, argv);

But does that really add much value? I myself don't think so, but if you
feel that it does I'm happy to add it in.

Thanks!

Patrick