| Message ID | 20181107122202.1813-3-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/2] range-diff doc: add a section about output stability | expand |
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.
On Thu, Nov 08 2018, Eric Sunshine wrote: > 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".
On Thu, Nov 8, 2018 at 5:34 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Thu, Nov 08 2018, Eric Sunshine wrote: > > 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: > > - if (!opts.output_format) > - opts.output_format = DIFF_FORMAT_PATCH; > + opts.output_format |= DIFF_FORMAT_PATCH; That is indeed what I mean. > 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"). This new --stat test also fails with Dscho's original git-range-diff implementation, even before 73a834e9e2 regressed the --no-patch case. The commit message seems to sell this patch as a pure regression-fix, so it feels wrong for it to be conflating a regression fix (--no-patch) with preparation for potential future improvements to other options (--stat, etc.). I could see this as a two-patch series in which patch 1/2 fixes the regression (with, say, "|="), and patch 2/2 generalizes setting 'opts.output_format' for the future. Alternately, if left as a single patch, perhaps the commit message could be fleshed out to better explain that it is doing more than merely fixing a regression (since it wasn't at all obvious to me, even after digging into the code earlier to come up with "|=", or now when trying to understand your response). > 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". Perhaps this sort of rationale, along with some explanatory examples could be added to the commit message to help the reader more fully understand the situation. Thanks for working on this.
On Fri, Nov 09 2018, Eric Sunshine wrote: > On Thu, Nov 8, 2018 at 5:34 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: >> On Thu, Nov 08 2018, Eric Sunshine wrote: >> > 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: >> >> - if (!opts.output_format) >> - opts.output_format = DIFF_FORMAT_PATCH; >> + opts.output_format |= DIFF_FORMAT_PATCH; > > That is indeed what I mean. *Nod* >> 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"). > > This new --stat test also fails with Dscho's original git-range-diff > implementation, even before 73a834e9e2 regressed the --no-patch case. > The commit message seems to sell this patch as a pure regression-fix, > so it feels wrong for it to be conflating a regression fix > (--no-patch) with preparation for potential future improvements to > other options (--stat, etc.). > > I could see this as a two-patch series in which patch 1/2 fixes the > regression (with, say, "|="), and patch 2/2 generalizes setting > 'opts.output_format' for the future. Alternately, if left as a single > patch, perhaps the commit message could be fleshed out to better > explain that it is doing more than merely fixing a regression (since > it wasn't at all obvious to me, even after digging into the code > earlier to come up with "|=", or now when trying to understand your > response). Yeah that makes sense. I did not see (but see now) that the --stat behavior was different now v.s. before your 73a834e9e2. >> 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". > > Perhaps this sort of rationale, along with some explanatory examples > could be added to the commit message to help the reader more fully > understand the situation. *Nod*
diff --git a/range-diff.c b/range-diff.c index bd8083f2d1..488844c0af 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 6aae364171..e497c1358f 100755 --- a/t/t3206-range-diff.sh +++ b/t/t3206-range-diff.sh @@ -122,6 +122,36 @@ test_expect_success 'changed commit' ' test_cmp expected actual ' +test_expect_success 'changed commit with --no-patch diff option' ' + git range-diff --no-color --no-patch topic...changed >actual && + cat >expected <<-EOF && + 1: 4de457d = 1: a4b3333 s/5/A/ + 2: fccce22 = 2: f51d370 s/4/A/ + 3: 147e64e ! 3: 0559556 s/11/B/ + 4: a63e992 ! 4: d966c5c s/12/B/ + EOF + test_cmp expected actual +' + +test_expect_success 'changed commit with --stat diff option' ' + git range-diff --no-color --stat topic...changed >actual && + cat >expected <<-EOF && + 1: 4de457d = 1: a4b3333 s/5/A/ + a => b | 0 + 1 file changed, 0 insertions(+), 0 deletions(-) + 2: fccce22 = 2: f51d370 s/4/A/ + a => b | 0 + 1 file changed, 0 insertions(+), 0 deletions(-) + 3: 147e64e ! 3: 0559556 s/11/B/ + a => b | 0 + 1 file changed, 0 insertions(+), 0 deletions(-) + 4: a63e992 ! 4: d966c5c s/12/B/ + a => b | 0 + 1 file changed, 0 insertions(+), 0 deletions(-) + EOF + test_cmp expected actual +' + test_expect_success 'changed commit with sm config' ' git range-diff --no-color --submodule=log topic...changed >actual && cat >expected <<-EOF &&
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. As noted in a change leading up to this ("range-diff doc: add a section about output stability", 2018-11-07) the output is not meant to be stable. So this regression test will likely need to be tweaked once we get a "proper" --stat option. See https://public-inbox.org/git/nycvar.QRO.7.76.6.1811071202480.39@tvgsbejvaqbjf.bet/ for a further explanation of the regression. The quoting of "EOF" here mirrors that of an earlier test. Perhaps that should be fixed, but let's leave that up to a later cleanup change. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- range-diff.c | 3 ++- t/t3206-range-diff.sh | 30 ++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-)