[PATCH] parse-options: deprecate OPT_BOOLEAN

[PATCH] parse-options: deprecate OPT_BOOLEAN

[Junio C Hamano]

It is natural to expect that an option defined with OPT_BOOLEAN() could be
used in this way:

	int option = -1; /* unspecified */

	struct option options[] = {
		OPT_BOOLEAN(0, "option", &option, "set option"),
                OPT_END()
	};
	parse_options(ac, av, prefix, options, usage, 0);

        if (option < 0)
        	... do the default thing ...
	else if (!option)
		... --no-option was given ...
	else
		... --option was given ...

to easily tell three cases apart:

 - There is no mention of the `--option` on the command line;
 - The variable is positively set with `--option`; or
 - The variable is explicitly negated with `--no-option`.

Unfortunately, this is not the case. OPT_BOOLEAN() increments the variable
every time `--option` is given, and resets it to zero when `--no-option`
is given.

As a first step to remedy this, introduce a true boolean OPT_BOOL(), and
rename OPT_BOOLEAN() to OPT_COUNTUP(). To help transitioning, OPT_BOOLEAN
and OPTION_BOOLEAN are defined as deprecated synonyms to OPT_COUNTUP and
OPTION_COUNTUP respectively.

This is what db7244b (parse-options new features., 2007-11-07) from four
years ago started by marking OPTION_BOOLEAN as "INCR would have been a
better name".

Some existing users do depend on the count-up semantics; for example,
users of OPT__VERBOSE() could use it to raise the verbosity level with
repeated use of `-v` on the command line, but they probably should be
rewritten to use OPT__VERBOSITY() instead these days.  I suspect that some
users of OPT__FORCE() may also use it to implement different level of
forcibleness but I didn't check.

On top of this patch, here are the remaining clean-up tasks that other
people can help:

 - Look at each hit in "git grep -e OPT_BOOLEAN"; trace all uses of the
   value that is set to the underlying variable, and if it can proven that
   the variable is only used as a boolean, replace it with OPT_BOOL(). If
   the caller does depend on the count-up semantics, replace it with
   OPT_COUNTUP() instead.

 - Same for OPTION_BOOLEAN; replace it with OPTION_SET_INT and arrange to
   set 1 to the variable for a true boolean, and otherwise replace it with
   OPTION_COUNTUP.

 - Look at each hit in "git grep -e OPT__VERBOSE -e OPT__QUIET" and see if
   they can be replaced with OPT__VERBOSITY().

I'll follow this message up with a separate patch as an example.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/technical/api-parse-options.txt |   16 +++++++++++-----
 parse-options.c                               |    4 ++--
 parse-options.h                               |   10 ++++++++--
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
index f6a4a36..acf1760 100644
--- a/Documentation/technical/api-parse-options.txt
+++ b/Documentation/technical/api-parse-options.txt
@@ -135,9 +135,14 @@ There are some macros to easily define options:
 	describes the group or an empty string.
 	Start the description with an upper-case letter.
 
-`OPT_BOOLEAN(short, long, &int_var, description)`::
-	Introduce a boolean option.
-	`int_var` is incremented on each use.
+`OPT_BOOL(short, long, &int_var, description)`::
+	Introduce a boolean option. `int_var` is set to one with
+	`--option` and set to zero with `--no-option`.
+
+`OPT_COUNTUP(short, long, &int_var, description)`::
+	Introduce a count-up option.
+	`int_var` is incremented on each use of `--option`, and
+	reset to zero with `--no-option`.
 
 `OPT_BIT(short, long, &int_var, description, mask)`::
 	Introduce a boolean option.
@@ -148,8 +153,9 @@ There are some macros to easily define options:
 	If used, `int_var` is bitwise-anded with the inverted `mask`.
 
 `OPT_SET_INT(short, long, &int_var, description, integer)`::
-	Introduce a boolean option.
-	If used, set `int_var` to `integer`.
+	Introduce an integer option.
+	`int_var` is set to `integer` with `--option`, and
+	reset to zero with `--no-option`.
 
 `OPT_SET_PTR(short, long, &ptr_var, description, ptr)`::
 	Introduce a boolean option.
diff --git a/parse-options.c b/parse-options.c
index 503ab5d..f0098eb 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -83,7 +83,7 @@ static int get_value(struct parse_opt_ctx_t *p,
 			*(int *)opt->value &= ~opt->defval;
 		return 0;
 
-	case OPTION_BOOLEAN:
+	case OPTION_COUNTUP:
 		*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
 		return 0;
 
@@ -319,7 +319,7 @@ static void parse_options_check(const struct option *opts)
 			err |= optbug(opts, "uses feature "
 					"not supported for dashless options");
 		switch (opts->type) {
-		case OPTION_BOOLEAN:
+		case OPTION_COUNTUP:
 		case OPTION_BIT:
 		case OPTION_NEGBIT:
 		case OPTION_SET_INT:
diff --git a/parse-options.h b/parse-options.h
index 59e0b52..22c0273 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -10,7 +10,7 @@ enum parse_opt_type {
 	/* options with no arguments */
 	OPTION_BIT,
 	OPTION_NEGBIT,
-	OPTION_BOOLEAN, /* _INCR would have been a better name */
+	OPTION_COUNTUP,
 	OPTION_SET_INT,
 	OPTION_SET_PTR,
 	/* options with arguments (usually) */
@@ -21,6 +21,9 @@ enum parse_opt_type {
 	OPTION_FILENAME
 };
 
+/* Deprecated synonym */
+#define OPTION_BOOLEAN OPTION_COUNTUP
+
 enum parse_opt_flags {
 	PARSE_OPT_KEEP_DASHDASH = 1,
 	PARSE_OPT_STOP_AT_NON_OPTION = 2,
@@ -122,10 +125,11 @@ struct option {
 				      PARSE_OPT_NOARG, NULL, (b) }
 #define OPT_NEGBIT(s, l, v, h, b)   { OPTION_NEGBIT, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG, NULL, (b) }
-#define OPT_BOOLEAN(s, l, v, h)     { OPTION_BOOLEAN, (s), (l), (v), NULL, \
+#define OPT_COUNTUP(s, l, v, h)     { OPTION_COUNTUP, (s), (l), (v), NULL, \
 				      (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_SET_PTR(s, l, v, h, p)  { OPTION_SET_PTR, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG, NULL, (p) }
 #define OPT_INTEGER(s, l, v, h)     { OPTION_INTEGER, (s), (l), (v), "n", (h) }
@@ -149,6 +153,8 @@ struct option {
 	{ OPTION_CALLBACK, (s), (l), (v), "when", (h), PARSE_OPT_OPTARG, \
 		parse_opt_color_flag_cb, (intptr_t)"always" }
 
+/* Deprecated synonym */
+#define OPT_BOOLEAN OPT_COUNTUP
 
 /* parse_options() will filter out the processed options and leave the
  * non-option arguments in argv[].

[PATCH] archive.c: use OPT_BOOL()

[Junio C Hamano]

The list variable (which is OPT_BOOLEAN) is initialized to 0 and only
checked against 0 in the code, so it is safe to use OPT_BOOL().

The worktree_attributes variable (which is OPT_BOOLEAN) is initialized to
0 and later assigned to a field with the same name in struct archive_args,
which is a bitfield of width 1. It is safe and even more correct to use
OPT_BOOL() here; the new test in 5001 demonstrates why using OPT_COUNTUP
is wrong.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * This is an example of "the remaining clean-up tasks" I mentioned.

 archive.c               |    4 ++--
 t/t5001-archive-attr.sh |    9 +++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/archive.c b/archive.c
index 3fd7f47..2ae740a 100644
--- a/archive.c
+++ b/archive.c
@@ -318,7 +318,7 @@ static int parse_archive_args(int argc, const char **argv,
 			"prepend prefix to each pathname in the archive"),
 		OPT_STRING('o', "output", &output, "file",
 			"write the archive to this file"),
-		OPT_BOOLEAN(0, "worktree-attributes", &worktree_attributes,
+		OPT_BOOL(0, "worktree-attributes", &worktree_attributes,
 			"read .gitattributes in working directory"),
 		OPT__VERBOSE(&verbose, "report archived files on stderr"),
 		OPT__COMPR('0', &compression_level, "store only", 0),
@@ -332,7 +332,7 @@ static int parse_archive_args(int argc, const char **argv,
 		OPT__COMPR_HIDDEN('8', &compression_level, 8),
 		OPT__COMPR('9', &compression_level, "compress better", 9),
 		OPT_GROUP(""),
-		OPT_BOOLEAN('l', "list", &list,
+		OPT_BOOL('l', "list", &list,
 			"list supported archive formats"),
 		OPT_GROUP(""),
 		OPT_STRING(0, "remote", &remote, "repo",
diff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh
index 02d4d22..f47d871 100755
--- a/t/t5001-archive-attr.sh
+++ b/t/t5001-archive-attr.sh
@@ -57,6 +57,15 @@ test_expect_missing	worktree/ignored
 test_expect_exists	worktree/ignored-by-tree
 test_expect_missing	worktree/ignored-by-worktree
 
+test_expect_success 'git archive --worktree-attributes option' '
+	git archive --worktree-attributes --worktree-attributes HEAD >worktree.tar &&
+	(mkdir worktree2 && cd worktree2 && "$TAR" xf -) <worktree.tar
+'
+
+test_expect_missing	worktree2/ignored
+test_expect_exists	worktree2/ignored-by-tree
+test_expect_missing	worktree2/ignored-by-worktree
+
 test_expect_success 'git archive vs. bare' '
 	(cd bare && git archive HEAD) >bare-archive.tar &&
 	test_cmp archive.tar bare-archive.tar

Re: [PATCH] parse-options: deprecate OPT_BOOLEAN

[Jeff King]

On Tue, Sep 27, 2011 at 04:56:49PM -0700, Junio C Hamano wrote:

> It is natural to expect that an option defined with OPT_BOOLEAN() could be
> used in this way:
> [...]
> to easily tell three cases apart:
> 
>  - There is no mention of the `--option` on the command line;
>  - The variable is positively set with `--option`; or
>  - The variable is explicitly negated with `--no-option`.
> 
> Unfortunately, this is not the case. OPT_BOOLEAN() increments the variable
> every time `--option` is given, and resets it to zero when `--no-option`
> is given.

Yes, please. I remember being bitten by this at one point. Your
transition plan makes sense to me.

Can OPT_UYN be folded into this, as well?

-Peff

Re: [PATCH] parse-options: deprecate OPT_BOOLEAN

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Sep 27, 2011 at 04:56:49PM -0700, Junio C Hamano wrote:
>
>> It is natural to expect that an option defined with OPT_BOOLEAN() could be
>> used in this way:
>> [...]
>> to easily tell three cases apart:
>> 
>>  - There is no mention of the `--option` on the command line;
>>  - The variable is positively set with `--option`; or
>>  - The variable is explicitly negated with `--no-option`.
>> 
>> Unfortunately, this is not the case. OPT_BOOLEAN() increments the variable
>> every time `--option` is given, and resets it to zero when `--no-option`
>> is given.
>
> Yes, please. I remember being bitten by this at one point. Your
> transition plan makes sense to me.

The best part of it is that this leaves many small bite-sized tasks, one
"struct option" per patch, most of which can be done by people who are not
uber experts in Git internals ;-).

> Can OPT_UYN be folded into this, as well?

Perhaps, but I would prefer to keep this simple at least during the first
pass of eradicating OPT_BOOLEAN and OPTION_BOOLEAN. After that is done, we
may want to tackle OPT_UYN() that uses 2 for "unset", which feels a tad
unnatural. It has only one user---even if it turns out that it was a
mistake, the damage would be relatively limited to fix it.

Re: [PATCH] parse-options: deprecate OPT_BOOLEAN

[Miles Bader]

Junio C Hamano <gitster@pobox.com> writes:
> As a first step to remedy this, introduce a true boolean OPT_BOOL(), and
> rename OPT_BOOLEAN() to OPT_COUNTUP().

Hmm ...  "OPT_COUNTED"?

-miles

-- 
values of β will give rise to dom!

[PATCH 1/3] parseopt: add OPT_NOOP_NOARG

[René Scharfe]

Add OPT_NOOP_NOARG, a helper macro to define deprecated options in a
standard way.  The help text is taken from the no-op option -r of
git revert.

The callback could be made to emit a (conditional?) warning later.  And
we could also add OPT_NOOP (requiring an argument) etc. as needed.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
Two follow-up patches use the new macro to replace OPTION_BOOLEAN in
three cases.  Could be squashed in.

 Documentation/technical/api-parse-options.txt |    5 +++++
 parse-options-cb.c                            |    5 +++++
 parse-options.h                               |    6 +++++-
 t/t0040-parse-options.sh                      |    2 +-
 test-parse-options.c                          |    1 +
 5 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
index f6a4a36..c209046 100644
--- a/Documentation/technical/api-parse-options.txt
+++ b/Documentation/technical/api-parse-options.txt
@@ -198,6 +198,11 @@ There are some macros to easily define options:
 	"auto", set `int_var` to 1 if stdout is a tty or a pager,
 	0 otherwise.
 
+`OPT_NOOP_NOARG(short, long)`::
+	Introduce an option that has no effect and takes no arguments.
+	Use it to hide deprecated options that are still to be recognized
+	and ignored silently.
+
 
 The last element of the array must be `OPT_END()`.
 
diff --git a/parse-options-cb.c b/parse-options-cb.c
index 6db0921..0de5fb1 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -123,3 +123,8 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset)
 	string_list_append(v, xstrdup(arg));
 	return 0;
 }
+
+int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
+{
+	return 0;
+}
diff --git a/parse-options.h b/parse-options.h
index 59e0b52..47c11f3 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -148,7 +148,10 @@ struct option {
 #define OPT_COLOR_FLAG(s, l, v, h) \
 	{ OPTION_CALLBACK, (s), (l), (v), "when", (h), PARSE_OPT_OPTARG, \
 		parse_opt_color_flag_cb, (intptr_t)"always" }
-
+#define OPT_NOOP_NOARG(s, l) \
+	{ OPTION_CALLBACK, (s), (l), NULL, NULL, \
+	  "no-op (backward compatibility)", \
+	  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, parse_opt_noop_cb }
 
 /* parse_options() will filter out the processed options and leave the
  * non-option arguments in argv[].
@@ -210,6 +213,7 @@ extern int parse_opt_verbosity_cb(const struct option *, const char *, int);
 extern int parse_opt_with_commit(const struct option *, const char *, int);
 extern int parse_opt_tertiary(const struct option *, const char *, int);
 extern int parse_opt_string_list(const struct option *, const char *, int);
+extern int parse_opt_noop_cb(const struct option *, const char *, int);
 
 #define OPT__VERBOSE(var, h)  OPT_BOOLEAN('v', "verbose", (var), (h))
 #define OPT__QUIET(var, h)    OPT_BOOLEAN('q', "quiet",   (var), (h))
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 007f39d..a1e4616 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -87,7 +87,7 @@ EOF
 test_expect_success 'long options' '
 	test-parse-options --boolean --integer 1729 --boolean --string2=321 \
 		--verbose --verbose --no-dry-run --abbrev=10 --file fi.le\
-		> output 2> output.err &&
+		--obsolete > output 2> output.err &&
 	test ! -s output.err &&
 	test_cmp expect output
 '
diff --git a/test-parse-options.c b/test-parse-options.c
index 91a5701..36487c4 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -54,6 +54,7 @@ int main(int argc, const char **argv)
 		OPT_STRING(0, "string2", &string, "str", "get another string"),
 		OPT_STRING(0, "st", &string, "st", "get another string (pervert ordering)"),
 		OPT_STRING('o', NULL, &string, "str", "get another string"),
+		OPT_NOOP_NOARG(0, "obsolete"),
 		OPT_SET_PTR(0, "default-string", &string,
 			"set string to default", (unsigned long)"default"),
 		OPT_STRING_LIST(0, "list", &list, "str", "add str to list"),
-- 
1.7.7.rc2

[PATCH 2/3] revert: use OPT_NOOP_NOARG

[René Scharfe]

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 builtin/revert.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 3117776..db5b1d4 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -72,12 +72,10 @@ static int option_parse_x(const struct option *opt,
 static void parse_args(int argc, const char **argv)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage();
-	int noop;
 	struct option options[] = {
 		OPT_BOOLEAN('n', "no-commit", &no_commit, "don't automatically commit"),
 		OPT_BOOLEAN('e', "edit", &edit, "edit the commit message"),
-		{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, "no-op (backward compatibility)",
-		  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 0 },
+		OPT_NOOP_NOARG('r', NULL),
 		OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by:"),
 		OPT_INTEGER('m', "mainline", &mainline, "parent number"),
 		OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
-- 
1.7.7.rc2

[PATCH 3/3] apply: use OPT_NOOP_NOARG

[René Scharfe]

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 builtin/apply.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index f2edc52..872e40a 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3831,7 +3831,6 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)
 	int i;
 	int errs = 0;
 	int is_not_gitdir = !startup_info->have_repository;
-	int binary;
 	int force_apply = 0;
 
 	const char *whitespace_option = NULL;
@@ -3850,12 +3849,8 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)
 			"ignore additions made by the patch"),
 		OPT_BOOLEAN(0, "stat", &diffstat,
 			"instead of applying the patch, output diffstat for the input"),
-		{ OPTION_BOOLEAN, 0, "allow-binary-replacement", &binary,
-		  NULL, "old option, now no-op",
-		  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG },
-		{ OPTION_BOOLEAN, 0, "binary", &binary,
-		  NULL, "old option, now no-op",
-		  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG },
+		OPT_NOOP_NOARG(0, "allow-binary-replacement"),
+		OPT_NOOP_NOARG(0, "binary"),
 		OPT_BOOLEAN(0, "numstat", &numstat,
 			"shows number of added and deleted lines in decimal notation"),
 		OPT_BOOLEAN(0, "summary", &summary,
-- 
1.7.7.rc2

Re: [PATCH 1/3] parseopt: add OPT_NOOP_NOARG

[Junio C Hamano]

Thanks.