git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Stefan Beller <sbeller@google.com>
To: gitster@pobox.com
Cc: git@vger.kernel.org, sbeller@google.com
Subject: [PATCH] parse-options: warn developers on negated options
Date: Tue,  5 Sep 2017 16:08:45 -0700	[thread overview]
Message-ID: <20170905230845.17108-1-sbeller@google.com> (raw)
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


  reply	other threads:[~2017-09-05 23:08 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-05 21:01 [PATCH] builtin/merge: honor commit-msg hook for merges Stefan Beller
2017-09-05 21:38 ` Junio C Hamano
2017-09-05 23:08   ` Stefan Beller [this message]
2017-09-06  1:52     ` [PATCH] parse-options: warn developers on negated options Junio C Hamano
2017-09-06  3:16       ` Junio C Hamano
2017-09-06 21:36         ` Stefan Beller
2017-09-06 23:41           ` Junio C Hamano
2017-09-05 23:29   ` [PATCHv2] builtin/merge: honor commit-msg hook for merges Stefan Beller
2017-09-06  1:57     ` Junio C Hamano
2017-09-06 22:11       ` Stefan Beller
2017-09-06 23:43         ` Junio C Hamano
2017-09-07 22:04   ` [PATCHv3] " Stefan Beller
2017-09-08  1:13     ` Junio C Hamano
2017-09-11 17:12       ` Stefan Beller
2017-09-16  6:22     ` Kaartic Sivaraam
2017-09-20 19:55       ` Stefan Beller
2017-09-21  1:10         ` Junio C Hamano
2017-09-21 20:29       ` [PATCH] Documentation/githooks: mention merge in commit-msg hook Stefan Beller
2017-09-22  1:58         ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170905230845.17108-1-sbeller@google.com \
    --to=sbeller@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).