On 15/01/2017 03:39, Junio C Hamano wrote:
René Scharfe <l.s.r@xxxxxx> writes:
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.
You are right; I appreciate this approach.
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).
Then it also shouldn't be too difficult to add
diff.<driver>.preamble = <regex>
diff.<driver>.xpreamble = <regex>
to override the heuristic used for function border detection in
exceptional cases.
You can argue about the naming now ;-) But I will use this for a start,
renaming/reworking it (or throwing it away) afterwards should be easy
once the code has been written.
Vegard