On Thu, Nov 08 2018, Eric Sunshine wrote:
> On Wed, Nov 7, 2018 at 7:22 AM Ævar Arnfjörð Bjarmason <ava...@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 <ava...@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".