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