git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Stefan Beller <sbeller@google.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>, Jonathan Nieder <jrnieder@gmail.com>, Jonathan Tan <jonathantanmy@google.com>, Brandon Williams <bmwill@google.com>, Jeff King <peff@peff.net>, Michael Haggerty <mhagger@alum.mit.edu>
Subject: Re: [PATCHv2 12/20] submodule.c: convert show_submodule_summary to use emit_line_fmt
Date: Thu, 18 May 2017 10:12:20 -0700
Message-ID: <CAGZ79kbXyjTaE=F7YU8yu8=eNzLS96sA5r5N4xrhmOfw+T4Lwg@mail.gmail.com> (raw)
In-Reply-To: <xmqqinkyvobp.fsf@gitster.mtv.corp.google.com>

On Wed, May 17, 2017 at 8:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>>> +static void show_submodule_header(struct diff_options *o, const char *path,
>>>>               struct object_id *one, struct object_id *two,
>>>>               unsigned dirty_submodule, const char *meta,
>>>>               const char *reset,
>>> ...
>>> How does capturing these lines help moved line detection, by the
>>> way?  These must never be matched with any other added or removed
>>> line in the real patch output.
>>
>> Why?
>
> What are buffered are not patch text, but informational text like
> "Submodule X contains untracked content", etc.  When a text file is
> modified elsewhere and lost a line that happened to say the same
> contents, we do not want to consider that such a line was moved to
> where a submodule had an untracked file.
>
> I have a suspicion that the two-pass buffering is done at too high a
> level in this series.  Doesn't the code (I haven't reached the end
> of the series) update emit_line() to buffer the patch text and these
> non-patch text with all the coloring and resetting sequences?
> Because the "ah, this old line removed corresponds to that new line
> that appears elsewhere?" logic do not want to see these color/reset
> sequence, the buffering code needs to become quite specific to how
> the current diff code is colored (e.g. a line must be painted in a
> single color and have reset at the end) and makes future change to
> color things differently almost impossible (e.g. imagine how you
> would add a "feature" that paints certain words on added lines
> differently?).

That could be added in ws.c:ws_check_emit, as these certain words
are similar to coloring whitespace.

It depends on the precedence of such a future feature, is the move
detection or the word highlighting more important to keep its color?

> Ahh, yes, I see NEEDSWORK comment in patch 19/20.
>
> Yes, I agree that this code really should be working in terms of
> offsets into pre/post images when finding matching changes, which
> probably should happen without letting fn_out_consume produce fully
> colored textual diff output in the first pass.  For the purpose of
> "moved lines detection", the logic to match a stretch of preimage
> lines with postimage lines do not want to bother with "--- a/$path"
> headers, and it does not want to care if a line that begins with '+'
> needs to be added by calling emit_add_line() that knows how to check
> ws errors or the payload needs to be painted in green.  After the
> first pass determines which added lines are not true addition but
> merely moved, the second pass would know how that '+' line needs to
> be painted a lot better (e.g. it may not be painted in green).
> Letting fn_out_consume() call emit_add_line() only to compute
> information (e.g. "'+'? ok, green") that the first pass does not
> want to see and the second pass will compute better is probably not
> a good longer term direction to go in.
>
> 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.

For word coloring, we'd invent a new state BPL_EMIT_WITH_BRACES,
that would only store the word and at output time we'd have to add
sign, braces, colors. Then a block movement detection is possible.
(and this would also work with offset/len into the files longer term)

For the submodule diffs, I am really looking forward how Brandons
current work is coming along to have a repository struct such that we
can process submodules in the same process. For this diffing the
repo object would need to learn about the attribute system of
the submodules, such that we can obtain the whitespace coloring
rules, as well as the config (submodule may be configured to use
different colors for diffs).

  reply index

Thread overview: 128+ messages in thread (expand / mbox.gz / Atom feed / [top])
2017-05-14  4:00 [RFC PATCH 00/19] Diff machine: highlight moved lines Stefan Beller
2017-05-14  4:00 ` [PATCH 01/19] diff: readability fix Stefan Beller
2017-05-14  4:01 ` [PATCH 02/19] diff: move line ending check into emit_hunk_header Stefan Beller
2017-05-15  6:48   ` Junio C Hamano
2017-05-15 16:13     ` Stefan Beller
2017-05-14  4:01 ` [PATCH 03/19] diff.c: drop 'nofirst' from emit_line_0 Stefan Beller
2017-05-15 18:26   ` Jonathan Tan
2017-05-15 18:33     ` Stefan Beller
2017-05-16 16:05       ` Jonathan Tan
2017-05-15 19:22   ` Brandon Williams
2017-05-15 19:35     ` Stefan Beller
2017-05-15 19:45       ` Brandon Williams
2017-05-14  4:01 ` [PATCH 04/19] diff.c: factor out diff_flush_patch_all_file_pairs Stefan Beller
2017-05-14  4:01 ` [PATCH 05/19] diff.c: emit_line_0 can handle no color setting Stefan Beller
2017-05-15 18:31   ` Jonathan Tan
2017-05-15 22:11     ` Stefan Beller
2017-05-14  4:01 ` [PATCH 06/19] diff: add emit_line_fmt Stefan Beller
2017-05-15 19:31   ` Brandon Williams
2017-05-14  4:01 ` [PATCH 07/19] diff.c: convert fn_out_consume to use emit_line_* Stefan Beller
2017-05-16  1:00   ` Junio C Hamano
2017-05-16  1:05     ` Junio C Hamano
2017-05-16 16:23       ` Stefan Beller
2017-05-14  4:01 ` [PATCH 08/19] diff.c: convert builtin_diff " Stefan Beller
2017-05-15 18:42   ` Jonathan Tan
2017-05-14  4:01 ` [PATCH 09/19] diff.c: convert emit_rewrite_diff " Stefan Beller
2017-05-14  4:01 ` [PATCH 10/19] diff.c: convert emit_rewrite_lines " Stefan Beller
2017-05-15 19:09   ` Jonathan Tan
2017-05-15 19:31     ` Stefan Beller
2017-05-14  4:01 ` [PATCH 11/19] submodule.c: convert show_submodule_summary to use emit_line_fmt Stefan Beller
2017-05-14  4:01 ` [PATCH 12/19] diff.c: convert emit_binary_diff_body to use emit_line_* Stefan Beller
2017-05-14  4:01 ` [PATCH 13/19] diff.c: convert show_stats " Stefan Beller
2017-05-14  4:01 ` [PATCH 14/19] diff.c: convert word diffing " Stefan Beller
2017-05-15 22:40   ` Jonathan Tan
2017-05-15 23:12     ` Stefan Beller
2017-05-14  4:01 ` [PATCH 15/19] diff.c: convert diff_flush " Stefan Beller
2017-05-15 20:21   ` Jonathan Tan
2017-05-15 22:08     ` Stefan Beller
2017-05-14  4:01 ` [PATCH 16/19] diff.c: convert diff_summary " Stefan Beller
2017-05-14  4:01 ` [PATCH 17/19] diff.c: factor out emit_line_ws for coloring whitespaces Stefan Beller
2017-05-14  4:01 ` [PATCH 18/19] diff: buffer all output if asked to Stefan Beller
2017-05-14  4:06   ` Jeff King
2017-05-14  4:25     ` Stefan Beller
2017-05-16  4:14   ` Jonathan Tan
2017-05-16 16:42     ` Stefan Beller
2017-05-14  4:01 ` [PATCH 19/19] diff.c: color moved lines differently Stefan Beller
2017-05-15 22:42   ` Brandon Williams
2017-05-16  4:34   ` Jonathan Tan
2017-05-16 12:31   ` Jeff King
2017-05-15 12:43 ` [RFC PATCH 00/19] Diff machine: highlight moved lines Junio C Hamano
2017-05-15 16:33   ` Stefan Beller
2017-05-17  2:58 ` [PATCHv2 00/20] " Stefan Beller
2017-05-17  2:58   ` [PATCHv2 01/20] diff: readability fix Stefan Beller
2017-05-17  2:58   ` [PATCHv2 02/20] diff: move line ending check into emit_hunk_header Stefan Beller
2017-05-17  2:58   ` [PATCHv2 03/20] diff.c: factor out diff_flush_patch_all_file_pairs Stefan Beller
2017-05-17  2:58   ` [PATCHv2 04/20] diff.c: teach emit_line_0 to accept sign parameter Stefan Beller
2017-05-17  2:58   ` [PATCHv2 05/20] diff.c: emit_line_0 can handle no color setting Stefan Beller
2017-05-17  2:58   ` [PATCHv2 06/20] diff.c: emit_line_0 takes parameter whether to output line prefix Stefan Beller
2017-05-17  2:58   ` [PATCHv2 07/20] diff.c: inline emit_line_0 into emit_line Stefan Beller
2017-05-17  2:58   ` [PATCHv2 08/20] diff.c: convert fn_out_consume to use " Stefan Beller
2017-05-17  2:58   ` [PATCHv2 09/20] diff.c: convert builtin_diff to use emit_line_* Stefan Beller
2017-05-17  2:58   ` [PATCHv2 10/20] diff.c: convert emit_rewrite_diff " Stefan Beller
2017-05-17  2:58   ` [PATCHv2 11/20] diff.c: convert emit_rewrite_lines " Stefan Beller
2017-05-17  5:03     ` Junio C Hamano
2017-05-17 21:16       ` Stefan Beller
2017-05-18  3:35     ` Junio C Hamano
2017-05-17  2:58   ` [PATCHv2 12/20] submodule.c: convert show_submodule_summary to use emit_line_fmt Stefan Beller
2017-05-17  5:19     ` Junio C Hamano
2017-05-17 21:05       ` Stefan Beller
2017-05-18  3:25         ` Junio C Hamano
2017-05-18 17:12           ` Stefan Beller [this message]
2017-05-20  4:50             ` Junio C Hamano
2017-05-20 22:00               ` Stefan Beller
2017-05-17  2:58   ` [PATCHv2 13/20] diff.c: convert emit_binary_diff_body to use emit_line_* Stefan Beller
2017-05-17  2:58   ` [PATCHv2 14/20] diff.c: convert show_stats " Stefan Beller
2017-05-17  2:58   ` [PATCHv2 15/20] diff.c: convert word diffing " Stefan Beller
2017-05-17  2:58   ` [PATCHv2 16/20] diff.c: convert diff_flush " Stefan Beller
2017-05-17  2:58   ` [PATCHv2 17/20] diff.c: convert diff_summary " Stefan Beller
2017-05-17  2:58   ` [PATCHv2 18/20] diff.c: emit_line includes whitespace highlighting Stefan Beller
2017-05-17  2:58   ` [PATCHv2 19/20] diff: buffer all output if asked to Stefan Beller
2017-05-17  2:58   ` [PATCHv2 20/20] diff.c: color moved lines differently Stefan Beller
2017-05-18 19:37   ` [PATCHv3 00/20] Diff machine: highlight moved lines Stefan Beller
2017-05-18 19:37     ` [PATCHv3 01/20] diff: readability fix Stefan Beller
2017-05-18 19:37     ` [PATCHv3 02/20] diff: move line ending check into emit_hunk_header Stefan Beller
2017-05-18 19:37     ` [PATCHv3 03/20] diff.c: factor out diff_flush_patch_all_file_pairs Stefan Beller
2017-05-18 19:37     ` [PATCHv3 04/20] diff.c: teach emit_line_0 to accept sign parameter Stefan Beller
2017-05-18 23:33       ` Jonathan Tan
2017-05-22 23:36         ` Stefan Beller
2017-05-18 19:37     ` [PATCHv3 05/20] diff.c: emit_line_0 can handle no color setting Stefan Beller
2017-05-18 19:37     ` [PATCHv3 06/20] diff.c: emit_line_0 takes parameter whether to output line prefix Stefan Beller
2017-05-18 19:37     ` [PATCHv3 07/20] diff.c: inline emit_line_0 into emit_line Stefan Beller
2017-05-18 19:37     ` [PATCHv3 08/20] diff.c: convert fn_out_consume to use " Stefan Beller
2017-05-18 19:37     ` [PATCHv3 09/20] diff.c: convert builtin_diff to use emit_line_* Stefan Beller
2017-05-18 19:37     ` [PATCHv3 10/20] diff.c: convert emit_rewrite_diff " Stefan Beller
2017-05-18 19:37     ` [PATCHv3 11/20] diff.c: convert emit_rewrite_lines " Stefan Beller
2017-05-18 19:37     ` [PATCHv3 12/20] submodule.c: convert show_submodule_summary to use emit_line_fmt Stefan Beller
2017-05-18 19:37     ` [PATCHv3 13/20] diff.c: convert emit_binary_diff_body to use emit_line_* Stefan Beller
2017-05-18 19:37     ` [PATCHv3 14/20] diff.c: convert show_stats " Stefan Beller
2017-05-18 19:37     ` [PATCHv3 15/20] diff.c: convert word diffing " Stefan Beller
2017-05-18 19:37     ` [PATCHv3 16/20] diff.c: convert diff_flush " Stefan Beller
2017-05-18 19:37     ` [PATCHv3 17/20] diff.c: convert diff_summary " Stefan Beller
2017-05-18 19:37     ` [PATCHv3 18/20] diff.c: emit_line includes whitespace highlighting Stefan Beller
2017-05-18 19:37     ` [PATCHv3 19/20] diff: buffer all output if asked to Stefan Beller
2017-05-18 19:37     ` [PATCHv3 20/20] diff.c: color moved lines differently Stefan Beller
2017-05-19 18:23       ` Jonathan Tan
2017-05-19 18:40         ` Stefan Beller
2017-05-19 19:34           ` Jonathan Tan
2017-05-23  2:40     ` [PATCHv4 00/17] Diff machine: highlight moved lines Stefan Beller
2017-05-23  2:40       ` [PATCHv4 01/17] diff: readability fix Stefan Beller
2017-05-23  2:40       ` [PATCHv4 02/17] diff: move line ending check into emit_hunk_header Stefan Beller
2017-05-23  2:40       ` [PATCHv4 03/17] diff.c: factor out diff_flush_patch_all_file_pairs Stefan Beller
2017-05-23  2:40       ` [PATCHv4 04/17] diff: introduce more flexible emit function Stefan Beller
2017-05-23  2:40       ` [PATCHv4 05/17] diff.c: convert fn_out_consume to use emit_line Stefan Beller
2017-05-23  2:40       ` [PATCHv4 06/17] diff.c: convert builtin_diff to use emit_line_* Stefan Beller
2017-05-23  2:40       ` [PATCHv4 07/17] diff.c: convert emit_rewrite_diff " Stefan Beller
2017-05-23  2:40       ` [PATCHv4 08/17] diff.c: convert emit_rewrite_lines " Stefan Beller
2017-05-23  2:40       ` [PATCHv4 09/17] submodule.c: convert show_submodule_summary to use emit_line_fmt Stefan Beller
2017-05-23  5:59         ` Junio C Hamano
2017-05-23 18:14           ` Stefan Beller
2017-05-23  2:40       ` [PATCHv4 10/17] diff.c: convert emit_binary_diff_body to use emit_line_* Stefan Beller
2017-05-23  2:40       ` [PATCHv4 11/17] diff.c: convert show_stats " Stefan Beller
2017-05-23  2:40       ` [PATCHv4 12/17] diff.c: convert word diffing " Stefan Beller
2017-05-23  2:40       ` [PATCHv4 13/17] diff.c: convert diff_flush " Stefan Beller
2017-05-23  2:40       ` [PATCHv4 14/17] diff.c: convert diff_summary " Stefan Beller
2017-05-23  2:40       ` [PATCHv4 15/17] diff.c: emit_line includes whitespace highlighting Stefan Beller
2017-05-23  2:40       ` [PATCHv4 16/17] diff: buffer all output if asked to Stefan Beller
2017-05-23  2:40       ` [PATCHv4 17/17] diff.c: color moved lines differently Stefan Beller
2017-05-27  1:04       ` [PATCHv4 00/17] Diff machine: highlight moved lines Jacob Keller
2017-05-30 21:38         ` Stefan Beller

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply to all the recipients using the --to, --cc,
  and --in-reply-to switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAGZ79kbXyjTaE=F7YU8yu8=eNzLS96sA5r5N4xrhmOfw+T4Lwg@mail.gmail.com' \
    --to=sbeller@google.com \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jonathantanmy@google.com \
    --cc=jrnieder@gmail.com \
    --cc=mhagger@alum.mit.edu \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox