[PATCH v3 1/7] git-compat-util: introduce skip_to_optional_arg()

[PATCH v3 1/7] git-compat-util: introduce skip_to_optional_arg()

[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_arg(arg, "--key", &arg)) {
	/* do something with arg */
}

This also introduces skip_to_optional_arg_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_arg() should be encouraged compared to
skip_to_optional_arg_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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-compat-util.h | 23 +++++++++++++++++++++++
 strbuf.c          | 22 ++++++++++++++++++++++
 2 files changed, 45 insertions(+)

The changes compared to v2 are:

  - s/_val/_arg/ in the name of the functions
  - s/val/arg/ in the name of the third argument of the functions
  - works with NULL as third argument of the functions
  - "--relative" is handled correctly in a separate new patch from Junio
  - tests are added for "--relative" in a separate new patch from Jake
  - a new test reindenting patch from Junio is added

diff --git a/git-compat-util.h b/git-compat-util.h
index cedad4d581..68b2ad531e 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 "arg"
+ * 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 "arg" 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 "arg" 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_arg_default(const char *str, const char *prefix,
+				 const char **arg, const char *def);
+
+static inline int skip_to_optional_arg(const char *str, const char *prefix,
+				       const char **arg)
+{
+	return skip_to_optional_arg_default(str, prefix, arg, "");
+}
+
 /*
  * 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..29169b8ef8 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -11,6 +11,28 @@ int starts_with(const char *str, const char *prefix)
 			return 0;
 }
 
+int skip_to_optional_arg_default(const char *str, const char *prefix,
+				 const char **arg, const char *def)
+{
+	const char *p;
+
+	if (!skip_prefix(str, prefix, &p))
+		return 0;
+
+	if (!*p) {
+		if (arg)
+			*arg = def;
+		return 1;
+	}
+
+	if (*p != '=')
+		return 0;
+
+	if (arg)
+		*arg = 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.361.g8b07d831d0

[PATCH v3 2/7] index-pack: use skip_to_optional_arg()

[Christian Couder]

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

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

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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..4c51aec81f 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_arg(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_arg(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.361.g8b07d831d0

[PATCH v3 3/7] diff: use skip_to_optional_arg()

[Christian Couder]

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

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

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/diff.c b/diff.c
index 2ebe2227b4..464a53adb5 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_arg(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_arg(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_arg(arg, "--break-rewrites", NULL)) {
 		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_arg(arg, "--find-renames", NULL)) {
 		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_arg(arg, "--find-copies", NULL)) {
 		if (options->detect_rename == DIFF_DETECT_COPY)
 			options->flags.find_copies_harder = 1;
 		if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)
-- 
2.15.1.361.g8b07d831d0

[PATCH v3 4/7] diff: use skip_to_optional_arg_default()

[Christian Couder]

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

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

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/diff.c b/diff.c
index 464a53adb5..28e1ab168f 100644
--- a/diff.c
+++ b/diff.c
@@ -4623,9 +4623,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_arg_default(arg, "--color", &arg, "always")) {
 		int value = git_config_colorbool(NULL, arg);
 		if (value < 0)
 			return error("option `color' expects \"always\", \"auto\", or \"never\"");
@@ -4645,14 +4643,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_arg_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)
@@ -4691,15 +4684,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_arg_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_arg_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.361.g8b07d831d0

[PATCH v3 5/7] diff: use skip_to_optional_arg_default() in parsing --relative

[Christian Couder]

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

Helped-by: Jacob Keller <jacob.keller@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index 28e1ab168f..3f14cdace6 100644
--- a/diff.c
+++ b/diff.c
@@ -4563,11 +4563,10 @@ 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"))
+	else if (skip_to_optional_arg_default(arg, "--relative", &arg, NULL)) {
 		options->flags.relative_name = 1;
-	else if (skip_prefix(arg, "--relative=", &arg)) {
-		options->flags.relative_name = 1;
-		options->prefix = arg;
+		if (arg)
+			options->prefix = arg;
 	}
 
 	/* xdiff options */
-- 
2.15.1.361.g8b07d831d0

[PATCH v3 6/7] diff: add tests for --relative without optional prefix value

[Christian Couder]

From: Jacob Keller <jacob.keller@gmail.com>

We already have tests for --relative, but they currently only test when
a prefix has been provided. This fails to test the case where --relative
by itself should use the current directory as the prefix.

Teach the check_$type functions to take a directory argument to indicate
which subdirectory to run the git commands in. Add a new test which uses
this to test --relative without a prefix value.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 t/t4045-diff-relative.sh | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index 3950f5034d..7d68a6e2a5 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -13,6 +13,7 @@ test_expect_success 'setup' '
 '
 
 check_diff() {
+dir=$1; shift
 expect=$1; shift
 cat >expected <<EOF
 diff --git a/$expect b/$expect
@@ -24,50 +25,54 @@ index 0000000..25c05ef
 +other content
 EOF
 test_expect_success "-p $*" "
-	git diff -p $* HEAD^ >actual &&
+	git -C '$dir' diff -p $* HEAD^ >actual &&
 	test_cmp expected actual
 "
 }
 
 check_numstat() {
+dir=$1; shift
 expect=$1; shift
 cat >expected <<EOF
 1	0	$expect
 EOF
 test_expect_success "--numstat $*" "
 	echo '1	0	$expect' >expected &&
-	git diff --numstat $* HEAD^ >actual &&
+	git -C '$dir' diff --numstat $* HEAD^ >actual &&
 	test_cmp expected actual
 "
 }
 
 check_stat() {
+dir=$1; shift
 expect=$1; shift
 cat >expected <<EOF
  $expect | 1 +
  1 file changed, 1 insertion(+)
 EOF
 test_expect_success "--stat $*" "
-	git diff --stat $* HEAD^ >actual &&
+	git -C '$dir' diff --stat $* HEAD^ >actual &&
 	test_i18ncmp expected actual
 "
 }
 
 check_raw() {
+dir=$1; shift
 expect=$1; shift
 cat >expected <<EOF
 :000000 100644 0000000000000000000000000000000000000000 25c05ef3639d2d270e7fe765a67668f098092bc5 A	$expect
 EOF
 test_expect_success "--raw $*" "
-	git diff --no-abbrev --raw $* HEAD^ >actual &&
+	git -C '$dir' diff --no-abbrev --raw $* HEAD^ >actual &&
 	test_cmp expected actual
 "
 }
 
 for type in diff numstat stat raw; do
-	check_$type file2 --relative=subdir/
-	check_$type file2 --relative=subdir
-	check_$type dir/file2 --relative=sub
+	check_$type . file2 --relative=subdir/
+	check_$type . file2 --relative=subdir
+	check_$type . dir/file2 --relative=sub
+	check_$type subdir file2 --relative
 done
 
 test_done
-- 
2.15.1.361.g8b07d831d0

[PATCH v3 7/7] t4045: reindent to make helpers readable

[Christian Couder]

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

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4045-diff-relative.sh | 104 +++++++++++++++++++++++++----------------------
 1 file changed, 56 insertions(+), 48 deletions(-)

diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index 7d68a6e2a5..4df55884c4 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -12,60 +12,68 @@ test_expect_success 'setup' '
 	git commit -m one
 '
 
-check_diff() {
-dir=$1; shift
-expect=$1; shift
-cat >expected <<EOF
-diff --git a/$expect b/$expect
-new file mode 100644
-index 0000000..25c05ef
---- /dev/null
-+++ b/$expect
-@@ -0,0 +1 @@
-+other content
-EOF
-test_expect_success "-p $*" "
-	git -C '$dir' diff -p $* HEAD^ >actual &&
-	test_cmp expected actual
-"
+check_diff () {
+	dir=$1
+	shift
+	expect=$1
+	shift
+	cat >expected <<-EOF
+	diff --git a/$expect b/$expect
+	new file mode 100644
+	index 0000000..25c05ef
+	--- /dev/null
+	+++ b/$expect
+	@@ -0,0 +1 @@
+	+other content
+	EOF
+	test_expect_success "-p $*" "
+		git -C '$dir' diff -p $* HEAD^ >actual &&
+		test_cmp expected actual
+	"
 }
 
-check_numstat() {
-dir=$1; shift
-expect=$1; shift
-cat >expected <<EOF
-1	0	$expect
-EOF
-test_expect_success "--numstat $*" "
-	echo '1	0	$expect' >expected &&
-	git -C '$dir' diff --numstat $* HEAD^ >actual &&
-	test_cmp expected actual
-"
+check_numstat () {
+	dir=$1
+	shift
+	expect=$1
+	shift
+	cat >expected <<-EOF
+	1	0	$expect
+	EOF
+	test_expect_success "--numstat $*" "
+		echo '1	0	$expect' >expected &&
+		git -C '$dir' diff --numstat $* HEAD^ >actual &&
+		test_cmp expected actual
+	"
 }
 
-check_stat() {
-dir=$1; shift
-expect=$1; shift
-cat >expected <<EOF
- $expect | 1 +
- 1 file changed, 1 insertion(+)
-EOF
-test_expect_success "--stat $*" "
-	git -C '$dir' diff --stat $* HEAD^ >actual &&
-	test_i18ncmp expected actual
-"
+check_stat () {
+	dir=$1
+	shift
+	expect=$1
+	shift
+	cat >expected <<-EOF
+	 $expect | 1 +
+	 1 file changed, 1 insertion(+)
+	EOF
+	test_expect_success "--stat $*" "
+		git -C '$dir' diff --stat $* HEAD^ >actual &&
+		test_i18ncmp expected actual
+	"
 }
 
-check_raw() {
-dir=$1; shift
-expect=$1; shift
-cat >expected <<EOF
-:000000 100644 0000000000000000000000000000000000000000 25c05ef3639d2d270e7fe765a67668f098092bc5 A	$expect
-EOF
-test_expect_success "--raw $*" "
-	git -C '$dir' diff --no-abbrev --raw $* HEAD^ >actual &&
-	test_cmp expected actual
-"
+check_raw () {
+	dir=$1
+	shift
+	expect=$1
+	shift
+	cat >expected <<-EOF
+	:000000 100644 0000000000000000000000000000000000000000 25c05ef3639d2d270e7fe765a67668f098092bc5 A	$expect
+	EOF
+	test_expect_success "--raw $*" "
+		git -C '$dir' diff --no-abbrev --raw $* HEAD^ >actual &&
+		test_cmp expected actual
+	"
 }
 
 for type in diff numstat stat raw; do
-- 
2.15.1.361.g8b07d831d0

Re: [PATCH v3 1/7] git-compat-util: introduce skip_to_optional_arg()

[Jeff King]

On Sat, Dec 09, 2017 at 09:40:07PM +0100, Christian Couder wrote:

> The changes compared to v2 are:
> 
>   - s/_val/_arg/ in the name of the functions
>   - s/val/arg/ in the name of the third argument of the functions
>   - works with NULL as third argument of the functions

This whole series looks OK to me, but this third point made me wonder:
what would be the use of allowing NULL for the "arg" parameter?

I didn't see any use of this in the series, and I'm having trouble
figuring out how it would be useful. E.g., if I do:

  if (skip_to_optional_arg(arg, "--foo", NULL))
     ...

what can I do in "..."? I know we matched _some_ type of "--foo", but I
cannot know whether it was "--foo" or "--foo=bar", nor what "bar" is. It
could only be used by some kind of vague validator to say "well, at
least this looks like an option that I _could_ parse if I wanted to".

So I guess I don't mind it, as it does the most reasonable thing it can
when passed NULL, but I would be surprised if we ever actually exercise
the code path.

-Peff

Re: [PATCH v3 1/7] git-compat-util: introduce skip_to_optional_arg()

[Jeff King]

On Sun, Dec 10, 2017 at 09:31:18AM -0500, Jeff King wrote:

> On Sat, Dec 09, 2017 at 09:40:07PM +0100, Christian Couder wrote:
> 
> > The changes compared to v2 are:
> > 
> >   - s/_val/_arg/ in the name of the functions
> >   - s/val/arg/ in the name of the third argument of the functions
> >   - works with NULL as third argument of the functions
> 
> This whole series looks OK to me, but this third point made me wonder:
> what would be the use of allowing NULL for the "arg" parameter?
> 
> I didn't see any use of this in the series, and I'm having trouble
> figuring out how it would be useful. E.g., if I do:
> 
>   if (skip_to_optional_arg(arg, "--foo", NULL))
>      ...
> 
> what can I do in "..."? I know we matched _some_ type of "--foo", but I
> cannot know whether it was "--foo" or "--foo=bar", nor what "bar" is. It
> could only be used by some kind of vague validator to say "well, at
> least this looks like an option that I _could_ parse if I wanted to".
> 
> So I guess I don't mind it, as it does the most reasonable thing it can
> when passed NULL, but I would be surprised if we ever actually exercise
> the code path.

And of course as soon as I sent this, I went back and double-checked.
And indeed I totally missed this call:

  +       else if (starts_with(arg, "-B") ||
  +                skip_to_optional_arg(arg, "--break-rewrites", NULL)) {
	  if ((options->break_opt = diff_scoreopt_parse(arg)) == -1)

So that's kind-of weird, because we are parsing "-B", etc, and then
expecting it to be _reparsed_ by diff_scoreopt_parse. So the two
callsites must always match. IMHO this ought to do either:

  - we should just ask diff_scoreopt_parser to tell us if this was a
    valid option that it understood

or

  - parse up to the "=", and then ask the scoreopt parser to parse the
    remainder. This would require us passing 'B'/'C'/'M' to the
    function ourselves, I think that's a better pattern. It means we
    could reuse the parser for things like config values if we wanted to
    (our current diff.renames is a bool, but it would not be
    unreasonable for it to take a score).

None of that is a mess of your creation, though, so I'm OK punting on it
for now.

-Peff

Re: [PATCH v3 1/7] git-compat-util: introduce skip_to_optional_arg()

[Christian Couder]

On Sun, Dec 10, 2017 at 3:39 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Dec 10, 2017 at 09:31:18AM -0500, Jeff King wrote:
>
>> On Sat, Dec 09, 2017 at 09:40:07PM +0100, Christian Couder wrote:
>>
>> > The changes compared to v2 are:
>> >
>> >   - s/_val/_arg/ in the name of the functions
>> >   - s/val/arg/ in the name of the third argument of the functions
>> >   - works with NULL as third argument of the functions
>>
>> This whole series looks OK to me, but this third point made me wonder:
>> what would be the use of allowing NULL for the "arg" parameter?
>>
>> I didn't see any use of this in the series, and I'm having trouble
>> figuring out how it would be useful. E.g., if I do:
>>
>>   if (skip_to_optional_arg(arg, "--foo", NULL))
>>      ...
>>
>> what can I do in "..."? I know we matched _some_ type of "--foo", but I
>> cannot know whether it was "--foo" or "--foo=bar", nor what "bar" is. It
>> could only be used by some kind of vague validator to say "well, at
>> least this looks like an option that I _could_ parse if I wanted to".
>>
>> So I guess I don't mind it, as it does the most reasonable thing it can
>> when passed NULL, but I would be surprised if we ever actually exercise
>> the code path.
>
> And of course as soon as I sent this, I went back and double-checked.
> And indeed I totally missed this call:
>
>   +       else if (starts_with(arg, "-B") ||
>   +                skip_to_optional_arg(arg, "--break-rewrites", NULL)) {
>           if ((options->break_opt = diff_scoreopt_parse(arg)) == -1)

Yeah, calls like this were discussed in:
https://public-inbox.org/git/xmqqh8t6o9me.fsf@gitster.mtv.corp.google.com/

> So that's kind-of weird, because we are parsing "-B", etc, and then
> expecting it to be _reparsed_ by diff_scoreopt_parse. So the two
> callsites must always match. IMHO this ought to do either:
>
>   - we should just ask diff_scoreopt_parser to tell us if this was a
>     valid option that it understood
>
> or
>
>   - parse up to the "=", and then ask the scoreopt parser to parse the
>     remainder. This would require us passing 'B'/'C'/'M' to the
>     function ourselves, I think that's a better pattern. It means we
>     could reuse the parser for things like config values if we wanted to
>     (our current diff.renames is a bool, but it would not be
>     unreasonable for it to take a score).
>
> None of that is a mess of your creation, though, so I'm OK punting on it
> for now.

Yeah, this could be part of the #leftoverbits.

Re: [PATCH v3 1/7] git-compat-util: introduce skip_to_optional_arg()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>
>   +       else if (starts_with(arg, "-B") ||
>   +                skip_to_optional_arg(arg, "--break-rewrites", NULL)) {
> 	  if ((options->break_opt = diff_scoreopt_parse(arg)) == -1)
>
> So that's kind-of weird, because we are parsing "-B", etc, and then
> expecting it to be _reparsed_ by diff_scoreopt_parse. So the two
> callsites must always match.

Correct.  diff_scoreopt_parse() can be coaxed to fit better within
this if/else if/... cascade by making it take a pointer to .break_opt
field and have it return "did I handle the -B/break-rewrites?" etc.,
but otherwise, this shows that skip_to_optional_arg() has impedance
mismatch with its current API.  And the NULL thing serves as a reminder
that skip_to_optional_arg() is used _only_ as a prefix check and not
for parsing.