Re: [PATCHv3] diff.c: emit moved lines with a different color
- From
Jacob Keller <jacob.keller@gmail.com>
- Date
- Sep 7, 2016, 21:21 UTC
- Message-ID
- <CA+P7+xp8KeeKY3jkWeSNzUvV1_cXOGpES4Ymj3BRTmM06jhY0Q@mail.gmail.com>
- In-Reply-To
- <CAGZ79kbSG-9xdmG3LWEsVoJnhEr5pcL8D-95E99a+=JaXGOXCA@mail.gmail.com>
On Wed, Sep 7, 2016 at 11:02 AM, Stefan Beller <sbeller@google.com> wrote:
Show 33 quoted lines
> On Wed, Sep 7, 2016 at 10:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> as we do not want to see dashes ('moved-old'), I think I'l go with
>>> "movedfrom" and "movedto".
>>
>> OK. They would be color.diff.movedFrom and color.diff.movedTo in
>> the doc, and "movedfrom" and "movedto" in the code (as the caller
>> already downcased them for you to strcmp()).
>>
>>> When going through the second pass and actually emitting colored lines
>>> we only find matches in the hash map if the current line AND the previous line
>>> match as we lookup by hash code, i.e. if we have a moved line, but the
>>> previous line
>>> changed we do not find it in the hashmap and we don't color it, such
>>> that the reviewer
>>> can spot a permutation.
>>
>> Hmph. Does this have impact on a line that was at the beginning or
>> the end of a file that got moved to the beginning or the end of a
>> file (four permutations, of 9 if you throw in "middle"), because
>> some cases it does not have a "previous" line?
>
> I spotted that problem as well. We need to adapt the algorithm a bit more:
>
> If the previous line is of the same kind (i.e. starting with + or -),
> then we have to take it into account,
> otherwise (i.e. previous line is different, such as header, hunk header,
> or different sign)
> then ignore previous line, as the previous line is already
> having a different color.
>
> That works for the very first line of a file as well.That should work to resolve some issues, and ensure the first line of a hunk works correctly.
Thanks, Jake