Re: [PATCHv3 0/9] Removing deprecated parsing macros

Thanks.  Queued this at the tip of 'pu'.  There seem to be some
fallouts found in the test suite, though.

On 08/06/2013 08:39 AM, Junio C Hamano wrote:

Thanks.  Queued this at the tip of 'pu'.  There seem to be some
fallouts found in the test suite, though.

Thanks. I am sorry for forgetting 'make test' before sending patches.
And the test suite is correct.

e35ea450 (branch, commit, name-rev: ease up boolean conditions)
is faulty. In builtin/branch.c the variables 'delete' and 'rename' 
are not used as a true boolean but I assumed so. 
These are parsed in via OPT_BIT and depending if you pass -d or -D 
for deletion you have value 1 or 2 in delete.

Hence this change is incorrect:
-	if (!!delete + !!rename + !!force_create + !!list + !!new_upstream + !!unset_upstream > 1)
+	if (delete + rename + force_create + list + unset_upstream +
+	    !!new_upstream > 1)
 		usage_with_options(builtin_branch_usage, options);

The current patch e35ea450 ([PATCHv3 5/9] branch, commit, name-rev: ease up boolean conditions)
can be dropped/reverted and I'll resend it. (The order doesn't matter, 
you can just drop that commit and apply the resend version on top of 
origin/sb/parseopt-boolean-removal

Stefan

Now that the variables are readin by OPT_BOOL, which makes sure
to have the values being 0 or 1 after reading, we do not need
the double negation to map any other value to 1 for integer
variables.

Signed-off-by: Stefan Beller <redacted>
---
 builtin/branch.c   | 3 ++-
 builtin/commit.c   | 2 +-
 builtin/name-rev.c | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 4daed0b..0dca694 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c

@@ -872,7 +872,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (with_commit || merge_filter != NO_FILTER)
 		list = 1;
 
-	if (!!delete + !!rename + !!force_create + !!list + !!new_upstream + !!unset_upstream > 1)
+	if (force_create + list + unset_upstream +
+	    !!delete + !!rename + !!new_upstream > 1)
 		usage_with_options(builtin_branch_usage, options);
 
 	if (abbrev == -1)

diff --git a/builtin/commit.c b/builtin/commit.c
index c20426b..b0f86c8 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -1072,7 +1072,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (patch_interactive)
 		interactive = 1;
 
-	if (!!also + !!only + !!all + !!interactive > 1)
+	if (also + only + all + interactive > 1)
 		die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
 	if (argc == 0 && (also || (only && !amend)))
 		die(_("No paths with --include/--only does not make sense."));

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index a908a34..20fcf8c 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c

@@ -331,7 +331,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix, opts, name_rev_usage, 0);
-	if (!!all + !!transform_stdin + !!argc > 1) {
+	if (all + transform_stdin + !!argc > 1) {
 		error("Specify either a list, or --all, not both!");
 		usage_with_options(name_rev_usage, opts);
 	}

-- 
1.8.4.rc0.16.g7fca822.dirty

On Tue, Aug 6, 2013 at 9:07 AM, Stefan Beller
[off-list ref] wrote:

Now that the variables are readin by OPT_BOOL, which makes sure

Do you mean s/readin/read in/ ?

Or should it be s/readin/set/ ?

to have the values being 0 or 1 after reading, we do not need
the double negation to map any other value to 1 for integer
variables.

Signed-off-by: Stefan Beller <redacted>

On 08/06/2013 08:46 PM, Eric Sunshine wrote:

On Tue, Aug 6, 2013 at 9:07 AM, Stefan Beller
[off-list ref] wrote:

quoted

Now that the variables are readin by OPT_BOOL, which makes sure

Do you mean s/readin/read in/ ?

Or should it be s/readin/set/ ?

quoted

to have the values being 0 or 1 after reading, we do not need
the double negation to map any other value to 1 for integer
variables.

Signed-off-by: Stefan Beller <redacted>

I think s/readin/set/ is best.
Also s/after reading/after parsing/

Now that the variables are set by OPT_BOOL, which makes sure
to have the values being 0 or 1 after parsing, we do not need
the double negation to map any other value to 1 for integer
variables.

Signed-off-by: Stefan Beller <redacted>
---
 builtin/branch.c   | 3 ++-
 builtin/commit.c   | 2 +-
 builtin/name-rev.c | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 4daed0b..0dca694 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c

@@ -872,7 +872,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (with_commit || merge_filter != NO_FILTER)
 		list = 1;
 
-	if (!!delete + !!rename + !!force_create + !!list + !!new_upstream + !!unset_upstream > 1)
+	if (force_create + list + unset_upstream +
+	    !!delete + !!rename + !!new_upstream > 1)
 		usage_with_options(builtin_branch_usage, options);
 
 	if (abbrev == -1)

diff --git a/builtin/commit.c b/builtin/commit.c
index c20426b..b0f86c8 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -1072,7 +1072,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (patch_interactive)
 		interactive = 1;
 
-	if (!!also + !!only + !!all + !!interactive > 1)
+	if (also + only + all + interactive > 1)
 		die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
 	if (argc == 0 && (also || (only && !amend)))
 		die(_("No paths with --include/--only does not make sense."));

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index a908a34..20fcf8c 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c

@@ -331,7 +331,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix, opts, name_rev_usage, 0);
-	if (!!all + !!transform_stdin + !!argc > 1) {
+	if (all + transform_stdin + !!argc > 1) {
 		error("Specify either a list, or --all, not both!");
 		usage_with_options(name_rev_usage, opts);
 	}

-- 
1.8.4.rc0.16.g7fca822.dirty