[PATCH 0/3] parse-options: no- symmetry

[PATCH 0/3] parse-options: no- symmetry

[René Scharfe]

Boolean long options can be negated by adding a "no-" at the
beginning, unless the flag PARSE_OPT_NONEG is set.  There are
several options defined to start out with "no-" already (e.g.
format-patch --no-numbered).  Their negation with a second
"no-" looks a bit strange.

The flag PARSE_OPT_NEGHELP was introduced to avoid this
awkwardness.  It is used twice (in fast-export and grep) to
define option pairs (--data/--no-data and --index/--no-index)
whose negative part is shown in the help text.

However, PARSE_OPT_NEGHELP is strange as well.  Short options
defined with it do the opposite of what the help text says
(we currently don't have any).  And the multiple negations
are confusing.

This series adds the ability for users to negate options that
already start with "no-" by simply leaving it out.  The last
patch removes PARSE_OPT_NEGHELP as it isn't needed anymore at
that point.

  test-parse-options: convert to OPT_BOOL()
  parse-options: allow positivation of options starting with no-
  parse-options: remove PARSE_OPT_NEGHELP

 Documentation/technical/api-parse-options.txt |    3 +-
 builtin/fast-export.c                         |    4 +-
 builtin/grep.c                                |   15 +++----
 parse-options.c                               |   33 ++++++++------
 parse-options.h                               |    4 --
 t/t0040-parse-options.sh                      |   60 ++++++++++++++++++++++++-
 test-parse-options.c                          |   12 +++--
 7 files changed, 95 insertions(+), 36 deletions(-)

-- 
1.7.9.2

[PATCH 1/3] test-parse-options: convert to OPT_BOOL()

[René Scharfe]

Introduce OPT_BOOL() to test-parse-options and add some tests for
these "true" boolean options. Rename OPT_BOOLEAN to OPT_COUNTUP and
OPTION_BOOLEAN to OPTION_COUNTUP as well.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 t/t0040-parse-options.sh |   60 ++++++++++++++++++++++++++++++++++++++++++++--
 test-parse-options.c     |   12 ++++++----
 2 files changed, 66 insertions(+), 6 deletions(-)

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index a1e4616..79aefe2 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -10,7 +10,10 @@ test_description='our own option parser'
 cat > expect << EOF
 usage: test-parse-options <options>
 
-    -b, --boolean         get a boolean
+    --yes                 get a boolean
+    -D, --no-doubt        begins with 'no-'
+    -B, --no-fear         be brave
+    -b, --boolean         increment by one
     -4, --or4             bitwise-or boolean with ...0100
     --neg-or4             same as --no-or4
 
@@ -53,6 +56,59 @@ test_expect_success 'test help' '
 
 mv expect expect.err
 
+cat >expect.template <<EOF
+boolean: 0
+integer: 0
+timestamp: 0
+string: (not set)
+abbrev: 7
+verbose: 0
+quiet: no
+dry run: no
+file: (not set)
+EOF
+
+check() {
+	what="$1" &&
+	shift &&
+	expect="$1" &&
+	shift &&
+	sed "s/^$what .*/$what $expect/" <expect.template >expect &&
+	test-parse-options $* >output 2>output.err &&
+	test ! -s output.err &&
+	test_cmp expect output
+}
+
+check_unknown() {
+	case "$1" in
+	--*)
+		echo error: unknown option \`${1#--}\' >expect ;;
+	-*)
+		echo error: unknown switch \`${1#-}\' >expect ;;
+	esac &&
+	cat expect.err >>expect &&
+	test_must_fail test-parse-options $* >output 2>output.err &&
+	test ! -s output &&
+	test_cmp expect output.err
+}
+
+test_expect_success 'OPT_BOOL() #1' 'check boolean: 1 --yes'
+test_expect_success 'OPT_BOOL() #2' 'check boolean: 1 --no-doubt'
+test_expect_success 'OPT_BOOL() #3' 'check boolean: 1 -D'
+test_expect_success 'OPT_BOOL() #4' 'check boolean: 1 --no-fear'
+test_expect_success 'OPT_BOOL() #5' 'check boolean: 1 -B'
+
+test_expect_success 'OPT_BOOL() is idempotent #1' 'check boolean: 1 --yes --yes'
+test_expect_success 'OPT_BOOL() is idempotent #2' 'check boolean: 1 -DB'
+
+test_expect_success 'OPT_BOOL() negation #1' 'check boolean: 0 -D --no-yes'
+test_expect_success 'OPT_BOOL() negation #2' 'check boolean: 0 -D --no-no-doubt'
+
+test_expect_success 'OPT_BOOL() no negation #1' 'check_unknown --fear'
+test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown --no-no-fear'
+
+test_expect_failure 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt'
+
 cat > expect << EOF
 boolean: 2
 integer: 1729
@@ -296,7 +352,7 @@ test_expect_success 'OPT_NEGBIT() works' '
 	test_cmp expect output
 '
 
-test_expect_success 'OPT_BOOLEAN() with PARSE_OPT_NODASH works' '
+test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' '
 	test-parse-options + + + + + + > 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 36487c4..3c9510a 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -37,7 +37,11 @@ int main(int argc, const char **argv)
 		NULL
 	};
 	struct option options[] = {
-		OPT_BOOLEAN('b', "boolean", &boolean, "get a boolean"),
+		OPT_BOOL(0, "yes", &boolean, "get a boolean"),
+		OPT_BOOL('D', "no-doubt", &boolean, "begins with 'no-'"),
+		{ OPTION_SET_INT, 'B', "no-fear", &boolean, NULL,
+		  "be brave", PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },
+		OPT_COUNTUP('b', "boolean", &boolean, "increment by one"),
 		OPT_BIT('4', "or4", &boolean,
 			"bitwise-or boolean with ...0100", 4),
 		OPT_NEGBIT(0, "neg-or4", &boolean, "same as --no-or4", 4),
@@ -62,11 +66,11 @@ int main(int argc, const char **argv)
 		OPT_ARGUMENT("quux", "means --quux"),
 		OPT_NUMBER_CALLBACK(&integer, "set integer to NUM",
 			number_callback),
-		{ OPTION_BOOLEAN, '+', NULL, &boolean, NULL, "same as -b",
+		{ OPTION_COUNTUP, '+', NULL, &boolean, NULL, "same as -b",
 		  PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_NODASH },
-		{ OPTION_BOOLEAN, 0, "ambiguous", &ambiguous, NULL,
+		{ OPTION_COUNTUP, 0, "ambiguous", &ambiguous, NULL,
 		  "positive ambiguity", PARSE_OPT_NOARG | PARSE_OPT_NONEG },
-		{ OPTION_BOOLEAN, 0, "no-ambiguous", &ambiguous, NULL,
+		{ OPTION_COUNTUP, 0, "no-ambiguous", &ambiguous, NULL,
 		  "negative ambiguity", PARSE_OPT_NOARG | PARSE_OPT_NONEG },
 		OPT_GROUP("Standard options"),
 		OPT__ABBREV(&abbrev),
-- 
1.7.9.2

[PATCH 2/3] parse-options: allow positivation of options starting, with no-

[René Scharfe]

Long options can be negated by adding no- right after the leading
two dashes. This is useful e.g. to override options set by aliases.

For options that are defined to start with no- already, this looks
a bit funny. Allow such options to also be negated by removing the
prefix.

The following thirteen options are affected:

	apply          --no-add
	bisect--helper --no-checkout
	checkout-index --no-create
	clone          --no-checkout --no-hardlinks
	commit         --no-verify   --no-post-rewrite
	format-patch   --no-binary
	hash-object    --no-filters
	read-tree      --no-sparse-checkout
	revert         --no-commit
	show-branch    --no-name
	update-ref     --no-deref

The following five are NOT affected because they are defined with
PARSE_OPT_NONEG or the non-negated version is defined as well:

	branch       --no-merged
	format-patch --no-stat             --no-numbered
	update-index --no-assume-unchanged --no-skip-worktree

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 Documentation/technical/api-parse-options.txt |    3 ++-
 parse-options.c                               |   27 ++++++++++++++++---------
 t/t0040-parse-options.sh                      |    2 +-
 3 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
index 4b92514..2527b7e 100644
--- a/Documentation/technical/api-parse-options.txt
+++ b/Documentation/technical/api-parse-options.txt
@@ -39,7 +39,8 @@ The parse-options API allows:
 * Short options may be bundled, e.g. `-a -b` can be specified as `-ab`.
 
 * Boolean long options can be 'negated' (or 'unset') by prepending
-  `no-`, e.g. `\--no-abbrev` instead of `\--abbrev`.
+  `no-`, e.g. `\--no-abbrev` instead of `\--abbrev`. Conversely,
+  options that begin with `no-` can be 'negated' by removing it.
 
 * Options and non-option arguments can clearly be separated using the `\--`
   option, e.g. `-a -b \--option \-- \--this-is-a-file` indicates that
diff --git a/parse-options.c b/parse-options.c
index f0098eb..8906841 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -193,13 +193,14 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
 		arg_end = arg + strlen(arg);
 
 	for (; options->type != OPTION_END; options++) {
-		const char *rest;
-		int flags = 0;
+		const char *rest, *long_name = options->long_name;
+		int flags = 0, opt_flags = 0;
 
-		if (!options->long_name)
+		if (!long_name)
 			continue;
 
-		rest = skip_prefix(arg, options->long_name);
+again:
+		rest = skip_prefix(arg, long_name);
 		if (options->type == OPTION_ARGUMENT) {
 			if (!rest)
 				continue;
@@ -212,7 +213,7 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
 		}
 		if (!rest) {
 			/* abbreviated? */
-			if (!strncmp(options->long_name, arg, arg_end - arg)) {
+			if (!strncmp(long_name, arg, arg_end - arg)) {
 is_abbreviated:
 				if (abbrev_option) {
 					/*
@@ -227,7 +228,7 @@ is_abbreviated:
 				if (!(flags & OPT_UNSET) && *arg_end)
 					p->opt = arg_end + 1;
 				abbrev_option = options;
-				abbrev_flags = flags;
+				abbrev_flags = flags ^ opt_flags;
 				continue;
 			}
 			/* negation allowed? */
@@ -239,12 +240,18 @@ is_abbreviated:
 				goto is_abbreviated;
 			}
 			/* negated? */
-			if (strncmp(arg, "no-", 3))
+			if (prefixcmp(arg, "no-")) {
+				if (!prefixcmp(long_name, "no-")) {
+					long_name += 3;
+					opt_flags |= OPT_UNSET;
+					goto again;
+				}
 				continue;
+			}
 			flags |= OPT_UNSET;
-			rest = skip_prefix(arg + 3, options->long_name);
+			rest = skip_prefix(arg + 3, long_name);
 			/* abbreviated and negated? */
-			if (!rest && !prefixcmp(options->long_name, arg + 3))
+			if (!rest && !prefixcmp(long_name, arg + 3))
 				goto is_abbreviated;
 			if (!rest)
 				continue;
@@ -254,7 +261,7 @@ is_abbreviated:
 				continue;
 			p->opt = rest + 1;
 		}
-		return get_value(p, options, flags);
+		return get_value(p, options, flags ^ opt_flags);
 	}
 
 	if (ambiguous_option)
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 79aefe2..aa57299 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -107,7 +107,7 @@ test_expect_success 'OPT_BOOL() negation #2' 'check boolean: 0 -D --no-no-doubt'
 test_expect_success 'OPT_BOOL() no negation #1' 'check_unknown --fear'
 test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown --no-no-fear'
 
-test_expect_failure 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt'
+test_expect_success 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt'
 
 cat > expect << EOF
 boolean: 2
-- 
1.7.9.2

Re: [PATCH 2/3] parse-options: allow positivation of options starting, with no-

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Long options can be negated by adding no- right after the leading
> two dashes. This is useful e.g. to override options set by aliases.
>
> For options that are defined to start with no- already, this looks
> a bit funny. Allow such options to also be negated by removing the
> prefix.

True about "a bit funny", but the fact that the solution has to touch
parse-options.c confuses me.

I would naïvely expect that it would be sufficient to update an existing
definition for "--no-frotz" that uses PARSE_OPT_NONEG to instead define
"--frotz" that by itself is a no-op, and "--no-frotz" would cause whatever
the option currently means, with an update to the help text that says
something to the effect that "--frotz by itself is meaningless and is
always used as --no-frotz".

There must be a reason the patch had to take an approach in the opposite
direction to allow removal of --[no-] prefix, but it is not obvious to me
what it is.

Note that I am _not_ saying that this is a bad change. I am just saying
that it is unclear why we still want two different ways to support the
"--no-frotz" option, one by defining "frotz" option that allows "no" to be
prefixed, and the other by defining "no-frotz" that allows "no-" to be
stripped.

Re: [PATCH 2/3] parse-options: allow positivation of options starting, with no-

[Thomas Rast]

Junio C Hamano <gitster@pobox.com> writes:

> I would naïvely expect that it would be sufficient to update an existing
> definition for "--no-frotz" that uses PARSE_OPT_NONEG to instead define
> "--frotz" that by itself is a no-op, and "--no-frotz" would cause whatever
> the option currently means, with an update to the help text that says
> something to the effect that "--frotz by itself is meaningless and is
> always used as --no-frotz".

Doesn't that last quote already answer your question?  It would be
rather awkward to see, in 'git apply -h',

  --add                 Also apply additions in the patch.  This is the
                        default; use --no-add to disable it.

Compare to the current concise wording

  --no-add              ignore additions made by the patch

which lists the main (or mainly used) form in the left column, and
doesn't have to implicitly mention the 'no-' convention again to make
the help display useful.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 2/3] parse-options: allow positivation of options starting, with no-

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> I would naïvely expect that it would be sufficient to update an existing
>> definition for "--no-frotz" that uses PARSE_OPT_NONEG to instead define
>> "--frotz" that by itself is a no-op, and "--no-frotz" would cause whatever
>> the option currently means, with an update to the help text that says
>> something to the effect that "--frotz by itself is meaningless and is
>> always used as --no-frotz".
>
> Doesn't that last quote already answer your question?

Yes, but only partly.  I would agree with the awkwardness in

> It would be rather awkward to see, in 'git apply -h',
>
>   --add                 Also apply additions in the patch.  This is the
>                         default; use --no-add to disable it.

but it feeels somewhat questionable that the solution to get this:

>
> Compare to the current concise wording
>
>   --no-add              ignore additions made by the patch

is to define OPT_BOOL("no-add") that does not have any hint (other than
the fact that the option name begins with 3 character "no-") that this is
an already negated boolean and the "no-" negation can be removed.

This means an option "no-$foo" can never mean anything but "not foo".  Not
that we would have to or necessarily want to support an option to give the
number of foo as --no-foo=47, as --num-foo=47 is a perfectly good spelling
for such an option.

If it were OPT_BOOL("no-foo", OPT_ISNEG | ...) that signals the parser
that:

 - the option name is already negative;
 - the leading "no-" is to be removed to negate it; and
 - no extra leading "no-", i.e. "--no-no-foo", is accepted.

I probably wouldn't have felt this uneasy iffiness.

But it is just a minor issue.

Re: [PATCH 2/3] parse-options: allow positivation of options starting, with no-

[René Scharfe]

Am 27.02.2012 18:18, schrieb Junio C Hamano:
> Thomas Rast<trast@inf.ethz.ch>  writes:
>
>> Junio C Hamano<gitster@pobox.com>  writes:
>>
>>> I would naïvely expect that it would be sufficient to update an existing
>>> definition for "--no-frotz" that uses PARSE_OPT_NONEG to instead define
>>> "--frotz" that by itself is a no-op, and "--no-frotz" would cause whatever
>>> the option currently means, with an update to the help text that says
>>> something to the effect that "--frotz by itself is meaningless and is
>>> always used as --no-frotz".
>>
>> Doesn't that last quote already answer your question?
>
> Yes, but only partly.  I would agree with the awkwardness in
>
>> It would be rather awkward to see, in 'git apply -h',
>>
>>    --add                 Also apply additions in the patch.  This is the
>>                          default; use --no-add to disable it.
>
> but it feeels somewhat questionable that the solution to get this:
>
>>
>> Compare to the current concise wording
>>
>>    --no-add              ignore additions made by the patch
>
> is to define OPT_BOOL("no-add") that does not have any hint (other than
> the fact that the option name begins with 3 character "no-") that this is
> an already negated boolean and the "no-" negation can be removed.

The parser already knows that the prefix "no-" negates an option.  It 
currenmtly only applies this knowledge if that prefix is added, but not 
if it is removed, which is inconsistent.

> This means an option "no-$foo" can never mean anything but "not foo".  Not
> that we would have to or necessarily want to support an option to give the
> number of foo as --no-foo=47, as --num-foo=47 is a perfectly good spelling
> for such an option.

With the patch, you can define a --no-foo option that doesn't accept 
--foo as its negation by specifying PARSE_OPT_NONEG.  That would also 
forbid --no-no-foo, though, but that's probably a good thing.

> If it were OPT_BOOL("no-foo", OPT_ISNEG | ...) that signals the parser
> that:
>
>   - the option name is already negative;
>   - the leading "no-" is to be removed to negate it; and
>   - no extra leading "no-", i.e. "--no-no-foo", is accepted.
>
> I probably wouldn't have felt this uneasy iffiness.

Teaching the parser to understand that removal of the prefix "no-" 
negates an option on top of its existing knowledge that adding it does 
the same just adds the other side of the same coin, which was curiously 
missing.

The patch does not forbid adding "no-" to an option that already starts 
with "no-".  This stricter rule would be easy to add, but since that is 
currently the only way to negate such options, it would break backwards 
compatibility and thus should be added in a separate patch, if at all.

With the patch, the following guidelines are followed:

	- "no-" means no, for both developers and users.
	- The user doesn't have to to say "no-no-".

The results feels simpler to me.

René

Re: [PATCH 2/3] parse-options: allow positivation of options starting, with no-

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> The patch does not forbid adding "no-" to an option that already starts
> with "no-".  This stricter rule would be easy to add, but since that is
> currently the only way to negate such options, it would break backwards
> compatibility and thus should be added in a separate patch, if at all.
>
> With the patch, the following guidelines are followed:
>
> 	- "no-" means no, for both developers and users.
> 	- The user doesn't have to to say "no-no-".
>
> The results feels simpler to me.

Sounds fair.

I agree that the backward compatibility of --no-no-foo is a potential
problem, if any of the actions controlled by "--no-foo" option defaults to
the behaviour when "--no-foo" is given.  Among the existing 13 that you
listed, I do not think there is any that tempts any existing user to ask
for negation with "--no-no-foo" form, so I think we should be Ok.

[PATCH 4/3] parse-options: disallow --no-no-sth

[René Scharfe]

Now that options whose definition starts with "no-" can be negated
by removing said "no-", there is no need anymore to allow them to
be negated by adding a second "no-", which just looks silly.

The following thirteen options are affected:

	apply          --no-add
	bisect--helper --no-checkout
	checkout-index --no-create
	clone          --no-checkout --no-hardlinks
	commit         --no-verify   --no-post-rewrite
	format-patch   --no-binary
	hash-object    --no-filters
	read-tree      --no-sparse-checkout
	revert         --no-commit
	show-branch    --no-name
	update-ref     --no-deref

E.g., with this patch --no-add and --add (its reverse) are still
accepted by git apply, but --no-no-add isn't anymore.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 parse-options.c          |    3 +++
 t/t0040-parse-options.sh |    4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 1908996..dc59bba 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -248,6 +248,9 @@ is_abbreviated:
 				}
 				continue;
 			}
+			/* double negation? */
+			if (!prefixcmp(long_name, "no-"))
+				continue;
 			flags |= OPT_UNSET;
 			rest = skip_prefix(arg + 3, long_name);
 			/* abbreviated and negated? */
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index a44bcb9..b124f3c 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -101,11 +101,11 @@ test_expect_success 'OPT_BOOL() #5' 'check boolean: 1 -B'
 test_expect_success 'OPT_BOOL() is idempotent #1' 'check boolean: 1 --yes --yes'
 test_expect_success 'OPT_BOOL() is idempotent #2' 'check boolean: 1 -DB'
 
-test_expect_success 'OPT_BOOL() negation #1' 'check boolean: 0 -D --no-yes'
-test_expect_success 'OPT_BOOL() negation #2' 'check boolean: 0 -D --no-no-doubt'
+test_expect_success 'OPT_BOOL() negation' 'check boolean: 0 -D --no-yes'
 
 test_expect_success 'OPT_BOOL() no negation #1' 'check_unknown --fear'
 test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown --no-no-fear'
+test_expect_success 'OPT_BOOL() no negation #3' 'check_unknown --no-no-doubt'
 
 test_expect_success 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt'
 
-- 
1.7.9.2

Re: [PATCH 4/3] parse-options: disallow --no-no-sth

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Now that options whose definition starts with "no-" can be negated
> by removing said "no-", there is no need anymore to allow them to
> be negated by adding a second "no-", which just looks silly.

Thanks.  But accepting them silently and do what the user would have
expected, especially if we do not advertise it, would not hurt anybody,
no?

Re: [PATCH 4/3] parse-options: disallow --no-no-sth

[René Scharfe]

Am 28.02.2012 22:15, schrieb Junio C Hamano:
> René Scharfe<rene.scharfe@lsrfire.ath.cx>  writes:
>
>> Now that options whose definition starts with "no-" can be negated
>> by removing said "no-", there is no need anymore to allow them to
>> be negated by adding a second "no-", which just looks silly.
>
> Thanks.  But accepting them silently and do what the user would have
> expected, especially if we do not advertise it, would not hurt anybody,
> no?

Yes, that's why I didn't include this fourth patch from the beginning. 
However, I understood your comment "Among the existing 13 that you
listed, I do not think there is any that tempts any existing user to ask
for negation with "--no-no-foo" form, so I think we should be Ok." to 
mean that nobody uses the double-no form of the existing negative 
options anyway, and therefore we should disallow it.

Please drop this extra patch if I misunderstood you, in order to keep 
backward compatibility.

René

Re: [PATCH 4/3] parse-options: disallow --no-no-sth

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Please drop this extra patch if I misunderstood you, in order to keep
> backward compatibility.

I do not think it makes practical difference between accepting silently or
forbidding, so let's stop at 3/3 and keep --no-no-sth working.

Thanks.

[PATCH 3/3] parse-options: remove PARSE_OPT_NEGHELP

[René Scharfe]

PARSE_OPT_NEGHELP is confusing because short options defined with that
flag do the opposite of what the helptext says. It is also not needed
anymore now that options starting with no- can be negated by removing
that prefix. Convert its only two users to OPT_BOOL() and then remove
support for PARSE_OPT_NEGHELP.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 builtin/fast-export.c |    4 +---
 builtin/grep.c        |   15 +++++++--------
 parse-options.c       |    6 ++----
 parse-options.h       |    4 ----
 4 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 08fed98..19509ea 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -647,9 +647,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 			     "Output full tree for each commit"),
 		OPT_BOOLEAN(0, "use-done-feature", &use_done_feature,
 			     "Use the done feature to terminate the stream"),
-		{ OPTION_NEGBIT, 0, "data", &no_data, NULL,
-			"Skip output of blob data",
-			PARSE_OPT_NOARG | PARSE_OPT_NEGHELP, NULL, 1 },
+		OPT_BOOL(0, "no-data", &no_data, "Skip output of blob data"),
 		OPT_END()
 	};
 
diff --git a/builtin/grep.c b/builtin/grep.c
index e4ea900..b151467 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -671,7 +671,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	struct string_list path_list = STRING_LIST_INIT_NODUP;
 	int i;
 	int dummy;
-	int use_index = 1;
+	int no_index = 0;
 	enum {
 		pattern_type_unspecified = 0,
 		pattern_type_bre,
@@ -684,9 +684,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_BOOLEAN(0, "cached", &cached,
 			"search in index instead of in the work tree"),
-		{ OPTION_BOOLEAN, 0, "index", &use_index, NULL,
-			"finds in contents not managed by git",
-			PARSE_OPT_NOARG | PARSE_OPT_NEGHELP },
+		OPT_BOOL(0, "no-index", &no_index,
+			 "finds in contents not managed by git"),
 		OPT_BOOLEAN(0, "untracked", &untracked,
 			"search in both tracked and untracked files"),
 		OPT_SET_INT(0, "exclude-standard", &opt_exclude,
@@ -851,7 +850,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		break; /* nothing */
 	}
 
-	if (use_index && !startup_info->have_repository)
+	if (!no_index && !startup_info->have_repository)
 		/* die the same way as if we did it at the beginning */
 		setup_git_directory();
 
@@ -963,11 +962,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	if (!show_in_pager)
 		setup_pager();
 
-	if (!use_index && (untracked || cached))
+	if (no_index && (untracked || cached))
 		die(_("--cached or --untracked cannot be used with --no-index."));
 
-	if (!use_index || untracked) {
-		int use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude;
+	if (no_index || untracked) {
+		int use_exclude = (opt_exclude < 0) ? !no_index : !!opt_exclude;
 		if (list.nr)
 			die(_("--no-index or --untracked cannot be used with revs."));
 		hit = grep_directory(&opt, &pathspec, use_exclude);
diff --git a/parse-options.c b/parse-options.c
index 8906841..1908996 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -533,7 +533,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 			continue;
 
 		pos = fprintf(outfile, "    ");
-		if (opts->short_name && !(opts->flags & PARSE_OPT_NEGHELP)) {
+		if (opts->short_name) {
 			if (opts->flags & PARSE_OPT_NODASH)
 				pos += fprintf(outfile, "%c", opts->short_name);
 			else
@@ -542,9 +542,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 		if (opts->long_name && opts->short_name)
 			pos += fprintf(outfile, ", ");
 		if (opts->long_name)
-			pos += fprintf(outfile, "--%s%s",
-				(opts->flags & PARSE_OPT_NEGHELP) ?  "no-" : "",
-				opts->long_name);
+			pos += fprintf(outfile, "--%s", opts->long_name);
 		if (opts->type == OPTION_NUMBER)
 			pos += fprintf(outfile, "-NUM");
 
diff --git a/parse-options.h b/parse-options.h
index 2e811dc..def9ced 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -40,7 +40,6 @@ enum parse_opt_option_flags {
 	PARSE_OPT_LASTARG_DEFAULT = 16,
 	PARSE_OPT_NODASH = 32,
 	PARSE_OPT_LITERAL_ARGHELP = 64,
-	PARSE_OPT_NEGHELP = 128,
 	PARSE_OPT_SHELL_EVAL = 256
 };
 
@@ -90,9 +89,6 @@ typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
  *   PARSE_OPT_LITERAL_ARGHELP: says that argh shouldn't be enclosed in brackets
  *				(i.e. '<argh>') in the help message.
  *				Useful for options with multiple parameters.
- *   PARSE_OPT_NEGHELP: says that the long option should always be shown with
- *				the --no prefix in the usage message. Sometimes
- *				useful for users of OPTION_NEGBIT.
  *
  * `callback`::
  *   pointer to the callback to use for OPTION_CALLBACK or
-- 
1.7.9.2

Re: [PATCH 3/3] parse-options: remove PARSE_OPT_NEGHELP

[Jeff King]

On Sat, Feb 25, 2012 at 08:15:56PM +0100, René Scharfe wrote:

> diff --git a/builtin/grep.c b/builtin/grep.c
> index e4ea900..b151467 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -671,7 +671,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  	struct string_list path_list = STRING_LIST_INIT_NODUP;
>  	int i;
>  	int dummy;
> -	int use_index = 1;
> +	int no_index = 0;
>  	enum {
>  		pattern_type_unspecified = 0,
>  		pattern_type_bre,
> @@ -684,9 +684,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  	struct option options[] = {
>  		OPT_BOOLEAN(0, "cached", &cached,
>  			"search in index instead of in the work tree"),
> -		{ OPTION_BOOLEAN, 0, "index", &use_index, NULL,
> -			"finds in contents not managed by git",
> -			PARSE_OPT_NOARG | PARSE_OPT_NEGHELP },
> +		OPT_BOOL(0, "no-index", &no_index,
> +			 "finds in contents not managed by git"),
>  		OPT_BOOLEAN(0, "untracked", &untracked,
>  			"search in both tracked and untracked files"),
>  		OPT_SET_INT(0, "exclude-standard", &opt_exclude,
> @@ -851,7 +850,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  		break; /* nothing */
>  	}
>  
> -	if (use_index && !startup_info->have_repository)
> +	if (!no_index && !startup_info->have_repository)

Hmm. We usually try to avoid these sorts of double negations in the
code, as they can often make the logic hard to read. In this case, it is
not _so_ bad, because out of the 4 uses of use_index/no_index, only one
is "!no_index", and it is in a relatively simple conditional.

But I do feel like the original was slightly easier to read, and that
getting rid of NEGHELP is restricting how the developer can express the
options.

I think your original motivation was that NEGHELP lead to confusion
where the name of the option does not match its description. Would it be
better to simply be explicit that an option is a reversed boolean (i.e.,
what the user specifies on the command line and what is in the code are
naturally opposites). Like:

 OPT_REVERSE_BOOL(0, "no-index", &use_index,
             "finds in contents not managed by git"),

Using NEGHELP, the "reverse" is between the option name and the
description, which is very subtle. Here it is between the option name
and the variable, which is hopefully a little more explicit (especially
with the big REVERSE in the macro name).

I dunno. Given that there are only two uses of NEGHELP, and that they
don't come out too badly, I don't care _too_ much. But I have seen some
really tortured logic with double-negations like this, and I'm concerned
that a few months down the road somebody is going to want NEGHELP (or
something similar) in a case where it actually does really impact
readability.

-Peff

Re: [PATCH 3/3] parse-options: remove PARSE_OPT_NEGHELP

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... Would it be
> better to simply be explicit that an option is a reversed boolean (i.e.,
> what the user specifies on the command line and what is in the code are
> naturally opposites). Like:
>
>  OPT_REVERSE_BOOL(0, "no-index", &use_index,
>              "finds in contents not managed by git"),

You said it much better than my attempt ;-).

> Using NEGHELP, the "reverse" is between the option name and the
> description, which is very subtle. Here it is between the option name
> and the variable, which is hopefully a little more explicit (especially
> with the big REVERSE in the macro name).
>
> I dunno. Given that there are only two uses of NEGHELP, and that they
> don't come out too badly, I don't care _too_ much. But I have seen some
> really tortured logic with double-negations like this, and I'm concerned
> that a few months down the road somebody is going to want NEGHELP (or
> something similar) in a case where it actually does really impact
> readability.

Yeah, I share a similar minor and iffy feeling about the result.

Re: [PATCH 3/3] parse-options: remove PARSE_OPT_NEGHELP

[René Scharfe]

Am 27.02.2012 19:25, schrieb Jeff King:
> On Sat, Feb 25, 2012 at 08:15:56PM +0100, René Scharfe wrote:
>
>> diff --git a/builtin/grep.c b/builtin/grep.c
>> index e4ea900..b151467 100644
>> --- a/builtin/grep.c
>> +++ b/builtin/grep.c
>> @@ -671,7 +671,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>>   	struct string_list path_list = STRING_LIST_INIT_NODUP;
>>   	int i;
>>   	int dummy;
>> -	int use_index = 1;
>> +	int no_index = 0;
>>   	enum {
>>   		pattern_type_unspecified = 0,
>>   		pattern_type_bre,
>> @@ -684,9 +684,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>>   	struct option options[] = {
>>   		OPT_BOOLEAN(0, "cached",&cached,
>>   			"search in index instead of in the work tree"),
>> -		{ OPTION_BOOLEAN, 0, "index",&use_index, NULL,
>> -			"finds in contents not managed by git",
>> -			PARSE_OPT_NOARG | PARSE_OPT_NEGHELP },
>> +		OPT_BOOL(0, "no-index",&no_index,
>> +			 "finds in contents not managed by git"),
>>   		OPT_BOOLEAN(0, "untracked",&untracked,
>>   			"search in both tracked and untracked files"),
>>   		OPT_SET_INT(0, "exclude-standard",&opt_exclude,
>> @@ -851,7 +850,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>>   		break; /* nothing */
>>   	}
>>
>> -	if (use_index&&  !startup_info->have_repository)
>> +	if (!no_index&&  !startup_info->have_repository)

[Unrelated: The whitespace in the two lines above and before ampersands 
in general was damaged by Thunderbird.  First time I noticed.]

> Hmm. We usually try to avoid these sorts of double negations in the
> code, as they can often make the logic hard to read. In this case, it is
> not _so_ bad, because out of the 4 uses of use_index/no_index, only one
> is "!no_index", and it is in a relatively simple conditional.

The variable could be named "unmanaged", "external" or similar instead 
of "no_index".  The latter just matches the option name and thus was the 
obvious first choice to me.

> But I do feel like the original was slightly easier to read, and that
> getting rid of NEGHELP is restricting how the developer can express the
> options.
>
> I think your original motivation was that NEGHELP lead to confusion
> where the name of the option does not match its description. Would it be
> better to simply be explicit that an option is a reversed boolean (i.e.,
> what the user specifies on the command line and what is in the code are
> naturally opposites). Like:
>
>   OPT_REVERSE_BOOL(0, "no-index",&use_index,
>               "finds in contents not managed by git"),

It's better than NEGHELP, but I find your use of two negations (REVERSE 
and "no-") confusing.  We don't need to invent new OPT_ types for this, 
by the way, we can just do this:

	OPT_NEGBIT(0, "no-index", &use_index,
	           "finds in contents not managed by git", 1),

It certainly shortens the patch.

> Using NEGHELP, the "reverse" is between the option name and the
> description, which is very subtle. Here it is between the option name
> and the variable, which is hopefully a little more explicit (especially
> with the big REVERSE in the macro name).

We have precedence for OPT_NEGBIT in grep, although the double negations 
for -h and --full-name are required because both turn off bits that 
other options turn on, while for --no-index it wouldn't be strictly 
needed, as there is no option that overrules it except --index.

I don't care too much either way, though.  The changes from patch 2 (the 
no no-no one) are not restricted to OPT_BOOL.

> I dunno. Given that there are only two uses of NEGHELP, and that they
> don't come out too badly, I don't care _too_ much. But I have seen some
> really tortured logic with double-negations like this, and I'm concerned
> that a few months down the road somebody is going to want NEGHELP (or
> something similar) in a case where it actually does really impact
> readability.

I'm curious to see a case that can be solved better using NEGHELP, but 
we can always add it back if we find such a beast.  I'd much rather see 
it go until then because of it's non-obvious semantics.

René

Re: [PATCH 3/3] parse-options: remove PARSE_OPT_NEGHELP

[Jeff King]

On Mon, Feb 27, 2012 at 11:26:16PM +0100, René Scharfe wrote:

> >  OPT_REVERSE_BOOL(0, "no-index",&use_index,
> >              "finds in contents not managed by git"),
> 
> It's better than NEGHELP, but I find your use of two negations
> (REVERSE and "no-") confusing.  We don't need to invent new OPT_
> types for this, by the way, we can just do this:
> 
> 	OPT_NEGBIT(0, "no-index", &use_index,
> 	           "finds in contents not managed by git", 1),
> 
> It certainly shortens the patch.

Right. The point is that the code and the option want to look at the
conditional in two different ways. So you have to have negate it
_somewhere_. Either at point of use (your original patch), between the
option name and the variable name (what I wrote above), or using a
reversed help text (the original code before your patch).

I think we both agree that NEGHELP is the worst one. I have a slight
preference for doing the reversal at the point of the option definition,
if only because we know it happens at a single point, and it lets the
actual code (which is usually the trickier part) be more clear. But
beyond that, I think it is largely a matter of aesthetics.

> >I dunno. Given that there are only two uses of NEGHELP, and that they
> >don't come out too badly, I don't care _too_ much. But I have seen some
> >really tortured logic with double-negations like this, and I'm concerned
> >that a few months down the road somebody is going to want NEGHELP (or
> >something similar) in a case where it actually does really impact
> >readability.
> 
> I'm curious to see a case that can be solved better using NEGHELP,
> but we can always add it back if we find such a beast.  I'd much
> rather see it go until then because of it's non-obvious semantics.

Yeah, I should have spelled out my "or something similar" more. I'd
rather see something like NEGBIT above than NEGHELP; the latter is a bit
too subtle.

-Peff

[PATCH 3/3 v2] parse-options: remove PARSE_OPT_NEGHELP

[René Scharfe]

PARSE_OPT_NEGHELP is confusing because short options defined with that
flag do the opposite of what the helptext says. It is also not needed
anymore now that options starting with no- can be negated by removing
that prefix. Convert its only two users to OPT_NEGBIT() and OPT_BOOL()
and then remove support for PARSE_OPT_NEGHELP.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
This version doesn't invert the logic in grep anymore.

 builtin/fast-export.c |    4 +---
 builtin/grep.c        |    5 ++---
 parse-options.c       |    6 ++----
 parse-options.h       |    4 ----
 4 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 08fed98..19509ea 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -647,9 +647,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 			     "Output full tree for each commit"),
 		OPT_BOOLEAN(0, "use-done-feature", &use_done_feature,
 			     "Use the done feature to terminate the stream"),
-		{ OPTION_NEGBIT, 0, "data", &no_data, NULL,
-			"Skip output of blob data",
-			PARSE_OPT_NOARG | PARSE_OPT_NEGHELP, NULL, 1 },
+		OPT_BOOL(0, "no-data", &no_data, "Skip output of blob data"),
 		OPT_END()
 	};
 
diff --git a/builtin/grep.c b/builtin/grep.c
index e4ea900..643938d 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -684,9 +684,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_BOOLEAN(0, "cached", &cached,
 			"search in index instead of in the work tree"),
-		{ OPTION_BOOLEAN, 0, "index", &use_index, NULL,
-			"finds in contents not managed by git",
-			PARSE_OPT_NOARG | PARSE_OPT_NEGHELP },
+		OPT_NEGBIT(0, "no-index", &use_index,
+			 "finds in contents not managed by git", 1),
 		OPT_BOOLEAN(0, "untracked", &untracked,
 			"search in both tracked and untracked files"),
 		OPT_SET_INT(0, "exclude-standard", &opt_exclude,
diff --git a/parse-options.c b/parse-options.c
index 8906841..1908996 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -533,7 +533,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 			continue;
 
 		pos = fprintf(outfile, "    ");
-		if (opts->short_name && !(opts->flags & PARSE_OPT_NEGHELP)) {
+		if (opts->short_name) {
 			if (opts->flags & PARSE_OPT_NODASH)
 				pos += fprintf(outfile, "%c", opts->short_name);
 			else
@@ -542,9 +542,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 		if (opts->long_name && opts->short_name)
 			pos += fprintf(outfile, ", ");
 		if (opts->long_name)
-			pos += fprintf(outfile, "--%s%s",
-				(opts->flags & PARSE_OPT_NEGHELP) ?  "no-" : "",
-				opts->long_name);
+			pos += fprintf(outfile, "--%s", opts->long_name);
 		if (opts->type == OPTION_NUMBER)
 			pos += fprintf(outfile, "-NUM");
 
diff --git a/parse-options.h b/parse-options.h
index 2e811dc..def9ced 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -40,7 +40,6 @@ enum parse_opt_option_flags {
 	PARSE_OPT_LASTARG_DEFAULT = 16,
 	PARSE_OPT_NODASH = 32,
 	PARSE_OPT_LITERAL_ARGHELP = 64,
-	PARSE_OPT_NEGHELP = 128,
 	PARSE_OPT_SHELL_EVAL = 256
 };
 
@@ -90,9 +89,6 @@ typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
  *   PARSE_OPT_LITERAL_ARGHELP: says that argh shouldn't be enclosed in brackets
  *				(i.e. '<argh>') in the help message.
  *				Useful for options with multiple parameters.
- *   PARSE_OPT_NEGHELP: says that the long option should always be shown with
- *				the --no prefix in the usage message. Sometimes
- *				useful for users of OPTION_NEGBIT.
  *
  * `callback`::
  *   pointer to the callback to use for OPTION_CALLBACK or
-- 
1.7.9.2

Re: [PATCH 3/3 v2] parse-options: remove PARSE_OPT_NEGHELP

[Jeff King]

On Tue, Feb 28, 2012 at 08:06:09PM +0100, René Scharfe wrote:

> PARSE_OPT_NEGHELP is confusing because short options defined with that
> flag do the opposite of what the helptext says. It is also not needed
> anymore now that options starting with no- can be negated by removing
> that prefix. Convert its only two users to OPT_NEGBIT() and OPT_BOOL()
> and then remove support for PARSE_OPT_NEGHELP.
> 
> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
> ---
> This version doesn't invert the logic in grep anymore.

Thanks, I like this one much better.

-Peff