Re: [PATCH 74/76] range-diff: use parse_options() instead of diff_opt_parse()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jan 17, 2019 at 5:12 AM Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> wrote:
>
> Diff's internal option parsing is now done with 'struct option', which
> makes it possible to combine all diff options to range-diff and parse
> everything all at once. Parsing code becomes simpler,

Cool. I like the series up to here.

I skipped most of the conversion
patches, but looked at some and they seem to be very regularly
constructed, as a later step we might want to move all the diff parsing
out of diff.c into diff-options.{c,h,} or such.

> and we get a
> looong 'git range-diff -h'

This is an interesting tidbit to put into the commit message.

range-diff is interesting in that in it is unclear where the options
should take effect. My mental model of range-diff is

    diff --inner-options-1 <range1> >tmp1
    diff --inner-options-2 <range2> >tmp2
    diff --outer-options tmp 1 tmp2

and for most operations we would want to have the inner
options to be the same. However there are cases of changing
one of the inner options, example at
https://public-inbox.org/git/20180810001010.58870-1-sbeller@xxxxxxxxxx/

But even when we assume this to be a corner case for
weird research of our own options, it is unclear to me
if the options should apply to the inner diffs or to the
outer diff or both.

As far as I read the patch, the options are applied to both
inner and outer, which may be ok?

I would think that sometimes you want to control only the
inner options, e.g. file copy/rename/move detection
thresholds. And sometimes you want to control the outer
options only (white space error highlighting?)




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux