Re: weird diff output?

[Jacob Keller <jacob.keller@gmail.com>]

On Thu, Mar 31, 2016 at 6:47 AM, Jeff King <peff@peff.net> wrote:
> On Wed, Mar 30, 2016 at 12:31:30PM -0700, Jacob Keller wrote:
>
>> So far I've only found a single location that ends up looking worse
>> within the Linux kernel. Diffs of some Kbuild settings result in
>> something like
>>
>> before:
>>
>>           If unsure, say Y.
>> +
>> +config RMI4_I2C
>> +       tristate "RMI4 I2C Support"
>> +       depends on RMI4_CORE && I2C
>> +       help
>> +         Say Y here if you want to support RMI4 devices connected to an I2C
>> +         bus.
>> +
>> +         If unsure, say Y.
>>
>> after:
>>
>>           required for all RMI4 device support.
>>
>> +         If unsure, say Y.
>> +
>> +config RMI4_I2C
>> +       tristate "RMI4 I2C Support"
>> +       depends on RMI4_CORE && I2C
>> +       help
>> +         Say Y here if you want to support RMI4 devices connected to an I2C
>> +         bus.
>> +
>>           If unsure, say Y.
>>
>> So in this particular instance which has multiple blank lines and is a
>> similar issue as with Stefan's note above, this is where the heuristic
>> falls apart. At least for C code this is basically vanishingly small
>> compared to the number of comment header fix ups.
>>
>> I think it may be that Stefan's suggestions above may be on the right
>> track to resolve that too.
>
> This is a tricky one. There _aren't_ actually multiple blank lines in
> the ambiguous area, because this particular example comes at the very
> end of the file. Try:
>
>   git show 8d99758dee3 drivers/input/rmi4/Kconfig
>
> which adds a block in the middle of the file. It looks good both before
> and after running through the script. Now look at this example:
>
>   git show fdf51604f10 drivers/input/rmi4/Kconfig
>
> which looks like:
>
> diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig
> index 5ea60e3..cc3f7c5 100644
> --- a/drivers/input/rmi4/Kconfig
> +++ b/drivers/input/rmi4/Kconfig
> @@ -8,3 +8,12 @@ config RMI4_CORE
>           required for all RMI4 device support.
>
>           If unsure, say Y.
> +
> +config RMI4_I2C
> +       tristate "RMI4 I2C Support"
> +       depends on RMI4_CORE && I2C
> +       help
> +         Say Y here if you want to support RMI4 devices connected to an I2C
> +         bus.
> +
> +         If unsure, say Y.
>
>
> Note that there is no trailing context, as we're adding at the end of
> the file. So the ambiguous portion consists of only two lines: an empty
> line, and "If unsure...". And we bump the latter to the top, per the
> heuristic (it's the exact opposite of every other case, where the blank
> line is a true delimiter).
>
> As a human, I think the indentation here is the real syntactic clue. But
> getting into indentation heuristics is probably insane.
>
> The script could probably make this work by disabling itself if the hunk
> is at the end of the diffed file (i.e., we don't see more context lines
> afterward). That covers any case like this where newline _is_ a
> delimiter, but we just have some internal newlines, too. It wouldn't
> cover the case where we had internal newlines but used some other
> paragraph delimiter, but based on the results so far, that seems rather
> rare.
>
> Something like this:
>
> --- foo.pl.orig 2016-03-31 09:44:44.281232230 -0400
> +++ foo.pl      2016-03-31 09:44:34.901232632 -0400
> @@ -24,13 +24,15 @@
>        push @hunk, $_;
>        $state = STATE_IN_CHUNK;
>      } else {
> -      flush_hunk();
> +      print @hunk;
> +      @hunk = ();
>        $state = STATE_NONE;
>        print;
>      }
>    }
>  }
> -flush_hunk();
> +print @hunk;
> +@hunk = ();
>
>  sub flush_hunk {
>    my $context_len = 0;
>
> -Peff

I started attempting to implement this heuristic within xdiff, but I
am at a loss as to how xdiff actually works. I suspect this would go
in xdi_change_compact or after it, but I really don't understand how
xdiff represents the diffs at all...

Thanks,
Jake