* [PATCH] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
@ 2013-10-11 11:24 Yoshioka Tsuneo
2013-10-11 13:07 ` [PATCH v2] " Yoshioka Tsuneo
0 siblings, 1 reply; 31+ messages in thread
From: Yoshioka Tsuneo @ 2013-10-11 11:24 UTC (permalink / raw)
To: git@vger.kernel.org
"git diff -M --stat" can detect rename and show renamed file name like
"foofoofoo => barbarbar", but if destination filename is long the line
is shortened like "...barbarbar" so there is no way to know whether the
file is renamed or existed in the source commit.
This commit makes it visible like "...foo => ...bar".
Signed-off-by: Tsuneo Yoshioka <yoshiokatsuneo@gmail.com>
---
diff.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 47 insertions(+), 7 deletions(-)
diff --git a/diff.c b/diff.c
index a04a34d..9b55546 100644
--- a/diff.c
+++ b/diff.c
@@ -1643,13 +1643,53 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
len = name_width;
name_len = strlen(name);
if (name_width < name_len) {
- char *slash;
- prefix = "...";
- len -= 3;
- name += name_len - len;
- slash = strchr(name, '/');
- if (slash)
- name = slash;
+ char *arrow = strstr(name, " => ");
+ if(arrow){
+ int prefix_len = (name_width- 4) / 2;
+ int f_omit;
+ char *pre_arrow = alloca(name_width + 10);
+ char *post_arrow = arrow + 4;
+ char *prefix_buf = alloca(name_width + 10);
+ char *pre_arrow_slash = NULL;
+
+ if(arrow - name < prefix_len){
+ prefix_len = (int)(arrow - name);
+ f_omit = 0;
+ }else{
+ prefix_len -= 3;
+ f_omit = 1;
+ }
+ strncpy(pre_arrow, arrow - prefix_len, prefix_len);
+ pre_arrow[prefix_len] = '¥0';
+ pre_arrow_slash = strchr(pre_arrow, '/');
+ if(f_omit && pre_arrow_slash){
+ pre_arrow = pre_arrow_slash;
+ }
+ sprintf(prefix_buf, "%s%s => ", (f_omit ? "..." : ""), pre_arrow);
+ prefix = prefix_buf;
+
+ if(strlen(post_arrow) > name_width - strlen(prefix)){
+ char *post_arrow_slash = NULL;
+
+ post_arrow += strlen(post_arrow) - (name_width - strlen(prefix) - 3);
+ strcat(prefix_buf, "...");
+ post_arrow_slash = strchr(post_arrow, '/');
+ if(post_arrow_slash){
+ post_arrow = post_arrow_slash;
+ }
+ name = post_arrow;
+ name_len = (int) (name_width - strlen(prefix));
+ }
+ len -= strlen(prefix);
+ }else{
+ char *slash = NULL;
+ prefix = "...";
+ len -= 3;
+ name += name_len - len;
+ slash = strchr(name, '/');
+ if (slash)
+ name = slash;
+ }
}
if (file->is_binary) {
--
1.8.4.475.g867697c
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
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 ` Yoshioka Tsuneo
2013-10-11 18:19 ` [spf:guess,mismatch] " Sam Vilain
` (2 more replies)
0 siblings, 3 replies; 31+ messages in thread
From: Yoshioka Tsuneo @ 2013-10-11 13:07 UTC (permalink / raw)
To: git@vger.kernel.org
"git diff -M --stat" can detect rename and show renamed file name like
"foofoofoo => barbarbar", but if destination filename is long the line
is shortened like "...barbarbar" so there is no way to know whether the
file is renamed or existed in the source commit.
This commit makes it visible like "...foo => ...bar".
Signed-off-by: Tsuneo Yoshioka <yoshiokatsuneo@gmail.com>
---
diff.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 51 insertions(+), 7 deletions(-)
diff --git a/diff.c b/diff.c
index a04a34d..3aeaf3e 100644
--- a/diff.c
+++ b/diff.c
@@ -1643,13 +1643,57 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
len = name_width;
name_len = strlen(name);
if (name_width < name_len) {
- char *slash;
- prefix = "...";
- len -= 3;
- name += name_len - len;
- slash = strchr(name, '/');
- if (slash)
- name = slash;
+ char *arrow = strstr(name, " => ");
+ if (arrow) {
+ int prefix_len = (name_width - 4) / 2;
+ int f_omit;
+ int f_brace = 0;
+ char *pre_arrow = alloca(name_width + 10);
+ char *post_arrow = arrow + 4;
+ char *prefix_buf = alloca(name_width + 10);
+ char *pre_arrow_slash = NULL;
+
+ if (arrow - name < prefix_len) {
+ prefix_len = (int)(arrow - name);
+ f_omit = 0;
+ } else {
+ prefix_len -= 3;
+ f_omit = 1;
+ if (name[0] == '{') {
+ prefix_len -= 1;
+ f_brace = 1;
+ }
+ }
+ prefix_len = ((prefix_len >= 0) ? prefix_len : 0);
+ strncpy(pre_arrow, arrow - prefix_len, prefix_len);
+ pre_arrow[prefix_len] = '¥0';
+ pre_arrow_slash = strchr(pre_arrow, '/');
+ if (f_omit && pre_arrow_slash)
+ pre_arrow = pre_arrow_slash;
+ sprintf(prefix_buf, "%s%s%s => ", (f_brace ? "{" : ""), (f_omit ? "..." : ""), pre_arrow);
+ prefix = prefix_buf;
+
+ if (strlen(post_arrow) > name_width - strlen(prefix)) {
+ char *post_arrow_slash = NULL;
+
+ post_arrow += strlen(post_arrow) - (name_width - strlen(prefix) - 3);
+ strcat(prefix_buf, "...");
+ post_arrow_slash = strchr(post_arrow, '/');
+ if (post_arrow_slash)
+ post_arrow = post_arrow_slash;
+ name = post_arrow;
+ name_len = (int) (name_width - strlen(prefix));
+ }
+ len -= strlen(prefix);
+ } else {
+ char *slash = NULL;
+ prefix = "...";
+ len -= 3;
+ name += name_len - len;
+ slash = strchr(name, '/');
+ if (slash)
+ name = slash;
+ }
}
if (file->is_binary) {
--
1.8.4.475.g867697c
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [spf:guess,mismatch] [PATCH v2] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
2013-10-11 13:07 ` [PATCH v2] " Yoshioka Tsuneo
@ 2013-10-11 18:19 ` Sam Vilain
2013-10-12 5:35 ` Keshav Kini
2013-10-12 20:48 ` [PATCH v3] " Yoshioka Tsuneo
2013-10-12 20:48 ` [PATCH v3] " Yoshioka Tsuneo
2 siblings, 1 reply; 31+ messages in thread
From: Sam Vilain @ 2013-10-11 18:19 UTC (permalink / raw)
To: Yoshioka Tsuneo, git@vger.kernel.org
On 10/11/2013 06:07 AM, Yoshioka Tsuneo wrote:
> + prefix_len = ((prefix_len >= 0) ? prefix_len : 0);
> + strncpy(pre_arrow, arrow - prefix_len, prefix_len);
> + pre_arrow[prefix_len] = '¥0';
This seems to be an encoding mistake; was this supposed to be an ASCII
arrow?
Sam
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [spf:guess,mismatch] [PATCH v2] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
2013-10-11 18:19 ` [spf:guess,mismatch] " Sam Vilain
@ 2013-10-12 5:35 ` Keshav Kini
2013-10-12 20:52 ` Yoshioka Tsuneo
0 siblings, 1 reply; 31+ messages in thread
From: Keshav Kini @ 2013-10-12 5:35 UTC (permalink / raw)
To: git
Sam Vilain <sam@vilain.net> writes:
> On 10/11/2013 06:07 AM, Yoshioka Tsuneo wrote:
>> + prefix_len = ((prefix_len >= 0) ? prefix_len : 0);
>> + strncpy(pre_arrow, arrow - prefix_len, prefix_len);
>> + pre_arrow[prefix_len] = '¥0';
>
>
> This seems to be an encoding mistake; was this supposed to be an ASCII
> arrow?
That's supposed to be a null terminator character, '\0'. See
https://en.wikipedia.org/wiki/Yen_symbol#Code_page_932
-Keshav
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v3] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
2013-10-11 13:07 ` [PATCH v2] " Yoshioka Tsuneo
2013-10-11 18:19 ` [spf:guess,mismatch] " Sam Vilain
@ 2013-10-12 20:48 ` Yoshioka Tsuneo
2013-10-13 20:29 ` Thomas Rast
` (2 more replies)
2013-10-12 20:48 ` [PATCH v3] " Yoshioka Tsuneo
2 siblings, 3 replies; 31+ messages in thread
From: Yoshioka Tsuneo @ 2013-10-12 20:48 UTC (permalink / raw)
To: git@vger.kernel.org
"git diff -M --stat" can detect rename and show renamed file name like
"foofoofoo => barbarbar", but if destination filename is long the line
is shortened like "...barbarbar" so there is no way to know whether the
file is renamed or existed in the source commit.
This commit makes it visible like "...foo => ...bar".
Signed-off-by: Tsuneo Yoshioka <yoshiokatsuneo@gmail.com>
---
diff.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 51 insertions(+), 7 deletions(-)
diff --git a/diff.c b/diff.c
index a04a34d..3aeaf3e 100644
--- a/diff.c
+++ b/diff.c
@@ -1643,13 +1643,57 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
len = name_width;
name_len = strlen(name);
if (name_width < name_len) {
- char *slash;
- prefix = "...";
- len -= 3;
- name += name_len - len;
- slash = strchr(name, '/');
- if (slash)
- name = slash;
+ char *arrow = strstr(name, " => ");
+ if (arrow) {
+ int prefix_len = (name_width - 4) / 2;
+ int f_omit;
+ int f_brace = 0;
+ char *pre_arrow = alloca(name_width + 10);
+ char *post_arrow = arrow + 4;
+ char *prefix_buf = alloca(name_width + 10);
+ char *pre_arrow_slash = NULL;
+
+ if (arrow - name < prefix_len) {
+ prefix_len = (int)(arrow - name);
+ f_omit = 0;
+ } else {
+ prefix_len -= 3;
+ f_omit = 1;
+ if (name[0] == '{') {
+ prefix_len -= 1;
+ f_brace = 1;
+ }
+ }
+ prefix_len = ((prefix_len >= 0) ? prefix_len : 0);
+ strncpy(pre_arrow, arrow - prefix_len, prefix_len);
+ pre_arrow[prefix_len] = '\0';
+ pre_arrow_slash = strchr(pre_arrow, '/');
+ if (f_omit && pre_arrow_slash)
+ pre_arrow = pre_arrow_slash;
+ sprintf(prefix_buf, "%s%s%s => ", (f_brace ? "{" : ""), (f_omit ? "..." : ""), pre_arrow);
+ prefix = prefix_buf;
+
+ if (strlen(post_arrow) > name_width - strlen(prefix)) {
+ char *post_arrow_slash = NULL;
+
+ post_arrow += strlen(post_arrow) - (name_width - strlen(prefix) - 3);
+ strcat(prefix_buf, "...");
+ post_arrow_slash = strchr(post_arrow, '/');
+ if (post_arrow_slash)
+ post_arrow = post_arrow_slash;
+ name = post_arrow;
+ name_len = (int) (name_width - strlen(prefix));
+ }
+ len -= strlen(prefix);
+ } else {
+ char *slash = NULL;
+ prefix = "...";
+ len -= 3;
+ name += name_len - len;
+ slash = strchr(name, '/');
+ if (slash)
+ name = slash;
+ }
}
if (file->is_binary) {
--
1.8.4.475.g867697c
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v3] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
2013-10-11 13:07 ` [PATCH v2] " Yoshioka Tsuneo
2013-10-11 18:19 ` [spf:guess,mismatch] " Sam Vilain
2013-10-12 20:48 ` [PATCH v3] " Yoshioka Tsuneo
@ 2013-10-12 20:48 ` Yoshioka Tsuneo
2 siblings, 0 replies; 31+ messages in thread
From: Yoshioka Tsuneo @ 2013-10-12 20:48 UTC (permalink / raw)
To: git@vger.kernel.org
"git diff -M --stat" can detect rename and show renamed file name like
"foofoofoo => barbarbar", but if destination filename is long the line
is shortened like "...barbarbar" so there is no way to know whether the
file is renamed or existed in the source commit.
This commit makes it visible like "...foo => ...bar".
Signed-off-by: Tsuneo Yoshioka <yoshiokatsuneo@gmail.com>
---
diff.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 51 insertions(+), 7 deletions(-)
diff --git a/diff.c b/diff.c
index a04a34d..3aeaf3e 100644
--- a/diff.c
+++ b/diff.c
@@ -1643,13 +1643,57 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
len = name_width;
name_len = strlen(name);
if (name_width < name_len) {
- char *slash;
- prefix = "...";
- len -= 3;
- name += name_len - len;
- slash = strchr(name, '/');
- if (slash)
- name = slash;
+ char *arrow = strstr(name, " => ");
+ if (arrow) {
+ int prefix_len = (name_width - 4) / 2;
+ int f_omit;
+ int f_brace = 0;
+ char *pre_arrow = alloca(name_width + 10);
+ char *post_arrow = arrow + 4;
+ char *prefix_buf = alloca(name_width + 10);
+ char *pre_arrow_slash = NULL;
+
+ if (arrow - name < prefix_len) {
+ prefix_len = (int)(arrow - name);
+ f_omit = 0;
+ } else {
+ prefix_len -= 3;
+ f_omit = 1;
+ if (name[0] == '{') {
+ prefix_len -= 1;
+ f_brace = 1;
+ }
+ }
+ prefix_len = ((prefix_len >= 0) ? prefix_len : 0);
+ strncpy(pre_arrow, arrow - prefix_len, prefix_len);
+ pre_arrow[prefix_len] = '\0';
+ pre_arrow_slash = strchr(pre_arrow, '/');
+ if (f_omit && pre_arrow_slash)
+ pre_arrow = pre_arrow_slash;
+ sprintf(prefix_buf, "%s%s%s => ", (f_brace ? "{" : ""), (f_omit ? "..." : ""), pre_arrow);
+ prefix = prefix_buf;
+
+ if (strlen(post_arrow) > name_width - strlen(prefix)) {
+ char *post_arrow_slash = NULL;
+
+ post_arrow += strlen(post_arrow) - (name_width - strlen(prefix) - 3);
+ strcat(prefix_buf, "...");
+ post_arrow_slash = strchr(post_arrow, '/');
+ if (post_arrow_slash)
+ post_arrow = post_arrow_slash;
+ name = post_arrow;
+ name_len = (int) (name_width - strlen(prefix));
+ }
+ len -= strlen(prefix);
+ } else {
+ char *slash = NULL;
+ prefix = "...";
+ len -= 3;
+ name += name_len - len;
+ slash = strchr(name, '/');
+ if (slash)
+ name = slash;
+ }
}
if (file->is_binary) {
--
1.8.4.475.g867697c
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [spf:guess,mismatch] [PATCH v2] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
2013-10-12 5:35 ` Keshav Kini
@ 2013-10-12 20:52 ` Yoshioka Tsuneo
0 siblings, 0 replies; 31+ messages in thread
From: Yoshioka Tsuneo @ 2013-10-12 20:52 UTC (permalink / raw)
To: git@vger.kernel.org, Keshav Kini
Hello
On Oct 12, 2013, at 8:35 AM, Keshav Kini <keshav.kini@gmail.com> wrote:
> Sam Vilain <sam@vilain.net> writes:
>
>> On 10/11/2013 06:07 AM, Yoshioka Tsuneo wrote:
>>> + prefix_len = ((prefix_len >= 0) ? prefix_len : 0);
>>> + strncpy(pre_arrow, arrow - prefix_len, prefix_len);
>>> + pre_arrow[prefix_len] = '¥0';
>>
>>
>> This seems to be an encoding mistake; was this supposed to be an ASCII
>> arrow?
>
> That's supposed to be a null terminator character, '\0'. See
> https://en.wikipedia.org/wiki/Yen_symbol#Code_page_932
Thank you for pointing out the encoding issue.
It looks, I need to change encoding of "pbcopy" command to
"en_US.UTF-8" from "C" on Mac OS X.
I just sent updated patch as "PATCH v3".
Thanks !
---
Tsuneo Yoshioka (吉岡 恒夫)
yoshiokatsuneo@gmail.com
On Oct 12, 2013, at 8:35 AM, Keshav Kini <keshav.kini@gmail.com> wrote:
> Sam Vilain <sam@vilain.net> writes:
>
>> On 10/11/2013 06:07 AM, Yoshioka Tsuneo wrote:
>>> + prefix_len = ((prefix_len >= 0) ? prefix_len : 0);
>>> + strncpy(pre_arrow, arrow - prefix_len, prefix_len);
>>> + pre_arrow[prefix_len] = '¥0';
>>
>>
>> This seems to be an encoding mistake; was this supposed to be an ASCII
>> arrow?
>
> That's supposed to be a null terminator character, '\0'. See
> https://en.wikipedia.org/wiki/Yen_symbol#Code_page_932
>
> -Keshav
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v3] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
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:45 ` [PATCH v4] " Yoshioka Tsuneo
2 siblings, 1 reply; 31+ messages in thread
From: Thomas Rast @ 2013-10-13 20:29 UTC (permalink / raw)
To: Yoshioka Tsuneo; +Cc: git@vger.kernel.org
Hi,
Yoshioka Tsuneo <yoshiokatsuneo@gmail.com> writes:
> "git diff -M --stat" can detect rename and show renamed file name like
> "foofoofoo => barbarbar", but if destination filename is long the line
> is shortened like "...barbarbar" so there is no way to know whether the
> file is renamed or existed in the source commit.
Thanks for your patch! I think this is indeed something that should be
fixed.
Can you explain the algorithm chosen in the commit message or a block
comment in the code? I find it much easier to follow large code blocks
(like the one you added) with a prior notion of what it tries to do.
[As an aside, Documentation/SubmittingPatches says
The body should provide a meaningful commit message, which:
. explains the problem the change tries to solve, iow, what is wrong
with the current code without the change.
. justifies the way the change solves the problem, iow, why the
result with the change is better.
. alternate solutions considered but discarded, if any.
Observe that you explained the first item very well, but not the
others.]
> This commit makes it visible like "...foo => ...bar".
Also, you should rewrite this to be in the imperative mood:
Make sure there is always an arrow, e.g., "...foo => ...bar".
or some such.
[Again from SubmittingPatches:
Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour.]
> Signed-off-by: Tsuneo Yoshioka <yoshiokatsuneo@gmail.com>
> ---
> diff.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 51 insertions(+), 7 deletions(-)
Can you add a test? Perhaps like the one below. (You can squash it
into your commit if you like it.)
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
diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
index 2f327b7..03d6371 100755
--- a/t/t4001-diff-rename.sh
+++ b/t/t4001-diff-rename.sh
@@ -156,4 +156,16 @@ test_expect_success 'rename pretty print common prefix and suffix overlap' '
test_i18ngrep " d/f/{ => f}/e " output
'
+test_expect_success 'rename of very long path shows =>' '
+ mkdir long_dirname_that_does_not_fit_in_a_single_line &&
+ mkdir another_extremely_long_path_but_not_the_same_as_the_first &&
+ cp path1 long_dirname*/ &&
+ git add long_dirname*/path1 &&
+ test_commit add_long_pathname &&
+ git mv long_dirname*/path1 another_extremely_*/ &&
+ test_commit move_long_pathname &&
+ git diff -M --stat HEAD^ HEAD >output &&
+ test_i18ngrep "=>.*path1" output
+'
+
test_done
--
Thomas Rast
tr@thomasrast.ch
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v3] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
2013-10-12 20:48 ` [PATCH v3] " Yoshioka Tsuneo
2013-10-13 20:29 ` Thomas Rast
@ 2013-10-14 19:04 ` Duy Nguyen
2013-10-15 9:46 ` Yoshioka Tsuneo
2013-10-15 9:45 ` [PATCH v4] " Yoshioka Tsuneo
2 siblings, 1 reply; 31+ messages in thread
From: Duy Nguyen @ 2013-10-14 19:04 UTC (permalink / raw)
To: Yoshioka Tsuneo; +Cc: git@vger.kernel.org
On Sun, Oct 13, 2013 at 3:48 AM, Yoshioka Tsuneo
<yoshiokatsuneo@gmail.com> wrote:
> "git diff -M --stat" can detect rename and show renamed file name like
> "foofoofoo => barbarbar", but if destination filename is long the line
> is shortened like "...barbarbar" so there is no way to know whether the
> file is renamed or existed in the source commit.
> This commit makes it visible like "...foo => ...bar".
>
> Signed-off-by: Tsuneo Yoshioka <yoshiokatsuneo@gmail.com>
> ---
> diff.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 51 insertions(+), 7 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index a04a34d..3aeaf3e 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1643,13 +1643,57 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
> len = name_width;
> name_len = strlen(name);
> if (name_width < name_len) {
> - char *slash;
> - prefix = "...";
> - len -= 3;
> - name += name_len - len;
> - slash = strchr(name, '/');
> - if (slash)
> - name = slash;
> + char *arrow = strstr(name, " => ");
> + if (arrow) {
This looks iffy. What if " => " is part of the path name?
file->is_renamed would be a more reliable sign. In that case I think
you just need an ellipsis version of pprint_rename() (i.e. drop the
result of previous pprint_rename() on the floor and create a new
string with "..." and " => " in your pprint_ellipsis_rename or
something)
> + int prefix_len = (name_width - 4) / 2;
> + int f_omit;
> + int f_brace = 0;
> + char *pre_arrow = alloca(name_width + 10);
> + char *post_arrow = arrow + 4;
> + char *prefix_buf = alloca(name_width + 10);
> + char *pre_arrow_slash = NULL;
> +
> + if (arrow - name < prefix_len) {
> + prefix_len = (int)(arrow - name);
> + f_omit = 0;
> + } else {
> + prefix_len -= 3;
> + f_omit = 1;
> + if (name[0] == '{') {
> + prefix_len -= 1;
> + f_brace = 1;
> + }
> + }
> + prefix_len = ((prefix_len >= 0) ? prefix_len : 0);
> + strncpy(pre_arrow, arrow - prefix_len, prefix_len);
> + pre_arrow[prefix_len] = '\0';
> + pre_arrow_slash = strchr(pre_arrow, '/');
> + if (f_omit && pre_arrow_slash)
> + pre_arrow = pre_arrow_slash;
> + sprintf(prefix_buf, "%s%s%s => ", (f_brace ? "{" : ""), (f_omit ? "..." : ""), pre_arrow);
> + prefix = prefix_buf;
> +
> + if (strlen(post_arrow) > name_width - strlen(prefix)) {
> + char *post_arrow_slash = NULL;
> +
> + post_arrow += strlen(post_arrow) - (name_width - strlen(prefix) - 3);
> + strcat(prefix_buf, "...");
> + post_arrow_slash = strchr(post_arrow, '/');
> + if (post_arrow_slash)
> + post_arrow = post_arrow_slash;
> + name = post_arrow;
> + name_len = (int) (name_width - strlen(prefix));
> + }
> + len -= strlen(prefix);
> + } else {
> + char *slash = NULL;
> + prefix = "...";
> + len -= 3;
> + name += name_len - len;
> + slash = strchr(name, '/');
> + if (slash)
> + name = slash;
> + }
> }
>
> if (file->is_binary) {
> --
> 1.8.4.475.g867697c
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Duy
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v4] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
2013-10-12 20:48 ` [PATCH v3] " Yoshioka Tsuneo
2013-10-13 20:29 ` Thomas Rast
2013-10-14 19:04 ` Duy Nguyen
@ 2013-10-15 9:45 ` Yoshioka Tsuneo
2013-10-15 10:07 ` Felipe Contreras
2013-10-15 10:24 ` [PATCH v5] " Yoshioka Tsuneo
2 siblings, 2 replies; 31+ messages in thread
From: Yoshioka Tsuneo @ 2013-10-15 9:45 UTC (permalink / raw)
To: git@vger.kernel.org
"git diff -M --stat" can detect rename and show renamed file name like
"foofoofoo => barbarbar". But if destination filename is long, the line
is shortened like "...barbarbar" so there is no way to know whether the
file is renamed or existed in the source commit.
Make sure there is always an arrow, like "...foo => ...bar".
The output can contains curly braces('{','}') for grouping.
So, in general, the outpu 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>, and <sfx> trying to
have the maximum length the same because those will be equaly important.
Signed-off-by: Tsuneo Yoshioka <yoshiokatsuneo@gmail.com>
Test-added-by: Thomas Rast <trast@inf.ethz.ch>
---
diff.c | 183 +++++++++++++++++++++++++++++++++++++++++++------
t/t4001-diff-rename.sh | 12 ++++
2 files changed, 173 insertions(+), 22 deletions(-)
diff --git a/diff.c b/diff.c
index a04a34d..7f907ed 100644
--- a/diff.c
+++ b/diff.c
@@ -1258,11 +1258,10 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
}
}
-static char *pprint_rename(const char *a, const char *b)
+static void pprint_rename_find_common_prefix_suffix(const char *a, const char *b, struct strbuf *pfx, struct strbuf *a_mid, struct strbuf *b_mid, struct strbuf *sfx)
{
const char *old = a;
const char *new = b;
- struct strbuf name = STRBUF_INIT;
int pfx_length, sfx_length;
int pfx_adjust_for_slash;
int len_a = strlen(a);
@@ -1272,10 +1271,9 @@ static char *pprint_rename(const char *a, const char *b)
int qlen_b = quote_c_style(b, NULL, NULL, 0);
if (qlen_a || qlen_b) {
- quote_c_style(a, &name, NULL, 0);
- strbuf_addstr(&name, " => ");
- quote_c_style(b, &name, NULL, 0);
- return strbuf_detach(&name, NULL);
+ quote_c_style(a, a_mid, NULL, 0);
+ quote_c_style(b, b_mid, NULL, 0);
+ return;
}
/* Find common prefix */
@@ -1321,18 +1319,149 @@ static char *pprint_rename(const char *a, const char *b)
a_midlen = 0;
if (b_midlen < 0)
b_midlen = 0;
+
+ strbuf_add(pfx, a, pfx_length);
+ strbuf_add(a_mid, a + pfx_length, a_midlen);
+ strbuf_add(b_mid, b + pfx_length, b_midlen);
+ strbuf_add(sfx, a + len_a - sfx_length, sfx_length);
+}
- strbuf_grow(&name, pfx_length + a_midlen + b_midlen + sfx_length + 7);
- if (pfx_length + sfx_length) {
- strbuf_add(&name, a, pfx_length);
+/*
+ * Omit each parts to fix in name_width.
+ * Formatted string is "<pfx>{<a_mid> => <b_mid>}<sfx>".
+ * At first, omit <pfx> as long as possible.
+ * If it is not enough, omit <a_mid>, <b_mid>, <sfx> by tring to set the length of
+ * those 3 parts(including "...") to the same.
+ * Ex:
+ * "foofoofoo => barbarbar"
+ * will be like
+ * "...foo => ...bar".
+ * "long_parent{foofoofoo => barbarbar}longfilename"
+ * will be like
+ * "...parent{...foofoo => ...barbar}...lename"
+ */
+static void pprint_rename_omit(struct strbuf *pfx, struct strbuf *a_mid, struct strbuf *b_mid, struct strbuf *sfx, int name_width)
+{
+
+#define ARROW " => "
+#define ELLIPSIS "..."
+#define swap(a,b) myswap((a),(b),sizeof(a))
+
+#define myswap(a, b, size) do { \
+unsigned char mytmp[size]; \
+memcpy(mytmp, &a, size); \
+memcpy(&a, &b, size); \
+memcpy(&b, mytmp, size); \
+} while (0)
+
+ int use_curly_braces = (pfx->len > 0) || (sfx->len > 0);
+ size_t name_len;
+ size_t len;
+ size_t part_lengths[4];
+ size_t max_part_len = 0;
+ size_t remainder_part_len = 0;
+ int i, j;
+
+ name_len = pfx->len + a_mid->len + b_mid->len + sfx->len + strlen(ARROW) + (use_curly_braces?2:0);
+
+ if (name_len <= name_width){
+ /* Everthing fits in name_width */
+ return;
+ }
+
+ if(use_curly_braces){
+ if(strlen(ELLIPSIS) + (name_len - pfx->len) <= name_width){
+ /*
+ Just omitting left of '{' is enough
+ Ex: ...aaa{foofoofoo => bar}file
+ */
+ strbuf_splice(pfx, name_len - pfx->len, name_width - (name_len - pfx->len), ELLIPSIS, strlen(ELLIPSIS));
+ return;
+ }else{
+ if (pfx->len > strlen(ELLIPSIS)) {
+ /*
+ Just omitting left of '{' is not enough
+ name will be "...{SOMETHING}SOMETHING"
+ */
+ strbuf_reset(pfx);
+ strbuf_addstr(pfx, ELLIPSIS);
+ }
+ }
+ }
+
+ /* available length for a_mid, b_mid and sfx */
+ len = name_width - strlen(ARROW) - (use_curly_braces?2:0);
+
+ /* a_mid, b_mid, sfx will be have the same max, including ellipsis("..."). */
+ part_lengths[0] = (int)a_mid->len;
+ part_lengths[1] = (int)b_mid->len;
+ part_lengths[2] = (int)sfx->len;
+
+ /* bubble sort of part_lengths, descending order */
+ for(i=0;i<3;i++){
+ for(j= i+1; j<3; j++){
+ if(part_lengths[j] > part_lengths[i]){
+ swap(part_lengths[i], part_lengths[j]);
+ }
+ }
+ }
+
+ if (part_lengths[1] + part_lengths[1] + part_lengths[2] <= len) {
+ /*
+ * "{...foofoo => barbar}file"
+ * There is only one omitting part.
+ */
+ max_part_len = len - part_lengths[1] - part_lengths[2];
+ } else if (part_lengths[2] + part_lengths[2] + part_lengths[2] <= len){
+ /*
+ * "{...foofoo => ...barbar}file"
+ * There are 2 omitting part.
+ */
+ max_part_len = (len - part_lengths[2])/2;
+ remainder_part_len = (len - part_lengths[2]) - max_part_len * 2;
+ } else {
+ /*
+ * "{...ofoo => ...rbar}...file"
+ * There are 3 omitting part.
+ */
+ max_part_len = len / 3;
+ remainder_part_len = len - (max_part_len) * 3;
+ }
+
+ if (max_part_len < strlen(ELLIPSIS))
+ max_part_len = strlen(ELLIPSIS);
+
+ if (sfx->len > max_part_len)
+ strbuf_splice(sfx, 0, sfx->len - max_part_len + strlen(ELLIPSIS), ELLIPSIS, strlen(ELLIPSIS));
+ if (remainder_part_len==2)
+ max_part_len++;
+ if (a_mid->len > max_part_len)
+ strbuf_splice(a_mid, 0, a_mid->len - max_part_len + strlen(ELLIPSIS), ELLIPSIS, strlen(ELLIPSIS));
+ if (remainder_part_len==1)
+ max_part_len++;
+ if (b_mid->len > max_part_len)
+ strbuf_splice(b_mid, 0, b_mid->len - max_part_len + strlen(ELLIPSIS), ELLIPSIS, strlen(ELLIPSIS));
+}
+
+static char *pprint_rename(const char *a, const char *b, int name_width)
+{
+ struct strbuf pfx = STRBUF_INIT, a_mid = STRBUF_INIT, b_mid = STRBUF_INIT, sfx = STRBUF_INIT;
+ struct strbuf name = STRBUF_INIT;
+
+ pprint_rename_find_common_prefix_suffix(a, b, &pfx, &a_mid, &b_mid, &sfx);
+ pprint_rename_omit(&pfx, &a_mid, &b_mid, &sfx, name_width);
+
+ strbuf_grow(&name, pfx.len + a_mid.len + b_mid.len + sfx.len + 7);
+ if (pfx.len + sfx.len) {
+ strbuf_addbuf(&name, &pfx);
strbuf_addch(&name, '{');
}
- strbuf_add(&name, a + pfx_length, a_midlen);
+ strbuf_addbuf(&name, &a_mid);
strbuf_addstr(&name, " => ");
- strbuf_add(&name, b + pfx_length, b_midlen);
- if (pfx_length + sfx_length) {
+ strbuf_addbuf(&name, &b_mid);
+ if (pfx.len + sfx.len) {
strbuf_addch(&name, '}');
- strbuf_add(&name, a + len_a - sfx_length, sfx_length);
+ strbuf_addbuf(&name, &sfx);
}
return strbuf_detach(&name, NULL);
}
@@ -1418,23 +1547,31 @@ static void show_graph(FILE *file, char ch, int cnt, const char *set, const char
fprintf(file, "%s", reset);
}
-static void fill_print_name(struct diffstat_file *file)
+static void fill_print_name(struct diffstat_file *file, int name_width)
{
char *pname;
- if (file->print_name)
- return;
-
if (!file->is_renamed) {
struct strbuf buf = STRBUF_INIT;
+ if (file->print_name)
+ return;
if (quote_c_style(file->name, &buf, NULL, 0)) {
pname = strbuf_detach(&buf, NULL);
} else {
pname = file->name;
strbuf_release(&buf);
}
+ if(strlen(pname) > name_width){
+ struct strbuf buf2 = STRBUF_INIT;
+ strbuf_addstr(&buf2, "...");
+ strbuf_addstr(&buf2, pname + strlen(pname) - name_width - 3);
+ }
} else {
- pname = pprint_rename(file->from_name, file->name);
+ if (file->print_name){
+ free(file->print_name);
+ file->print_name = NULL;
+ }
+ pname = pprint_rename(file->from_name, file->name, name_width);
}
file->print_name = pname;
}
@@ -1517,7 +1654,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
count++; /* not shown == room for one more */
continue;
}
- fill_print_name(file);
+ fill_print_name(file, INT_MAX);
len = strlen(file->print_name);
if (max_len < len)
max_len = len;
@@ -1629,7 +1766,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
for (i = 0; i < count; i++) {
const char *prefix = "";
struct diffstat_file *file = data->files[i];
- char *name = file->print_name;
+ char *name;
uintmax_t added = file->added;
uintmax_t deleted = file->deleted;
int name_len;
@@ -1637,6 +1774,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
if (!file->is_interesting && (added + deleted == 0))
continue;
+ fill_print_name(file, name_width);
+ name = file->print_name;
/*
* "scale" the filename
*/
@@ -1772,7 +1911,7 @@ static void show_numstat(struct diffstat_t *data, struct diff_options *options)
"%"PRIuMAX"\t%"PRIuMAX"\t",
file->added, file->deleted);
if (options->line_termination) {
- fill_print_name(file);
+ fill_print_name(file, INT_MAX);
if (!file->is_renamed)
write_name_quoted(file->name, options->file,
options->line_termination);
@@ -4258,7 +4397,7 @@ static void show_mode_change(FILE *file, struct diff_filepair *p, int show_name,
static void show_rename_copy(FILE *file, const char *renamecopy, struct diff_filepair *p,
const char *line_prefix)
{
- char *names = pprint_rename(p->one->path, p->two->path);
+ char *names = pprint_rename(p->one->path, p->two->path, INT_MAX);
fprintf(file, " %s %s (%d%%)\n", renamecopy, names, similarity_index(p));
free(names);
diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
index 2f327b7..03d6371 100755
--- a/t/t4001-diff-rename.sh
+++ b/t/t4001-diff-rename.sh
@@ -156,4 +156,16 @@ test_expect_success 'rename pretty print common prefix and suffix overlap' '
test_i18ngrep " d/f/{ => f}/e " output
'
+test_expect_success 'rename of very long path shows =>' '
+ mkdir long_dirname_that_does_not_fit_in_a_single_line &&
+ mkdir another_extremely_long_path_but_not_the_same_as_the_first &&
+ cp path1 long_dirname*/ &&
+ git add long_dirname*/path1 &&
+ test_commit add_long_pathname &&
+ git mv long_dirname*/path1 another_extremely_*/ &&
+ test_commit move_long_pathname &&
+ git diff -M --stat HEAD^ HEAD >output &&
+ test_i18ngrep "=>.*path1" output
+'
+
test_done
--
1.8.4.475.g867697c
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v3] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
2013-10-13 20:29 ` Thomas Rast
@ 2013-10-15 9:46 ` Yoshioka Tsuneo
0 siblings, 0 replies; 31+ messages in thread
From: Yoshioka Tsuneo @ 2013-10-15 9:46 UTC (permalink / raw)
To: Thomas Rast, git@vger.kernel.org
Hello Thomas
Thank you very much for your kind review.
Now, I just posted "PATCH v4" that will include your suggestion like keeping "{", "}"
while omitting, improving commit message and comment, and test.
Thanks!
---
Tsuneo Yoshioka (吉岡 恒夫)
yoshiokatsuneo@gmail.com
On Oct 13, 2013, at 11:29 PM, Thomas Rast <tr@thomasrast.ch> wrote:
> Hi,
>
> Yoshioka Tsuneo <yoshiokatsuneo@gmail.com> writes:
>
>> "git diff -M --stat" can detect rename and show renamed file name like
>> "foofoofoo => barbarbar", but if destination filename is long the line
>> is shortened like "...barbarbar" so there is no way to know whether the
>> file is renamed or existed in the source commit.
>
> Thanks for your patch! I think this is indeed something that should be
> fixed.
>
> Can you explain the algorithm chosen in the commit message or a block
> comment in the code? I find it much easier to follow large code blocks
> (like the one you added) with a prior notion of what it tries to do.
>
> [As an aside, Documentation/SubmittingPatches says
>
> The body should provide a meaningful commit message, which:
>
> . explains the problem the change tries to solve, iow, what is wrong
> with the current code without the change.
>
> . justifies the way the change solves the problem, iow, why the
> result with the change is better.
>
> . alternate solutions considered but discarded, if any.
>
> Observe that you explained the first item very well, but not the
> others.]
>
>> This commit makes it visible like "...foo => ...bar".
>
> Also, you should rewrite this to be in the imperative mood:
>
> Make sure there is always an arrow, e.g., "...foo => ...bar".
>
> or some such.
>
> [Again from SubmittingPatches:
>
> Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
> instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
> to do frotz", as if you are giving orders to the codebase to change
> its behaviour.]
>
>> Signed-off-by: Tsuneo Yoshioka <yoshiokatsuneo@gmail.com>
>> ---
>> diff.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 51 insertions(+), 7 deletions(-)
>
> Can you add a test? Perhaps like the one below. (You can squash it
> into your commit if you like it.)
>
> 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
>
> diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
> index 2f327b7..03d6371 100755
> --- a/t/t4001-diff-rename.sh
> +++ b/t/t4001-diff-rename.sh
> @@ -156,4 +156,16 @@ test_expect_success 'rename pretty print common prefix and suffix overlap' '
> test_i18ngrep " d/f/{ => f}/e " output
> '
>
> +test_expect_success 'rename of very long path shows =>' '
> + mkdir long_dirname_that_does_not_fit_in_a_single_line &&
> + mkdir another_extremely_long_path_but_not_the_same_as_the_first &&
> + cp path1 long_dirname*/ &&
> + git add long_dirname*/path1 &&
> + test_commit add_long_pathname &&
> + git mv long_dirname*/path1 another_extremely_*/ &&
> + test_commit move_long_pathname &&
> + git diff -M --stat HEAD^ HEAD >output &&
> + test_i18ngrep "=>.*path1" output
> +'
> +
> test_done
>
> --
> Thomas Rast
> tr@thomasrast.ch
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v3] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
2013-10-14 19:04 ` Duy Nguyen
@ 2013-10-15 9:46 ` Yoshioka Tsuneo
0 siblings, 0 replies; 31+ messages in thread
From: Yoshioka Tsuneo @ 2013-10-15 9:46 UTC (permalink / raw)
To: Duy Nguyen, git@vger.kernel.org
Hello Duy
Thank you very much your suggestion.
As you suggested, I tried to reuse intermediate result of pprint_rename(), instead of
parsing the output again.
I just posted the new patch as "PATCH v4"
Thanks !
---
Tsuneo Yoshioka (吉岡 恒夫)
yoshiokatsuneo@gmail.com
On Oct 14, 2013, at 10:04 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sun, Oct 13, 2013 at 3:48 AM, Yoshioka Tsuneo
> <yoshiokatsuneo@gmail.com> wrote:
>> "git diff -M --stat" can detect rename and show renamed file name like
>> "foofoofoo => barbarbar", but if destination filename is long the line
>> is shortened like "...barbarbar" so there is no way to know whether the
>> file is renamed or existed in the source commit.
>> This commit makes it visible like "...foo => ...bar".
>>
>> Signed-off-by: Tsuneo Yoshioka <yoshiokatsuneo@gmail.com>
>> ---
>> diff.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 51 insertions(+), 7 deletions(-)
>>
>> diff --git a/diff.c b/diff.c
>> index a04a34d..3aeaf3e 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -1643,13 +1643,57 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>> len = name_width;
>> name_len = strlen(name);
>> if (name_width < name_len) {
>> - char *slash;
>> - prefix = "...";
>> - len -= 3;
>> - name += name_len - len;
>> - slash = strchr(name, '/');
>> - if (slash)
>> - name = slash;
>> + char *arrow = strstr(name, " => ");
>> + if (arrow) {
>
> This looks iffy. What if " => " is part of the path name?
> file->is_renamed would be a more reliable sign. In that case I think
> you just need an ellipsis version of pprint_rename() (i.e. drop the
> result of previous pprint_rename() on the floor and create a new
> string with "..." and " => " in your pprint_ellipsis_rename or
> something)
>
>> + int prefix_len = (name_width - 4) / 2;
>> + int f_omit;
>> + int f_brace = 0;
>> + char *pre_arrow = alloca(name_width + 10);
>> + char *post_arrow = arrow + 4;
>> + char *prefix_buf = alloca(name_width + 10);
>> + char *pre_arrow_slash = NULL;
>> +
>> + if (arrow - name < prefix_len) {
>> + prefix_len = (int)(arrow - name);
>> + f_omit = 0;
>> + } else {
>> + prefix_len -= 3;
>> + f_omit = 1;
>> + if (name[0] == '{') {
>> + prefix_len -= 1;
>> + f_brace = 1;
>> + }
>> + }
>> + prefix_len = ((prefix_len >= 0) ? prefix_len : 0);
>> + strncpy(pre_arrow, arrow - prefix_len, prefix_len);
>> + pre_arrow[prefix_len] = '\0';
>> + pre_arrow_slash = strchr(pre_arrow, '/');
>> + if (f_omit && pre_arrow_slash)
>> + pre_arrow = pre_arrow_slash;
>> + sprintf(prefix_buf, "%s%s%s => ", (f_brace ? "{" : ""), (f_omit ? "..." : ""), pre_arrow);
>> + prefix = prefix_buf;
>> +
>> + if (strlen(post_arrow) > name_width - strlen(prefix)) {
>> + char *post_arrow_slash = NULL;
>> +
>> + post_arrow += strlen(post_arrow) - (name_width - strlen(prefix) - 3);
>> + strcat(prefix_buf, "...");
>> + post_arrow_slash = strchr(post_arrow, '/');
>> + if (post_arrow_slash)
>> + post_arrow = post_arrow_slash;
>> + name = post_arrow;
>> + name_len = (int) (name_width - strlen(prefix));
>> + }
>> + len -= strlen(prefix);
>> + } else {
>> + char *slash = NULL;
>> + prefix = "...";
>> + len -= 3;
>> + name += name_len - len;
>> + slash = strchr(name, '/');
>> + if (slash)
>> + name = slash;
>> + }
>> }
>>
>> if (file->is_binary) {
>> --
>> 1.8.4.475.g867697c
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Duy
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v4] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
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
1 sibling, 1 reply; 31+ messages in thread
From: Felipe Contreras @ 2013-10-15 10:07 UTC (permalink / raw)
To: Yoshioka Tsuneo; +Cc: git@vger.kernel.org
On Tue, Oct 15, 2013 at 4:45 AM, Yoshioka Tsuneo
<yoshiokatsuneo@gmail.com> wrote:
>
> "git diff -M --stat" can detect rename and show renamed file name like
> "foofoofoo => barbarbar". But if destination filename is long, the line
> is shortened like "...barbarbar" so there is no way to know whether the
> file is renamed or existed in the source commit.
> Make sure there is always an arrow, like "...foo => ...bar".
> The output can contains curly braces('{','}') for grouping.
> So, in general, the outpu 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>, and <sfx> trying to
> have the maximum length the same because those will be equaly important.
This has similar style issues as v1.
> -static char *pprint_rename(const char *a, const char *b)
> +static void pprint_rename_find_common_prefix_suffix(const char *a, const char *b, struct strbuf *pfx, struct strbuf *a_mid, struct strbuf *b_mid, struct strbuf *sfx)
> {
> const char *old = a;
> const char *new = b;
> - struct strbuf name = STRBUF_INIT;
> int pfx_length, sfx_length;
> int pfx_adjust_for_slash;
> int len_a = strlen(a);
> @@ -1272,10 +1271,9 @@ static char *pprint_rename(const char *a, const char *b)
> int qlen_b = quote_c_style(b, NULL, NULL, 0);
>
> if (qlen_a || qlen_b) {
> - quote_c_style(a, &name, NULL, 0);
> - strbuf_addstr(&name, " => ");
> - quote_c_style(b, &name, NULL, 0);
> - return strbuf_detach(&name, NULL);
> + quote_c_style(a, a_mid, NULL, 0);
> + quote_c_style(b, b_mid, NULL, 0);
> + return;
> }
>
> /* Find common prefix */
> @@ -1321,18 +1319,149 @@ static char *pprint_rename(const char *a, const char *b)
> a_midlen = 0;
> if (b_midlen < 0)
> b_midlen = 0;
> +
> + strbuf_add(pfx, a, pfx_length);
> + strbuf_add(a_mid, a + pfx_length, a_midlen);
> + strbuf_add(b_mid, b + pfx_length, b_midlen);
> + strbuf_add(sfx, a + len_a - sfx_length, sfx_length);
> +}
>
> - strbuf_grow(&name, pfx_length + a_midlen + b_midlen + sfx_length + 7);
> - if (pfx_length + sfx_length) {
> - strbuf_add(&name, a, pfx_length);
> +/*
> + * Omit each parts to fix in name_width.
> + * Formatted string is "<pfx>{<a_mid> => <b_mid>}<sfx>".
> + * At first, omit <pfx> as long as possible.
> + * If it is not enough, omit <a_mid>, <b_mid>, <sfx> by tring to set the length of
> + * those 3 parts(including "...") to the same.
> + * Ex:
> + * "foofoofoo => barbarbar"
> + * will be like
> + * "...foo => ...bar".
> + * "long_parent{foofoofoo => barbarbar}longfilename"
> + * will be like
> + * "...parent{...foofoo => ...barbar}...lename"
> + */
> +static void pprint_rename_omit(struct strbuf *pfx, struct strbuf *a_mid, struct strbuf *b_mid, struct strbuf *sfx, int name_width)
Seems like this line needs to be broken.
> +{
> +
> +#define ARROW " => "
> +#define ELLIPSIS "..."
> +#define swap(a,b) myswap((a),(b),sizeof(a))
I'm not entirely sure, but I think this should be:
#define swap(a, b) myswap((a), (b), sizeof(a))
> +
> +#define myswap(a, b, size) do { \
> +unsigned char mytmp[size]; \
> +memcpy(mytmp, &a, size); \
> +memcpy(&a, &b, size); \
> +memcpy(&b, mytmp, size); \
> +} while (0)
> +
> + int use_curly_braces = (pfx->len > 0) || (sfx->len > 0);
> + size_t name_len;
> + size_t len;
> + size_t part_lengths[4];
> + size_t max_part_len = 0;
> + size_t remainder_part_len = 0;
> + int i, j;
> +
> + name_len = pfx->len + a_mid->len + b_mid->len + sfx->len + strlen(ARROW) + (use_curly_braces?2:0);
> +
> + if (name_len <= name_width){
if () {
> + /* Everthing fits in name_width */
> + return;
> + }
> +
> + if(use_curly_braces){
Ditto.
> + if(strlen(ELLIPSIS) + (name_len - pfx->len) <= name_width){
Ditto.
> + /*
> + Just omitting left of '{' is enough
> + Ex: ...aaa{foofoofoo => bar}file
> + */
> + strbuf_splice(pfx, name_len - pfx->len, name_width - (name_len - pfx->len), ELLIPSIS, strlen(ELLIPSIS));
> + return;
> + }else{
} else {
> + if (pfx->len > strlen(ELLIPSIS)) {
> + /*
> + Just omitting left of '{' is not enough
> + name will be "...{SOMETHING}SOMETHING"
> + */
> + strbuf_reset(pfx);
> + strbuf_addstr(pfx, ELLIPSIS);
> + }
> + }
> + }
> +
> + /* available length for a_mid, b_mid and sfx */
> + len = name_width - strlen(ARROW) - (use_curly_braces?2:0);
use_curly_braces ? 2 : 0
> +
> + /* a_mid, b_mid, sfx will be have the same max, including ellipsis("..."). */
> + part_lengths[0] = (int)a_mid->len;
> + part_lengths[1] = (int)b_mid->len;
> + part_lengths[2] = (int)sfx->len;
> +
> + /* bubble sort of part_lengths, descending order */
> + for(i=0;i<3;i++){
for (i = 0; i < 3; i++) {
> + for(j= i+1; j<3; j++){
Ditto.
> + if(part_lengths[j] > part_lengths[i]){
if ()
foo;
(it's a single line, no need for braces, In fact all the fors could
get rid of them, but not really required.)
> + swap(part_lengths[i], part_lengths[j]);
> + }
> + }
> + }
> +
> + if (part_lengths[1] + part_lengths[1] + part_lengths[2] <= len) {
> + /*
> + * "{...foofoo => barbar}file"
> + * There is only one omitting part.
> + */
> + max_part_len = len - part_lengths[1] - part_lengths[2];
> + } else if (part_lengths[2] + part_lengths[2] + part_lengths[2] <= len){
} else if () {
> + /*
> + * "{...foofoo => ...barbar}file"
> + * There are 2 omitting part.
> + */
> + max_part_len = (len - part_lengths[2])/2;
(len - part_lengths[2]) / 2
> + remainder_part_len = (len - part_lengths[2]) - max_part_len * 2;
> + } else {
> + /*
> + * "{...ofoo => ...rbar}...file"
> + * There are 3 omitting part.
> + */
> + max_part_len = len / 3;
> + remainder_part_len = len - (max_part_len) * 3;
> + }
> +
> + if (max_part_len < strlen(ELLIPSIS))
> + max_part_len = strlen(ELLIPSIS);
> +
> + if (sfx->len > max_part_len)
> + strbuf_splice(sfx, 0, sfx->len - max_part_len + strlen(ELLIPSIS), ELLIPSIS, strlen(ELLIPSIS));
> + if (remainder_part_len==2)
remainder_part_len == 2
> + max_part_len++;
> + if (a_mid->len > max_part_len)
> + strbuf_splice(a_mid, 0, a_mid->len - max_part_len + strlen(ELLIPSIS), ELLIPSIS, strlen(ELLIPSIS));
> + if (remainder_part_len==1)
Ditto.
> + max_part_len++;
> + if (b_mid->len > max_part_len)
> + strbuf_splice(b_mid, 0, b_mid->len - max_part_len + strlen(ELLIPSIS), ELLIPSIS, strlen(ELLIPSIS));
> +}
> +
> +static char *pprint_rename(const char *a, const char *b, int name_width)
> +{
> + struct strbuf pfx = STRBUF_INIT, a_mid = STRBUF_INIT, b_mid = STRBUF_INIT, sfx = STRBUF_INIT;
> + struct strbuf name = STRBUF_INIT;
> +
> + pprint_rename_find_common_prefix_suffix(a, b, &pfx, &a_mid, &b_mid, &sfx);
> + pprint_rename_omit(&pfx, &a_mid, &b_mid, &sfx, name_width);
> +
> + strbuf_grow(&name, pfx.len + a_mid.len + b_mid.len + sfx.len + 7);
> + if (pfx.len + sfx.len) {
> + strbuf_addbuf(&name, &pfx);
> strbuf_addch(&name, '{');
> }
> - strbuf_add(&name, a + pfx_length, a_midlen);
> + strbuf_addbuf(&name, &a_mid);
> strbuf_addstr(&name, " => ");
> - strbuf_add(&name, b + pfx_length, b_midlen);
> - if (pfx_length + sfx_length) {
> + strbuf_addbuf(&name, &b_mid);
> + if (pfx.len + sfx.len) {
> strbuf_addch(&name, '}');
> - strbuf_add(&name, a + len_a - sfx_length, sfx_length);
> + strbuf_addbuf(&name, &sfx);
> }
> return strbuf_detach(&name, NULL);
> }
> @@ -1418,23 +1547,31 @@ static void show_graph(FILE *file, char ch, int cnt, const char *set, const char
> fprintf(file, "%s", reset);
> }
>
> -static void fill_print_name(struct diffstat_file *file)
> +static void fill_print_name(struct diffstat_file *file, int name_width)
> {
> char *pname;
>
> - if (file->print_name)
> - return;
> -
> if (!file->is_renamed) {
> struct strbuf buf = STRBUF_INIT;
> + if (file->print_name)
> + return;
> if (quote_c_style(file->name, &buf, NULL, 0)) {
> pname = strbuf_detach(&buf, NULL);
> } else {
> pname = file->name;
> strbuf_release(&buf);
> }
> + if(strlen(pname) > name_width){
if () {
> + struct strbuf buf2 = STRBUF_INIT;
> + strbuf_addstr(&buf2, "...");
> + strbuf_addstr(&buf2, pname + strlen(pname) - name_width - 3);
> + }
> } else {
> - pname = pprint_rename(file->from_name, file->name);
> + if (file->print_name){
Ditto
> + free(file->print_name);
> + file->print_name = NULL;
> + }
> + pname = pprint_rename(file->from_name, file->name, name_width);
> }
> file->print_name = pname;
> }
> @@ -1517,7 +1654,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
> count++; /* not shown == room for one more */
> continue;
> }
> - fill_print_name(file);
> + fill_print_name(file, INT_MAX);
> len = strlen(file->print_name);
> if (max_len < len)
> max_len = len;
> @@ -1629,7 +1766,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
> for (i = 0; i < count; i++) {
> const char *prefix = "";
> struct diffstat_file *file = data->files[i];
> - char *name = file->print_name;
> + char *name;
> uintmax_t added = file->added;
> uintmax_t deleted = file->deleted;
> int name_len;
> @@ -1637,6 +1774,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
> if (!file->is_interesting && (added + deleted == 0))
> continue;
>
> + fill_print_name(file, name_width);
> + name = file->print_name;
> /*
> * "scale" the filename
> */
> @@ -1772,7 +1911,7 @@ static void show_numstat(struct diffstat_t *data, struct diff_options *options)
> "%"PRIuMAX"\t%"PRIuMAX"\t",
> file->added, file->deleted);
> if (options->line_termination) {
> - fill_print_name(file);
> + fill_print_name(file, INT_MAX);
> if (!file->is_renamed)
> write_name_quoted(file->name, options->file,
> options->line_termination);
> @@ -4258,7 +4397,7 @@ static void show_mode_change(FILE *file, struct diff_filepair *p, int show_name,
> static void show_rename_copy(FILE *file, const char *renamecopy, struct diff_filepair *p,
> const char *line_prefix)
> {
> - char *names = pprint_rename(p->one->path, p->two->path);
> + char *names = pprint_rename(p->one->path, p->two->path, INT_MAX);
>
> fprintf(file, " %s %s (%d%%)\n", renamecopy, names, similarity_index(p));
> free(names);
> diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
> index 2f327b7..03d6371 100755
> --- a/t/t4001-diff-rename.sh
> +++ b/t/t4001-diff-rename.sh
> @@ -156,4 +156,16 @@ test_expect_success 'rename pretty print common prefix and suffix overlap' '
> test_i18ngrep " d/f/{ => f}/e " output
> '
>
> +test_expect_success 'rename of very long path shows =>' '
> + mkdir long_dirname_that_does_not_fit_in_a_single_line &&
> + mkdir another_extremely_long_path_but_not_the_same_as_the_first &&
> + cp path1 long_dirname*/ &&
> + git add long_dirname*/path1 &&
> + test_commit add_long_pathname &&
> + git mv long_dirname*/path1 another_extremely_*/ &&
> + test_commit move_long_pathname &&
> + git diff -M --stat HEAD^ HEAD >output &&
> + test_i18ngrep "=>.*path1" output
> +'
> +
> test_done
--
Felipe Contreras
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v5] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
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 22:54 ` Junio C Hamano
2013-10-16 9:53 ` [PATCH v6] " Yoshioka Tsuneo
1 sibling, 2 replies; 31+ messages in thread
From: Yoshioka Tsuneo @ 2013-10-15 10:24 UTC (permalink / raw)
To: git@vger.kernel.org
"git diff -M --stat" can detect rename and show renamed file name like
"foofoofoo => barbarbar". But if destination filename is long, the line
is shortened like "...barbarbar" so there is no way to know whether the
file is renamed or existed in the source commit.
Make sure there is always an arrow, like "...foo => ...bar".
The output can contains curly braces('{','}') for grouping.
So, in general, the outpu 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>, and <sfx> trying to
have the maximum length the same because those will be equaly important.
Signed-off-by: Tsuneo Yoshioka <yoshiokatsuneo@gmail.com>
Test-added-by: Thomas Rast <trast@inf.ethz.ch>
---
diff.c | 187 +++++++++++++++++++++++++++++++++++++++++++------
t/t4001-diff-rename.sh | 12 ++++
2 files changed, 177 insertions(+), 22 deletions(-)
diff --git a/diff.c b/diff.c
index a04a34d..cf50807 100644
--- a/diff.c
+++ b/diff.c
@@ -1258,11 +1258,12 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
}
}
-static char *pprint_rename(const char *a, const char *b)
+static void pprint_rename_find_common_prefix_suffix(const char *a, const char *b
+ , struct strbuf *pfx, struct strbuf *a_mid
+ , struct strbuf *b_mid, struct strbuf *sfx)
{
const char *old = a;
const char *new = b;
- struct strbuf name = STRBUF_INIT;
int pfx_length, sfx_length;
int pfx_adjust_for_slash;
int len_a = strlen(a);
@@ -1272,10 +1273,9 @@ static char *pprint_rename(const char *a, const char *b)
int qlen_b = quote_c_style(b, NULL, NULL, 0);
if (qlen_a || qlen_b) {
- quote_c_style(a, &name, NULL, 0);
- strbuf_addstr(&name, " => ");
- quote_c_style(b, &name, NULL, 0);
- return strbuf_detach(&name, NULL);
+ quote_c_style(a, a_mid, NULL, 0);
+ quote_c_style(b, b_mid, NULL, 0);
+ return;
}
/* Find common prefix */
@@ -1321,18 +1321,151 @@ static char *pprint_rename(const char *a, const char *b)
a_midlen = 0;
if (b_midlen < 0)
b_midlen = 0;
+
+ strbuf_add(pfx, a, pfx_length);
+ strbuf_add(a_mid, a + pfx_length, a_midlen);
+ strbuf_add(b_mid, b + pfx_length, b_midlen);
+ strbuf_add(sfx, a + len_a - sfx_length, sfx_length);
+}
+
+/*
+ * Omit each parts to fix in name_width.
+ * Formatted string is "<pfx>{<a_mid> => <b_mid>}<sfx>".
+ * At first, omit <pfx> as long as possible.
+ * If it is not enough, omit <a_mid>, <b_mid>, <sfx> by tring to set the length of
+ * those 3 parts(including "...") to the same.
+ * Ex:
+ * "foofoofoo => barbarbar"
+ * will be like
+ * "...foo => ...bar".
+ * "long_parent{foofoofoo => barbarbar}longfilename"
+ * will be like
+ * "...parent{...foofoo => ...barbar}...lename"
+ */
+static void pprint_rename_omit(struct strbuf *pfx, struct strbuf *a_mid, struct strbuf *b_mid
+ , struct strbuf *sfx, int name_width)
+{
+
+#define ARROW " => "
+#define ELLIPSIS "..."
+#define swap(a, b) myswap((a), (b), sizeof(a))
+
+#define myswap(a, b, size) do { \
+unsigned char mytmp[size]; \
+memcpy(mytmp, &a, size); \
+memcpy(&a, &b, size); \
+memcpy(&b, mytmp, size); \
+} while (0)
+
+ int use_curly_braces = (pfx->len > 0) || (sfx->len > 0);
+ size_t name_len;
+ size_t len;
+ size_t part_lengths[4];
+ size_t max_part_len = 0;
+ size_t remainder_part_len = 0;
+ int i, j;
+
+ name_len = pfx->len + a_mid->len + b_mid->len + sfx->len + strlen(ARROW)
+ + (use_curly_braces ? 2 : 0);
+
+ if (name_len <= name_width) {
+ /* Everthing fits in name_width */
+ return;
+ }
+
+ if (use_curly_braces) {
+ if (strlen(ELLIPSIS) + (name_len - pfx->len) <= name_width) {
+ /*
+ Just omitting left of '{' is enough
+ Ex: ...aaa{foofoofoo => bar}file
+ */
+ strbuf_splice(pfx, name_len - pfx->len, name_width - (name_len - pfx->len), ELLIPSIS, strlen(ELLIPSIS));
+ return;
+ } else {
+ if (pfx->len > strlen(ELLIPSIS)) {
+ /*
+ Just omitting left of '{' is not enough
+ name will be "...{SOMETHING}SOMETHING"
+ */
+ strbuf_reset(pfx);
+ strbuf_addstr(pfx, ELLIPSIS);
+ }
+ }
+ }
- strbuf_grow(&name, pfx_length + a_midlen + b_midlen + sfx_length + 7);
- if (pfx_length + sfx_length) {
- strbuf_add(&name, a, pfx_length);
+ /* available length for a_mid, b_mid and sfx */
+ len = name_width - strlen(ARROW) - (use_curly_braces ? 2 : 0);
+
+ /* a_mid, b_mid, sfx will be have the same max, including ellipsis("..."). */
+ part_lengths[0] = (int)a_mid->len;
+ part_lengths[1] = (int)b_mid->len;
+ part_lengths[2] = (int)sfx->len;
+
+ /* bubble sort of part_lengths, descending order */
+ for (i=0; i<3; i++) {
+ for (j=i+1; j<3; j++) {
+ if (part_lengths[j] > part_lengths[i]) {
+ swap(part_lengths[i], part_lengths[j]);
+ }
+ }
+ }
+
+ if (part_lengths[1] + part_lengths[1] + part_lengths[2] <= len) {
+ /*
+ * "{...foofoo => barbar}file"
+ * There is only one omitting part.
+ */
+ max_part_len = len - part_lengths[1] - part_lengths[2];
+ } else if (part_lengths[2] + part_lengths[2] + part_lengths[2] <= len) {
+ /*
+ * "{...foofoo => ...barbar}file"
+ * There are 2 omitting part.
+ */
+ max_part_len = (len - part_lengths[2]) / 2;
+ remainder_part_len = (len - part_lengths[2]) - max_part_len * 2;
+ } else {
+ /*
+ * "{...ofoo => ...rbar}...file"
+ * There are 3 omitting part.
+ */
+ max_part_len = len / 3;
+ remainder_part_len = len - (max_part_len) * 3;
+ }
+
+ if (max_part_len < strlen(ELLIPSIS))
+ max_part_len = strlen(ELLIPSIS);
+
+ if (sfx->len > max_part_len)
+ strbuf_splice(sfx, 0, sfx->len - max_part_len + strlen(ELLIPSIS), ELLIPSIS, strlen(ELLIPSIS));
+ if (remainder_part_len == 2)
+ max_part_len++;
+ if (a_mid->len > max_part_len)
+ strbuf_splice(a_mid, 0, a_mid->len - max_part_len + strlen(ELLIPSIS), ELLIPSIS, strlen(ELLIPSIS));
+ if (remainder_part_len == 1)
+ max_part_len++;
+ if (b_mid->len > max_part_len)
+ strbuf_splice(b_mid, 0, b_mid->len - max_part_len + strlen(ELLIPSIS), ELLIPSIS, strlen(ELLIPSIS));
+}
+
+static char *pprint_rename(const char *a, const char *b, int name_width)
+{
+ struct strbuf pfx = STRBUF_INIT, a_mid = STRBUF_INIT, b_mid = STRBUF_INIT, sfx = STRBUF_INIT;
+ struct strbuf name = STRBUF_INIT;
+
+ pprint_rename_find_common_prefix_suffix(a, b, &pfx, &a_mid, &b_mid, &sfx);
+ pprint_rename_omit(&pfx, &a_mid, &b_mid, &sfx, name_width);
+
+ strbuf_grow(&name, pfx.len + a_mid.len + b_mid.len + sfx.len + 7);
+ if (pfx.len + sfx.len) {
+ strbuf_addbuf(&name, &pfx);
strbuf_addch(&name, '{');
}
- strbuf_add(&name, a + pfx_length, a_midlen);
+ strbuf_addbuf(&name, &a_mid);
strbuf_addstr(&name, " => ");
- strbuf_add(&name, b + pfx_length, b_midlen);
- if (pfx_length + sfx_length) {
+ strbuf_addbuf(&name, &b_mid);
+ if (pfx.len + sfx.len) {
strbuf_addch(&name, '}');
- strbuf_add(&name, a + len_a - sfx_length, sfx_length);
+ strbuf_addbuf(&name, &sfx);
}
return strbuf_detach(&name, NULL);
}
@@ -1418,23 +1551,31 @@ static void show_graph(FILE *file, char ch, int cnt, const char *set, const char
fprintf(file, "%s", reset);
}
-static void fill_print_name(struct diffstat_file *file)
+static void fill_print_name(struct diffstat_file *file, int name_width)
{
char *pname;
- if (file->print_name)
- return;
-
if (!file->is_renamed) {
struct strbuf buf = STRBUF_INIT;
+ if (file->print_name)
+ return;
if (quote_c_style(file->name, &buf, NULL, 0)) {
pname = strbuf_detach(&buf, NULL);
} else {
pname = file->name;
strbuf_release(&buf);
}
+ if (strlen(pname) > name_width) {
+ struct strbuf buf2 = STRBUF_INIT;
+ strbuf_addstr(&buf2, "...");
+ strbuf_addstr(&buf2, pname + strlen(pname) - name_width - 3);
+ }
} else {
- pname = pprint_rename(file->from_name, file->name);
+ if (file->print_name) {
+ free(file->print_name);
+ file->print_name = NULL;
+ }
+ pname = pprint_rename(file->from_name, file->name, name_width);
}
file->print_name = pname;
}
@@ -1517,7 +1658,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
count++; /* not shown == room for one more */
continue;
}
- fill_print_name(file);
+ fill_print_name(file, INT_MAX);
len = strlen(file->print_name);
if (max_len < len)
max_len = len;
@@ -1629,7 +1770,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
for (i = 0; i < count; i++) {
const char *prefix = "";
struct diffstat_file *file = data->files[i];
- char *name = file->print_name;
+ char *name;
uintmax_t added = file->added;
uintmax_t deleted = file->deleted;
int name_len;
@@ -1637,6 +1778,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
if (!file->is_interesting && (added + deleted == 0))
continue;
+ fill_print_name(file, name_width);
+ name = file->print_name;
/*
* "scale" the filename
*/
@@ -1772,7 +1915,7 @@ static void show_numstat(struct diffstat_t *data, struct diff_options *options)
"%"PRIuMAX"\t%"PRIuMAX"\t",
file->added, file->deleted);
if (options->line_termination) {
- fill_print_name(file);
+ fill_print_name(file, INT_MAX);
if (!file->is_renamed)
write_name_quoted(file->name, options->file,
options->line_termination);
@@ -4258,7 +4401,7 @@ static void show_mode_change(FILE *file, struct diff_filepair *p, int show_name,
static void show_rename_copy(FILE *file, const char *renamecopy, struct diff_filepair *p,
const char *line_prefix)
{
- char *names = pprint_rename(p->one->path, p->two->path);
+ char *names = pprint_rename(p->one->path, p->two->path, INT_MAX);
fprintf(file, " %s %s (%d%%)\n", renamecopy, names, similarity_index(p));
free(names);
diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
index 2f327b7..03d6371 100755
--- a/t/t4001-diff-rename.sh
+++ b/t/t4001-diff-rename.sh
@@ -156,4 +156,16 @@ test_expect_success 'rename pretty print common prefix and suffix overlap' '
test_i18ngrep " d/f/{ => f}/e " output
'
+test_expect_success 'rename of very long path shows =>' '
+ mkdir long_dirname_that_does_not_fit_in_a_single_line &&
+ mkdir another_extremely_long_path_but_not_the_same_as_the_first &&
+ cp path1 long_dirname*/ &&
+ git add long_dirname*/path1 &&
+ test_commit add_long_pathname &&
+ git mv long_dirname*/path1 another_extremely_*/ &&
+ test_commit move_long_pathname &&
+ git diff -M --stat HEAD^ HEAD >output &&
+ test_i18ngrep "=>.*path1" output
+'
+
test_done
--
1.8.4.475.g867697c
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v4] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
2013-10-15 10:07 ` Felipe Contreras
@ 2013-10-15 10:24 ` Yoshioka Tsuneo
0 siblings, 0 replies; 31+ messages in thread
From: Yoshioka Tsuneo @ 2013-10-15 10:24 UTC (permalink / raw)
To: Felipe Contreras, git@vger.kernel.org
Hello Felipe
Thank you for pointing out the style issue again.
I just fixed it and posted as [PATCH v5].
Thanks!
---
Tsuneo Yoshioka (吉岡 恒夫)
yoshiokatsuneo@gmail.com
On Oct 15, 2013, at 1:07 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> On Tue, Oct 15, 2013 at 4:45 AM, Yoshioka Tsuneo
> <yoshiokatsuneo@gmail.com> wrote:
>>
>> "git diff -M --stat" can detect rename and show renamed file name like
>> "foofoofoo => barbarbar". But if destination filename is long, the line
>> is shortened like "...barbarbar" so there is no way to know whether the
>> file is renamed or existed in the source commit.
>> Make sure there is always an arrow, like "...foo => ...bar".
>> The output can contains curly braces('{','}') for grouping.
>> So, in general, the outpu 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>, and <sfx> trying to
>> have the maximum length the same because those will be equaly important.
>
> This has similar style issues as v1.
>
>> -static char *pprint_rename(const char *a, const char *b)
>> +static void pprint_rename_find_common_prefix_suffix(const char *a, const char *b, struct strbuf *pfx, struct strbuf *a_mid, struct strbuf *b_mid, struct strbuf *sfx)
>> {
>> const char *old = a;
>> const char *new = b;
>> - struct strbuf name = STRBUF_INIT;
>> int pfx_length, sfx_length;
>> int pfx_adjust_for_slash;
>> int len_a = strlen(a);
>> @@ -1272,10 +1271,9 @@ static char *pprint_rename(const char *a, const char *b)
>> int qlen_b = quote_c_style(b, NULL, NULL, 0);
>>
>> if (qlen_a || qlen_b) {
>> - quote_c_style(a, &name, NULL, 0);
>> - strbuf_addstr(&name, " => ");
>> - quote_c_style(b, &name, NULL, 0);
>> - return strbuf_detach(&name, NULL);
>> + quote_c_style(a, a_mid, NULL, 0);
>> + quote_c_style(b, b_mid, NULL, 0);
>> + return;
>> }
>>
>> /* Find common prefix */
>> @@ -1321,18 +1319,149 @@ static char *pprint_rename(const char *a, const char *b)
>> a_midlen = 0;
>> if (b_midlen < 0)
>> b_midlen = 0;
>> +
>> + strbuf_add(pfx, a, pfx_length);
>> + strbuf_add(a_mid, a + pfx_length, a_midlen);
>> + strbuf_add(b_mid, b + pfx_length, b_midlen);
>> + strbuf_add(sfx, a + len_a - sfx_length, sfx_length);
>> +}
>>
>> - strbuf_grow(&name, pfx_length + a_midlen + b_midlen + sfx_length + 7);
>> - if (pfx_length + sfx_length) {
>> - strbuf_add(&name, a, pfx_length);
>> +/*
>> + * Omit each parts to fix in name_width.
>> + * Formatted string is "<pfx>{<a_mid> => <b_mid>}<sfx>".
>> + * At first, omit <pfx> as long as possible.
>> + * If it is not enough, omit <a_mid>, <b_mid>, <sfx> by tring to set the length of
>> + * those 3 parts(including "...") to the same.
>> + * Ex:
>> + * "foofoofoo => barbarbar"
>> + * will be like
>> + * "...foo => ...bar".
>> + * "long_parent{foofoofoo => barbarbar}longfilename"
>> + * will be like
>> + * "...parent{...foofoo => ...barbar}...lename"
>> + */
>> +static void pprint_rename_omit(struct strbuf *pfx, struct strbuf *a_mid, struct strbuf *b_mid, struct strbuf *sfx, int name_width)
>
> Seems like this line needs to be broken.
>
>> +{
>> +
>> +#define ARROW " => "
>> +#define ELLIPSIS "..."
>> +#define swap(a,b) myswap((a),(b),sizeof(a))
>
> I'm not entirely sure, but I think this should be:
>
> #define swap(a, b) myswap((a), (b), sizeof(a))
>
>> +
>> +#define myswap(a, b, size) do { \
>> +unsigned char mytmp[size]; \
>> +memcpy(mytmp, &a, size); \
>> +memcpy(&a, &b, size); \
>> +memcpy(&b, mytmp, size); \
>> +} while (0)
>> +
>> + int use_curly_braces = (pfx->len > 0) || (sfx->len > 0);
>> + size_t name_len;
>> + size_t len;
>> + size_t part_lengths[4];
>> + size_t max_part_len = 0;
>> + size_t remainder_part_len = 0;
>> + int i, j;
>> +
>> + name_len = pfx->len + a_mid->len + b_mid->len + sfx->len + strlen(ARROW) + (use_curly_braces?2:0);
>> +
>> + if (name_len <= name_width){
>
> if () {
>
>> + /* Everthing fits in name_width */
>> + return;
>> + }
>> +
>> + if(use_curly_braces){
>
> Ditto.
>
>> + if(strlen(ELLIPSIS) + (name_len - pfx->len) <= name_width){
>
> Ditto.
>
>> + /*
>> + Just omitting left of '{' is enough
>> + Ex: ...aaa{foofoofoo => bar}file
>> + */
>> + strbuf_splice(pfx, name_len - pfx->len, name_width - (name_len - pfx->len), ELLIPSIS, strlen(ELLIPSIS));
>> + return;
>> + }else{
>
> } else {
>
>> + if (pfx->len > strlen(ELLIPSIS)) {
>> + /*
>> + Just omitting left of '{' is not enough
>> + name will be "...{SOMETHING}SOMETHING"
>> + */
>> + strbuf_reset(pfx);
>> + strbuf_addstr(pfx, ELLIPSIS);
>> + }
>> + }
>> + }
>> +
>> + /* available length for a_mid, b_mid and sfx */
>> + len = name_width - strlen(ARROW) - (use_curly_braces?2:0);
>
> use_curly_braces ? 2 : 0
>
>> +
>> + /* a_mid, b_mid, sfx will be have the same max, including ellipsis("..."). */
>> + part_lengths[0] = (int)a_mid->len;
>> + part_lengths[1] = (int)b_mid->len;
>> + part_lengths[2] = (int)sfx->len;
>> +
>> + /* bubble sort of part_lengths, descending order */
>> + for(i=0;i<3;i++){
>
> for (i = 0; i < 3; i++) {
>
>> + for(j= i+1; j<3; j++){
>
> Ditto.
>
>> + if(part_lengths[j] > part_lengths[i]){
>
> if ()
> foo;
>
> (it's a single line, no need for braces, In fact all the fors could
> get rid of them, but not really required.)
>
>> + swap(part_lengths[i], part_lengths[j]);
>> + }
>> + }
>> + }
>> +
>> + if (part_lengths[1] + part_lengths[1] + part_lengths[2] <= len) {
>> + /*
>> + * "{...foofoo => barbar}file"
>> + * There is only one omitting part.
>> + */
>> + max_part_len = len - part_lengths[1] - part_lengths[2];
>> + } else if (part_lengths[2] + part_lengths[2] + part_lengths[2] <= len){
>
> } else if () {
>
>> + /*
>> + * "{...foofoo => ...barbar}file"
>> + * There are 2 omitting part.
>> + */
>> + max_part_len = (len - part_lengths[2])/2;
>
> (len - part_lengths[2]) / 2
>
>> + remainder_part_len = (len - part_lengths[2]) - max_part_len * 2;
>> + } else {
>> + /*
>> + * "{...ofoo => ...rbar}...file"
>> + * There are 3 omitting part.
>> + */
>> + max_part_len = len / 3;
>> + remainder_part_len = len - (max_part_len) * 3;
>> + }
>> +
>> + if (max_part_len < strlen(ELLIPSIS))
>> + max_part_len = strlen(ELLIPSIS);
>> +
>> + if (sfx->len > max_part_len)
>> + strbuf_splice(sfx, 0, sfx->len - max_part_len + strlen(ELLIPSIS), ELLIPSIS, strlen(ELLIPSIS));
>> + if (remainder_part_len==2)
>
> remainder_part_len == 2
>
>> + max_part_len++;
>> + if (a_mid->len > max_part_len)
>> + strbuf_splice(a_mid, 0, a_mid->len - max_part_len + strlen(ELLIPSIS), ELLIPSIS, strlen(ELLIPSIS));
>> + if (remainder_part_len==1)
>
> Ditto.
>
>> + max_part_len++;
>> + if (b_mid->len > max_part_len)
>> + strbuf_splice(b_mid, 0, b_mid->len - max_part_len + strlen(ELLIPSIS), ELLIPSIS, strlen(ELLIPSIS));
>> +}
>> +
>> +static char *pprint_rename(const char *a, const char *b, int name_width)
>> +{
>> + struct strbuf pfx = STRBUF_INIT, a_mid = STRBUF_INIT, b_mid = STRBUF_INIT, sfx = STRBUF_INIT;
>> + struct strbuf name = STRBUF_INIT;
>> +
>> + pprint_rename_find_common_prefix_suffix(a, b, &pfx, &a_mid, &b_mid, &sfx);
>> + pprint_rename_omit(&pfx, &a_mid, &b_mid, &sfx, name_width);
>> +
>> + strbuf_grow(&name, pfx.len + a_mid.len + b_mid.len + sfx.len + 7);
>
>> + if (pfx.len + sfx.len) {
>> + strbuf_addbuf(&name, &pfx);
>> strbuf_addch(&name, '{');
>> }
>> - strbuf_add(&name, a + pfx_length, a_midlen);
>> + strbuf_addbuf(&name, &a_mid);
>> strbuf_addstr(&name, " => ");
>> - strbuf_add(&name, b + pfx_length, b_midlen);
>> - if (pfx_length + sfx_length) {
>> + strbuf_addbuf(&name, &b_mid);
>> + if (pfx.len + sfx.len) {
>> strbuf_addch(&name, '}');
>> - strbuf_add(&name, a + len_a - sfx_length, sfx_length);
>> + strbuf_addbuf(&name, &sfx);
>> }
>> return strbuf_detach(&name, NULL);
>> }
>> @@ -1418,23 +1547,31 @@ static void show_graph(FILE *file, char ch, int cnt, const char *set, const char
>> fprintf(file, "%s", reset);
>> }
>>
>> -static void fill_print_name(struct diffstat_file *file)
>> +static void fill_print_name(struct diffstat_file *file, int name_width)
>> {
>> char *pname;
>>
>> - if (file->print_name)
>> - return;
>> -
>> if (!file->is_renamed) {
>> struct strbuf buf = STRBUF_INIT;
>> + if (file->print_name)
>> + return;
>> if (quote_c_style(file->name, &buf, NULL, 0)) {
>> pname = strbuf_detach(&buf, NULL);
>> } else {
>> pname = file->name;
>> strbuf_release(&buf);
>> }
>> + if(strlen(pname) > name_width){
>
> if () {
>
>> + struct strbuf buf2 = STRBUF_INIT;
>> + strbuf_addstr(&buf2, "...");
>> + strbuf_addstr(&buf2, pname + strlen(pname) - name_width - 3);
>> + }
>> } else {
>> - pname = pprint_rename(file->from_name, file->name);
>> + if (file->print_name){
>
> Ditto
>
>> + free(file->print_name);
>> + file->print_name = NULL;
>> + }
>> + pname = pprint_rename(file->from_name, file->name, name_width);
>> }
>> file->print_name = pname;
>> }
>> @@ -1517,7 +1654,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>> count++; /* not shown == room for one more */
>> continue;
>> }
>> - fill_print_name(file);
>> + fill_print_name(file, INT_MAX);
>> len = strlen(file->print_name);
>> if (max_len < len)
>> max_len = len;
>> @@ -1629,7 +1766,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>> for (i = 0; i < count; i++) {
>> const char *prefix = "";
>> struct diffstat_file *file = data->files[i];
>> - char *name = file->print_name;
>> + char *name;
>> uintmax_t added = file->added;
>> uintmax_t deleted = file->deleted;
>> int name_len;
>> @@ -1637,6 +1774,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>> if (!file->is_interesting && (added + deleted == 0))
>> continue;
>>
>> + fill_print_name(file, name_width);
>> + name = file->print_name;
>> /*
>> * "scale" the filename
>> */
>> @@ -1772,7 +1911,7 @@ static void show_numstat(struct diffstat_t *data, struct diff_options *options)
>> "%"PRIuMAX"\t%"PRIuMAX"\t",
>> file->added, file->deleted);
>> if (options->line_termination) {
>> - fill_print_name(file);
>> + fill_print_name(file, INT_MAX);
>> if (!file->is_renamed)
>> write_name_quoted(file->name, options->file,
>> options->line_termination);
>> @@ -4258,7 +4397,7 @@ static void show_mode_change(FILE *file, struct diff_filepair *p, int show_name,
>> static void show_rename_copy(FILE *file, const char *renamecopy, struct diff_filepair *p,
>> const char *line_prefix)
>> {
>> - char *names = pprint_rename(p->one->path, p->two->path);
>> + char *names = pprint_rename(p->one->path, p->two->path, INT_MAX);
>>
>> fprintf(file, " %s %s (%d%%)\n", renamecopy, names, similarity_index(p));
>> free(names);
>> diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
>> index 2f327b7..03d6371 100755
>> --- a/t/t4001-diff-rename.sh
>> +++ b/t/t4001-diff-rename.sh
>> @@ -156,4 +156,16 @@ test_expect_success 'rename pretty print common prefix and suffix overlap' '
>> test_i18ngrep " d/f/{ => f}/e " output
>> '
>>
>> +test_expect_success 'rename of very long path shows =>' '
>> + mkdir long_dirname_that_does_not_fit_in_a_single_line &&
>> + mkdir another_extremely_long_path_but_not_the_same_as_the_first &&
>> + cp path1 long_dirname*/ &&
>> + git add long_dirname*/path1 &&
>> + test_commit add_long_pathname &&
>> + git mv long_dirname*/path1 another_extremely_*/ &&
>> + test_commit move_long_pathname &&
>> + git diff -M --stat HEAD^ HEAD >output &&
>> + test_i18ngrep "=>.*path1" output
>> +'
>> +
>> test_done
>
> --
> Felipe Contreras
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
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
1 sibling, 2 replies; 31+ messages in thread
From: Junio C Hamano @ 2013-10-15 22:54 UTC (permalink / raw)
To: Yoshioka Tsuneo; +Cc: git@vger.kernel.org
Yoshioka Tsuneo <yoshiokatsuneo@gmail.com> writes:
> "git diff -M --stat" can detect rename and show renamed file name like
> "foofoofoo => barbarbar". But if destination filename is long, the line
> is shortened like "...barbarbar" so there is no way to know whether the
> file is renamed or existed in the source commit.
Is "destination" filename more special than the source filename?
Perhaps "s/if destination filename is/if filenames are/"?
Note: I do not want you to reroll using the suggested
wording without explanation; it may be possible that I am
missing something obvious and do not understand why you
singled out destination, in which case I'd rather see it
explained better in the log message than the potentially
suboptimal suggestion I made in the review without
understanding the issue. Of course, it is possible that you
want to do the same when source is overlong, in which case
you can just say "Yeah, you're right; will reroll".
The above applies to all the other comments in this message.
Also "s/source commit/original/". You may not be comparing two
commits after all.
> Make sure there is always an arrow, like "...foo => ...bar".
> The output can contains curly braces('{','}') for grouping.
s/contains/contain/;
> So, in general, the outpu format is "<pfx>{<mid_a> => <mid_b>}<sfx>"
s/outpu/&t/;
> 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>, and <sfx> trying to
> have the maximum length the same because those will be equaly important.
A sound reasoning.
> Signed-off-by: Tsuneo Yoshioka <yoshiokatsuneo@gmail.com>
> Test-added-by: Thomas Rast <trast@inf.ethz.ch>
> ---
> diff.c | 187 +++++++++++++++++++++++++++++++++++++++++++------
> t/t4001-diff-rename.sh | 12 ++++
> 2 files changed, 177 insertions(+), 22 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index a04a34d..cf50807 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1258,11 +1258,12 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
> }
> }
>
> -static char *pprint_rename(const char *a, const char *b)
> +static void pprint_rename_find_common_prefix_suffix(const char *a, const char *b
> + , struct strbuf *pfx, struct strbuf *a_mid
> + , struct strbuf *b_mid, struct strbuf *sfx)
What kind of line splitting is this?
I think the real issue is that the function name is overly long, but
aside from that,
- comma comes at the end of the line, not at the beginning of the
next line;
- the second and subsequent lines are indented, but not more than
the usual line width (align with the first letter inside the
opening parenthesis of the first line);
- a_mid and b_mid are more "alike" than pfx and a_mid.
so I would expect to see it more like:
static void abbrev_rename(const char *a, const char *b,
struct strbuf *pfx,
struct strbuf *a_mid, struct strbuf *b_mid,
struct strbuf *sfx)
Note that the suggested name does not say "pprint", because in your
version of this file, the code around here is no longer doing any
printing. The caller does so after using this function to decide
how to abbreviate renames, so naming the helper function after what
it does (e.g. abbreviate renames) is more appropriate.
> {
> const char *old = a;
> const char *new = b;
> - struct strbuf name = STRBUF_INIT;
> int pfx_length, sfx_length;
> int pfx_adjust_for_slash;
> int len_a = strlen(a);
> @@ -1272,10 +1273,9 @@ static char *pprint_rename(const char *a, const char *b)
> int qlen_b = quote_c_style(b, NULL, NULL, 0);
>
> if (qlen_a || qlen_b) {
> - quote_c_style(a, &name, NULL, 0);
> - strbuf_addstr(&name, " => ");
> - quote_c_style(b, &name, NULL, 0);
> - return strbuf_detach(&name, NULL);
> + quote_c_style(a, a_mid, NULL, 0);
> + quote_c_style(b, b_mid, NULL, 0);
> + return;
> }
>
> /* Find common prefix */
> @@ -1321,18 +1321,151 @@ static char *pprint_rename(const char *a, const char *b)
> a_midlen = 0;
> if (b_midlen < 0)
> b_midlen = 0;
> +
Trailing whitespace (there are many others you added to this file; I
won't bother to point out all of them).
> + strbuf_add(pfx, a, pfx_length);
> + strbuf_add(a_mid, a + pfx_length, a_midlen);
> + strbuf_add(b_mid, b + pfx_length, b_midlen);
> + strbuf_add(sfx, a + len_a - sfx_length, sfx_length);
> +}
> +
> +/*
> + * Omit each parts to fix in name_width.
> + * Formatted string is "<pfx>{<a_mid> => <b_mid>}<sfx>".
> + * At first, omit <pfx> as long as possible.
> + * If it is not enough, omit <a_mid>, <b_mid>, <sfx> by tring to set the length of
> + * those 3 parts(including "...") to the same.
> + * Ex:
> + * "foofoofoo => barbarbar"
> + * will be like
> + * "...foo => ...bar".
> + * "long_parent{foofoofoo => barbarbar}longfilename"
> + * will be like
> + * "...parent{...foofoo => ...barbar}...lename"
> + */
> +static void pprint_rename_omit(struct strbuf *pfx, struct strbuf *a_mid, struct strbuf *b_mid
> + , struct strbuf *sfx, int name_width)
Bad line splitting.
> +{
> +
> +#define ARROW " => "
> +#define ELLIPSIS "..."
Ugly and leaks these symbols after the function is done using them
to the remainder of this file. Write them like this instead, perhaps?
static const char arrow[] = " => ";
static const char dots[] = "...";
> +#define swap(a, b) myswap((a), (b), sizeof(a))
> +
> +#define myswap(a, b, size) do { \
> +unsigned char mytmp[size]; \
> +memcpy(mytmp, &a, size); \
> +memcpy(&a, &b, size); \
> +memcpy(&b, mytmp, size); \
> +} while (0)
These are totally unneeded, I suspect (see below).
> +
> + int use_curly_braces = (pfx->len > 0) || (sfx->len > 0);
> + size_t name_len;
> + size_t len;
> + size_t part_lengths[4];
Do not name an array in plural, i.e. elements[], unless there is a
compelling reason to do so. By using singular, e.g. element[], the
third element can be spelled as element[3], which is more logical
than having to call it elements[3].
Side note. a notable exception is an array that is used as a
hash-table and frequently passed around as an argument; you
are usually not interested in iterating over it in ascending
order, and being able to call such a collection of things
"things" in plural, e.g. struct object objects[], is more
important.
> + size_t max_part_len = 0;
> + size_t remainder_part_len = 0;
> + int i, j;
> +
> + name_len = pfx->len + a_mid->len + b_mid->len + sfx->len + strlen(ARROW)
> + + (use_curly_braces ? 2 : 0);
> +
> + if (name_len <= name_width) {
> + /* Everthing fits in name_width */
> + return;
> + }
> +
> + if (use_curly_braces) {
> + if (strlen(ELLIPSIS) + (name_len - pfx->len) <= name_width) {
> + /*
> + Just omitting left of '{' is enough
> + Ex: ...aaa{foofoofoo => bar}file
> + */
/*
* We format our multi-line
* comments like
* this.
*/
> + strbuf_splice(pfx, name_len - pfx->len, name_width - (name_len - pfx->len), ELLIPSIS, strlen(ELLIPSIS));
Overlong line.
Is the math for the second and third arguments correct? If you are
making "abcdefghij" into "...hij", you would splice at position 0
for length up to 'g', so it felt strange to see any arithmetic as
the second argument, but I didn't look at this code very closely.
> + return;
> + } else {
> + if (pfx->len > strlen(ELLIPSIS)) {
> + /*
> + Just omitting left of '{' is not enough
> + name will be "...{SOMETHING}SOMETHING"
> + */
> + strbuf_reset(pfx);
> + strbuf_addstr(pfx, ELLIPSIS);
> + }
> + }
> + }
>
> - strbuf_grow(&name, pfx_length + a_midlen + b_midlen + sfx_length + 7);
> - if (pfx_length + sfx_length) {
> - strbuf_add(&name, a, pfx_length);
> + /* available length for a_mid, b_mid and sfx */
> + len = name_width - strlen(ARROW) - (use_curly_braces ? 2 : 0);
> +
> + /* a_mid, b_mid, sfx will be have the same max, including ellipsis("..."). */
> + part_lengths[0] = (int)a_mid->len;
> + part_lengths[1] = (int)b_mid->len;
> + part_lengths[2] = (int)sfx->len;
What are these casts about? strbuf.len is of size_t which is
already the correct type for part_length[].
> +
> + /* bubble sort of part_lengths, descending order */
Do not bubble sort. Unless there is a compelling reason not to
(liek you are in a performance critical section and want to use a
custom sort algorithm), just let the platform-supplied qsort(3) do
the job by writing a small comparison function.
> + for (i=0; i<3; i++) {
> + for (j=i+1; j<3; j++) {
> + if (part_lengths[j] > part_lengths[i]) {
> + swap(part_lengths[i], part_lengths[j]);
> + }
> + }
> + }
> +
> + if (part_lengths[1] + part_lengths[1] + part_lengths[2] <= len) {
> + /*
> + * "{...foofoo => barbar}file"
> + * There is only one omitting part.
s/omitting/omitted/;
> + */
> + max_part_len = len - part_lengths[1] - part_lengths[2];
> + } else if (part_lengths[2] + part_lengths[2] + part_lengths[2] <= len) {
> + /*
> + * "{...foofoo => ...barbar}file"
> + * There are 2 omitting part.
s/omitting part/omitted parts/;
> + */
> + max_part_len = (len - part_lengths[2]) / 2;
> + remainder_part_len = (len - part_lengths[2]) - max_part_len * 2;
> + } else {
> + /*
> + * "{...ofoo => ...rbar}...file"
> + * There are 3 omitting part.
Likewise.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
2013-10-15 22:54 ` Junio C Hamano
@ 2013-10-15 22:58 ` Keshav Kini
2013-10-16 9:53 ` Yoshioka Tsuneo
1 sibling, 0 replies; 31+ messages in thread
From: Keshav Kini @ 2013-10-15 22:58 UTC (permalink / raw)
To: git
Junio C Hamano <gitster@pobox.com> writes:
> Yoshioka Tsuneo <yoshiokatsuneo@gmail.com> writes:
>
>> "git diff -M --stat" can detect rename and show renamed file name like
>> "foofoofoo => barbarbar". But if destination filename is long, the line
>> is shortened like "...barbarbar" so there is no way to know whether the
>> file is renamed or existed in the source commit.
>
> Is "destination" filename more special than the source filename?
> Perhaps "s/if destination filename is/if filenames are/"?
>
> Note: I do not want you to reroll using the suggested
> wording without explanation; it may be possible that I am
> missing something obvious and do not understand why you
> singled out destination, in which case I'd rather see it
> explained better in the log message than the potentially
> suboptimal suggestion I made in the review without
> understanding the issue. Of course, it is possible that you
> want to do the same when source is overlong, in which case
> you can just say "Yeah, you're right; will reroll".
>
> The above applies to all the other comments in this message.
>
> Also "s/source commit/original/". You may not be comparing two
> commits after all.
>
>> Make sure there is always an arrow, like "...foo => ...bar".
>> The output can contains curly braces('{','}') for grouping.
>
> s/contains/contain/;
>
>> So, in general, the outpu format is "<pfx>{<mid_a> => <mid_b>}<sfx>"
>
> s/outpu/&t/;
>
>> 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>, and <sfx> trying to
>> have the maximum length the same because those will be equaly important.
>
> A sound reasoning.
Also s/equaly/equally/;
-Keshav
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v6] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
2013-10-15 10:24 ` [PATCH v5] " Yoshioka Tsuneo
2013-10-15 22:54 ` Junio C Hamano
@ 2013-10-16 9:53 ` Yoshioka Tsuneo
2013-10-17 19:29 ` Junio C Hamano
` (2 more replies)
1 sibling, 3 replies; 31+ messages in thread
From: Yoshioka Tsuneo @ 2013-10-16 9:53 UTC (permalink / raw)
To: git@vger.kernel.org
"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>, and <sfx> trying to
have the maximum length the same because those will be equally important.
Signed-off-by: Tsuneo Yoshioka <yoshiokatsuneo@gmail.com>
Test-added-by: Thomas Rast <trast@inf.ethz.ch>
---
diff.c | 180 +++++++++++++++++++++++++++++++++++++++++++------
t/t4001-diff-rename.sh | 12 ++++
2 files changed, 170 insertions(+), 22 deletions(-)
diff --git a/diff.c b/diff.c
index a04a34d..afe6a36 100644
--- a/diff.c
+++ b/diff.c
@@ -1258,11 +1258,13 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
}
}
-static char *pprint_rename(const char *a, const char *b)
+static void find_common_prefix_suffix(const char *a, const char *b,
+ struct strbuf *pfx,
+ struct strbuf *a_mid, struct strbuf *b_mid,
+ struct strbuf *sfx)
{
const char *old = a;
const char *new = b;
- struct strbuf name = STRBUF_INIT;
int pfx_length, sfx_length;
int pfx_adjust_for_slash;
int len_a = strlen(a);
@@ -1272,10 +1274,9 @@ static char *pprint_rename(const char *a, const char *b)
int qlen_b = quote_c_style(b, NULL, NULL, 0);
if (qlen_a || qlen_b) {
- quote_c_style(a, &name, NULL, 0);
- strbuf_addstr(&name, " => ");
- quote_c_style(b, &name, NULL, 0);
- return strbuf_detach(&name, NULL);
+ quote_c_style(a, a_mid, NULL, 0);
+ quote_c_style(b, b_mid, NULL, 0);
+ return;
}
/* Find common prefix */
@@ -1322,17 +1323,142 @@ static char *pprint_rename(const char *a, const char *b)
if (b_midlen < 0)
b_midlen = 0;
- strbuf_grow(&name, pfx_length + a_midlen + b_midlen + sfx_length + 7);
- if (pfx_length + sfx_length) {
- strbuf_add(&name, a, pfx_length);
+ strbuf_add(pfx, a, pfx_length);
+ strbuf_add(a_mid, a + pfx_length, a_midlen);
+ strbuf_add(b_mid, b + pfx_length, b_midlen);
+ strbuf_add(sfx, a + len_a - sfx_length, sfx_length);
+}
+
+static int compare_size_t_descending_order(const void *left, const void *right)
+{
+ size_t left_val = *(size_t *)left;
+ size_t right_val = *(size_t *)right;
+ return (int)(right_val - left_val);
+}
+
+/*
+ * Omit each parts to fix in name_width.
+ * Formatted string is "<pfx>{<a_mid> => <b_mid>}<sfx>".
+ * At first, omit <pfx> as long as possible.
+ * If it is not enough, omit <a_mid>, <b_mid>, <sfx> by tring to set the length of
+ * those 3 parts(including "...") to the same.
+ * Ex:
+ * "foofoofoo => barbarbar"
+ * will be like
+ * "...foo => ...bar".
+ * "long_parent{foofoofoo => barbarbar}longfilename"
+ * will be like
+ * "...parent{...foofoo => ...barbar}...lename"
+ */
+static void rename_omit(struct strbuf *pfx,
+ struct strbuf *a_mid, struct strbuf *b_mid,
+ struct strbuf *sfx,
+ int name_width)
+{
+ static const char arrow[] = " => ";
+ static const char dots[] = "...";
+ int use_curly_braces = (pfx->len > 0) || (sfx->len > 0);
+ size_t name_len;
+ size_t len;
+ size_t part_length[3];
+ size_t max_part_len = 0;
+ size_t remainder_part_len = 0;
+
+ name_len = pfx->len + a_mid->len + b_mid->len + sfx->len + strlen(arrow)
+ + (use_curly_braces ? 2 : 0);
+
+ if (name_len <= name_width) {
+ /* Everthing fits in name_width */
+ return;
+ }
+
+ if (use_curly_braces) {
+ if (strlen(dots) + (name_len - pfx->len) <= name_width) {
+ /*
+ * Just omitting left of '{' is enough
+ * Ex: ...aaa{foofoofoo => bar}file
+ */
+ strbuf_splice(pfx, 0, name_len - name_width + strlen(dots), dots, strlen(dots));
+ return;
+ } else {
+ if (pfx->len > strlen(dots)) {
+ /*
+ * Just omitting left of '{' is not enough
+ * name will be "...{SOMETHING}SOMETHING"
+ */
+ strbuf_reset(pfx);
+ strbuf_addstr(pfx, dots);
+ }
+ }
+ }
+
+ /* available length for a_mid, b_mid and sfx */
+ len = name_width - strlen(arrow) - (use_curly_braces ? 2 : 0);
+
+ /* a_mid, b_mid, sfx will be have the same max, including ellipsis("..."). */
+ part_length[0] = a_mid->len;
+ part_length[1] = b_mid->len;
+ part_length[2] = sfx->len;
+
+ qsort(part_length, sizeof(part_length)/sizeof(part_length[0]), sizeof(part_length[0])
+ , compare_size_t_descending_order);
+
+ if (part_length[1] + part_length[1] + part_length[2] <= len) {
+ /*
+ * "{...foofoo => barbar}file"
+ * There is only one omitted part.
+ */
+ max_part_len = len - part_length[1] - part_length[2];
+ } else if (part_length[2] + part_length[2] + part_length[2] <= len) {
+ /*
+ * "{...foofoo => ...barbar}file"
+ * There are 2 omitted parts.
+ */
+ max_part_len = (len - part_length[2]) / 2;
+ remainder_part_len = (len - part_length[2]) - max_part_len * 2;
+ } else {
+ /*
+ * "{...ofoo => ...rbar}...file"
+ * There are 3 omitted parts.
+ */
+ max_part_len = len / 3;
+ remainder_part_len = len - (max_part_len) * 3;
+ }
+
+ if (max_part_len < strlen(dots))
+ max_part_len = strlen(dots);
+
+ if (sfx->len > max_part_len)
+ strbuf_splice(sfx, 0, sfx->len - max_part_len + strlen(dots), dots, strlen(dots));
+ if (remainder_part_len == 2)
+ max_part_len++;
+ if (a_mid->len > max_part_len)
+ strbuf_splice(a_mid, 0, a_mid->len - max_part_len + strlen(dots), dots, strlen(dots));
+ if (remainder_part_len == 1)
+ max_part_len++;
+ if (b_mid->len > max_part_len)
+ strbuf_splice(b_mid, 0, b_mid->len - max_part_len + strlen(dots), dots, strlen(dots));
+}
+
+static char *pprint_rename(const char *a, const char *b, int name_width)
+{
+ struct strbuf pfx = STRBUF_INIT, a_mid = STRBUF_INIT, b_mid = STRBUF_INIT, sfx = STRBUF_INIT;
+ struct strbuf name = STRBUF_INIT;
+
+ find_common_prefix_suffix(a, b, &pfx, &a_mid, &b_mid, &sfx);
+ rename_omit(&pfx, &a_mid, &b_mid, &sfx, name_width);
+
+ strbuf_grow(&name, pfx.len + a_mid.len + b_mid.len + sfx.len + 7);
+ if (pfx.len + sfx.len) {
+ strbuf_addbuf(&name, &pfx);
strbuf_addch(&name, '{');
}
- strbuf_add(&name, a + pfx_length, a_midlen);
+ strbuf_addbuf(&name, &a_mid);
strbuf_addstr(&name, " => ");
- strbuf_add(&name, b + pfx_length, b_midlen);
- if (pfx_length + sfx_length) {
+ strbuf_addbuf(&name, &b_mid);
+ if (pfx.len + sfx.len) {
strbuf_addch(&name, '}');
- strbuf_add(&name, a + len_a - sfx_length, sfx_length);
+ strbuf_addbuf(&name, &sfx);
}
return strbuf_detach(&name, NULL);
}
@@ -1418,23 +1544,31 @@ static void show_graph(FILE *file, char ch, int cnt, const char *set, const char
fprintf(file, "%s", reset);
}
-static void fill_print_name(struct diffstat_file *file)
+static void fill_print_name(struct diffstat_file *file, int name_width)
{
char *pname;
- if (file->print_name)
- return;
-
if (!file->is_renamed) {
struct strbuf buf = STRBUF_INIT;
+ if (file->print_name)
+ return;
if (quote_c_style(file->name, &buf, NULL, 0)) {
pname = strbuf_detach(&buf, NULL);
} else {
pname = file->name;
strbuf_release(&buf);
}
+ if (strlen(pname) > name_width) {
+ struct strbuf buf2 = STRBUF_INIT;
+ strbuf_addstr(&buf2, "...");
+ strbuf_addstr(&buf2, pname + strlen(pname) - name_width - 3);
+ }
} else {
- pname = pprint_rename(file->from_name, file->name);
+ if (file->print_name) {
+ free(file->print_name);
+ file->print_name = NULL;
+ }
+ pname = pprint_rename(file->from_name, file->name, name_width);
}
file->print_name = pname;
}
@@ -1517,7 +1651,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
count++; /* not shown == room for one more */
continue;
}
- fill_print_name(file);
+ fill_print_name(file, INT_MAX);
len = strlen(file->print_name);
if (max_len < len)
max_len = len;
@@ -1629,7 +1763,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
for (i = 0; i < count; i++) {
const char *prefix = "";
struct diffstat_file *file = data->files[i];
- char *name = file->print_name;
+ char *name;
uintmax_t added = file->added;
uintmax_t deleted = file->deleted;
int name_len;
@@ -1637,6 +1771,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
if (!file->is_interesting && (added + deleted == 0))
continue;
+ fill_print_name(file, name_width);
+ name = file->print_name;
/*
* "scale" the filename
*/
@@ -1772,7 +1908,7 @@ static void show_numstat(struct diffstat_t *data, struct diff_options *options)
"%"PRIuMAX"\t%"PRIuMAX"\t",
file->added, file->deleted);
if (options->line_termination) {
- fill_print_name(file);
+ fill_print_name(file, INT_MAX);
if (!file->is_renamed)
write_name_quoted(file->name, options->file,
options->line_termination);
@@ -4258,7 +4394,7 @@ static void show_mode_change(FILE *file, struct diff_filepair *p, int show_name,
static void show_rename_copy(FILE *file, const char *renamecopy, struct diff_filepair *p,
const char *line_prefix)
{
- char *names = pprint_rename(p->one->path, p->two->path);
+ char *names = pprint_rename(p->one->path, p->two->path, INT_MAX);
fprintf(file, " %s %s (%d%%)\n", renamecopy, names, similarity_index(p));
free(names);
diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
index 2f327b7..03d6371 100755
--- a/t/t4001-diff-rename.sh
+++ b/t/t4001-diff-rename.sh
@@ -156,4 +156,16 @@ test_expect_success 'rename pretty print common prefix and suffix overlap' '
test_i18ngrep " d/f/{ => f}/e " output
'
+test_expect_success 'rename of very long path shows =>' '
+ mkdir long_dirname_that_does_not_fit_in_a_single_line &&
+ mkdir another_extremely_long_path_but_not_the_same_as_the_first &&
+ cp path1 long_dirname*/ &&
+ git add long_dirname*/path1 &&
+ test_commit add_long_pathname &&
+ git mv long_dirname*/path1 another_extremely_*/ &&
+ test_commit move_long_pathname &&
+ git diff -M --stat HEAD^ HEAD >output &&
+ test_i18ngrep "=>.*path1" output
+'
+
test_done
--
1.8.4.475.g867697c
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v5] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
2013-10-15 22:54 ` Junio C Hamano
2013-10-15 22:58 ` Keshav Kini
@ 2013-10-16 9:53 ` Yoshioka Tsuneo
1 sibling, 0 replies; 31+ messages in thread
From: Yoshioka Tsuneo @ 2013-10-16 9:53 UTC (permalink / raw)
To: git@vger.kernel.org, Junio C Hamano, Keshav Kini
Hello Junio, Keshav
Thank you very much for your detailed reviewing.
I just tried to update the patch and posted as "[PATCH v6]".
---
Tsuneo Yoshioka (吉岡 恒夫)
yoshiokatsuneo@gmail.com
On Oct 16, 2013, at 1:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Yoshioka Tsuneo <yoshiokatsuneo@gmail.com> writes:
>
>> "git diff -M --stat" can detect rename and show renamed file name like
>> "foofoofoo => barbarbar". But if destination filename is long, the line
>> is shortened like "...barbarbar" so there is no way to know whether the
>> file is renamed or existed in the source commit.
>
> Is "destination" filename more special than the source filename?
> Perhaps "s/if destination filename is/if filenames are/"?
>
> Note: I do not want you to reroll using the suggested
> wording without explanation; it may be possible that I am
> missing something obvious and do not understand why you
> singled out destination, in which case I'd rather see it
> explained better in the log message than the potentially
> suboptimal suggestion I made in the review without
> understanding the issue. Of course, it is possible that you
> want to do the same when source is overlong, in which case
> you can just say "Yeah, you're right; will reroll".
>
> The above applies to all the other comments in this message.
>
> Also "s/source commit/original/". You may not be comparing two
> commits after all.
>
>> Make sure there is always an arrow, like "...foo => ...bar".
>> The output can contains curly braces('{','}') for grouping.
>
> s/contains/contain/;
>
>> So, in general, the outpu format is "<pfx>{<mid_a> => <mid_b>}<sfx>"
>
> s/outpu/&t/;
>
>> 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>, and <sfx> trying to
>> have the maximum length the same because those will be equaly important.
>
> A sound reasoning.
>
>> Signed-off-by: Tsuneo Yoshioka <yoshiokatsuneo@gmail.com>
>> Test-added-by: Thomas Rast <trast@inf.ethz.ch>
>> ---
>> diff.c | 187 +++++++++++++++++++++++++++++++++++++++++++------
>> t/t4001-diff-rename.sh | 12 ++++
>> 2 files changed, 177 insertions(+), 22 deletions(-)
>>
>> diff --git a/diff.c b/diff.c
>> index a04a34d..cf50807 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -1258,11 +1258,12 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
>> }
>> }
>>
>> -static char *pprint_rename(const char *a, const char *b)
>> +static void pprint_rename_find_common_prefix_suffix(const char *a, const char *b
>> + , struct strbuf *pfx, struct strbuf *a_mid
>> + , struct strbuf *b_mid, struct strbuf *sfx)
>
> What kind of line splitting is this?
>
> I think the real issue is that the function name is overly long, but
> aside from that,
>
> - comma comes at the end of the line, not at the beginning of the
> next line;
>
> - the second and subsequent lines are indented, but not more than
> the usual line width (align with the first letter inside the
> opening parenthesis of the first line);
>
> - a_mid and b_mid are more "alike" than pfx and a_mid.
>
> so I would expect to see it more like:
>
> static void abbrev_rename(const char *a, const char *b,
> struct strbuf *pfx,
> struct strbuf *a_mid, struct strbuf *b_mid,
> struct strbuf *sfx)
>
> Note that the suggested name does not say "pprint", because in your
> version of this file, the code around here is no longer doing any
> printing. The caller does so after using this function to decide
> how to abbreviate renames, so naming the helper function after what
> it does (e.g. abbreviate renames) is more appropriate.
>
>> {
>> const char *old = a;
>> const char *new = b;
>> - struct strbuf name = STRBUF_INIT;
>> int pfx_length, sfx_length;
>> int pfx_adjust_for_slash;
>> int len_a = strlen(a);
>> @@ -1272,10 +1273,9 @@ static char *pprint_rename(const char *a, const char *b)
>> int qlen_b = quote_c_style(b, NULL, NULL, 0);
>>
>> if (qlen_a || qlen_b) {
>> - quote_c_style(a, &name, NULL, 0);
>> - strbuf_addstr(&name, " => ");
>> - quote_c_style(b, &name, NULL, 0);
>> - return strbuf_detach(&name, NULL);
>> + quote_c_style(a, a_mid, NULL, 0);
>> + quote_c_style(b, b_mid, NULL, 0);
>> + return;
>> }
>>
>> /* Find common prefix */
>> @@ -1321,18 +1321,151 @@ static char *pprint_rename(const char *a, const char *b)
>> a_midlen = 0;
>> if (b_midlen < 0)
>> b_midlen = 0;
>> +
>
> Trailing whitespace (there are many others you added to this file; I
> won't bother to point out all of them).
>
>> + strbuf_add(pfx, a, pfx_length);
>> + strbuf_add(a_mid, a + pfx_length, a_midlen);
>> + strbuf_add(b_mid, b + pfx_length, b_midlen);
>> + strbuf_add(sfx, a + len_a - sfx_length, sfx_length);
>> +}
>> +
>> +/*
>> + * Omit each parts to fix in name_width.
>> + * Formatted string is "<pfx>{<a_mid> => <b_mid>}<sfx>".
>> + * At first, omit <pfx> as long as possible.
>> + * If it is not enough, omit <a_mid>, <b_mid>, <sfx> by tring to set the length of
>> + * those 3 parts(including "...") to the same.
>> + * Ex:
>> + * "foofoofoo => barbarbar"
>> + * will be like
>> + * "...foo => ...bar".
>> + * "long_parent{foofoofoo => barbarbar}longfilename"
>> + * will be like
>> + * "...parent{...foofoo => ...barbar}...lename"
>> + */
>> +static void pprint_rename_omit(struct strbuf *pfx, struct strbuf *a_mid, struct strbuf *b_mid
>> + , struct strbuf *sfx, int name_width)
>
> Bad line splitting.
>
>> +{
>> +
>> +#define ARROW " => "
>> +#define ELLIPSIS "..."
>
> Ugly and leaks these symbols after the function is done using them
> to the remainder of this file. Write them like this instead, perhaps?
>
> static const char arrow[] = " => ";
> static const char dots[] = "...";
>
>> +#define swap(a, b) myswap((a), (b), sizeof(a))
>> +
>> +#define myswap(a, b, size) do { \
>> +unsigned char mytmp[size]; \
>> +memcpy(mytmp, &a, size); \
>> +memcpy(&a, &b, size); \
>> +memcpy(&b, mytmp, size); \
>> +} while (0)
>
> These are totally unneeded, I suspect (see below).
>
>> +
>> + int use_curly_braces = (pfx->len > 0) || (sfx->len > 0);
>> + size_t name_len;
>> + size_t len;
>> + size_t part_lengths[4];
>
> Do not name an array in plural, i.e. elements[], unless there is a
> compelling reason to do so. By using singular, e.g. element[], the
> third element can be spelled as element[3], which is more logical
> than having to call it elements[3].
>
> Side note. a notable exception is an array that is used as a
> hash-table and frequently passed around as an argument; you
> are usually not interested in iterating over it in ascending
> order, and being able to call such a collection of things
> "things" in plural, e.g. struct object objects[], is more
> important.
>
>> + size_t max_part_len = 0;
>> + size_t remainder_part_len = 0;
>> + int i, j;
>> +
>> + name_len = pfx->len + a_mid->len + b_mid->len + sfx->len + strlen(ARROW)
>> + + (use_curly_braces ? 2 : 0);
>> +
>> + if (name_len <= name_width) {
>> + /* Everthing fits in name_width */
>> + return;
>> + }
>> +
>> + if (use_curly_braces) {
>> + if (strlen(ELLIPSIS) + (name_len - pfx->len) <= name_width) {
>> + /*
>> + Just omitting left of '{' is enough
>> + Ex: ...aaa{foofoofoo => bar}file
>> + */
>
> /*
> * We format our multi-line
> * comments like
> * this.
> */
>
>> + strbuf_splice(pfx, name_len - pfx->len, name_width - (name_len - pfx->len), ELLIPSIS, strlen(ELLIPSIS));
>
> Overlong line.
>
> Is the math for the second and third arguments correct? If you are
> making "abcdefghij" into "...hij", you would splice at position 0
> for length up to 'g', so it felt strange to see any arithmetic as
> the second argument, but I didn't look at this code very closely.
>
>> + return;
>> + } else {
>> + if (pfx->len > strlen(ELLIPSIS)) {
>> + /*
>> + Just omitting left of '{' is not enough
>> + name will be "...{SOMETHING}SOMETHING"
>> + */
>> + strbuf_reset(pfx);
>> + strbuf_addstr(pfx, ELLIPSIS);
>> + }
>> + }
>> + }
>>
>> - strbuf_grow(&name, pfx_length + a_midlen + b_midlen + sfx_length + 7);
>> - if (pfx_length + sfx_length) {
>> - strbuf_add(&name, a, pfx_length);
>> + /* available length for a_mid, b_mid and sfx */
>> + len = name_width - strlen(ARROW) - (use_curly_braces ? 2 : 0);
>> +
>> + /* a_mid, b_mid, sfx will be have the same max, including ellipsis("..."). */
>> + part_lengths[0] = (int)a_mid->len;
>> + part_lengths[1] = (int)b_mid->len;
>> + part_lengths[2] = (int)sfx->len;
>
> What are these casts about? strbuf.len is of size_t which is
> already the correct type for part_length[].
>
>> +
>> + /* bubble sort of part_lengths, descending order */
>
> Do not bubble sort. Unless there is a compelling reason not to
> (liek you are in a performance critical section and want to use a
> custom sort algorithm), just let the platform-supplied qsort(3) do
> the job by writing a small comparison function.
>
>> + for (i=0; i<3; i++) {
>> + for (j=i+1; j<3; j++) {
>> + if (part_lengths[j] > part_lengths[i]) {
>> + swap(part_lengths[i], part_lengths[j]);
>> + }
>> + }
>> + }
>> +
>> + if (part_lengths[1] + part_lengths[1] + part_lengths[2] <= len) {
>> + /*
>> + * "{...foofoo => barbar}file"
>> + * There is only one omitting part.
>
> s/omitting/omitted/;
>
>> + */
>> + max_part_len = len - part_lengths[1] - part_lengths[2];
>> + } else if (part_lengths[2] + part_lengths[2] + part_lengths[2] <= len) {
>> + /*
>> + * "{...foofoo => ...barbar}file"
>> + * There are 2 omitting part.
>
> s/omitting part/omitted parts/;
>
>> + */
>> + max_part_len = (len - part_lengths[2]) / 2;
>> + remainder_part_len = (len - part_lengths[2]) - max_part_len * 2;
>> + } else {
>> + /*
>> + * "{...ofoo => ...rbar}...file"
>> + * There are 3 omitting part.
>
> Likewise.
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v6] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
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 19:53 ` Junio C Hamano
2013-10-17 22:08 ` [PATCH v7] " Yoshioka Tsuneo
2 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2013-10-17 19:29 UTC (permalink / raw)
To: Yoshioka Tsuneo; +Cc: git@vger.kernel.org
Yoshioka Tsuneo <yoshiokatsuneo@gmail.com> 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>, and <sfx> trying to
> have the maximum length the same because those will be equally important.
I somehow find this solid wall of text extremely hard to
read. Adding a blank line as a paragraph break may make it easier to
read, perhaps.
Also it is customary in our history to omit the full-stop from the
patch title on the Subject: line.
> + name_len = pfx->len + a_mid->len + b_mid->len + sfx->len + strlen(arrow)
> + + (use_curly_braces ? 2 : 0);
> +
> + if (name_len <= name_width) {
> + /* Everthing fits in name_width */
> + return;
> + }
Logic up to this point seems good; drop {} around a single statement
"return;", i.e.
if (name_len <= name_width)
return; /* everything fits */
> + } else {
> + if (pfx->len > strlen(dots)) {
> + /*
> + * Just omitting left of '{' is not enough
> + * name will be "...{SOMETHING}SOMETHING"
> + */
> + strbuf_reset(pfx);
> + strbuf_addstr(pfx, dots);
> + }
(mental note) ... otherwise, i.e. with a short common prefix, the
final result will be "ab{SOMETHING}SOMETHING", which is also fine
for the purpose of the remainder of this function.
> + }
> + }
> +
> + /* available length for a_mid, b_mid and sfx */
> + len = name_width - strlen(arrow) - (use_curly_braces ? 2 : 0);
> +
> + /* a_mid, b_mid, sfx will be have the same max, including ellipsis("..."). */
> + part_length[0] = a_mid->len;
> + part_length[1] = b_mid->len;
> + part_length[2] = sfx->len;
> +
> + qsort(part_length, sizeof(part_length)/sizeof(part_length[0]), sizeof(part_length[0])
> + , compare_size_t_descending_order);
In our code, comma does not come at the beginning of continued
line.
> + if (part_length[1] + part_length[1] + part_length[2] <= len) {
> + /*
> + * "{...foofoo => barbar}file"
> + * There is only one omitted part.
> + */
> + max_part_len = len - part_length[1] - part_length[2];
It would be clearer to explicitly set remainder to zero here, and
omit the initialization of the variable. That would make what the
three parts of if/elseif/else do more consistent.
> + } else if (part_length[2] + part_length[2] + part_length[2] <= len) {
> + /*
> + * "{...foofoo => ...barbar}file"
> + * There are 2 omitted parts.
> + */
> + max_part_len = (len - part_length[2]) / 2;
> + remainder_part_len = (len - part_length[2]) - max_part_len * 2;
> + } else {
> + /*
> + * "{...ofoo => ...rbar}...file"
> + * There are 3 omitted parts.
> + */
> + max_part_len = len / 3;
> + remainder_part_len = len - (max_part_len) * 3;
> + }
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.
> diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
> index 2f327b7..03d6371 100755
> --- a/t/t4001-diff-rename.sh
> +++ b/t/t4001-diff-rename.sh
> @@ -156,4 +156,16 @@ test_expect_success 'rename pretty print common prefix and suffix overlap' '
> test_i18ngrep " d/f/{ => f}/e " output
> '
>
> +test_expect_success 'rename of very long path shows =>' '
> + mkdir long_dirname_that_does_not_fit_in_a_single_line &&
> + mkdir another_extremely_long_path_but_not_the_same_as_the_first &&
> + cp path1 long_dirname*/ &&
> + git add long_dirname*/path1 &&
> + test_commit add_long_pathname &&
> + git mv long_dirname*/path1 another_extremely_*/ &&
> + test_commit move_long_pathname &&
> + git diff -M --stat HEAD^ HEAD >output &&
> + test_i18ngrep "=>.*path1" output
Does this have to be i18ngrep? I had a feeling that we would not
want this part of the output localized, in which case "grep" may be
more appropriate.
> +'
> +
> test_done
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v6] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
2013-10-16 9:53 ` [PATCH v6] " Yoshioka Tsuneo
2013-10-17 19:29 ` Junio C Hamano
@ 2013-10-17 19:53 ` Junio C Hamano
2013-10-17 22:08 ` [PATCH v7] " Yoshioka Tsuneo
2 siblings, 0 replies; 31+ messages in thread
From: Junio C Hamano @ 2013-10-17 19:53 UTC (permalink / raw)
To: Yoshioka Tsuneo; +Cc: git@vger.kernel.org
Yoshioka Tsuneo <yoshiokatsuneo@gmail.com> writes:
> 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 =>".
This is an explanation much easier to understand than the one in the
previous iteration. Thanks.
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v7] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible
2013-10-16 9:53 ` [PATCH v6] " Yoshioka Tsuneo
2013-10-17 19:29 ` Junio C Hamano
2013-10-17 19:53 ` Junio C Hamano
@ 2013-10-17 22:08 ` Yoshioka Tsuneo
2013-10-18 9:35 ` [PATCH v8] " Yoshioka Tsuneo
2 siblings, 1 reply; 31+ messages in thread
From: Yoshioka Tsuneo @ 2013-10-17 22:08 UTC (permalink / raw)
To: git@vger.kernel.org
"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>, and <sfx> trying to
have the same maximum length, but as long as filename part of <sfx>
is kept.
Signed-off-by: Tsuneo Yoshioka <yoshiokatsuneo@gmail.com>
Test-added-by: Thomas Rast <trast@inf.ethz.ch>
---
diff.c | 184 +++++++++++++++++++++++++++++++++++++++++++------
t/t4001-diff-rename.sh | 12 ++++
2 files changed, 174 insertions(+), 22 deletions(-)
diff --git a/diff.c b/diff.c
index a04a34d..cdf59c0 100644
--- a/diff.c
+++ b/diff.c
@@ -1258,11 +1258,13 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
}
}
-static char *pprint_rename(const char *a, const char *b)
+static void find_common_prefix_suffix(const char *a, const char *b,
+ struct strbuf *pfx,
+ struct strbuf *a_mid, struct strbuf *b_mid,
+ struct strbuf *sfx)
{
const char *old = a;
const char *new = b;
- struct strbuf name = STRBUF_INIT;
int pfx_length, sfx_length;
int pfx_adjust_for_slash;
int len_a = strlen(a);
@@ -1272,10 +1274,9 @@ static char *pprint_rename(const char *a, const char *b)
int qlen_b = quote_c_style(b, NULL, NULL, 0);
if (qlen_a || qlen_b) {
- quote_c_style(a, &name, NULL, 0);
- strbuf_addstr(&name, " => ");
- quote_c_style(b, &name, NULL, 0);
- return strbuf_detach(&name, NULL);
+ quote_c_style(a, a_mid, NULL, 0);
+ quote_c_style(b, b_mid, NULL, 0);
+ return;
}
/* Find common prefix */
@@ -1322,17 +1323,146 @@ static char *pprint_rename(const char *a, const char *b)
if (b_midlen < 0)
b_midlen = 0;
- strbuf_grow(&name, pfx_length + a_midlen + b_midlen + sfx_length + 7);
- if (pfx_length + sfx_length) {
- strbuf_add(&name, a, pfx_length);
+ strbuf_add(pfx, a, pfx_length);
+ strbuf_add(a_mid, a + pfx_length, a_midlen);
+ strbuf_add(b_mid, b + pfx_length, b_midlen);
+ strbuf_add(sfx, a + len_a - sfx_length, sfx_length);
+}
+
+/*
+ * Omit each parts to fix in name_width.
+ * Formatted string is "<pfx>{<a_mid> => <b_mid>}<sfx>".
+ * At first, omit <pfx> as long as possible.
+ * If it is not enough, omit <a_mid>, <b_mid>, <sfx> by tring to set the length of
+ * those 3 parts(including "...") to the same, but keeping filename part of <sfx>.
+ * Ex:
+ * "foofoofoo => barbarbar"
+ * will be like
+ * "...foo => ...bar".
+ * "long_parent{foofoofoo => barbarbar}path/filename"
+ * will be like
+ * "...parent{...foofoo => ...barbar}.../filename"
+ */
+static void rename_omit(struct strbuf *pfx,
+ struct strbuf *a_mid, struct strbuf *b_mid,
+ struct strbuf *sfx,
+ int name_width)
+{
+ static const char arrow[] = " => ";
+ static const char dots[] = "...";
+ int use_curly_braces = (pfx->len > 0) || (sfx->len > 0);
+ size_t name_len;
+ size_t max_part_len = 0;
+ size_t remainder_part_len = 0;
+ size_t left, right;
+ size_t sfx_minlen;
+ char *sfx_last_slash;
+ size_t max_sfx_len;
+
+ name_len = pfx->len + a_mid->len + b_mid->len + sfx->len + strlen(arrow)
+ + (use_curly_braces ? 2 : 0);
+
+ if (name_len <= name_width)
+ return; /* everything fits in name_width */
+
+ if (use_curly_braces) {
+ if (strlen(dots) + (name_len - pfx->len) <= name_width) {
+ /*
+ * Just omitting left of '{' is enough
+ * Ex: ...aaa{foofoofoo => bar}file
+ */
+ strbuf_splice(pfx, 0, name_len - name_width + strlen(dots), dots, strlen(dots));
+ return;
+ } else {
+ if (pfx->len > strlen(dots)) {
+ /*
+ * Just omitting left of '{' is not enough
+ * name will be "...{SOMETHING}SOMETHING"
+ */
+ strbuf_reset(pfx);
+ strbuf_addstr(pfx, dots);
+ }else{
+ /*
+ * If <pfx> is shorter than dots("..."),
+ * there is no sense to replace <pfx> to dots
+ * but name will be just like "a{SOMETHING}SOMETHING".
+ */
+ ;
+ }
+ }
+ }
+
+ left = 0;
+ right = name_width + 1;
+
+#define MIN(X, Y) ((X < Y) ? (X) : (Y))
+#define MAX(X, Y) ((X > Y) ? (X) : (Y))
+
+ /* Try to keep filename part(later than last '/') of <sfx> */
+ sfx_last_slash = strrchr(sfx->buf, '/');
+ if(sfx_last_slash){
+ sfx_minlen = MIN(sfx->len - (sfx_last_slash - sfx->buf) + strlen(dots), sfx->len);
+ }else{
+ sfx_minlen = sfx->len;
+ }
+ /* In case other than <sfx> is omitted like: "...{... => ...}<sfx>" */
+ sfx_minlen = MIN(sfx_minlen,
+ name_width
+ - MIN(strlen(dots), pfx->len)
+ - MIN(strlen(dots), a_mid->len)
+ - MIN(strlen(dots), b_mid->len)
+ - strlen(arrow)
+ - (use_curly_braces ? 2 : 0) );
+
+ /* binary search to find max_part_len(maximum length of omitted parts) */
+ while(left + 1 < right){
+ size_t mid = (left + right) / 2;
+
+ /* length of "<pfx>{<a_mid> => <b_mid>}<sfx>" */
+ size_t l = pfx->len + MIN(mid, a_mid->len) + MIN(mid, b_mid->len) + MAX(MIN(mid, sfx->len), sfx_minlen) + strlen(arrow) + (use_curly_braces ? 2 : 0);
+ if(l <= name_width){
+ left = mid;
+ remainder_part_len = name_width - l;
+ }else{
+ right = mid;
+ }
+ }
+ max_part_len = left;
+
+ if (max_part_len < strlen(dots))
+ max_part_len = strlen(dots);
+ max_sfx_len = MAX(max_part_len, sfx_minlen);
+ if (sfx->len > max_sfx_len)
+ strbuf_splice(sfx, 0, sfx->len - max_sfx_len + strlen(dots), dots, strlen(dots));
+ if (remainder_part_len == 2)
+ max_part_len++;
+ if (a_mid->len > max_part_len)
+ strbuf_splice(a_mid, 0, a_mid->len - max_part_len + strlen(dots), dots, strlen(dots));
+ if (remainder_part_len == 1)
+ max_part_len++;
+ if (b_mid->len > max_part_len)
+ strbuf_splice(b_mid, 0, b_mid->len - max_part_len + strlen(dots), dots, strlen(dots));
+}
+
+static char *pprint_rename(const char *a, const char *b, int name_width)
+{
+ struct strbuf pfx = STRBUF_INIT, a_mid = STRBUF_INIT, b_mid = STRBUF_INIT, sfx = STRBUF_INIT;
+ struct strbuf name = STRBUF_INIT;
+
+ find_common_prefix_suffix(a, b, &pfx, &a_mid, &b_mid, &sfx);
+ rename_omit(&pfx, &a_mid, &b_mid, &sfx, name_width);
+
+ strbuf_grow(&name, pfx.len + a_mid.len + b_mid.len + sfx.len + 7);
+ if (pfx.len + sfx.len) {
+ strbuf_addbuf(&name, &pfx);
strbuf_addch(&name, '{');
}
- strbuf_add(&name, a + pfx_length, a_midlen);
+ strbuf_addbuf(&name, &a_mid);
strbuf_addstr(&name, " => ");
- strbuf_add(&name, b + pfx_length, b_midlen);
- if (pfx_length + sfx_length) {
+ strbuf_addbuf(&name, &b_mid);
+ if (pfx.len + sfx.len) {
strbuf_addch(&name, '}');
- strbuf_add(&name, a + len_a - sfx_length, sfx_length);
+ strbuf_addbuf(&name, &sfx);
}
return strbuf_detach(&name, NULL);
}
@@ -1418,23 +1548,31 @@ static void show_graph(FILE *file, char ch, int cnt, const char *set, const char
fprintf(file, "%s", reset);
}
-static void fill_print_name(struct diffstat_file *file)
+static void fill_print_name(struct diffstat_file *file, int name_width)
{
char *pname;
- if (file->print_name)
- return;
-
if (!file->is_renamed) {
struct strbuf buf = STRBUF_INIT;
+ if (file->print_name)
+ return;
if (quote_c_style(file->name, &buf, NULL, 0)) {
pname = strbuf_detach(&buf, NULL);
} else {
pname = file->name;
strbuf_release(&buf);
}
+ if (strlen(pname) > name_width) {
+ struct strbuf buf2 = STRBUF_INIT;
+ strbuf_addstr(&buf2, "...");
+ strbuf_addstr(&buf2, pname + strlen(pname) - name_width - 3);
+ }
} else {
- pname = pprint_rename(file->from_name, file->name);
+ if (file->print_name) {
+ free(file->print_name);
+ file->print_name = NULL;
+ }
+ pname = pprint_rename(file->from_name, file->name, name_width);
}
file->print_name = pname;
}
@@ -1517,7 +1655,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
count++; /* not shown == room for one more */
continue;
}
- fill_print_name(file);
+ fill_print_name(file, INT_MAX);
len = strlen(file->print_name);
if (max_len < len)
max_len = len;
@@ -1629,7 +1767,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
for (i = 0; i < count; i++) {
const char *prefix = "";
struct diffstat_file *file = data->files[i];
- char *name = file->print_name;
+ char *name;
uintmax_t added = file->added;
uintmax_t deleted = file->deleted;
int name_len;
@@ -1637,6 +1775,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
if (!file->is_interesting && (added + deleted == 0))
continue;
+ fill_print_name(file, name_width);
+ name = file->print_name;
/*
* "scale" the filename
*/
@@ -1772,7 +1912,7 @@ static void show_numstat(struct diffstat_t *data, struct diff_options *options)
"%"PRIuMAX"\t%"PRIuMAX"\t",
file->added, file->deleted);
if (options->line_termination) {
- fill_print_name(file);
+ fill_print_name(file, INT_MAX);
if (!file->is_renamed)
write_name_quoted(file->name, options->file,
options->line_termination);
@@ -4258,7 +4398,7 @@ static void show_mode_change(FILE *file, struct diff_filepair *p, int show_name,
static void show_rename_copy(FILE *file, const char *renamecopy, struct diff_filepair *p,
const char *line_prefix)
{
- char *names = pprint_rename(p->one->path, p->two->path);
+ char *names = pprint_rename(p->one->path, p->two->path, INT_MAX);
fprintf(file, " %s %s (%d%%)\n", renamecopy, names, similarity_index(p));
free(names);
diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
index 2f327b7..f79b526 100755
--- a/t/t4001-diff-rename.sh
+++ b/t/t4001-diff-rename.sh
@@ -156,4 +156,16 @@ test_expect_success 'rename pretty print common prefix and suffix overlap' '
test_i18ngrep " d/f/{ => f}/e " output
'
+test_expect_success 'rename of very long path shows =>' '
+ mkdir long_dirname_that_does_not_fit_in_a_single_line &&
+ mkdir another_extremely_long_path_but_not_the_same_as_the_first &&
+ cp path1 long_dirname*/ &&
+ git add long_dirname*/path1 &&
+ test_commit add_long_pathname &&
+ git mv long_dirname*/path1 another_extremely_*/ &&
+ test_commit move_long_pathname &&
+ git diff -M --stat HEAD^ HEAD >output &&
+ grep "=>.*path1" output
+'
+
test_done
--
1.8.4.475.g867697c
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v6] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
2013-10-17 19:29 ` Junio C Hamano
@ 2013-10-17 22:08 ` Yoshioka Tsuneo
2013-10-17 22:38 ` Junio C Hamano
0 siblings, 1 reply; 31+ messages in thread
From: Yoshioka Tsuneo @ 2013-10-17 22:08 UTC (permalink / raw)
To: Junio C Hamano, git@vger.kernel.org
Hello Junio
Thank you very much for the reviewing.
I try to fix the issues, and posted the updated patch as "[PATCH v7]".
> 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.
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.
Thank you !
---
Tsuneo Yoshioka (吉岡 恒夫)
yoshiokatsuneo@gmail.com
On Oct 17, 2013, at 10:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Yoshioka Tsuneo <yoshiokatsuneo@gmail.com> 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>, and <sfx> trying to
>> have the maximum length the same because those will be equally important.
>
> I somehow find this solid wall of text extremely hard to
> read. Adding a blank line as a paragraph break may make it easier to
> read, perhaps.
>
> Also it is customary in our history to omit the full-stop from the
> patch title on the Subject: line.
>
>> + name_len = pfx->len + a_mid->len + b_mid->len + sfx->len + strlen(arrow)
>> + + (use_curly_braces ? 2 : 0);
>> +
>> + if (name_len <= name_width) {
>> + /* Everthing fits in name_width */
>> + return;
>> + }
>
> Logic up to this point seems good; drop {} around a single statement
> "return;", i.e.
>
> if (name_len <= name_width)
> return; /* everything fits */
>
>> + } else {
>> + if (pfx->len > strlen(dots)) {
>> + /*
>> + * Just omitting left of '{' is not enough
>> + * name will be "...{SOMETHING}SOMETHING"
>> + */
>> + strbuf_reset(pfx);
>> + strbuf_addstr(pfx, dots);
>> + }
>
> (mental note) ... otherwise, i.e. with a short common prefix, the
> final result will be "ab{SOMETHING}SOMETHING", which is also fine
> for the purpose of the remainder of this function.
>
>> + }
>> + }
>> +
>> + /* available length for a_mid, b_mid and sfx */
>> + len = name_width - strlen(arrow) - (use_curly_braces ? 2 : 0);
>> +
>> + /* a_mid, b_mid, sfx will be have the same max, including ellipsis("..."). */
>> + part_length[0] = a_mid->len;
>> + part_length[1] = b_mid->len;
>> + part_length[2] = sfx->len;
>> +
>> + qsort(part_length, sizeof(part_length)/sizeof(part_length[0]), sizeof(part_length[0])
>> + , compare_size_t_descending_order);
>
> In our code, comma does not come at the beginning of continued
> line.
>
>> + if (part_length[1] + part_length[1] + part_length[2] <= len) {
>> + /*
>> + * "{...foofoo => barbar}file"
>> + * There is only one omitted part.
>> + */
>> + max_part_len = len - part_length[1] - part_length[2];
>
> It would be clearer to explicitly set remainder to zero here, and
> omit the initialization of the variable. That would make what the
> three parts of if/elseif/else do more consistent.
>
>> + } else if (part_length[2] + part_length[2] + part_length[2] <= len) {
>> + /*
>> + * "{...foofoo => ...barbar}file"
>> + * There are 2 omitted parts.
>> + */
>> + max_part_len = (len - part_length[2]) / 2;
>> + remainder_part_len = (len - part_length[2]) - max_part_len * 2;
>> + } else {
>> + /*
>> + * "{...ofoo => ...rbar}...file"
>> + * There are 3 omitted parts.
>> + */
>> + max_part_len = len / 3;
>> + remainder_part_len = len - (max_part_len) * 3;
>> + }
>
> 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.
>
>> diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
>> index 2f327b7..03d6371 100755
>> --- a/t/t4001-diff-rename.sh
>> +++ b/t/t4001-diff-rename.sh
>> @@ -156,4 +156,16 @@ test_expect_success 'rename pretty print common prefix and suffix overlap' '
>> test_i18ngrep " d/f/{ => f}/e " output
>> '
>>
>> +test_expect_success 'rename of very long path shows =>' '
>> + mkdir long_dirname_that_does_not_fit_in_a_single_line &&
>> + mkdir another_extremely_long_path_but_not_the_same_as_the_first &&
>> + cp path1 long_dirname*/ &&
>> + git add long_dirname*/path1 &&
>> + test_commit add_long_pathname &&
>> + git mv long_dirname*/path1 another_extremely_*/ &&
>> + test_commit move_long_pathname &&
>> + git diff -M --stat HEAD^ HEAD >output &&
>> + test_i18ngrep "=>.*path1" output
>
> Does this have to be i18ngrep? I had a feeling that we would not
> want this part of the output localized, in which case "grep" may be
> more appropriate.
>
>> +'
>> +
>> test_done
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v6] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
2013-10-17 22:08 ` Yoshioka Tsuneo
@ 2013-10-17 22:38 ` Junio C Hamano
2013-10-18 9:35 ` Yoshioka Tsuneo
0 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2013-10-17 22:38 UTC (permalink / raw)
To: Yoshioka Tsuneo; +Cc: git@vger.kernel.org
Yoshioka Tsuneo <yoshiokatsuneo@gmail.com> writes:
> 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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v8] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible
2013-10-17 22:08 ` [PATCH v7] " Yoshioka Tsuneo
@ 2013-10-18 9:35 ` Yoshioka Tsuneo
2013-10-19 6:24 ` Thomas Rast
0 siblings, 1 reply; 31+ messages in thread
From: Yoshioka Tsuneo @ 2013-10-18 9:35 UTC (permalink / raw)
To: git@vger.kernel.org
"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 <yoshiokatsuneo@gmail.com>
Test-added-by: Thomas Rast <trast@inf.ethz.ch>
---
diff.c | 176 ++++++++++++++++++++++++++++++++++++++++++-------
t/t4001-diff-rename.sh | 12 ++++
2 files changed, 166 insertions(+), 22 deletions(-)
diff --git a/diff.c b/diff.c
index a04a34d..69c3e17 100644
--- a/diff.c
+++ b/diff.c
@@ -1258,11 +1258,13 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
}
}
-static char *pprint_rename(const char *a, const char *b)
+static void find_common_prefix_suffix(const char *a, const char *b,
+ struct strbuf *pfx,
+ struct strbuf *a_mid, struct strbuf *b_mid,
+ struct strbuf *sfx)
{
const char *old = a;
const char *new = b;
- struct strbuf name = STRBUF_INIT;
int pfx_length, sfx_length;
int pfx_adjust_for_slash;
int len_a = strlen(a);
@@ -1272,10 +1274,9 @@ static char *pprint_rename(const char *a, const char *b)
int qlen_b = quote_c_style(b, NULL, NULL, 0);
if (qlen_a || qlen_b) {
- quote_c_style(a, &name, NULL, 0);
- strbuf_addstr(&name, " => ");
- quote_c_style(b, &name, NULL, 0);
- return strbuf_detach(&name, NULL);
+ quote_c_style(a, a_mid, NULL, 0);
+ quote_c_style(b, b_mid, NULL, 0);
+ return;
}
/* Find common prefix */
@@ -1322,17 +1323,138 @@ static char *pprint_rename(const char *a, const char *b)
if (b_midlen < 0)
b_midlen = 0;
- strbuf_grow(&name, pfx_length + a_midlen + b_midlen + sfx_length + 7);
- if (pfx_length + sfx_length) {
- strbuf_add(&name, a, pfx_length);
+ strbuf_add(pfx, a, pfx_length);
+ strbuf_add(a_mid, a + pfx_length, a_midlen);
+ strbuf_add(b_mid, b + pfx_length, b_midlen);
+ strbuf_add(sfx, a + len_a - sfx_length, sfx_length);
+}
+
+/*
+ * Omit each parts to fix in name_width.
+ * Formatted string is "<pfx>{<a_mid> => <b_mid>}<sfx>".
+ * At first, omit <pfx> as long as possible.
+ * If it is not enough, omit <a_mid>, <b_mid> trying to set the length of
+ * those 2 parts(including "...") to the same.
+ * If it is not enough yet, omit <sfx>.
+ * Ex:
+ * "foofoofoo => barbarbar"
+ * will be like
+ * "...foo => ...bar".
+ * "long_parent{foofoofoo => barbarbar}path/filename"
+ * will be like
+ * "...parent{...foofoo => ...barbar}path/filename"
+ */
+static void rename_omit(struct strbuf *pfx,
+ struct strbuf *a_mid, struct strbuf *b_mid,
+ struct strbuf *sfx,
+ int name_width)
+{
+ static const char arrow[] = " => ";
+ static const char dots[] = "...";
+ int use_curly_braces = (pfx->len > 0) || (sfx->len > 0);
+ size_t name_len;
+ size_t max_part_len = 0;
+ size_t remainder_part_len = 0;
+ size_t left, right;
+ size_t max_sfx_len;
+ size_t sfx_len;
+
+ name_len = pfx->len + a_mid->len + b_mid->len + sfx->len + strlen(arrow)
+ + (use_curly_braces ? 2 : 0);
+
+ if (name_len <= name_width)
+ return; /* everything fits in name_width */
+
+ if (use_curly_braces) {
+ if (strlen(dots) + (name_len - pfx->len) <= name_width) {
+ /*
+ * Just omitting left of '{' is enough
+ * Ex: ...aaa{foofoofoo => bar}file
+ */
+ strbuf_splice(pfx, 0, name_len - name_width + strlen(dots), dots, strlen(dots));
+ return;
+ } else {
+ if (pfx->len > strlen(dots)) {
+ /*
+ * Just omitting left of '{' is not enough
+ * name will be "...{SOMETHING}SOMETHING"
+ */
+ strbuf_reset(pfx);
+ strbuf_addstr(pfx, dots);
+ }else{
+ /*
+ * If <pfx> is shorter than dots("..."),
+ * there is no sense to replace <pfx> to dots
+ * but name will be just like "a{SOMETHING}SOMETHING".
+ */
+ ;
+ }
+ }
+ }
+
+ left = 0;
+ right = name_width + 1;
+
+#define MIN(X, Y) ((X < Y) ? (X) : (Y))
+#define MAX(X, Y) ((X > Y) ? (X) : (Y))
+
+ /* In case other than <sfx> is omitted like: "...{... => ...}<sfx>" */
+ max_sfx_len = name_width
+ - MIN(strlen(dots), pfx->len)
+ - MIN(strlen(dots), a_mid->len)
+ - MIN(strlen(dots), b_mid->len)
+ - strlen(arrow)
+ - (use_curly_braces ? 2 : 0);
+ sfx_len = MIN(sfx->len, max_sfx_len);
+
+ /* binary search to find max_part_len(maximum length of omitted parts) */
+ while(left + 1 < right){
+ size_t mid = (left + right) / 2;
+
+ /* length of "<pfx>{<a_mid> => <b_mid>}<sfx>" */
+ size_t l = pfx->len + MIN(mid, a_mid->len) + MIN(mid, b_mid->len) + sfx_len + strlen(arrow) + (use_curly_braces ? 2 : 0);
+ if(l <= name_width){
+ left = mid;
+ remainder_part_len = name_width - l;
+ }else{
+ right = mid;
+ }
+ }
+ max_part_len = left;
+
+ if (max_part_len < strlen(dots))
+ max_part_len = strlen(dots);
+ if (sfx->len > sfx_len)
+ strbuf_splice(sfx, 0, sfx->len - sfx_len + strlen(dots), dots, strlen(dots));
+ if (remainder_part_len == 2)
+ max_part_len++;
+ if (a_mid->len > max_part_len)
+ strbuf_splice(a_mid, 0, a_mid->len - max_part_len + strlen(dots), dots, strlen(dots));
+ if (remainder_part_len == 1)
+ max_part_len++;
+ if (b_mid->len > max_part_len)
+ strbuf_splice(b_mid, 0, b_mid->len - max_part_len + strlen(dots), dots, strlen(dots));
+}
+
+static char *pprint_rename(const char *a, const char *b, int name_width)
+{
+ struct strbuf pfx = STRBUF_INIT, a_mid = STRBUF_INIT, b_mid = STRBUF_INIT, sfx = STRBUF_INIT;
+ struct strbuf name = STRBUF_INIT;
+
+ find_common_prefix_suffix(a, b, &pfx, &a_mid, &b_mid, &sfx);
+ rename_omit(&pfx, &a_mid, &b_mid, &sfx, name_width);
+
+ strbuf_grow(&name, pfx.len + a_mid.len + b_mid.len + sfx.len + 7);
+ if (pfx.len + sfx.len) {
+ strbuf_addbuf(&name, &pfx);
strbuf_addch(&name, '{');
}
- strbuf_add(&name, a + pfx_length, a_midlen);
+ strbuf_addbuf(&name, &a_mid);
strbuf_addstr(&name, " => ");
- strbuf_add(&name, b + pfx_length, b_midlen);
- if (pfx_length + sfx_length) {
+ strbuf_addbuf(&name, &b_mid);
+ if (pfx.len + sfx.len) {
strbuf_addch(&name, '}');
- strbuf_add(&name, a + len_a - sfx_length, sfx_length);
+ strbuf_addbuf(&name, &sfx);
}
return strbuf_detach(&name, NULL);
}
@@ -1418,23 +1540,31 @@ static void show_graph(FILE *file, char ch, int cnt, const char *set, const char
fprintf(file, "%s", reset);
}
-static void fill_print_name(struct diffstat_file *file)
+static void fill_print_name(struct diffstat_file *file, int name_width)
{
char *pname;
- if (file->print_name)
- return;
-
if (!file->is_renamed) {
struct strbuf buf = STRBUF_INIT;
+ if (file->print_name)
+ return;
if (quote_c_style(file->name, &buf, NULL, 0)) {
pname = strbuf_detach(&buf, NULL);
} else {
pname = file->name;
strbuf_release(&buf);
}
+ if (strlen(pname) > name_width) {
+ struct strbuf buf2 = STRBUF_INIT;
+ strbuf_addstr(&buf2, "...");
+ strbuf_addstr(&buf2, pname + strlen(pname) - name_width - 3);
+ }
} else {
- pname = pprint_rename(file->from_name, file->name);
+ if (file->print_name) {
+ free(file->print_name);
+ file->print_name = NULL;
+ }
+ pname = pprint_rename(file->from_name, file->name, name_width);
}
file->print_name = pname;
}
@@ -1517,7 +1647,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
count++; /* not shown == room for one more */
continue;
}
- fill_print_name(file);
+ fill_print_name(file, INT_MAX);
len = strlen(file->print_name);
if (max_len < len)
max_len = len;
@@ -1629,7 +1759,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
for (i = 0; i < count; i++) {
const char *prefix = "";
struct diffstat_file *file = data->files[i];
- char *name = file->print_name;
+ char *name;
uintmax_t added = file->added;
uintmax_t deleted = file->deleted;
int name_len;
@@ -1637,6 +1767,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
if (!file->is_interesting && (added + deleted == 0))
continue;
+ fill_print_name(file, name_width);
+ name = file->print_name;
/*
* "scale" the filename
*/
@@ -1772,7 +1904,7 @@ static void show_numstat(struct diffstat_t *data, struct diff_options *options)
"%"PRIuMAX"\t%"PRIuMAX"\t",
file->added, file->deleted);
if (options->line_termination) {
- fill_print_name(file);
+ fill_print_name(file, INT_MAX);
if (!file->is_renamed)
write_name_quoted(file->name, options->file,
options->line_termination);
@@ -4258,7 +4390,7 @@ static void show_mode_change(FILE *file, struct diff_filepair *p, int show_name,
static void show_rename_copy(FILE *file, const char *renamecopy, struct diff_filepair *p,
const char *line_prefix)
{
- char *names = pprint_rename(p->one->path, p->two->path);
+ char *names = pprint_rename(p->one->path, p->two->path, INT_MAX);
fprintf(file, " %s %s (%d%%)\n", renamecopy, names, similarity_index(p));
free(names);
diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
index 2f327b7..f79b526 100755
--- a/t/t4001-diff-rename.sh
+++ b/t/t4001-diff-rename.sh
@@ -156,4 +156,16 @@ test_expect_success 'rename pretty print common prefix and suffix overlap' '
test_i18ngrep " d/f/{ => f}/e " output
'
+test_expect_success 'rename of very long path shows =>' '
+ mkdir long_dirname_that_does_not_fit_in_a_single_line &&
+ mkdir another_extremely_long_path_but_not_the_same_as_the_first &&
+ cp path1 long_dirname*/ &&
+ git add long_dirname*/path1 &&
+ test_commit add_long_pathname &&
+ git mv long_dirname*/path1 another_extremely_*/ &&
+ test_commit move_long_pathname &&
+ git diff -M --stat HEAD^ HEAD >output &&
+ grep "=>.*path1" output
+'
+
test_done
--
1.8.4.475.g867697c
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v6] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
2013-10-17 22:38 ` Junio C Hamano
@ 2013-10-18 9:35 ` Yoshioka Tsuneo
0 siblings, 0 replies; 31+ messages in thread
From: Yoshioka Tsuneo @ 2013-10-18 9:35 UTC (permalink / raw)
To: Junio C Hamano, git@vger.kernel.org
Hello Junio
>> 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.
I'm not sure which is the better in general.
But anyway, I don't have strong opinion about this.
So, I just changed to keep the all of the <sfx> part(lator than '}').
I just sent the updated patch as "[PATCH v8]".
Thanks !
---
Tsuneo Yoshioka (吉岡 恒夫)
yoshiokatsuneo@gmail.com
On Oct 18, 2013, at 1:38 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Yoshioka Tsuneo <yoshiokatsuneo@gmail.com> writes:
>
>> 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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible
2013-10-18 9:35 ` [PATCH v8] " Yoshioka Tsuneo
@ 2013-10-19 6:24 ` Thomas Rast
2013-10-20 1:49 ` Yoshioka Tsuneo
0 siblings, 1 reply; 31+ messages in thread
From: Thomas Rast @ 2013-10-19 6:24 UTC (permalink / raw)
To: Yoshioka Tsuneo
Cc: git@vger.kernel.org, Junio C Hamano, Duy Nguyen, Felipe Contreras
Yoshioka Tsuneo <yoshiokatsuneo@gmail.com> 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 <yoshiokatsuneo@gmail.com>
> Test-added-by: Thomas Rast <trast@inf.ethz.ch>
> ---
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible
2013-10-19 6:24 ` Thomas Rast
@ 2013-10-20 1:49 ` Yoshioka Tsuneo
2013-10-22 18:09 ` Junio C Hamano
0 siblings, 1 reply; 31+ messages in thread
From: Yoshioka Tsuneo @ 2013-10-20 1:49 UTC (permalink / raw)
To: Thomas Rast, git@vger.kernel.org, Junio C Hamano
Cc: Duy Nguyen, Felipe Contreras
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 <gitster@pobox.com> 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 <gitster@pobox.com> wrote:
> Yoshioka Tsuneo <yoshiokatsuneo@gmail.com> writes:
>
>> 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 <tr@thomasrast.ch> wrote:
> Yoshioka Tsuneo <yoshiokatsuneo@gmail.com> 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 <yoshiokatsuneo@gmail.com>
>> Test-added-by: Thomas Rast <trast@inf.ethz.ch>
>> ---
>
> 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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible
2013-10-20 1:49 ` Yoshioka Tsuneo
@ 2013-10-22 18:09 ` Junio C Hamano
2013-10-22 20:14 ` Yoshioka Tsuneo
0 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2013-10-22 18:09 UTC (permalink / raw)
To: Yoshioka Tsuneo
Cc: Thomas Rast, git@vger.kernel.org, Duy Nguyen, Felipe Contreras
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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible
2013-10-22 18:09 ` Junio C Hamano
@ 2013-10-22 20:14 ` Yoshioka Tsuneo
2013-10-22 20:26 ` Junio C Hamano
0 siblings, 1 reply; 31+ messages in thread
From: Yoshioka Tsuneo @ 2013-10-22 20:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thomas Rast, git, Duy Nguyen, Felipe Contreras
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 <tr@thomasrast.ch> 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 <gitster@pobox.com> wrote:
> 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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible
2013-10-22 20:14 ` Yoshioka Tsuneo
@ 2013-10-22 20:26 ` Junio C Hamano
0 siblings, 0 replies; 31+ messages in thread
From: Junio C Hamano @ 2013-10-22 20:26 UTC (permalink / raw)
To: Yoshioka Tsuneo; +Cc: Thomas Rast, git, Duy Nguyen, Felipe Contreras
Yoshioka Tsuneo <yoshiokatsuneo@gmail.com> writes:
> 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.
Yes, but...
> # Though I know we can know more after seeing the implementation, anyway :-)
... you are very correct about this.
Note that I am not rejecting the topic yet. I am just saying that I
am not yet convinced the patch improves the situation where an
optimal solution (i.e. no information loss at all) cannot exist
because we do not have enough output columns to work with.
Thanks.
>> ...
>> 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.
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2013-10-22 20:26 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2013-10-22 20:14 ` Yoshioka Tsuneo
2013-10-22 20:26 ` Junio C Hamano
2013-10-12 20:48 ` [PATCH v3] " Yoshioka Tsuneo
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).