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$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=l.s.r@web$(echo .)de \
--cc=vegard.nossum@oracle$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox