Re: [PATCH 1/2] gpg-interface: refactor 'enum sign_mode' parsing

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 10, 2025, 16:50 UTC
Message-ID: <xmqq348utfox.fsf@gitster.g>
In-Reply-To: <20250910080839.2142651-2-christian.couder@gmail.com>

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

Show 38 quoted lines

> 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 ...

Show 22 quoted lines

> 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.

Show 16 quoted lines

> 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.

Show 6 quoted lines

> +/*
> + * 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.