Re: [PATCH v8] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible

From: Yoshioka Tsuneo <yoshiokatsuneo@gmail.com>
Date: Oct 22, 2013, 20:14 UTC
Message-ID: <2CB6100D-747E-4F65-8F73-7BA381AC4BD4@gmail.com>
In-Reply-To: <xmqqob6htbx9.fsf@gitster.dls.corp.google.com>

Hello Junio

Thank you for your comment.

Show 6 quoted lines

> 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).

To be more accurate, renaming output dose not always contains "{" or "}" if there is no common part in source and destination paths, although probably there are enough large possibility to include "{" or "}". And, in the original patch, "{" or "}" is not kept, but changed to be kept based Thomas Rast's feedback below. (So, there was no possibility to have "{… => …}" in the original patch.)

On Oct 13, 2013, at 11:29 PM, Thomas Rast <tr@thomasrast.ch> wrote:

Show 11 quoted lines

> Note that in the test, the generated line looks like this:
> 
> {..._does_not_fit_in_a_single_line => .../path1                          | 0
> 
> I don't want to go all bikesheddey, but I think it's somewhat
> unfortunate that the elided parts do not correspond to each other.  In
> particular, I think the closing brace should not be omitted.  Perhaps
> something like this would be ideal (making it up on the spot, don't
> count characters):
> 
> {...a_single_line => ..._as_the_first}/path1                          | 0

And, it might be a bit nicer for me if the patch can be rejected(or ignored as other patches) from the beginning if the concept does not fit anyway. # Though I know we can know more after seeing the implementation, anyway :-) # And, my original explanation about the patch might be not enough.

Thanks !

--- Tsuneo Yoshioka (吉岡恒夫) yoshiokatsuneo@gmail.com

On Oct 22, 2013, at 9:09 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 56 quoted lines

> 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.