Re: [PATCH v3] builtin/revert.c: refactor using an enum for cmd-action

From: Jeff King <peff@peff.net>
Date: Jan 12, 2024, 07:24 UTC
Message-ID: <20240112072404.GF618729@coredump.intra.peff.net>
In-Reply-To: <20240111200627.64199-1-mi.al.lohmann@gmail.com>

On Thu, Jan 11, 2024 at 09:06:27PM +0100, Michael Lohmann wrote:

Show 7 quoted lines

> > I agree with you why NONE is called as such.  If "revert" does not
> > take "--start" (I do not remember offhand), I would think it would
> > be better to follow suit.
> My point was that yes, it might be in sync with what the user passes in
> as arguments, but when I followed the code and saw lots of references to
> ACTION_NONE I was puzzled, since my intuition of that name was that
> _no action_ should be taken (which did not make sense to me).

Just my two cents as an observer who is very familiar with the idioms of Git's codebase: it's common for us to use NONE here to mean "an action has not been selected", which the code then translates to a default action. So that's what I would have chosen.

But your way of seeing it also makes sense to me. I think I just find the "START" name jarring because we do not use that word elsewhere to describe the action. What if you called it ACTION_DEFAULT? Then it is both the "default" value we give it, and also the default action (which is not otherwise named in the code).

As far as the enum vs char thing, I do not have a strong opinion (though I do tend to like enums myself). Here are a few minor style comments (again that are idiomatic for our code base):

Show 7 quoted lines

> +enum action {
> +	ACTION_START = 0,
> +	ACTION_CONTINUE,
> +	ACTION_SKIP,
> +	ACTION_ABORT,
> +	ACTION_QUIT,
> +};

Explicitly setting ACTION_START to 0 is good (even though it is not strictly necessary) because it makes it clear that we expect to use its truthiness later. But later...

Show 17 quoted lines

> @@ -183,9 +189,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
>  				"--edit", opts->edit > 0,
>  				NULL);
>  
> -	if (cmd) {
> -		opts->revs = NULL;
> -	} else {
> +	if (cmd == ACTION_START) {
>  		struct setup_revision_opt s_r_opt;
>  		opts->revs = xmalloc(sizeof(*opts->revs));
>  		repo_init_revisions(the_repository, opts->revs, NULL);
> @@ -198,6 +202,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
>  		memset(&s_r_opt, 0, sizeof(s_r_opt));
>  		s_r_opt.assume_dashdash = 1;
>  		argc = setup_revisions(argc, argv, opts->revs, &s_r_opt);
> +	} else {
> +		opts->revs = NULL;

We do not take advantage of that. It is still OK to do "if (cmd)" with the enum, and that's what I'd usually expect in our code base. There is no need for this hunk at all (which also switches the order of the conditional, which just seems like churn to me).

Show 5 quoted lines

> @@ -33,6 +41,17 @@ static const char * const cherry_pick_usage[] = {
>  	NULL
>  };
>  
> +static char* get_cmd_optionname(enum action cmd)

From CodingGuidelines:

  When declaring pointers, the star sides with the variable name, i.e.
  "char *string", not "char* string" or "char * string".  This makes it
  easier to understand code like "char *string, c;".

(Yes, I know there are arguments for the other way, too; but consistency is the most important thing, I think).

Show 9 quoted lines

> +{
> +	switch (cmd) {
> +	case ACTION_CONTINUE: return "--continue";
> +	case ACTION_SKIP: return "--skip";
> +	case ACTION_ABORT: return "--abort";
> +	case ACTION_QUIT: return "--quit";
> +	case ACTION_START: BUG("no commandline flag for ACTION_START");
> +	}
> +}

I find this perfectly readable, and is likely the way I'd write it in a personal project. But in this project I find we tend to stick to more conventional formatting, like:

  switch (cmd) {
  case ACTION_CONTINUE:
	return "--continue";
  case ACTION_SKIP:
	return "--skip";
  ...and so on...

Show 19 quoted lines

> @@ -144,20 +163,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
>  	}
>  
>  	/* Check for incompatible command line arguments */
> -	if (cmd) {
> -		char *this_operation;
> -		if (cmd == 'q')
> -			this_operation = "--quit";
> -		else if (cmd == 'c')
> -			this_operation = "--continue";
> -		else if (cmd == 's')
> -			this_operation = "--skip";
> -		else {
> -			assert(cmd == 'a');
> -			this_operation = "--abort";
> -		}
> -
> -		verify_opt_compatible(me, this_operation,
> +	if (cmd != ACTION_START)

Likewise here I'd probably leave this as "if (cmd)".

> [...]

Everything else looked pretty good to me.

-Peff