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

[Jacob Keller <jacob.keller@gmail.com>]

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