git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context

From
René Scharfe <l.s.r@web.de>
Date
Jan 15, 2017, 16:57 UTC
Message-ID
<2668771d-249b-659d-3a2c-a788d7d5ebd6@web.de>
In-Reply-To
<0c761135-2696-4b3d-0a4f-3d90edf5da2e@oracle.com>
Am 15.01.2017 um 11:06 schrieb Vegard Nossum:
Show 39 quoted lines
> On 15/01/2017 03:39, Junio C Hamano wrote:
>> René Scharfe <l.s.r@web.de> writes:
>>> How about extending the context upward only up to and excluding a line
>>> that is either empty *or* a function line?  That would limit the extra
>>> context to a single function in the worst case.
>>>
>>> Reducing context at the bottom with the aim to remove comments for the
>>> next section is more tricky as it could remove part of the function
>>> that we'd like to show if we get the boundary wrong.  How bad would it
>>> be to keep the southern border unchanged?
>>
>> I personally do not think there is any robust heuristic other than
>> Vegard's "a blank line may be a signal enough that lines before that
>> are not part of the beginning of the function", and I think your
>> "hence we look for a blank line but if there is a line that matches
>> the function header, stop there as we know we came too far back"
>> will be a good-enough safety measure.
>>
>> I also agree with you that we probably do not want to futz with the
>> southern border.
>
> You are right, trying to change the southern border in this way is not
> quite reliable if there are no empty lines whatsoever and can
> erroneously cause the function context to not include the bottom of the
> function being changed.
>
> I'm splitting the function boundary detection logic into separate
> functions and trying to solve the above case without breaking the tests
> (and adding a new test for the above case too).
>
> I'll see if I can additionally provide some toggles (flags or config
> variables) to control the new behaviour, what I had in mind was:
>
>   -W[=preamble,=no-preamble]
>   --function-context[=preamble,=no-preamble]
>   diff.functionContextPreamble = <bool>
>
> (where the new logic is controlled by the new config variable and
> overridden by the presence of =preamble or =no-preamble).

Adding comments before a function is useful, removing comments after a function sounds to me as only nice to have (under the assumption that they belong to the next function[*]). How bad would it be to only implement the first part (as in the patch I just sent) without adding new config settings or parameters?

Thanks, René

[*] Silly counter-example (the #endif line):
#ifdef SOMETHING
int f(...) {
	// implementation for SOMETHING
}
#else
inf f(...) {
	// implementation without SOMETHING
}
#endif /* SOMETHING */
Previous: Vegard NossumNext: Junio C Hamano
Message 11 of 15 in “xdiff: -W: relax end-of-file function detection”
  1. 1/3 xdiff: -W: relax end-of-file function detectionVegard Nossum, Jan 13, 2017
  2. 2/3 xdiff: -W: include immediately preceding non-empty lines in contextVegard Nossum, Jan 13, 2017
  3. René ScharfeJan 13, 2017
  4. Stefan BellerJan 13, 2017
  5. Junio C HamanoJan 13, 2017
  6. Vegard NossumJan 13, 2017
  7. Junio C HamanoJan 13, 2017
  8. René ScharfeJan 14, 2017
  9. Junio C HamanoJan 15, 2017
  10. Vegard NossumJan 15, 2017
  11. René ScharfeJan 15, 2017
  12. Junio C HamanoJan 15, 2017
  13. René ScharfeJan 15, 2017
  14. 3/3 t/t4051-diff-function-context: improve tests for new diff -W behaviourVegard Nossum, Jan 13, 2017
  15. René ScharfeJan 13, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.