[PATCH] parse-options: warn developers on negated options

From: Stefan Beller <sbeller@google.com>
Date: Sep 5, 2017, 23:08 UTC
Message-ID: <20170905230845.17108-1-sbeller@google.com>
In-Reply-To: <xmqqd174bzco.fsf@gitster.mtv.corp.google.com>

When compiling with -DDEVELOPER set (enabled via the Makefile DEVELOPER switch), inspect options if they are negated and warn about them.

 1. Negated options are usually are problem down the maintenance road:
    When a new negated option ("--no-foo") is introduced, then the option
    can be negated at run time("--no-no-foo"), which is usually confusing
    for boolean options.
 2. The handling of negated values (specifically double negations), usually
    require more brain power to get it right. In code that we own, we
    should strive to avoid double negatives ("!no_foo").

Signed-off-by: Stefan Beller <sbeller@google.com>
---

  #leftoverbits
  
  I was wondering if such a patch may be of value eventually (after all
  occurrences of "no-" options are fixed).
  This patch disallows all no- options, but we could be more open and allow
  --no-options that have the NO_NEG bit set.

 Makefile        | 3 ++-
 parse-options.c | 6 ++++++
 parse-options.h | 7 +++++--
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index aba0f5a0f9..9b87093f99 100644
--- a/Makefile
+++ b/Makefile
@@ -433,7 +433,8 @@ DEVELOPER_CFLAGS = -Werror \
 	-Wpointer-arith \
 	-Wstrict-prototypes \
 	-Wunused \
-	-Wvla
+	-Wvla \
+	-DDEVELOPER
 LDFLAGS =
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
diff --git a/parse-options.c b/parse-options.c
index 0dd9fc6a0d..08db926adf 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -403,6 +403,12 @@ static void parse_options_check(const struct option *opts)
 		if (opts->argh &&
 		    strcspn(opts->argh, " _") != strlen(opts->argh))
 			err |= optbug(opts, "multi-word argh should use dash to separate words");
+#if DEVELOPER
+		if ((opts->flags & PARSE_OPT_ERR_NEGATED) &&
+		    !strcmp("no-", opts->long_name))
+			BUG("Get %s fixed! double negation possible",
+			    opts->long_name);
+#endif
 	}
 	if (err)
 		exit(128);
diff --git a/parse-options.h b/parse-options.h
index af711227ae..9dc07fd9bb 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -38,7 +38,8 @@ enum parse_opt_option_flags {
 	PARSE_OPT_LASTARG_DEFAULT = 16,
 	PARSE_OPT_NODASH = 32,
 	PARSE_OPT_LITERAL_ARGHELP = 64,
-	PARSE_OPT_SHELL_EVAL = 256
+	PARSE_OPT_SHELL_EVAL = 256,
+	PARSE_OPT_ERR_NEGATED = 512
 };
 
 struct option;
@@ -124,7 +125,9 @@ struct option {
 				      (h), PARSE_OPT_NOARG }
 #define OPT_SET_INT(s, l, v, h, i)  { OPTION_SET_INT, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG, NULL, (i) }
-#define OPT_BOOL(s, l, v, h)        OPT_SET_INT(s, l, v, h, 1)
+#define OPT_BOOL(s, l, v, h)        { OPTION_SET_INT, (s), (l), (v), NULL, \
+				      (h), PARSE_OPT_NOARG|PARSE_OPT_ERR_NEGATED, \
+				      NULL, 1 }
 #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}
 #define OPT_CMDMODE(s, l, v, h, i)  { OPTION_CMDMODE, (s), (l), (v), NULL, \

-- 
2.14.0.rc0.3.g6c2e499285