Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context

["René Scharfe" <l.s.r@web.de>]

Am 15.01.2017 um 11:06 schrieb Vegard Nossum:
> On 15/01/2017 03:39, Junio C Hamano wrote:
>> René Scharfe <l.s.r@web.de> writes:
>>> How about extending the context upward only up to and excluding a line
>>> that is either empty *or* a function line?  That would limit the extra
>>> context to a single function in the worst case.
>>>
>>> Reducing context at the bottom with the aim to remove comments for the
>>> next section is more tricky as it could remove part of the function
>>> that we'd like to show if we get the boundary wrong.  How bad would it
>>> be to keep the southern border unchanged?
>>
>> I personally do not think there is any robust heuristic other than
>> Vegard's "a blank line may be a signal enough that lines before that
>> are not part of the beginning of the function", and I think your
>> "hence we look for a blank line but if there is a line that matches
>> the function header, stop there as we know we came too far back"
>> will be a good-enough safety measure.
>>
>> I also agree with you that we probably do not want to futz with the
>> southern border.
>
> You are right, trying to change the southern border in this way is not
> quite reliable if there are no empty lines whatsoever and can
> erroneously cause the function context to not include the bottom of the
> function being changed.
>
> I'm splitting the function boundary detection logic into separate
> functions and trying to solve the above case without breaking the tests
> (and adding a new test for the above case too).
>
> I'll see if I can additionally provide some toggles (flags or config
> variables) to control the new behaviour, what I had in mind was:
>
>   -W[=preamble,=no-preamble]
>   --function-context[=preamble,=no-preamble]
>   diff.functionContextPreamble = <bool>
>
> (where the new logic is controlled by the new config variable and
> overridden by the presence of =preamble or =no-preamble).

Adding comments before a function is useful, removing comments after a 
function sounds to me as only nice to have (under the assumption that 
they belong to the next function[*]).  How bad would it be to only 
implement the first part (as in the patch I just sent) without adding 
new config settings or parameters?

Thanks,
René


[*] Silly counter-example (the #endif line):
#ifdef SOMETHING
int f(...) {
	// implementation for SOMETHING
}
#else
inf f(...) {
	// implementation without SOMETHING
}
#endif /* SOMETHING */