Re: [PATCH v3 2/2] range-diff: fix regression in passing along diff options

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 8, 2018, 22:34 UTC
Message-ID: <87k1ln5hcx.fsf@evledraar.gmail.com>
In-Reply-To: <CAPig+cTeNcn_raJWfEK_C_du1v1KYQFTVmWA9B4DNHAXoFHw-w@mail.gmail.com>

On Thu, Nov 08 2018, Eric Sunshine wrote:

Show 28 quoted lines

> On Wed, Nov 7, 2018 at 7:22 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> In 73a834e9e2 ("range-diff: relieve callers of low-level configuration
>> burden", 2018-07-22) we broke passing down options like --no-patch,
>> --stat etc. Fix that regression, and add a test for some of these
>> options being passed down.
>
> Thanks both (Ævar and Dscho) for cleaning up my mess, and sorry for
> not responding sooner; I only just found time to read the discussion
> thread. One comment below...
>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> diff --git a/range-diff.c b/range-diff.c
>> @@ -453,7 +453,8 @@ int show_range_diff(const char *range1, const char *range2,
>>                 memcpy(&opts, diffopt, sizeof(opts));
>> -               opts.output_format = DIFF_FORMAT_PATCH;
>> +               if (!opts.output_format)
>> +                       opts.output_format = DIFF_FORMAT_PATCH;
>
> Looking at diff.c:parse_diff_opt() and enable_patch_output(), rather
> than introducing this new conditional, I'm thinking that a more
> correct fix would be:
>
>     opts.output_format |= DIFF_FORMAT_PATCH;
>
> (note the '|=' operator). This would result in 'opts.output_format'
> containing (DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT), just as it did
> prior to 73a834e9e2 when --no-patch was specified.

Maybe I'm misunderstanding, but if you mean this on top:

    diff --git a/range-diff.c b/range-diff.c
    index 488844c0af..ea317f92f9 100644
    --- a/range-diff.c
    +++ b/range-diff.c
    @@ -453,8 +453,7 @@ int show_range_diff(const char *range1, const char *range2,
                    struct strbuf indent = STRBUF_INIT;

                    memcpy(&opts, diffopt, sizeof(opts));
    -               if (!opts.output_format)
    -                       opts.output_format = DIFF_FORMAT_PATCH;
    +               opts.output_format |= DIFF_FORMAT_PATCH;
                    opts.flags.suppress_diff_headers = 1;
                    opts.flags.dual_color_diffed_diffs = dual_color;
                    opts.output_prefix = output_prefix_cb;

Then the --stat test I've added here fails, because unlike "diff" the "--stat" (and others) will implicitly "--patch" and you need "--no-patch" as well (again, unlike with "diff").

Right now --stat is pretty useless, but it could be made to make sense, and at that point (and earlier) I think it would be confusing if "range-diff" had different semantics with no options v.s. one option like "--stat" v.s. "--stat -p" compared to "diff".