From: Vegard Nossum <vegard.nossum@oracle.com>
To: "Junio C Hamano" <gitster@pobox.com>, "René Scharfe" <l.s.r@web.de>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context
Date: Fri, 13 Jan 2017 21:20:09 +0100 [thread overview]
Message-ID: <c74c260d-1a4d-39f6-a644-4f90a67d6d82@oracle.com> (raw)
In-Reply-To: <xmqqeg06hh6z.fsf@gitster.mtv.corp.google.com>
On 13/01/2017 20:51, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
>> 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.
>> 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
next prev parent reply other threads:[~2017-01-13 20:20 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-13 16:15 [PATCH 1/3] xdiff: -W: relax end-of-file function detection Vegard Nossum
2017-01-13 16:15 ` [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context Vegard Nossum
2017-01-13 18:19 ` René Scharfe
2017-01-13 18:44 ` Stefan Beller
2017-01-13 19:51 ` Junio C Hamano
2017-01-13 20:20 ` Vegard Nossum [this message]
2017-01-13 23:56 ` Junio C Hamano
2017-01-14 14:58 ` René Scharfe
2017-01-15 2:39 ` Junio C Hamano
2017-01-15 10:06 ` Vegard Nossum
2017-01-15 16:57 ` René Scharfe
2017-01-15 23:28 ` Junio C Hamano
2017-01-15 16:57 ` René Scharfe
2017-01-13 16:15 ` [PATCH 3/3] t/t4051-diff-function-context: improve tests for new diff -W behaviour Vegard Nossum
2017-01-13 17:49 ` [PATCH 1/3] xdiff: -W: relax end-of-file function detection René Scharfe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c74c260d-1a4d-39f6-a644-4f90a67d6d82@oracle.com \
--to=vegard.nossum@oracle.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=l.s.r@web.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).