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>, Jacob Keller <jacob.keller@gmail.com>, Michael Haggerty <mhagger@alum.mit.edu>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: What's cooking in git.git (Jun 2017, #03; Mon, 5)
Date: Mon, 5 Jun 2017 11:23:12 -0700
Message-ID: <CAGZ79kY2Z-fJYxczbzheu1hChLkKkdjEcDMwsP-hkN0TjUBotQ@mail.gmail.com> (raw)
In-Reply-To: <xmqq1sqzkrui.fsf@gitster.mtv.corp.google.com>

> * sb/diff-color-move (2017-06-01) 17 commits
>  - diff.c: color moved lines differently
>  - diff: buffer all output if asked to
>  - diff.c: emit_line includes whitespace highlighting
>  - diff.c: convert diff_summary to use emit_line_*
>  - diff.c: convert diff_flush to use emit_line_*
>  - diff.c: convert word diffing to use emit_line_*
>  - diff.c: convert show_stats to use emit_line_*
>  - diff.c: convert emit_binary_diff_body to use emit_line_*
>  - submodule.c: convert show_submodule_summary to use emit_line_fmt
>  - diff.c: convert emit_rewrite_lines to use emit_line_*
>  - diff.c: convert emit_rewrite_diff to use emit_line_*
>  - diff.c: convert builtin_diff to use emit_line_*
>  - diff.c: convert fn_out_consume to use emit_line
>  - diff: introduce more flexible emit function
>  - diff.c: factor out diff_flush_patch_all_file_pairs
>  - diff: move line ending check into emit_hunk_header
>  - diff: readability fix
>
>  "git diff" has been taught to optionally paint new lines that are
>  the same as deleted lines elsewhere differently from genuinely new
>  lines.
>
>  Are we happy with these changes?

I advertised this series e.g. for reviewing Brandons
repo object refactoring series and used it myself to inspect
some patches there[1]. I am certainly happy (but biased) with
what we have available there.

Jacob intended to use this series
for review as well, but has given no opinion yet.

You seemed to have used it for js/blame-lib?

--
Those patches had a wide reviewer audience cc'd,
so I would think people are aware of this series.

--
Things to come, but not in this series as they are more advanced:

    Discuss if a block/line needs a minimum requirement.

When doing reviews with this series, a couple of lines such
as "\t\t}" were marked as a moved, which is not wrong as they
really occurred in the text with opposing sign.
But it was annoying as it drew my attention to just closing
braces, which IMO is not the point of code review.

To solve this issue I had the idea of a "minimum requirement", e.g.
* at least 3 consecutive lines or
* at least one line with at least 3 non-ws characters or
* compute the entropy of a given moved block and if it is too low, do
  not mark it up.

I am not sure if such a "minimum requirement" is the right approach
at all. The nature of this discussion comes close to the diff heuristics
at which Michael did present a wonderful solution, hence I had him cc'd
on the series as he may have some good insights on how to improve
the diffs. :)

--
In conclusion:

We are happy to move to next as it seems technically sound.

But we want more exposure on usage to point out UX bugs.
(e.g. is the default mode for just giving --color-moved good for the
majority of people/use cases? Are there subtle annoyances such
as the closing braces?)

So maybe merge to next with the strong option to evict it
when finding more fundamentally wrong things?

Thanks,
Stefan

[1]
https://public-inbox.org/git/CAGZ79kZJF9iDsVgyi-hSKb6N8w0uhVCU4W-r89F0eRJPXe_4Og@mail.gmail.com/

  reply index

Thread overview: 14+ messages in thread (expand / mbox.gz / Atom feed / [top])
2017-06-05  3:59 Junio C Hamano
2017-06-05 18:23 ` Stefan Beller [this message]
2017-06-06  1:10   ` Junio C Hamano
2017-06-06  6:52     ` Jacob Keller
2017-06-08  5:41       ` Jacob Keller
2017-06-13 22:19         ` Stefan Beller
2017-06-14  9:54           ` Junio C Hamano
2017-06-14 18:44             ` Stefan Beller
2017-06-06  6:44   ` Jacob Keller
2017-06-06  9:50   ` Michael Haggerty
2017-06-06 22:05     ` Jacob Keller
2017-06-07 18:28       ` Stefan Beller
2017-06-07 21:58         ` Ævar Arnfjörð Bjarmason
2017-06-07 22:05           ` 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=CAGZ79kY2Z-fJYxczbzheu1hChLkKkdjEcDMwsP-hkN0TjUBotQ@mail.gmail.com \
    --to=sbeller@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jacob.keller@gmail.com \
    --cc=mhagger@alum.mit.edu \
    /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