| Message ID | 20181109101803.3038-4-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | range-diff fixes | expand |
On Friday, November 9, 2018 3:18:03 AM MST Ævar Arnfjörð Bjarmason wrote: > > But we should behave consistently with "diff" in anticipation of such > output being useful in the future, because it would make for confusing > UI if two "diff" and "range-diff" behaved differently when it came to 's/ two//' > how they interpret diff options. >
On Fri, Nov 9, 2018 at 5:18 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > Make the behavior when diff options (e.g. "--stat") are passed > consistent with how "diff" behaves. > [...] > 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, > - opts.output_format |= DIFF_FORMAT_PATCH; > + if (!opts.output_format) > + opts.output_format |= DIFF_FORMAT_PATCH; I think this can just be '=' now instead of '|=' (to avoid confusing the reader, even if it's functionally equivalent).
Eric Sunshine <sunshine@sunshineco.com> writes: > On Fri, Nov 9, 2018 at 5:18 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: >> Make the behavior when diff options (e.g. "--stat") are passed >> consistent with how "diff" behaves. >> [...] >> 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, >> - opts.output_format |= DIFF_FORMAT_PATCH; >> + if (!opts.output_format) >> + opts.output_format |= DIFF_FORMAT_PATCH; > > I think this can just be '=' now instead of '|=' (to avoid confusing > the reader, even if it's functionally equivalent). Hmph, could the condition in the future change to - if (!opts.output_format) + if (! (opts.output_format & DIFF_FORMAT_MASK)) opts.output_format |= DIFF_FORMAT_PATCH if we ever gain a new "output_format" bit that does not affect how we show the diff in a major way, and that is on by default? If so, I think "|=" is more future-proof. Otherwise, "=" is indeed more clear way to spell the intention.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh > index ab44e085d5..9352f65280 100755 > --- a/t/t3206-range-diff.sh > +++ b/t/t3206-range-diff.sh > @@ -140,37 +140,15 @@ test_expect_success 'changed commit with --stat diff option' ' > 1: 4de457d = 1: a4b3333 s/5/A/ > a => b | 0 > 1 file changed, 0 insertions(+), 0 deletions(-) > - $four_spaces This removes all references to four_spaces=" " assigned at the very beginning of this test piece, so that assignment should also go, no? > 2: fccce22 = 2: f51d370 s/4/A/ > a => b | 0 > 1 file changed, 0 insertions(+), 0 deletions(-) > - $four_spaces > 3: 147e64e ! 3: 0559556 s/11/B/ > a => b | 0 > 1 file changed, 0 insertions(+), 0 deletions(-) > - $four_spaces > - @@ -10,7 +10,7 @@ > - 9 > - 10 > - -11 > - -+B > - ++BB > - 12 > - 13 > - 14 > 4: a63e992 ! 4: d966c5c s/12/B/ > a => b | 0 > 1 file changed, 0 insertions(+), 0 deletions(-) > - $four_spaces > - @@ -8,7 +8,7 @@ > - @@ > - 9 > - 10 > - - B > - + BB > - -12 > - +B > - 13 > EOF > test_cmp expected actual > '
diff --git a/range-diff.c b/range-diff.c index ea317f92f9..72bde281f3 100644 --- a/range-diff.c +++ b/range-diff.c @@ -453,7 +453,8 @@ int show_range_diff(const char *range1, const char *range2, struct strbuf indent = STRBUF_INIT; memcpy(&opts, diffopt, sizeof(opts)); - opts.output_format |= DIFF_FORMAT_PATCH; + if (!opts.output_format) + 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; diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh index ab44e085d5..9352f65280 100755 --- a/t/t3206-range-diff.sh +++ b/t/t3206-range-diff.sh @@ -140,37 +140,15 @@ test_expect_success 'changed commit with --stat diff option' ' 1: 4de457d = 1: a4b3333 s/5/A/ a => b | 0 1 file changed, 0 insertions(+), 0 deletions(-) - $four_spaces 2: fccce22 = 2: f51d370 s/4/A/ a => b | 0 1 file changed, 0 insertions(+), 0 deletions(-) - $four_spaces 3: 147e64e ! 3: 0559556 s/11/B/ a => b | 0 1 file changed, 0 insertions(+), 0 deletions(-) - $four_spaces - @@ -10,7 +10,7 @@ - 9 - 10 - -11 - -+B - ++BB - 12 - 13 - 14 4: a63e992 ! 4: d966c5c s/12/B/ a => b | 0 1 file changed, 0 insertions(+), 0 deletions(-) - $four_spaces - @@ -8,7 +8,7 @@ - @@ - 9 - 10 - - B - + BB - -12 - +B - 13 EOF test_cmp expected actual '
Make the behavior when diff options (e.g. "--stat") are passed consistent with how "diff" behaves. Before 73a834e9e2 ("range-diff: relieve callers of low-level configuration burden", 2018-07-22) running range-diff with "--stat" would produce stat output and the diff output, as opposed to how "diff" behaves where once "--stat" is specified "--patch" also needs to be provided to emit the patch output. As noted in a previous change ("range-diff doc: add a section about output stability", 2018-11-07) the "--stat" output with "range-diff" is useless at the moment. But we should behave consistently with "diff" in anticipation of such output being useful in the future, because it would make for confusing UI if two "diff" and "range-diff" behaved differently when it came to how they interpret diff options. The new behavior is also consistent with the existing documentation added in ba931edd28 ("range-diff: populate the man page", 2018-08-13). See "[...]also accepts the regular diff options[...]" in git-range-diff(1). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- range-diff.c | 3 ++- t/t3206-range-diff.sh | 22 ---------------------- 2 files changed, 2 insertions(+), 23 deletions(-)