René Scharfe [off-list ref] writes:
Am 13.01.2017 um 17:15 schrieb Vegard Nossum:
quoted
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.
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.
On 13/01/2017 20:51, Junio C Hamano wrote:
René Scharfe [off-list ref] writes:
quoted
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.
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.
For the 0% who don't put even a single blank line between their
functions, it will probably not work as expected, but then again I have
never seen such a coding style in any language, so I am doubtful if this
is something that needs to be taken into account in the first place.
quoted
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.
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).
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.
I don't like specifying a fixed number, that negates almost all the
reason for using -W in the first place; I would much prefer adding
a config variable to control the -W behaviour (or a new diff flag).
Vegard