Re: [PATCH v6] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
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 part(lator than '}'). I just sent the updated patch as "[PATCH v8]". Thanks ! --- Tsuneo Yoshioka (吉岡 恒夫) yoshiokatsu...@gmail.com On Oct 18, 2013, at 1:38 AM, Junio C Hamano wrote: > Yoshioka Tsuneo 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. -- 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 v6] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
Yoshioka Tsuneo 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. -- 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 v6] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
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 (吉岡 恒夫) yoshiokatsu...@gmail.com On Oct 17, 2013, at 10:29 PM, Junio C Hamano wrote: > Yoshioka Tsuneo 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 "{ => }" >> To keep arrow("=>"), try to omit as long as possible at first >> because later part or changing part will be the more important part. >> If it is not enough, shorten , , and 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" >> + * T
Re: [PATCH v6] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
Yoshioka Tsuneo 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. -- 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 v6] diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
Yoshioka Tsuneo 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 "{ => }" > To keep arrow("=>"), try to omit as long as possible at first > because later part or changing part will be the more important part. > If it is not enough, shorten , , and 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_d
[PATCH v6] 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". 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 "{ => }" To keep arrow("=>"), try to omit as long as possible at first because later part or changing part will be the more important part. If it is not enough, shorten , , and trying to have the maximum length the same because those will be equally important. Signed-off-by: Tsuneo Yoshioka Test-added-by: Thomas Rast --- 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 "{ => }". + * At first, omit as long as possible. + * If it is not enough, omit , , 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 +