From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 10 Sep 2025 16:50:22 GMT
Subject: Re: [PATCH 1/2] gpg-interface: refactor 'enum sign_mode' parsing
Message-ID: <xmqq348utfox.fsf@gitster.g>
In-Reply-To: <20250910080839.2142651-2-christian.couder@gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index c06ee0b213..3994a8f898 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -37,8 +37,6 @@ static const char *const fast_export_usage[] = {
>  	NULL
>  };
>  
> -enum sign_mode { SIGN_ABORT, SIGN_VERBATIM, SIGN_STRIP, SIGN_WARN_VERBATIM, SIGN_WARN_STRIP };
> -
> ...
>  static int parse_opt_sign_mode(const struct option *opt,
> -				     const char *arg, int unset)
> +			       const char *arg, int unset)
>  {
>  	enum sign_mode *val = opt->value;
> +
>  	if (unset)
>  		return 0;
> -	else if (!strcmp(arg, "abort"))
> -		*val = SIGN_ABORT;
> -	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
> -		*val = SIGN_VERBATIM;
> -	else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn"))
> -		*val = SIGN_WARN_VERBATIM;
> -	else if (!strcmp(arg, "warn-strip"))
> -		*val = SIGN_WARN_STRIP;
> -	else if (!strcmp(arg, "strip"))
> -		*val = SIGN_STRIP;
> -	else
> -		return error("Unknown %s mode: %s", opt->long_name, arg);
> -	return 0;
> +
> +	if (!parse_sign_mode(arg, val))
> +		return 0;
> +
> +	return error("Unknown %s mode: %s", opt->long_name, arg);
>  }

I am not sure if it is correct for the "unset" codepath doing
nothing on *val before returning, but changing that is of course not
in scope of this patch at all.  And the result of the change in this
function is very pleasing to read.  And ...

> diff --git a/gpg-interface.c b/gpg-interface.c
> index 06e7fb5060..2f4f0e32cb 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -1125,3 +1125,20 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
> ...
> +int parse_sign_mode(const char *arg, enum sign_mode *mode)
> +{
> +	if (!strcmp(arg, "abort"))
> +		*mode = SIGN_ABORT;
> +	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
> +		*mode = SIGN_VERBATIM;
> +	else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn"))
> +		*mode = SIGN_WARN_VERBATIM;
> +	else if (!strcmp(arg, "warn-strip"))
> +		*mode = SIGN_WARN_STRIP;
> +	else if (!strcmp(arg, "strip"))
> +		*mode = SIGN_STRIP;
> +	else
> +		return -1;
> +	return 0;
> +}

... this is of course very much expected.

> diff --git a/gpg-interface.h b/gpg-interface.h
> index 60ddf8bbfa..44856cc55f 100644
> --- a/gpg-interface.h
> +++ b/gpg-interface.h
> @@ -104,4 +104,19 @@ int check_signature(struct signature_check *sigc,
>  void print_signature_buffer(const struct signature_check *sigc,
>  			    unsigned flags);
>  
> +/* Modes for --signed-tags=<mode> and --signed-commits=<mode> options */
> +enum sign_mode {
> +	SIGN_ABORT,
> +	SIGN_WARN_VERBATIM,
> +	SIGN_VERBATIM,
> +	SIGN_WARN_STRIP,
> +	SIGN_STRIP,
> +};

And this is much easier to read in the header file for all to see.

> +/*
> + * Return 0 if `arg` can be parsed into an `enum sign_mode`. Return -1
> + * otherwise.
> + */
> +int parse_sign_mode(const char *arg, enum sign_mode *mode);
> +

Nice.