Re: [PATCH 02/18] Add a new builtin: branch-diff

From: Stefan Beller <sbeller@google.com>
Date: May 3, 2018, 16:43 UTC
Message-ID: <CAGZ79kZAidPafdfu1NGwwpVo1Vy=vKOV+EREE2=-ct_sbo7Gkg@mail.gmail.com>
In-Reply-To: <8bc517e35d4842f8d9d98f3b99adb9475d6db2d2.1525361419.git.johannes.schindelin@gmx.de>

Hi Johannes,

On Thu, May 3, 2018 at 8:30 AM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

> This builtin does not do a whole lot so far, apart from showing a usage
> that is oddly similar to that of `git tbdiff`. And for a good reason:
> the next commits will turn `branch-diff` into a full-blown replacement
> for `tbdiff`.

While I appreciate the 1:1 re-implementation, I'll comment as if this was a newly invented tool, questioning design choices. They are probably chosen pretty well, and fudge facotrs as below are at tweaked to a reasonable factor, but I'll try to look with fresh eyes.

>
> At this point, we ignore tbdiff's color options, as they will all be
> implemented later and require some patches to the diff machinery.

Speaking of colors, for origin/sb/blame-color Junio hinted at re-using
cyan for "uninteresting" parts to deliver a consistent color scheme for
Git. Eventually he dreams of having 2 layers of indirection IIUC, with
    "uninteresting" -> cyan
    "repeated lines in blame" -> uninteresting

Maybe we can fit the coloring of this tool in this scheme, too?

> +       double creation_weight = 0.6;

I wondered if we use doubles in Gits code base at all, and I found

khash.h:59:static const double __ac_HASH_UPPER = 0.77; pack-bitmap-write.c:248: static const double REUSE_BITMAP_THRESHOLD = 0.2; pack-bitmap.c:751: static const double REUSE_PERCENT = 0.9;

all other occurrences of `git grep double` are mentioning it in other contexts (e.g. "double linked list" or comments).

When implementing diff heuristics in 433860f3d0b (diff: improve positioning of add/delete blocks in diffs, 2016-09-05), Michael broke it down to fixed integers instead of floating point.

Do we need to dynamic of a floating point, or would a rather small range suffice here? (Also see rename detection settings, that take percents as integers)

Thanks, Stefan