threads / patch / 50281

patch, 3 partsnd/diff-parseopt fixups

Subject: [PATCH 0/3] nd/diff-parseopt fixups

## tl;dr

10 messages between Jan 22, 2019 and Jan 22, 2019. Diffs are folded; open one to read it.

replies: 9people: 2as markdown or json

Nguyễn Thái Ngọc Duy· Jan 22, 2019, 00:26 UTC · lore

Let me see if I could just fix up this series on top like this. Failing this, I'll break it down and resend in small chunks.

This redoes a85e5a0108 (parse-options: allow keep-unknown + stop-at-non-opt combination - 2019-01-17), deletes dead functions and rewrites b9b760ed1c (diff.c: convert -B|--break-rewrites - 2019-01-17) a bit.

Nguyễn Thái Ngọc Duy (3):
  Revert "parse-options: allow keep-unknown + stop-at-non-opt
    combination"
  diff.c: delete unused functions
  diff.c: simplify diff_opt_break_rewrites()

 diff.c          | 74 ++-----------------------------------------------
 parse-options.c |  4 +++
 2 files changed, 6 insertions(+), 72 deletions(-)

-- 
2.20.1.560.g70ca8b83ee

Nguyễn Thái Ngọc Duy· Jan 22, 2019, 00:26 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 1/3] Revert "parse-options: allow keep-unknown + stop-at-non-opt combination"

This reverts commit a85e5a01080e35da0f94bf98f5f334c64e214ce3, which is a revert of 0d260f9a09 (parseopt: prevent KEEP_UNKNOWN and STOP_AT_NON_OPTION from being used together - 2009-03-09).

As described in 0d260f9a09, this option combination is tricky to get right and should be prevented unless you know what you're doing. In the one-shot mode, the diff and revision parsers are very well aware of remaining arguments being potentially unknown options or revisions and should handle the combination safely.

Reinstate the check added by 0d260f9a09. Modify it a bit to let one-shot mode through.

Noticed-by: Stefan Beller <sbeller@google.com>
---
 parse-options.c | 4 ++++
 1 file changed, 4 insertions(+)

Show changes to parse-options.c +4 −0

diff --git a/parse-options.c b/parse-options.c
index a0ff8ea225..cec74522e5 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -466,6 +466,10 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 	ctx->prefix = prefix;
 	ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);
 	ctx->flags = flags;
+	if ((flags & PARSE_OPT_KEEP_UNKNOWN) &&
+	    (flags & PARSE_OPT_STOP_AT_NON_OPTION) &&
+	    !(flags & PARSE_OPT_ONE_SHOT))
+		BUG("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
 	if ((flags & PARSE_OPT_ONE_SHOT) &&
 	    (flags & PARSE_OPT_KEEP_ARGV0))
 		BUG("Can't keep argv0 if you don't have it");

-- 
2.20.1.560.g70ca8b83ee

Nguyễn Thái Ngọc Duy· Jan 22, 2019, 00:26 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 3/3] diff.c: simplify diff_opt_break_rewrites()

Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
 diff.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

Show changes to diff.c +2 −7

diff --git a/diff.c b/diff.c
index da5ba835ce..2351571251 100644
--- a/diff.c
+++ b/diff.c
@@ -4814,20 +4814,15 @@ static int diff_opt_break_rewrites(const struct option *opt,
 				   const char *arg, int unset)
 {
 	int *break_opt = opt->value;
-	int opt1, opt2;
+	int opt1, opt2 = 0;
 
 	BUG_ON_OPT_NEG(unset);
 	if (!arg)
 		arg = "";
 	opt1 = parse_rename_score(&arg);
-	switch (*arg) {
-	case '\0':
-		opt2 = 0;
-		break;
-	case '/':
+	if (*arg == '/') {
 		arg++;
 		opt2 = parse_rename_score(&arg);
-		break;
 	}
 	if (*arg != 0)
 		return error(_("%s expects <n>/<m> form"), opt->long_name);

-- 
2.20.1.560.g70ca8b83ee

Junio C Hamano· Jan 22, 2019, 20:50 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 3/3] diff.c: simplify diff_opt_break_rewrites()

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 19 quoted lines

> -	int opt1, opt2;
> +	int opt1, opt2 = 0;
>  
>  	BUG_ON_OPT_NEG(unset);
>  	if (!arg)
>  		arg = "";
>  	opt1 = parse_rename_score(&arg);
> -	switch (*arg) {
> -	case '\0':
> -		opt2 = 0;
> -		break;
> -	case '/':
> +	if (*arg == '/') {
>  		arg++;
>  		opt2 = parse_rename_score(&arg);
> -		break;
>  	}
>  	if (*arg != 0)
>  		return error(_("%s expects <n>/<m> form"), opt->long_name);

Good. I was about to complain on the lack of "default" that catches the error at the end of "<n>" in the switch(), but because this follow-up validation is trying to catch "<n>" form (i.e. single score without slash) whose "<n>" is malformed, and "<n>/<m>" form whose "<m>" is malformed, which is kind of clever, but it is not very easy to understand what is going on, it makes sense to get rid of the switch() and make it if() statement.

It would make it even easier to follow if you did

	if (*arg == '/') {
		opt2 = ...;
		arg++;
	} else {
		opt2 = 0;
	}
	if (*arg)
		return error(...);

It makes it clear that opt2==0 means <n> form and not <n>/<m> form, by having an explicit assignment while we parse what *arg points at, without the initialization to 0 in the variable definition.

But this should be squashed in the original patch. Having an "oops, it was horribly unreadble---how about doing something like this?" incremental is good during a discussion, and having a "what we have seen is basically good and proven solid, but here is a small improvement" incremental is good for code that is stable enough to build on (read: at least in 'next'), but it is not particularly good for a topic not yet in 'next' yet.

Junio C Hamano· Jan 22, 2019, 23:18 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] diff.c: simplify diff_opt_break_rewrites()

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

Show 6 quoted lines

> It would make it even easier to follow if you did
>
> 	if (*arg == '/') {
> 		opt2 = ...;
> 		arg++;
> 	} else {

Oops, this should read "else if (!*arg) {", of course, to match the original. Sorry for the noise.

> 		opt2 = 0;
> 	}

And then we'd want a blank line here to make it clear that the previous if/else cascade has finished, and the error checking we see next is not part of it.

Show 6 quoted lines

> 	if (*arg)
> 		return error(...);
>
> It makes it clear that opt2==0 means <n> form and not <n>/<m> form,
> by having an explicit assignment while we parse what *arg points at,
> without the initialization to 0 in the variable definition.

Nguyễn Thái Ngọc Duy· Jan 22, 2019, 00:26 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 2/3] diff.c: delete unused functions

After the parseopt conversion, a few functions are no longer needed. Delete them.

Noticed-by: SZEDER Gábor <szeder.dev@gmail.com>
Noticed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
 diff.c | 65 ----------------------------------------------------------
 1 file changed, 65 deletions(-)

Show changes to diff.c +0 −65

diff --git a/diff.c b/diff.c
index 747e691190..da5ba835ce 100644
--- a/diff.c
+++ b/diff.c
@@ -104,11 +104,6 @@ static const char *color_diff_slots[] = {
 	[DIFF_FILE_NEW_BOLD]	      = "newBold",
 };
 
-static NORETURN void die_want_option(const char *option_name)
-{
-	die(_("option '%s' requires a value"), option_name);
-}
-
 define_list_config_array_extra(color_diff_slots, {"plain"});
 
 static int parse_diff_color_slot(const char *var)
@@ -4578,66 +4573,6 @@ void diff_setup_done(struct diff_options *options)
 	FREE_AND_NULL(options->parseopts);
 }
 
-static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)
-{
-	char c, *eq;
-	int len;
-
-	if (*arg != '-')
-		return 0;
-	c = *++arg;
-	if (!c)
-		return 0;
-	if (c == arg_short) {
-		c = *++arg;
-		if (!c)
-			return 1;
-		if (val && isdigit(c)) {
-			char *end;
-			int n = strtoul(arg, &end, 10);
-			if (*end)
-				return 0;
-			*val = n;
-			return 1;
-		}
-		return 0;
-	}
-	if (c != '-')
-		return 0;
-	arg++;
-	eq = strchrnul(arg, '=');
-	len = eq - arg;
-	if (!len || strncmp(arg, arg_long, len))
-		return 0;
-	if (*eq) {
-		int n;
-		char *end;
-		if (!isdigit(*++eq))
-			return 0;
-		n = strtoul(eq, &end, 10);
-		if (*end)
-			return 0;
-		*val = n;
-	}
-	return 1;
-}
-
-static inline int short_opt(char opt, const char **argv,
-			    const char **optarg)
-{
-	const char *arg = argv[0];
-	if (arg[0] != '-' || arg[1] != opt)
-		return 0;
-	if (arg[2] != '\0') {
-		*optarg = arg + 2;
-		return 1;
-	}
-	if (!argv[1])
-		die("Option '%c' requires a value", opt);
-	*optarg = argv[1];
-	return 2;
-}
-
 int parse_long_opt(const char *opt, const char **argv,
 		   const char **optarg)
 {

-- 
2.20.1.560.g70ca8b83ee

Nguyễn Thái Ngọc Duy· Jan 22, 2019, 00:30 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 0/3] nd/diff-parseopt fixups

Resend with s-o-b. Oops.

Nguyễn Thái Ngọc Duy (3):
  Revert "parse-options: allow keep-unknown + stop-at-non-opt
    combination"
  diff.c: delete unused functions
  diff.c: simplify diff_opt_break_rewrites()

 diff.c          | 74 ++-----------------------------------------------
 parse-options.c |  4 +++
 2 files changed, 6 insertions(+), 72 deletions(-)

-- 
2.20.1.560.g70ca8b83ee

Nguyễn Thái Ngọc Duy· Jan 22, 2019, 00:30 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 1/3] Revert "parse-options: allow keep-unknown + stop-at-non-opt combination"

This reverts commit a85e5a01080e35da0f94bf98f5f334c64e214ce3, which is a revert of 0d260f9a09 (parseopt: prevent KEEP_UNKNOWN and STOP_AT_NON_OPTION from being used together - 2009-03-09).

Reinstate the check added by 0d260f9a09. Modify it a bit to let one-shot mode through.

Noticed-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 parse-options.c | 4 ++++
 1 file changed, 4 insertions(+)

Show changes to parse-options.c +4 −0

diff --git a/parse-options.c b/parse-options.c
index a0ff8ea225..cec74522e5 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -466,6 +466,10 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 	ctx->prefix = prefix;
 	ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);
 	ctx->flags = flags;
+	if ((flags & PARSE_OPT_KEEP_UNKNOWN) &&
+	    (flags & PARSE_OPT_STOP_AT_NON_OPTION) &&
+	    !(flags & PARSE_OPT_ONE_SHOT))
+		BUG("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
 	if ((flags & PARSE_OPT_ONE_SHOT) &&
 	    (flags & PARSE_OPT_KEEP_ARGV0))
 		BUG("Can't keep argv0 if you don't have it");

-- 
2.20.1.560.g70ca8b83ee

Nguyễn Thái Ngọc Duy· Jan 22, 2019, 00:30 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 2/3] diff.c: delete unused functions

After the parseopt conversion, a few functions are no longer needed. Delete them.

Noticed-by: SZEDER Gábor <szeder.dev@gmail.com>
Noticed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 65 ----------------------------------------------------------
 1 file changed, 65 deletions(-)

Show changes to diff.c +0 −65

diff --git a/diff.c b/diff.c
index 747e691190..da5ba835ce 100644
--- a/diff.c
+++ b/diff.c
@@ -104,11 +104,6 @@ static const char *color_diff_slots[] = {
 	[DIFF_FILE_NEW_BOLD]	      = "newBold",
 };
 
-static NORETURN void die_want_option(const char *option_name)
-{
-	die(_("option '%s' requires a value"), option_name);
-}
-
 define_list_config_array_extra(color_diff_slots, {"plain"});
 
 static int parse_diff_color_slot(const char *var)
@@ -4578,66 +4573,6 @@ void diff_setup_done(struct diff_options *options)
 	FREE_AND_NULL(options->parseopts);
 }
 
-static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)
-{
-	char c, *eq;
-	int len;
-
-	if (*arg != '-')
-		return 0;
-	c = *++arg;
-	if (!c)
-		return 0;
-	if (c == arg_short) {
-		c = *++arg;
-		if (!c)
-			return 1;
-		if (val && isdigit(c)) {
-			char *end;
-			int n = strtoul(arg, &end, 10);
-			if (*end)
-				return 0;
-			*val = n;
-			return 1;
-		}
-		return 0;
-	}
-	if (c != '-')
-		return 0;
-	arg++;
-	eq = strchrnul(arg, '=');
-	len = eq - arg;
-	if (!len || strncmp(arg, arg_long, len))
-		return 0;
-	if (*eq) {
-		int n;
-		char *end;
-		if (!isdigit(*++eq))
-			return 0;
-		n = strtoul(eq, &end, 10);
-		if (*end)
-			return 0;
-		*val = n;
-	}
-	return 1;
-}
-
-static inline int short_opt(char opt, const char **argv,
-			    const char **optarg)
-{
-	const char *arg = argv[0];
-	if (arg[0] != '-' || arg[1] != opt)
-		return 0;
-	if (arg[2] != '\0') {
-		*optarg = arg + 2;
-		return 1;
-	}
-	if (!argv[1])
-		die("Option '%c' requires a value", opt);
-	*optarg = argv[1];
-	return 2;
-}
-
 int parse_long_opt(const char *opt, const char **argv,
 		   const char **optarg)
 {

-- 
2.20.1.560.g70ca8b83ee

Nguyễn Thái Ngọc Duy· Jan 22, 2019, 00:30 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 3/3] diff.c: simplify diff_opt_break_rewrites()

Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

Show changes to diff.c +2 −7

diff --git a/diff.c b/diff.c
index da5ba835ce..2351571251 100644
--- a/diff.c
+++ b/diff.c
@@ -4814,20 +4814,15 @@ static int diff_opt_break_rewrites(const struct option *opt,
 				   const char *arg, int unset)
 {
 	int *break_opt = opt->value;
-	int opt1, opt2;
+	int opt1, opt2 = 0;
 
 	BUG_ON_OPT_NEG(unset);
 	if (!arg)
 		arg = "";
 	opt1 = parse_rename_score(&arg);
-	switch (*arg) {
-	case '\0':
-		opt2 = 0;
-		break;
-	case '/':
+	if (*arg == '/') {
 		arg++;
 		opt2 = parse_rename_score(&arg);
-		break;
 	}
 	if (*arg != 0)
 		return error(_("%s expects <n>/<m> form"), opt->long_name);

-- 
2.20.1.560.g70ca8b83ee