threads / patch / 47372

v2, 4 partsgit-compat-util: introduce skip_to_optional_val()

Subject: [PATCH v2 1/4] git-compat-util: introduce skip_to_optional_val()

## tl;dr

7 messages between Dec 4, 2017 and Dec 4, 2017. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

Christian Couder· Dec 4, 2017, 12:56 UTC · lore
From: Christian Couder <christian.couder@gmail.com>
We often accept both a "--key" option and a "--key=<val>" option.
These options currently are parsed using something like:
if (!strcmp(arg, "--key")) {
	/* do something */
} else if (skip_prefix(arg, "--key=", &arg)) {
	/* do something with arg */
}
which is a bit cumbersome compared to just:
if (skip_to_optional_val(arg, "--key", &arg)) {
	/* do something with arg */
}

This also introduces skip_to_optional_val_default() for the few cases where something different should be done when the first argument is exactly "--key" than when it is exactly "--key=".

In general it is better for UI consistency and simplicity if "--key" and "--key=" do the same thing though, so that using skip_to_optional_val() should be encouraged compared to skip_to_optional_val_default().

Note that these functions can be used to parse any "key=value" string where "key" is also considered as valid, not just command line options.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 git-compat-util.h | 23 +++++++++++++++++++++++
 strbuf.c          | 20 ++++++++++++++++++++
 2 files changed, 43 insertions(+)

After thinking about it a bit more and taking a look at the current code, I thought that it was probably best to have both skip_to_optional_val() and skip_to_optional_val_default().

The changes compared to previous version are:
  - 2 new functions instead of 1
  - "optional" instead of "opt" in the function names
  - the big function is not inlined
  - more code in diff.c is simplified using the functions 
Show changes to 2 files +43 −0

git-compat-util.h, strbuf.c

diff --git a/git-compat-util.h b/git-compat-util.h
index cedad4d581..2858d66f85 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -484,6 +484,29 @@ static inline int skip_prefix(const char *str, const char *prefix,
 	return 0;
 }
 
+/*
+ * If the string "str" is the same as the string in "prefix", then the "val"
+ * parameter is set to the "def" parameter and 1 is returned.
+ * If the string "str" begins with the string found in "prefix" and then a
+ * "=" sign, then the "val" parameter is set to "str + strlen(prefix) + 1"
+ * (i.e., to the point in the string right after the prefix and the "=" sign),
+ * and 1 is returned.
+ *
+ * Otherwise, return 0 and leave "val" untouched.
+ *
+ * When we accept both a "--key" and a "--key=<val>" option, this function
+ * can be used instead of !strcmp(arg, "--key") and then
+ * skip_prefix(arg, "--key=", &arg) to parse such an option.
+ */
+int skip_to_optional_val_default(const char *str, const char *prefix,
+				 const char **val, const char *def);
+
+static inline int skip_to_optional_val(const char *str, const char *prefix,
+				       const char **val)
+{
+	return skip_to_optional_val_default(str, prefix, val, "");
+}
+
 /*
  * Like skip_prefix, but promises never to read past "len" bytes of the input
  * buffer, and returns the remaining number of bytes in "out" via "outlen".
diff --git a/strbuf.c b/strbuf.c
index 323c49ceb3..3430499f9e 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -11,6 +11,26 @@ int starts_with(const char *str, const char *prefix)
 			return 0;
 }
 
+int skip_to_optional_val_default(const char *str, const char *prefix,
+				 const char **val, const char *def)
+{
+	const char *p;
+
+	if (!skip_prefix(str, prefix, &p))
+		return 0;
+
+	if (!*p) {
+		*val = def;
+		return 1;
+	}
+
+	if (*p != '=')
+		return 0;
+
+	*val = p + 1;
+	return 1;
+}
+
 /*
  * Used as the default ->buf value, so that people can always assume
  * buf is non NULL and ->buf is NUL terminated even for a freshly
-- 
2.15.1.274.g3f22e311ce.dirty
Christian Couder· Dec 4, 2017, 12:56 UTC · re: Christian Couder · lore

[PATCH v2 2/4] index-pack: use skip_to_optional_val()

From: Christian Couder <christian.couder@gmail.com>

Let's simplify index-pack option parsing using skip_to_optional_val().

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/index-pack.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)
Show changes to builtin/index-pack.c +3 −8
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 8ec459f522..20b4b85a6a 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1660,10 +1660,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 				from_stdin = 1;
 			} else if (!strcmp(arg, "--fix-thin")) {
 				fix_thin_pack = 1;
-			} else if (!strcmp(arg, "--strict")) {
-				strict = 1;
-				do_fsck_object = 1;
-			} else if (skip_prefix(arg, "--strict=", &arg)) {
+			} else if (skip_to_optional_val(arg, "--strict", &arg)) {
 				strict = 1;
 				do_fsck_object = 1;
 				fsck_set_msg_types(&fsck_options, arg);
@@ -1679,10 +1676,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 				verify = 1;
 				show_stat = 1;
 				stat_only = 1;
-			} else if (!strcmp(arg, "--keep")) {
-				keep_msg = "";
-			} else if (starts_with(arg, "--keep=")) {
-				keep_msg = arg + 7;
+			} else if (skip_to_optional_val(arg, "--keep", &keep_msg)) {
+				; /* nothing to do */
 			} else if (starts_with(arg, "--threads=")) {
 				char *end;
 				nr_threads = strtoul(arg+10, &end, 0);
-- 
2.15.1.274.g3f22e311ce.dirty
Christian Couder· Dec 4, 2017, 12:56 UTC · re: Christian Couder · lore

[PATCH v2 3/4] diff: use skip_to_optional_val()

From: Christian Couder <christian.couder@gmail.com>

Let's simplify diff option parsing using skip_to_optional_val().

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 diff.c | 29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)
Show changes to diff.c +10 −19
diff --git a/diff.c b/diff.c
index 2ebe2227b4..83509f0658 100644
--- a/diff.c
+++ b/diff.c
@@ -4508,17 +4508,12 @@ int diff_opt_parse(struct diff_options *options,
 		options->output_format |= DIFF_FORMAT_NUMSTAT;
 	else if (!strcmp(arg, "--shortstat"))
 		options->output_format |= DIFF_FORMAT_SHORTSTAT;
-	else if (!strcmp(arg, "-X") || !strcmp(arg, "--dirstat"))
-		return parse_dirstat_opt(options, "");
-	else if (skip_prefix(arg, "-X", &arg))
-		return parse_dirstat_opt(options, arg);
-	else if (skip_prefix(arg, "--dirstat=", &arg))
+	else if (skip_prefix(arg, "-X", &arg) ||
+		 skip_to_optional_val(arg, "--dirstat", &arg))
 		return parse_dirstat_opt(options, arg);
 	else if (!strcmp(arg, "--cumulative"))
 		return parse_dirstat_opt(options, "cumulative");
-	else if (!strcmp(arg, "--dirstat-by-file"))
-		return parse_dirstat_opt(options, "files");
-	else if (skip_prefix(arg, "--dirstat-by-file=", &arg)) {
+	else if (skip_to_optional_val(arg, "--dirstat-by-file", &arg)) {
 		parse_dirstat_opt(options, "files");
 		return parse_dirstat_opt(options, arg);
 	}
@@ -4540,13 +4535,13 @@ int diff_opt_parse(struct diff_options *options,
 		return stat_opt(options, av);
 
 	/* renames options */
-	else if (starts_with(arg, "-B") || starts_with(arg, "--break-rewrites=") ||
-		 !strcmp(arg, "--break-rewrites")) {
+	else if (starts_with(arg, "-B") ||
+		 skip_to_optional_val(arg, "--break-rewrites", &optarg)) {
 		if ((options->break_opt = diff_scoreopt_parse(arg)) == -1)
 			return error("invalid argument to -B: %s", arg+2);
 	}
-	else if (starts_with(arg, "-M") || starts_with(arg, "--find-renames=") ||
-		 !strcmp(arg, "--find-renames")) {
+	else if (starts_with(arg, "-M") ||
+		 skip_to_optional_val(arg, "--find-renames", &optarg)) {
 		if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)
 			return error("invalid argument to -M: %s", arg+2);
 		options->detect_rename = DIFF_DETECT_RENAME;
@@ -4554,8 +4549,8 @@ int diff_opt_parse(struct diff_options *options,
 	else if (!strcmp(arg, "-D") || !strcmp(arg, "--irreversible-delete")) {
 		options->irreversible_delete = 1;
 	}
-	else if (starts_with(arg, "-C") || starts_with(arg, "--find-copies=") ||
-		 !strcmp(arg, "--find-copies")) {
+	else if (starts_with(arg, "-C") ||
+		 skip_to_optional_val(arg, "--find-copies", &optarg)) {
 		if (options->detect_rename == DIFF_DETECT_COPY)
 			options->flags.find_copies_harder = 1;
 		if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)
@@ -4568,12 +4563,8 @@ int diff_opt_parse(struct diff_options *options,
 		options->flags.rename_empty = 1;
 	else if (!strcmp(arg, "--no-rename-empty"))
 		options->flags.rename_empty = 0;
-	else if (!strcmp(arg, "--relative"))
-		options->flags.relative_name = 1;
-	else if (skip_prefix(arg, "--relative=", &arg)) {
+	else if (skip_to_optional_val(arg, "--relative", &options->prefix))
 		options->flags.relative_name = 1;
-		options->prefix = arg;
-	}
 
 	/* xdiff options */
 	else if (!strcmp(arg, "--minimal"))
-- 
2.15.1.274.g3f22e311ce.dirty
Junio C Hamano· Dec 4, 2017, 14:23 UTC · re: Christian Couder · lore

Re: [PATCH v2 3/4] diff: use skip_to_optional_val()

Christian Couder <christian.couder@gmail.com> writes:
Show 11 quoted lines
> @@ -4540,13 +4535,13 @@ int diff_opt_parse(struct diff_options *options,
>  		return stat_opt(options, av);
>  
>  	/* renames options */
> -	else if (starts_with(arg, "-B") || starts_with(arg, "--break-rewrites=") ||
> -		 !strcmp(arg, "--break-rewrites")) {
> +	else if (starts_with(arg, "-B") ||
> +		 skip_to_optional_val(arg, "--break-rewrites", &optarg)) {
>  		if ((options->break_opt = diff_scoreopt_parse(arg)) == -1)
>  			return error("invalid argument to -B: %s", arg+2);
>  	}

This is curious; "optarg" gets something, but it is not used (what is passed to scoreopt_parse() is still "arg".

It merely is curious and not wrong; the actual parsing of the whole thing is done in scoreopt_parse() and skip_to_optional_val() is used merely as a substitute for "the thing must either be --break-rewrites or must begin with --break-rewrites=" check that is done with starts_with() and !strcmp().

It probably makes sense to allow skip_to_optional() to take a NULL for the third parameter to clarify a callsite like this. Otherwise the readers will wonder who consumes optarg, and why it is OK for it to be sometimes set and sometimes left undefined.

> -	else if (starts_with(arg, "-M") || starts_with(arg, "--find-renames=") ||
> -		 !strcmp(arg, "--find-renames")) {
> +	else if (starts_with(arg, "-M") ||
> +		 skip_to_optional_val(arg, "--find-renames", &optarg)) {
Likewise.
Show 11 quoted lines
> @@ -4554,8 +4549,8 @@ int diff_opt_parse(struct diff_options *options,
>  	else if (!strcmp(arg, "-D") || !strcmp(arg, "--irreversible-delete")) {
>  		options->irreversible_delete = 1;
>  	}
> -	else if (starts_with(arg, "-C") || starts_with(arg, "--find-copies=") ||
> -		 !strcmp(arg, "--find-copies")) {
> +	else if (starts_with(arg, "-C") ||
> +		 skip_to_optional_val(arg, "--find-copies", &optarg)) {
>  		if (options->detect_rename == DIFF_DETECT_COPY)
>  			options->flags.find_copies_harder = 1;
>  		if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)
Likewise.
Christian Couder· Dec 4, 2017, 12:56 UTC · re: Christian Couder · lore

[PATCH v2 4/4] diff: use skip_to_optional_val_default()

Let's simplify diff option parsing using skip_to_optional_val_default().

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 diff.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)
Show changes to diff.c +4 −16
diff --git a/diff.c b/diff.c
index 83509f0658..d234b4f0cb 100644
--- a/diff.c
+++ b/diff.c
@@ -4619,9 +4619,7 @@ int diff_opt_parse(struct diff_options *options,
 	else if (!strcmp(arg, "--no-follow")) {
 		options->flags.follow_renames = 0;
 		options->flags.default_follow_renames = 0;
-	} else if (!strcmp(arg, "--color"))
-		options->use_color = 1;
-	else if (skip_prefix(arg, "--color=", &arg)) {
+	} else if (skip_to_optional_val_default(arg, "--color", &arg, "always")) {
 		int value = git_config_colorbool(NULL, arg);
 		if (value < 0)
 			return error("option `color' expects \"always\", \"auto\", or \"never\"");
@@ -4641,14 +4639,9 @@ int diff_opt_parse(struct diff_options *options,
 		if (cm < 0)
 			die("bad --color-moved argument: %s", arg);
 		options->color_moved = cm;
-	} else if (!strcmp(arg, "--color-words")) {
-		options->use_color = 1;
-		options->word_diff = DIFF_WORDS_COLOR;
-	}
-	else if (skip_prefix(arg, "--color-words=", &arg)) {
+	} else if (skip_to_optional_val_default(arg, "--color-words", &options->word_regex, NULL)) {
 		options->use_color = 1;
 		options->word_diff = DIFF_WORDS_COLOR;
-		options->word_regex = arg;
 	}
 	else if (!strcmp(arg, "--word-diff")) {
 		if (options->word_diff == DIFF_WORDS_NONE)
@@ -4687,15 +4680,10 @@ int diff_opt_parse(struct diff_options *options,
 		options->flags.textconv_set_via_cmdline = 1;
 	} else if (!strcmp(arg, "--no-textconv"))
 		options->flags.allow_textconv = 0;
-	else if (!strcmp(arg, "--ignore-submodules")) {
-		options->flags.override_submodule_config = 1;
-		handle_ignore_submodules_arg(options, "all");
-	} else if (skip_prefix(arg, "--ignore-submodules=", &arg)) {
+	else if (skip_to_optional_val_default(arg, "--ignore-submodules", &arg, "all")) {
 		options->flags.override_submodule_config = 1;
 		handle_ignore_submodules_arg(options, arg);
-	} else if (!strcmp(arg, "--submodule"))
-		options->submodule_format = DIFF_SUBMODULE_LOG;
-	else if (skip_prefix(arg, "--submodule=", &arg))
+	} else if (skip_to_optional_val_default(arg, "--submodule", &arg, "log"))
 		return parse_submodule_opt(options, arg);
 	else if (skip_prefix(arg, "--ws-error-highlight=", &arg))
 		return parse_ws_error_highlight_opt(options, arg);
-- 
2.15.1.274.g3f22e311ce.dirty
Junio C Hamano· Dec 4, 2017, 14:28 UTC · re: Christian Couder · lore

Re: [PATCH v2 4/4] diff: use skip_to_optional_val_default()

Christian Couder <christian.couder@gmail.com> writes:
Show 5 quoted lines
> -	} else if (!strcmp(arg, "--submodule"))
> -		options->submodule_format = DIFF_SUBMODULE_LOG;
> -	else if (skip_prefix(arg, "--submodule=", &arg))
> +	} else if (skip_to_optional_val_default(arg, "--submodule", &arg, "log"))
>  		return parse_submodule_opt(options, arg);

It may be annoying if we later diagnose that DIFF_SUBMODULE_LOG may not be used in the context (e.g. together with some other optoins that are set in "options" variable) and have to throw an error message at the user. parse_submodule_opt() would not know if that string "log" came from the user or substituted by the helper, and in the latter case, "don't use --submodule=log here" is a message that is not quite appropriate.

This may be minor enough not matter in practice, but I dunno. I didn't look at other hunks in this step very carefully but I would expect that whenever you use "default" to substitute, you'll have the same information lossage.

Junio C Hamano· Dec 4, 2017, 14:31 UTC · re: Christian Couder · lore

Re: [PATCH v2 1/4] git-compat-util: introduce skip_to_optional_val()

Christian Couder <christian.couder@gmail.com> writes:
Show 40 quoted lines
> From: Christian Couder <christian.couder@gmail.com>
>
> We often accept both a "--key" option and a "--key=<val>" option.
>
> These options currently are parsed using something like:
>
> if (!strcmp(arg, "--key")) {
> 	/* do something */
> } else if (skip_prefix(arg, "--key=", &arg)) {
> 	/* do something with arg */
> }
>
> which is a bit cumbersome compared to just:
>
> if (skip_to_optional_val(arg, "--key", &arg)) {
> 	/* do something with arg */
> }
>
> This also introduces skip_to_optional_val_default() for the few
> cases where something different should be done when the first
> argument is exactly "--key" than when it is exactly "--key=".
>
> In general it is better for UI consistency and simplicity if
> "--key" and "--key=" do the same thing though, so that using
> skip_to_optional_val() should be encouraged compared to
> skip_to_optional_val_default().
>
> Note that these functions can be used to parse any "key=value"
> string where "key" is also considered as valid, not just
> command line options.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  git-compat-util.h | 23 +++++++++++++++++++++++
>  strbuf.c          | 20 ++++++++++++++++++++
>  2 files changed, 43 insertions(+)
>
> After thinking about it a bit more and taking a look at the
> current code, I thought that it was probably best to have
> both skip_to_optional_val() and skip_to_optional_val_default().

I guess we came to the same conclusion independently while our mails crossed ;-)

>   - 2 new functions instead of 1
>   - "optional" instead of "opt" in the function names

I thought that the more important part you agreed was s/val/arg/, though.

I had a few small comments on later steps, but I think these are moving in the right direction.

Thanks.

← back to recent threads