Re: [PATCH v3] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
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 (吉岡 恒夫)
yoshiokatsu...@gmail.com
On Oct 13, 2013, at 11:29 PM, Thomas Rast wrote:
> Hi,
>
> Yoshioka Tsuneo 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
>> ---
>> 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
> t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
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 (吉岡 恒夫)
yoshiokatsu...@gmail.com
On Oct 14, 2013, at 10:04 PM, Duy Nguyen wrote:
> On Sun, Oct 13, 2013 at 3:48 AM, Yoshioka Tsuneo
> 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
>> ---
>> 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)
>> + nam
Re: [PATCH v3] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
On Sun, Oct 13, 2013 at 3:48 AM, Yoshioka Tsuneo
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
> ---
> 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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a messa
Re: [PATCH v3] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
Hi,
Yoshioka Tsuneo 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
> ---
> 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
t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
"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
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
"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
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
