Re: [PATCH 5/7] diff.c: refactor internal representation for coloring moved code

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 3, 2018, 19:39 UTC
Message-ID: <87o9j0uljo.fsf@evledraar.gmail.com>
In-Reply-To: <20180402224854.86922-6-sbeller@google.com>

On Mon, Apr 02 2018, Stefan Beller wrote:

Show 12 quoted lines

> At the time the move coloring was implemented we thought an enum of modes
> is the best to configure this feature.  However as we want to tack on new
> features, the enum would grow exponentially.
>
> Refactor the code such that features are enabled via bits. Currently we can
> * activate the move detection,
> * enable the block detection on top, and
> * enable the dimming inside a block, though this could be done without
>   block detection as well (mode "plain, dimmed")
>
> Choose the flags to not be at bit position 2,3,4 as the next patch
> will occupy these.

When I've been playing with colorMoved the thing I've found really confusing is that the current config has confused two completely unrelated things (at least, from a user perspective), what underlying algorithm you use, and how the colors look.

I was helping someone at work the other day where they were trying:

    git -c color.diff.new="green bold" \
        -c color.diff.old="red bold" \
        -c color.diff.newMoved="green" \
        -c color.diff.oldMoved="red" \
        -c diff.colorMoved=plain show <commit>

But what gave better results was:

    git -c color.diff.new="green bold" \
        -c color.diff.old="red bold" \
        -c color.diff.newMoved="green" \
        -c color.diff.oldMoved="red" \
        -c diff.colorMoved=zebra \
        -c color.diff.oldMovedAlternative=red \
        -c color.diff.newMovedAlternative=green show <commit>

I don't have a public test commit to share (sorry), but I have an internal example where "plain" will consider a thing as falling under color.diff.old OR color.diff.oldMoved, but zebra will consider that whole part only color.diff.old.

I see now that that might be since only the "zebra" supports the *Alternative that it ends up "stealing" chunks from something that would have otherwise been classified differently, so I have no idea if there's an easy "solution", or if it's even a problem.

Sorry about being vague, I just dug this up from some old notes now after this patch jolted my memory about it.