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, 23:56 UTC
Message-ID: <xmqqbmvaecpl.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <c74c260d-1a4d-39f6-a644-4f90a67d6d82@oracle.com>

Vegard Nossum <vegard.nossum@oracle.com> writes:

Show 7 quoted lines

> The patch will work as intended and as expected for 95% of the users out
> there (javadoc, Doxygen, kerneldoc, etc. all have the comment
> immediately preceding the function) and fixes a very real problem for me
> (and I expect many others) _today_; for the remaining 5% (who put a
> blank line between their comment and the start of the function) it will
> revert back to the current behaviour, so there should be no regression
> for them.

I notice your 95% are all programming languages, but I am more worried about the contents written in non programming languages (René gave HTML an an example--there may be other types of contents that we programmer types do not deal with every day, but Git users depend on).

I am also more focused on keeping the codebase maintainable in good health by making sure that we made an effort to find a solution that is general-enough before solving a single specific problem you have today. We may end up deciding that a blank-line heuristics gives us good enough tradeoff, but I do not want us to make a decision before thinking.

Show 14 quoted lines

>> 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.
>
> Adding a regex instead of the simple "blank line" test doesn't seem very
> difficult to do, but I am doubtful that it will make any difference in
> practice. But that can be done incrementally as well by the people who
> would actually need it (who I strongly suspect do not exist in the first
> place).

At least, the damage can be limited to the cases we know would work well if we go that way.