All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Yoshioka Tsuneo <yoshiokatsuneo@gmail.com>
Cc: Thomas Rast <tr@thomasrast.ch>,
	"git\@vger.kernel.org" <git@vger.kernel.org>,
	Duy Nguyen <pclouds@gmail.com>,
	Felipe Contreras <felipe.contreras@gmail.com>
Subject: Re: [PATCH v8] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible
Date: Tue, 22 Oct 2013 11:09:38 -0700	[thread overview]
Message-ID: <xmqqob6htbx9.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: BB9AEFCE-0E64-4EAA-8DEA-9A8125B8C553@gmail.com

Yoshioka Tsuneo <yoshiokatsuneo@gmail.com> writes:

> Also, I guess Junio might be suspicious to the idea to keep arrow("=>") itself, maybe ?

I think there is no single "right" solution to this issue, and it
has to boils down to the taste.

When you are viewing "diff --stat -M" output in wide-enough medium,
you are seeing three pieces of information: what the source path
was, what the destination path will be, and what amount of change is
made with the change. When the output width is too narrow to show
these paths, with the current code, you see truncated destination
path, possibly without the source path, but this patch will show the
source and the destination paths, both of which are truncated even
more severely, because it always has to spend display columns for an
extra "..." (to show truncation of the source side), " => " (to show
that it is a rename), and <"{","}"> pair (again to show that it is a
rename).  If the destination does not fit, the output before this
patch would have thrown these away as part of left-truncation, to
show the destination path as maximally as possible.  We do not have
even half the width of the current "truncated to be destination
only" output for each path.

I am afraid that in the cases where the patch makes a difference,
what happens would be that you can no longer tell what source or
destination paths really are, because the leading directory part
gets truncated too much, and if we didn't have this patch, at least
you can tell what destination path is affected.  We would trade the
guessability of at least one path (the destination) with just a
single bit of information (an unidentifiable path got renamed to
another unidentifiable path).

I am not yet convinced that it is a good trade-off.  Especially
given the diffstat output is not about files but more about
contents, between an output in the extreme case the version after
the patch needs to produce

	{... => ...}/controller/Makefile | 7 +++++++

that tells us "7 lines were updated in the procedure to build some
unknown controller by copying or renaming from the build procedure
of some other unknown controller", and the output the current code
would give to the same rename

	.}/fooGadget/controller/Makefile | 7 +++++++
        
that tells us "7 lines were updated in the build procedure for the
foo Gadget", I think the latter contains more useful information,
even though it does lose one bit of information ("there was a rename
involved in producing this final path") compared to the version with
the patch.

So you are correct to say that I am still skeptical.

In any case, the output from "diff --stat -M" should match the
output from "apply --stat -M", I think.

  reply	other threads:[~2013-10-22 18:09 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-10-11 11:24 [PATCH] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible Yoshioka Tsuneo
2013-10-11 13:07 ` [PATCH v2] " Yoshioka Tsuneo
2013-10-11 18:19   ` [spf:guess,mismatch] " Sam Vilain
2013-10-12  5:35     ` Keshav Kini
2013-10-12 20:52       ` Yoshioka Tsuneo
2013-10-12 20:48   ` [PATCH v3] " Yoshioka Tsuneo
2013-10-13 20:29     ` Thomas Rast
2013-10-15  9:46       ` Yoshioka Tsuneo
2013-10-14 19:04     ` Duy Nguyen
2013-10-15  9:46       ` Yoshioka Tsuneo
2013-10-15  9:45     ` [PATCH v4] " Yoshioka Tsuneo
2013-10-15 10:07       ` Felipe Contreras
2013-10-15 10:24         ` Yoshioka Tsuneo
2013-10-15 10:24       ` [PATCH v5] " Yoshioka Tsuneo
2013-10-15 22:54         ` Junio C Hamano
2013-10-15 22:58           ` Keshav Kini
2013-10-16  9:53           ` Yoshioka Tsuneo
2013-10-16  9:53         ` [PATCH v6] " Yoshioka Tsuneo
2013-10-17 19:29           ` Junio C Hamano
2013-10-17 22:08             ` Yoshioka Tsuneo
2013-10-17 22:38               ` Junio C Hamano
2013-10-18  9:35                 ` Yoshioka Tsuneo
2013-10-17 19:53           ` Junio C Hamano
2013-10-17 22:08           ` [PATCH v7] " Yoshioka Tsuneo
2013-10-18  9:35             ` [PATCH v8] " Yoshioka Tsuneo
2013-10-19  6:24               ` Thomas Rast
2013-10-20  1:49                 ` Yoshioka Tsuneo
2013-10-22 18:09                   ` Junio C Hamano [this message]
2013-10-22 20:14                     ` Yoshioka Tsuneo
2013-10-22 20:26                       ` Junio C Hamano
2013-10-12 20:48   ` [PATCH v3] " Yoshioka Tsuneo

Reply instructions:

You may reply publicly 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

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

  git send-email \
    --in-reply-to=xmqqob6htbx9.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox.com \
    --cc=felipe.contreras@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=pclouds@gmail.com \
    --cc=tr@thomasrast.ch \
    --cc=yoshiokatsuneo@gmail.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.