Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 13, 2017, 19:51 UTC
- Message-ID
- <xmqqeg06hh6z.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <e55dc4dd-768b-8c9b-e3b2-e850d5d521f5@web.de>
René Scharfe <l.s.r@web.de> writes:
Show 24 quoted lines
> Am 13.01.2017 um 17:15 schrieb Vegard Nossum:
>> When using -W to include the whole function in the diff context, you
>> are typically doing this to be able to review the change in its entirety
>> within the context of the function. It is therefore almost always
>> desirable to include any comments that immediately precede the function.
>>
>> This also the fixes the case for C where the declaration is split across
>> multiple lines (where the first line of the declaration would not be
>> included in the output), e.g.:
>>
>> void
>> dummy(void)
>> {
>> ...
>> }
>>
>
> That's true, but I'm not sure "non-empty line before function line" is
> good enough a definition for desirable lines. It wouldn't work for
> people who don't believe in empty lines. Or for those that put a
> blank line between comment and function. (I have an opinion on such
> habits, but git diff should probably stay neutral.) And that's just
> for C code; I have no idea how this heuristic would hold up for other
> file types like HTML.As you are, I am fairly negative on the heuristic based on the "non-blank" thing. We tried once with compaction-heuristics already and it did not quite perform well. Let's not hardcode another one.
Show 5 quoted lines
> We can identify function lines with arbitrary precision (with a > xfuncname regex, if needed), but there is no accurate way to classify > lines as comments, or as the end of functions. Adding optional > regexes for single- and multi-line comments would help, at least for > C.
The funcline regexp is used for two related but different purposes. It identifies a single line to be placed on @@ ... @@ line before a diff hunk. This line however does not have to be at the beginning of a function. It has to be the line that conveys the most significant information (e.g. the name of the function).
The way "diff -W" codepath used it as if it were always the very first line of a function was bound to invite a patch like this, and if we want to be extra elaborate, I agree that an extra mechanism to say "the line the funcline regexp matches is not the beginning of a function, but the beginning is a line that matches this other regexp before that line" may help.
Do we really want to be that elaborate, though? I dunno.
I wonder if it would be sufficient to make -W take an optional number, e.g. "git show -W4", to add extre context lines before the funcline.