Re: [PATCH v2 7/7] blame: actually use the diff opts parsed from the command line

On 08/23/2016 11:56 AM, René Scharfe wrote:

Am 22.08.2016 um 13:22 schrieb Michael Haggerty:

quoted

"git blame" already parsed generic diff options from the command line
via diff_opt_parse(), but instead of passing the resulting xdl_opts to
xdi_diff(), it sent its own xdl_opts, which only reflected the values of
the self-parsed options "-w" and "--minimal". Instead, rely on
diff_opt_parse() to parse all of the diff options, including "-w" and
"--minimal", and pass the resulting xdl_opts to xdi_diff().

Sounds useful: It allows more fine-grained control over which whitespace
changes to ignore and which diff algorithm to use.  There is a bit of
overlap (e.g. with -b meaning show blank boundaries vs. ignore
whitespace changes), but with your patch blame's own options still take
precedence, so there should be no unpleasant surprises.

quoted

Signed-off-by: Michael Haggerty <redacted>
---
Somebody who knows more about how diff operations are configured
should please review this. I'm not certain that the change as
implemented won't have other unwanted side-effects, though of course
I checked that the test suite runs correctly.

I don't qualify, but I'll comment anyway..

quoted

 builtin/blame.c        |  11 ++--
 t/t4059-diff-indent.sh | 160
+++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 165 insertions(+), 6 deletions(-)
 create mode 100755 t/t4059-diff-indent.sh

This new test doesn't call git blame.  Does it belong to a different
commit?  And shouldn't the change to blame.c stand on its own, outside
of this series?

Thanks for catching this. I squashed the test incorrectly; it was meant
to be part of the previous commit. I'll fix it in v3.

The reason that I would prefer to change `blame` as part of this patch
series is that I think it would be disconcerting for `git diff` and `git
blame` to use different heuristics when computing diffs. It would make
their output inconsistent.

But probably that means passing just the new option to `git blame`
rather than passing all possible `diff` options through.

[...]

Your other comments may be moot given that changing `git blame` in this
way seems to have been a bad idea in the first place [1]. But I'll keep
them in mind if a new version contains similar code.

Thanks,
Michael

[1]
http://public-inbox.org/git/xmqqtwebwhbg.fsf@gitster.mtv.corp.google.com/