Re: [PATCH 1/2] gpg-interface: refactor 'enum sign_mode' parsing
From: Patrick Steinhardt <hidden>
Date: 2025-09-11 06:06:45
On Wed, Sep 10, 2025 at 10:08:38AM +0200, Christian Couder wrote:
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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.
+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