Re: [PATCHv2 12/20] submodule.c: convert show_submodule_summary to use emit_line_fmt

From: Stefan Beller <sbeller@google.com>
Date: May 20, 2017, 22:00 UTC
Message-ID: <CAGZ79kaHS2O90M8BXj_fEouSRgO+top7mkiawjqM7Xd=tUiX9g@mail.gmail.com>
In-Reply-To: <xmqqk25ct9lh.fsf@gitster.mtv.corp.google.com>

On Fri, May 19, 2017 at 9:50 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> That could be added in ws.c:ws_check_emit, as these certain words
>> are similar to coloring whitespace.
>
> I actually was envisioning of highlighting a part of a line, like
>
>     -Very <red>poor</red> SCM
>     +Very <green>nice</red> SCM
>
> which would be done by finding semi-matching removed and added lines
> in the same hunk (i.e. local buffering) and makes a coloring decision.
> That does not have any place in ws.c.

Yes such a feature would not want to be in ws.c

For the problem above, we're still fine with the given data structures IMO. Though it may hint at bad naming of the struct 'buffered_patch_line' as it is not a complete line.

Assuming the example above, running "show --word-diff" would produce the following "buffered_patch_lines"

{show_prefix=1, sign='\0', line="-Very ", set=NULL, reset=NULL} {show_prefix=0, sign='\0', line="{- poor}", set='red', reset='normal'} {show_prefix=0, sign='\0', line="{+ nice}", set='green', reset='normal'} {show_prefix=0, sign='\0', line=" SCM\n", set=NULL, reset=NULL}

so in the future, we may want to produce

{show_prefix=1, sign='-', line="Very ", set=NULL, reset=NULL} {show_prefix=0, sign='\0', line="poor", set='red', reset='normal'} {show_prefix=0, sign='\0', line=" SCM\n", set=NULL, reset=NULL} {show_prefix=1, sign='+', line="Very ", set=NULL, reset=NULL} {show_prefix=0, sign='\0', line="nice", set='gren', reset='normal'} {show_prefix=0, sign='\0', line=" SCM\n", set=NULL, reset=NULL}

instead.

I think the data structure can be used as-is, but is just mis-named. I'll fix that in a resend.

The algorithm to highlight what changed on a word level after a hunk would have to be put into the current hunk finding ("mark_color_as_moved"), and then split up the actual lines into pieces of a line and add different colors like we see above.

Show 18 quoted lines

>
>>> Having said that, we need to start somewhere, and I think it is a
>>> reasonable first-cut attempt to work on top of the textual output
>>> like this series does (IOW, while I do agree with the NEEDSWORK and
>>> the way this series currently does things must be revamped in the
>>> longer term, I do not think we should wait until that happens to
>>> start playing with this topic).
>>
>> Ok. I share a similar reaction to submodule diffs that we discuss above
>> and word coloring, that Jonathan Tan brought up off list.
>>
>> Both of them are broken in this implementation, but the NEEDSWORK
>> would hint at how to fix them.
>
> Yes, but if NEEDSWORK has to say "the current hack is working at a
> wrong level, we need to do all of this before producing textual
> diffs that are passed to the layer that colors lines", that wouldn't
> help that much as a hint X-<.

For the word coloring, I think we'd just need better post processing.

For cros-submodule move detection, we may want to wait in Brandons work to be able to run submodule stuff in-process and then we have the lines buffered directly there, so we can operate on them as well.

I agree that "NEEDSWORK: the current hack is working at a wrong level" is not useful. But I thought we're in agreement that it is not? I must have misunderstood parts of what you were saying.

By saying it could go to ws.c in my reply I rather meant: it could go into some post-processing function of the "buffered_patch_line" struct. Currently we only have ws_emit_line that does it, but we can do it either similarly or just split up one buffered_patch_line into more than one and color up each individually.

This would also be possible for us now, too. Instead of running it through ws_emit_line we could split up the line and color each piece differently. However that could be a problem in the algorithm to find similar hunks (as we do have different rules for added and deleted text).

Thanks, Stefan