Re: [PATCH v6] diff.c: keep arrow(=) on show_stats()'s shortened filename part to make rename visible.

2013-10-18 Thread Yoshioka Tsuneo
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 (吉岡 恒夫)
yoshiokatsu...@gmail.com




On Oct 18, 2013, at 1:38 AM, Junio C Hamano gits...@pobox.com wrote:

 Yoshioka Tsuneo yoshiokatsu...@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.

--
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.

2013-10-17 Thread Junio C Hamano
Yoshioka Tsuneo yoshiokatsu...@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 

Re: [PATCH v6] diff.c: keep arrow(=) on show_stats()'s shortened filename part to make rename visible.

2013-10-17 Thread Junio C Hamano
Yoshioka Tsuneo yoshiokatsu...@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.
--
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.

2013-10-17 Thread Yoshioka Tsuneo
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 gits...@pobox.com wrote:

 Yoshioka Tsuneo yoshiokatsu...@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;
 +}
 
 

Re: [PATCH v6] diff.c: keep arrow(=) on show_stats()'s shortened filename part to make rename visible.

2013-10-17 Thread Junio C Hamano
Yoshioka Tsuneo yoshiokatsu...@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.
--
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