Re: [PATCH 5/7] diff.c: add a blocks mode for moved code detection

[Stefan Beller <sbeller@google.com>]

On Tue, Apr 24, 2018 at 2:50 PM, Jonathan Tan <jonathantanmy@google.com> wrote:
> On Tue, 24 Apr 2018 14:03:28 -0700
> Stefan Beller <sbeller@google.com> wrote:
>
>> Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>  (https://public-inbox.org/git/87o9j0uljo.fsf@evledraar.gmail.com/)
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>
> Firstly, I don't know if this is the right solution- as written
> in the linked e-mail [1], the issue might be more that the config
> conflates 2 unrelated things, not that a certain intersection is
> missing.

The "plain zebra" or as I call them "blocks", has the "heuristic
for a minimum of 20 characters" and "few colors" as its defining
features, which solves that use case.

Stepping back a bit, we have different "building blocks"
at our disposal:
* move detection by line or block
* alternating blocks
* a heuristic to skip over small chunks (20 alnum chars)

These can be combined independently, so would
you expect the user to expect two options for them?
For example "--color-moved=zebra" could be split
into  "--skip-small --alternate-blocks"

Eventually we'll use various colors to inform the user
what these building blocks made of the diff.

Ævar wrote:

> Which is what I mean by the current config conflating two (to me)
> unrelated things. One is how we, via any method, detect what's moved or
> not, and the other is what color/format we use to present this to the
> user.

So instead of building blocks we rather want to split into algorithms
and presentation layer?

The presentation layer would be things like:
* use a different color for moved things
* alternate colors for adjacent blocks
* paint border of a block (dimmed zebra)

The algorithm side would be
* detect moves
* detect moves as blocks
* skip small heuristic

Am I still missing the big picture?

> [1] https://public-inbox.org/git/87muykuij7.fsf@evledraar.gmail.com/
>
> Optional: Probably better to put the link inline, instead of in the
> trailer.

ok.

>
>> -test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '
>> +test_expect_success 'detect blocks of moved code' '
>>       git reset --hard &&
>>       cat <<-\EOF >lines.txt &&
>>               long line 1
>> @@ -1271,6 +1271,52 @@ test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '
>>       test_config color.diff.newMovedDimmed "normal cyan" &&
>>       test_config color.diff.oldMovedAlternativeDimmed "normal blue" &&
>>       test_config color.diff.newMovedAlternativeDimmed "normal yellow" &&
>
> Add a comment here explaining that these colors do not appear in the
> output, but merely set to recognizable values to ensure that they do not
> appear in the output.

ok.