Re: [PATCH 1/2] gpg-interface: refactor 'enum sign_mode' parsing
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Sep 11, 2025, 06:06 UTC
- Message-ID
- <aMJm57VmWb3h167z@pks.im>
- In-Reply-To
- <20250910080839.2142651-2-christian.couder@gmail.com>
On Wed, Sep 10, 2025 at 10:08:38AM +0200, Christian Couder wrote:
Show 33 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
> @@ -59,24 +57,17 @@ static struct hashmap anonymized_seeds;
> static struct revision_sources revision_sources;
>
> 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);Would it make sense to maybe reverse the error handling and say something like:
if (parse_sign_mode(arg, val) < 0)
return error("Unknown %s mode: %s", opt->long_name, arg);return 0;
That reads a bit more natural to me at least.
Show 9 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 */
Nit: let's finish this sentence with a dot.
Show 13 quoted lines
> +enum sign_mode {
> + SIGN_ABORT,
> + SIGN_WARN_VERBATIM,
> + SIGN_VERBATIM,
> + SIGN_WARN_STRIP,
> + SIGN_STRIP,
> +};
> +
> +/*
> + * 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);Okay, makes sense.
Patrick