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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 23, 2016, 17:37 UTC
Message-ID: <xmqq1t3nkdrm.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <576C1803.5050905@alum.mit.edu>

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

Show 20 quoted lines

> 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.

Show 34 quoted lines

>>     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