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

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

[Christian Couder]

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 

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

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

[Christian Couder]

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(-)

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

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

[Christian Couder]

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(-)

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

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

[Christian Couder]

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(-)

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

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

[Junio C Hamano]

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

> @@ -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.

> @@ -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.

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

[Junio C Hamano]

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

> -	} 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.

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

[Junio C Hamano]

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

> 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.