On Thu, Nov 8, 2018 at 5:34 PM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
