Yoshioka Tsuneo [off-list ref] writes:
"git diff -M --stat" can detect rename and show renamed file name like
"foofoofoo => barbarbar".
Before this commit, this output is shortened always by omitting left most
part like "...foo => barbarbar". So, if the destination filename is too long,
source filename putting left or arrow can be totally omitted like
"...barbarbar", without including any of "foofoofoo =>".
In such a case where arrow symbol is omitted, there is no way to know
whether the file is renamed or existed in the original.
Make sure there is always an arrow, like "...foo => ...bar".
The output can contain curly braces('{','}') for grouping.
So, in general, the output format is "<pfx>{<mid_a> => <mid_b>}<sfx>"
To keep arrow("=>"), try to omit <pfx> as long as possible at first
because later part or changing part will be the more important part.
If it is not enough, shorten <mid_a>, <mid_b> trying to have the same
maximum length.
If it is not enough yet, omit <sfx>.
Signed-off-by: Tsuneo Yoshioka <redacted>
Test-added-by: Thomas Rast [off-list ref]
---
Can you briefly describe what you changed in v7 and v8, both compared to
earlier versions and between v7 and v8?
It would be very nice if you could always include such a "patch
changelog" after the "---" above. git-am will ignore the text between
"---" and the diff, so you can write comments for the reviewers there
without creating noise in the commit message.
Also, please keep reviewers in the Cc list for future discussion/patches
so that they will see them.
--
Thomas Rast
tr@thomasrast.ch
Hello Thomas
Can you briefly describe what you changed in v7 and v8, both compared to
earlier versions and between v7 and v8?
On v7, <sfx>'s basename part is tried to kept. On v7, whole <sfx> part is tried to kept.
For example, in case below:
parent_path{sourceDirectory => DestinationDirectory}path1/path2//longlongFilename.txt
On v7, this can be like:
…{...ceDirectory => …onDirectory}.../longlongFilename.txt
On v8, it will be like:
…{...irectory => …irectory}path1/path2/longlongFilename.txt
This change is based on the review from Junio below.
(I myself is not sure what is the better way.)
================================
On Oct 17, 2013, at 10:29 PM, Junio C Hamano [off-list ref] wrote:I am not sure if distributing the burden of truncation equally to
three parts so that the resulting pieces are of similar lengths is
really a good idea. Between these two
{...SourceDirectory => ...nationDirectory}...ileThatWasMoved
{...ceDirectory => ...ionDirectory}nameOfTheFileThatWasMoved
that attempt to show that the file nameOfTheFileThatWasMoved was
moved from the longSourceDirectory to the DestinationDirectory, the
latter is much more informative, I would think.
On Oct 18, 2013, at 1:38 AM, Junio C Hamano [off-list ref] wrote:
Yoshioka Tsuneo [off-list ref] writes:
quoted
In the "[PATCH v7]", I changed to keep filename part of suffix to handle
above case, but not always keep directory part because I feel totally
keeping all part of long suffix including directory name may cause output like:
…{… => …}…ongPath1/LongPath2/nameOfTheFileThatWasMoved
And, above may be worse than:
...{...ceDirectory => …ionDirectory}.../nameOfTheFileThatWasMoved
I think.
I am not sure if I agree.
Losing LongPath2 part may be more significant data loss than losing
a single bit that says the change is a rename, as the latter may not
quite tell us what these two directories were anyway.
================================
Also, I guess Junio might be suspicious to the idea to keep arrow("=>") itself, maybe ?
=================================
(From What's cooking in git.git (Oct 2013, #04; Fri, 18))
- diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible
Attempts to give more weight on the fact that a filepair represents
a rename than showing substring of the actual path when diffstat
lines are not wide enough.
I am not sure if that is solving a right problem, though.
=================================
Thanks!
---
Tsuneo Yoshioka (吉岡 恒夫)
yoshiokatsuneo@gmail.com
On Oct 19, 2013, at 9:24 AM, Thomas Rast [off-list ref] wrote:
Yoshioka Tsuneo [off-list ref] writes:
quoted
"git diff -M --stat" can detect rename and show renamed file name like
"foofoofoo => barbarbar".
Before this commit, this output is shortened always by omitting left most
part like "...foo => barbarbar". So, if the destination filename is too long,
source filename putting left or arrow can be totally omitted like
"...barbarbar", without including any of "foofoofoo =>".
In such a case where arrow symbol is omitted, there is no way to know
whether the file is renamed or existed in the original.
Make sure there is always an arrow, like "...foo => ...bar".
The output can contain curly braces('{','}') for grouping.
So, in general, the output format is "<pfx>{<mid_a> => <mid_b>}<sfx>"
To keep arrow("=>"), try to omit <pfx> as long as possible at first
because later part or changing part will be the more important part.
If it is not enough, shorten <mid_a>, <mid_b> trying to have the same
maximum length.
If it is not enough yet, omit <sfx>.
Signed-off-by: Tsuneo Yoshioka <redacted>
Test-added-by: Thomas Rast [off-list ref]
---
Can you briefly describe what you changed in v7 and v8, both compared to
earlier versions and between v7 and v8?
It would be very nice if you could always include such a "patch
changelog" after the "---" above. git-am will ignore the text between
"---" and the diff, so you can write comments for the reviewers there
without creating noise in the commit message.
Also, please keep reviewers in the Cc list for future discussion/patches
so that they will see them.
--
Thomas Rast
tr@thomasrast.ch
Yoshioka Tsuneo [off-list ref] 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.
Hello Junio
Thank you for your comment.
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 [off-list ref] wrote: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 [off-list ref] wrote:
Yoshioka Tsuneo [off-list ref] writes:
quoted
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.