[PATCH 1/3] git-compat-util: introduce skip_to_opt_val()

[PATCH 1/3] git-compat-util: introduce skip_to_opt_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_opt_val(arg, "--key", &arg)) {
	/* do something with arg */
}

Note that, when using skip_to_opt_val(), it is not possible any more
to do something different when the first argument is exactly "--key"
than when it is exactly "--key=", but in most cases we already don't
make any difference, which is a good thing.

Note that "opt" in the function name actually means "optional" as
the function 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 | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

Another possibility would be to add a "const char *default"
argument to the function, and to do: 

	if (!*p) {
		*val = default;
		return 1;
	}

This could make the function more useful in some cases.

I also wonder if the function is too big to be inlined, and
in that case, in which file it should be added. 

diff --git a/git-compat-util.h b/git-compat-util.h
index cedad4d581..7ee040388f 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -534,6 +534,41 @@ static inline int ends_with(const char *str, const char *suffix)
 	return strip_suffix(str, suffix, &len);
 }
 
+/*
+ * If the string "str" is the same as the string in "prefix", then the "val"
+ * parameter is set to the empty string 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.
+ */
+static inline int skip_to_opt_val(const char *str, const char *prefix,
+				  const char **val)
+{
+	const char *p;
+
+	if (!skip_prefix(str, prefix, &p))
+		return 0;
+
+	if (!*p) {
+		*val = "";
+		return 1;
+	}
+
+	if (*p == '=') {
+		*val = p + 1;
+		return 1;
+	}
+
+	return 0;
+}
+
 #define SWAP(a, b) do {						\
 	void *_swap_a_ptr = &(a);				\
 	void *_swap_b_ptr = &(b);				\
-- 
2.15.1.271.g1a4e40aa5d.dirty

[PATCH 2/3] index-pack: use skip_to_opt_val()

[Christian Couder]

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

Let's simplify index-pack option parsing using skip_to_opt_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..5cf252c885 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_opt_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_opt_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.271.g1a4e40aa5d.dirty

[PATCH 3/3] diff: use skip_to_opt_val()

[Christian Couder]

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

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

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 diff.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/diff.c b/diff.c
index 2ebe2227b4..067b498187 100644
--- a/diff.c
+++ b/diff.c
@@ -4508,17 +4508,11 @@ 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_opt_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_opt_val(arg, "--dirstat-by-file", &arg)) {
 		parse_dirstat_opt(options, "files");
 		return parse_dirstat_opt(options, arg);
 	}
@@ -4540,13 +4534,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_opt_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_opt_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 +4548,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_opt_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)
-- 
2.15.1.271.g1a4e40aa5d.dirty

Re: [PATCH 3/3] diff: use skip_to_opt_val()

[Jacob Keller]

On Sun, Dec 3, 2017 at 9:04 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> From: Christian Couder <christian.couder@gmail.com>
>
> Let's simplify diff option parsing using skip_to_opt_val().
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  diff.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 2ebe2227b4..067b498187 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4508,17 +4508,11 @@ 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_opt_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_opt_val(arg, "--dirstat-by-file", &arg)) {
>                 parse_dirstat_opt(options, "files");
>                 return parse_dirstat_opt(options, arg);
>         }
> @@ -4540,13 +4534,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_opt_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_opt_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 +4548,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_opt_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)
> --
> 2.15.1.271.g1a4e40aa5d.dirty
>

This causes a regression in the --relative option which prevents it
from working properly.

If I have a repository with a modified file in a subdirectory, such as:

a/file

then git diff-index --relative --name-only HEAD from within "a" will
return "a/file" instead of "file"

This breaks git completion, (among other things).

Thanks,
Jake

Re: [PATCH 3/3] diff: use skip_to_opt_val()

[Jacob Keller]

On Wed, Dec 6, 2017 at 4:16 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Sun, Dec 3, 2017 at 9:04 AM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> From: Christian Couder <christian.couder@gmail.com>
>>
>> Let's simplify diff option parsing using skip_to_opt_val().
>>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>>  diff.c | 22 ++++++++--------------
>>  1 file changed, 8 insertions(+), 14 deletions(-)
>>
>> diff --git a/diff.c b/diff.c
>> index 2ebe2227b4..067b498187 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -4508,17 +4508,11 @@ 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_opt_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_opt_val(arg, "--dirstat-by-file", &arg)) {
>>                 parse_dirstat_opt(options, "files");
>>                 return parse_dirstat_opt(options, arg);
>>         }
>> @@ -4540,13 +4534,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_opt_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_opt_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 +4548,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_opt_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)
>> --
>> 2.15.1.271.g1a4e40aa5d.dirty
>>
>
> This causes a regression in the --relative option which prevents it
> from working properly.
>
> If I have a repository with a modified file in a subdirectory, such as:
>
> a/file
>
> then git diff-index --relative --name-only HEAD from within "a" will
> return "a/file" instead of "file"
>
> This breaks git completion, (among other things).
>
> Thanks,
> Jake

I believe this occurs because skip_to_optional_val overwrites the arg
even if it's not there, and the --relative argument expects prefix to
remain unchanged if the optional value is not provided.

Thanks,
Jake

Re: [PATCH 3/3] diff: use skip_to_opt_val()

[Christian Couder]

On Thu, Dec 7, 2017 at 1:18 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Wed, Dec 6, 2017 at 4:16 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
>>
>> This causes a regression in the --relative option which prevents it
>> from working properly.
>>
>> If I have a repository with a modified file in a subdirectory, such as:
>>
>> a/file
>>
>> then git diff-index --relative --name-only HEAD from within "a" will
>> return "a/file" instead of "file"
>>
>> This breaks git completion, (among other things).

Ok, thanks for the report. I will fix that.

> I believe this occurs because skip_to_optional_val overwrites the arg
> even if it's not there, and the --relative argument expects prefix to
> remain unchanged if the optional value is not provided.

Yeah, that makes sense.

Re: [PATCH 1/3] git-compat-util: introduce skip_to_opt_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_opt_val(arg, "--key", &arg)) {
> 	/* do something with arg */
> }

Sounds sensible; especially if there are many existing code that can
be shortened by using this helper, that is.

> Note that, when using skip_to_opt_val(), it is not possible any more
> to do something different when the first argument is exactly "--key"
> than when it is exactly "--key=", but in most cases we already don't
> make any difference, which is a good thing.

Note that "it is not possible" is misleading.  skip_to_opt_val()
could be written to allow the caller to tell the difference if you
chose to, but *you* made it impossible by assigning "" to arg upon
seeing "--key".  You could assign NULL to arg in that case, and
"--key" and "--key=" can be differenciated by checking arg; the
former will give you NULL and the latter "".

Not that I am convinced that it is a bad idea to deliberately lose
information like this implementation does.  At least not yet.

If there will be no conceivable caller that wants to differenticate
between the two, the implementation that "loses information" can
claim to be easier to use, as the callers do not have to be forced
to write something like:

	if (skip_to_optional_val(arg, "--key", &arg)
		do_something(arg ? arg : "");

to treat them the same.

Having said all that, I would expect skip_to_optional_val() (as a
name of the helper I suspect skip_to_optional_arg() is more
appropriate, though) to store NULL in the arg thing if there is no
optional argument given.  Also, the caller does not have to even do
the 'arg ? arg : ""' dance if it is so common and natural for the
application to treat "--key" and "--key=" equivalently, as it is
expected that the actual code to work on the arg, i.e. do_something()
in the above example, _should_ be prepared to take NULL and "" and
treat them the same way (that is the definition of "it is so common
and natural for the application to treat them the same).

So, I think you identified interesting pattern that can be cleaned
up by introducing a helper, but I am not sure if I agree with the
exact design of the helper.

> Note that "opt" in the function name actually means "optional" as
> the function can be used to parse any "key=value" string where "key"
> is also considered as valid, not just command line options.

Yup.  This paragraph is a good thing to have in the proposed log
message, to explain the reason why you force callers of this helper
to spell out the leading dashes like "--key" (not "key").  I think
that it is a sane design of the input side of this function---it
does not limit it to an overly narrow case of command line option
processing.  For the same reason, I think it is saner design of the
output side to allow callers to tell between "key=" and "key".

While staring at this helper and writing "does not limit to command
line option processing", it occurs to me that a helper that allows
you to look for an optional ':' (instead of '=') may also be useful,
so the final version might become a pair of functions, perhaps like
so:

    int skip_to_optional_delim(const char *s,
			       const char *prefix, char delim,
			       const char **rest)
    {
	const char *p;

	if (!skip_prefix(str, prefix, &p))
		return 0;
	if (!*p)
		*rest = NULL;
	else if (*p != delim)
		return 0;
	else
		*rest = p + 1;
	return 1;
    }

    int skip_to_optional_arg(const char *s,
			     const char *prefix,
			     const char **arg)
    {
	return skip_to_optional_delim(s, prefix, '=', arg);
    }

As the first one would not (by definition) have any callers
initially after your series, it can be static to a file that
implements both of them and it is OK to expose only the latter to
the public.  

I do think it is a premature optimization to inline this function,
whose initial callers will be (from the look of the remainder of
your patches) command line parsing that sits farthest from any
performance critical code.

>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  git-compat-util.h | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> Another possibility would be to add a "const char *default"
> argument to the function, and to do: 
>
> 	if (!*p) {
> 		*val = default;
> 		return 1;
> 	}
>
> This could make the function more useful in some cases.
>
> I also wonder if the function is too big to be inlined, and
> in that case, in which file it should be added. 
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index cedad4d581..7ee040388f 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -534,6 +534,41 @@ static inline int ends_with(const char *str, const char *suffix)
>  	return strip_suffix(str, suffix, &len);
>  }
>  
> +/*
> + * If the string "str" is the same as the string in "prefix", then the "val"
> + * parameter is set to the empty string 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.
> + */
> +static inline int skip_to_opt_val(const char *str, const char *prefix,
> +				  const char **val)
> +{
> +	const char *p;
> +
> +	if (!skip_prefix(str, prefix, &p))
> +		return 0;
> +
> +	if (!*p) {
> +		*val = "";
> +		return 1;
> +	}
> +
> +	if (*p == '=') {
> +		*val = p + 1;
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
>  #define SWAP(a, b) do {						\
>  	void *_swap_a_ptr = &(a);				\
>  	void *_swap_b_ptr = &(b);				\

Re: [PATCH 1/3] git-compat-util: introduce skip_to_opt_val()

[Christian Couder]

On Sun, Dec 3, 2017 at 7:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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_opt_val(arg, "--key", &arg)) {
>>       /* do something with arg */
>> }
>
> Sounds sensible; especially if there are many existing code that can
> be shortened by using this helper, that is.
>
>> Note that, when using skip_to_opt_val(), it is not possible any more
>> to do something different when the first argument is exactly "--key"
>> than when it is exactly "--key=", but in most cases we already don't
>> make any difference, which is a good thing.
>
> Note that "it is not possible" is misleading.  skip_to_opt_val()
> could be written to allow the caller to tell the difference if you
> chose to, but *you* made it impossible by assigning "" to arg upon
> seeing "--key".  You could assign NULL to arg in that case, and
> "--key" and "--key=" can be differenciated by checking arg; the
> former will give you NULL and the latter "".

Yeah, what I meant was "the design of the function in this patch makes
it impossible..."
That's why I wrote after the diffstat:

"""Another possibility would be to add a "const char *default"
argument to the function, and to do:

      if (!*p) {
              *val = default;
              return 1;
      }

This could make the function more useful in some cases."""

But yeah I could have added the above to the commit message and it
hopefully would have made it clear what I meant.

Anyway there is a design choice to be made. Adding a "const char
*default" argument makes the function more generic, but this might not
be very useful as there are few cases that might benefit. And if we
want to make the command line interface consistent and perhaps avoid
user errors, we might want to have a rule that says that "--key" and
"--key=" should always give the same result. In this case we may also
want to make it easy to implement options that follow the rule.

So my preference is to not add the "const char *default" argument to
the function. But it is not a strong preference.

> Not that I am convinced that it is a bad idea to deliberately lose
> information like this implementation does.  At least not yet.
>
> If there will be no conceivable caller that wants to differenticate
> between the two, the implementation that "loses information" can
> claim to be easier to use, as the callers do not have to be forced
> to write something like:
>
>         if (skip_to_optional_val(arg, "--key", &arg)
>                 do_something(arg ? arg : "");
>
> to treat them the same.
>
> Having said all that, I would expect skip_to_optional_val() (as a
> name of the helper I suspect skip_to_optional_arg() is more
> appropriate, though)

Ok I will rename it skip_to_optional_arg().

> to store NULL in the arg thing if there is no
> optional argument given.  Also, the caller does not have to even do
> the 'arg ? arg : ""' dance if it is so common and natural for the
> application to treat "--key" and "--key=" equivalently, as it is
> expected that the actual code to work on the arg, i.e. do_something()
> in the above example, _should_ be prepared to take NULL and "" and
> treat them the same way (that is the definition of "it is so common
> and natural for the application to treat them the same).

I'd rather add the "const char *default" argument to the function
rather than have the function just set "arg" to NULL. I think setting
"arg" to NULL increases the risk of crashes and makes it too easy to
make "--key" and "--key=" behave differently which I think is not
consistent and not intuitive.

> So, I think you identified interesting pattern that can be cleaned
> up by introducing a helper, but I am not sure if I agree with the
> exact design of the helper.

Ok, maybe the above explains the design a bit better.

>> Note that "opt" in the function name actually means "optional" as
>> the function can be used to parse any "key=value" string where "key"
>> is also considered as valid, not just command line options.
>
> Yup.  This paragraph is a good thing to have in the proposed log
> message, to explain the reason why you force callers of this helper
> to spell out the leading dashes like "--key" (not "key").  I think
> that it is a sane design of the input side of this function---it
> does not limit it to an overly narrow case of command line option
> processing.  For the same reason, I think it is saner design of the
> output side to allow callers to tell between "key=" and "key".
>
> While staring at this helper and writing "does not limit to command
> line option processing", it occurs to me that a helper that allows
> you to look for an optional ':' (instead of '=') may also be useful,
> so the final version might become a pair of functions, perhaps like
> so:
>
>     int skip_to_optional_delim(const char *s,
>                                const char *prefix, char delim,
>                                const char **rest)
>     {
>         const char *p;
>
>         if (!skip_prefix(str, prefix, &p))
>                 return 0;
>         if (!*p)
>                 *rest = NULL;
>         else if (*p != delim)
>                 return 0;
>         else
>                 *rest = p + 1;
>         return 1;
>     }
>
>     int skip_to_optional_arg(const char *s,
>                              const char *prefix,
>                              const char **arg)
>     {
>         return skip_to_optional_delim(s, prefix, '=', arg);
>     }
>
> As the first one would not (by definition) have any callers
> initially after your series, it can be static to a file that
> implements both of them and it is OK to expose only the latter to
> the public.

Yeah, I thought about the above, but I am not very much interested in
implementing it now. I wonder if the callers will always want
skip_to_optional_delim() to take only one delim character or if they
will want more than one delim character.

> I do think it is a premature optimization to inline this function,
> whose initial callers will be (from the look of the remainder of
> your patches) command line parsing that sits farthest from any
> performance critical code.

Ok, I will not inline it.

Thanks!

Re: [PATCH 1/3] git-compat-util: introduce skip_to_opt_val()

[Junio C Hamano]

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

> Anyway there is a design choice to be made. Adding a "const char
> *default" argument makes the function more generic,...

I didn't suggest anything of that sort, and I do not understand why
you are repeatedly talking about "default" that you considered and
rejected, as if it were an alternative viable option.  I agree that
"default" is not yet a good idea and it is a solution to a problem
that is not yet shown to exist.  

On the other hand, just assigning NULL to *arg when you did not see
a delimiting '=', on the other hand, is an alternative option that
is viable.

> .... I think setting
> "arg" to NULL increases the risk of crashes and makes it too easy to
> make "--key" and "--key=" behave differently which I think is not
> consistent and not intuitive.

So now this is very specific to the need of command line argument
parsing and is not a generic thing?  You cannot have your cake and
eat it too, though.

Re: [PATCH 1/3] git-compat-util: introduce skip_to_opt_val()

[Christian Couder]

On Sun, Dec 3, 2017 at 11:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> Anyway there is a design choice to be made. Adding a "const char
>> *default" argument makes the function more generic,...
>
> I didn't suggest anything of that sort, and I do not understand why
> you are repeatedly talking about "default" that you considered and
> rejected, as if it were an alternative viable option.  I agree that
> "default" is not yet a good idea and it is a solution to a problem
> that is not yet shown to exist.
>
> On the other hand, just assigning NULL to *arg when you did not see
> a delimiting '=', on the other hand, is an alternative option that
> is viable.

What I am saying is that I'd rather have a lot of code like:

        if (skip_to_optional_val(arg, "--key", &arg, "") /* the last
argument is the default value */
                do_something(arg);

than a lot of code like this:

        if (skip_to_optional_val(arg, "--key", &arg) /* no default can
be passed, NULL is the default */
                do_something(arg ? arg : "");

because in the former case the `arg ? arg : ""` pattern is captured
inside the function, so the code is simpler.

In the few cases where do_something() accepts NULL and does something
different with it, the former can be changed to:

        if (skip_to_optional_val(arg, "--key", &arg, NULL) /* the last
argument is the default value */
                do_something(arg);

So yeah I rejected it, but my preference is not strong and I never
said or thought that it was not viable. I just think that there are
few cases that might benefit. So the benefits are not big and it might
be better for consistency and simplicity of the UI to nudge people to
make "--key" and "--key=" behave the same. That's why having "" as the
default and no default argument is a little better in my opinion.

>> .... I think setting
>> "arg" to NULL increases the risk of crashes and makes it too easy to
>> make "--key" and "--key=" behave differently which I think is not
>> consistent and not intuitive.
>
> So now this is very specific to the need of command line argument
> parsing and is not a generic thing?  You cannot have your cake and
> eat it too, though.

I think that even when we are not parsing options, it is probably good
in general for UI consistency and simplicity that "key" and "key=" are
interpreted in the same way.

Re: [PATCH 1/3] git-compat-util: introduce skip_to_opt_val()

[Junio C Hamano]

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

> In the few cases where do_something() accepts NULL and does something
> different with it, the former can be changed to:
>
>         if (skip_to_optional_val(arg, "--key", &arg, NULL) /* the last
> argument is the default value */
>                 do_something(arg);

That's the thing.  If do_something() that takes "" and NULL and
behaves differently is rare, that indicates that the existing code
may not be committed to treat "--key" and "--key=''" the same in the
first place, and I am not 100% convinced that I want to see us
committed to force that design throughout the system by introducing
a helper that hardcodes the equivalence and encourages to use it.

Imagine a command that takes "--do-something" option and does that
"something" unconditionally.  We may later extend it to take an
optional argument, i.e. "--do-something=c1,c2,...", to tell the
command to do that "something" under some but not all conditions.
The values c1,c2 would tell that we want that something done only
under either conditions c1 or c2 holds true.

It would be natural to expect that "--do-something=" to do that
"something" under no condition (i.e. as if no such option was
given); that would help scripts that accumulate the set of
conditions in a variable and say "--do-something=$when", by making
it a no-op when the variable $when turns out to be an empty string.
"--do-something" without "=" would not want to mean the same thing.

The above observation makes me suspect that it depends on the "key"
what "--key=$value" we want to be equivalent to "--key".  In the
"--do-something" case, we do not want to pretend as if we got an
empty string; instead we'd pretend as if we got "always" or
something like that.

And your "default" would work well for this "default is tied to what
the key is" paradigm, i.e.

	skip_to_optional_arg(arg, "--do-something", &arg, "always")

would make us treat "--do-something" and "--do-something=always" the
same way.

If it turns out that the default arg almost always is an empty
string, I do not mind 

	#define skip_to_opt_arg(s,k,v) skip_to_optional_arg(s,k,v,"")

of course.