Re: [PATCH] diff compaction heuristic: favor shortest neighboring blank lines

[Junio C Hamano <gitster@pobox.com>]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Scoring heuristic:
>
>>     # A place to accumulate bonus factors (positive makes this
>>     # index more favored):
>>     bonus = 0
>> 
>>     # Bonuses based on the location of blank lines:
>>     if pre_blank and not blank:
>>         bonus += 3
>>     elif blank and not pre_blank:
>>         bonus += 2
>>     elif blank and pre_blank:
>>         bonus += 1
>> 
>>     # Now fill in missing indent values:
>>     if pre_indent is None:
>>         pre_indent = 0
>> 
>>     if post_indent is None:
>>         post_indent = 0

These "missing" are to handle the lines at the boundary of shiftable
block, or do you have pre_indent if a line is the earliest possible
one that could be included in the hunk by shifting, as long as it is
not at the beginning of the file?

I think "this is indented deeper than the previous, and it wants to
stay with the previous if possible" is an excellent heuristic, which
you have below.  I wonder if "however, the previous cannot be
included in the hunk by shifting, hence we try to keep this together
with it by favouring its exclusion from the hunk" could be part of
the same heuristics, if you do have pre_indent value for a line at
the boundary of shiftable block.

Also I am wondering if 0 is a sensible thing to use as the fallback
value.  When pre_indent is not definable, turning it to 0 and
declaring that our indented line is indented deeper than the
previous line and penalizing with "bonus -= 4" (which you do) feels
just as fishy as turning the undefined pre_indent to maxint and
declaring that our indented line is indented shallower than the
previous line (which you don't, but is just as justifiable, I would
think).

Thanks.

>>     if blank:
>>         indent = post_indent
>> 
>>     if indent > pre_indent:
>>         # The line is indented more than its predecessor. It is
>>         # preferable to keep these lines together, so we score it
>>         # based on the larger indent:
>>         score = indent
>>         bonus -= 4
>> 
>>     elif indent < pre_indent:
>>         # The line is indented less than its predecessor. It could
>>         # be that this line is the start of a new block (e.g., of
>>         # an "else" block, or of a block without a block
>>         # terminator) or it could be the end of the previous
>>         # block.
>>         if indent < post_indent:
>>             # The following line is indented more. So it is likely
>>             # that this line is the start of a block. It's a
>>             # pretty good place to split, so score it based on the
>>             # smaller indent:
>>             score = indent
>>             bonus += 2
>>         else:
>>             # This was probably the end of a block. We score based
>>             # on the line's own indent:
>>             score = indent
>> 
>>     else:
>>         # The line has the same indentation level as its
>>         # predecessor. We score it based on its own indent:
>>         score = indent
>> 
>>     return 10 * score - bonus