From: Junio C Hamano <gitster@pobox.com>
To: Vegard Nossum <vegard.nossum@oracle.com>
Cc: "René Scharfe" <l.s.r@web.de>, git@vger.kernel.org
Subject: Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context
Date: Fri, 13 Jan 2017 15:56:22 -0800 [thread overview]
Message-ID: <xmqqbmvaecpl.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <c74c260d-1a4d-39f6-a644-4f90a67d6d82@oracle.com> (Vegard Nossum's message of "Fri, 13 Jan 2017 21:20:09 +0100")
Vegard Nossum <vegard.nossum@oracle.com> writes:
> 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.
>> 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.
next prev parent reply other threads:[~2017-01-13 23:56 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
2017-01-13 23:56 ` Junio C Hamano [this message]
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqbmvaecpl.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=l.s.r@web.de \
--cc=vegard.nossum@oracle.com \
/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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.