Re: [PATCH v5] diff.c: keep arrow(=) on show_stats()'s shortened filename part to make rename visible.
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 (吉岡 恒夫)
yoshiokatsu...@gmail.com
On Oct 16, 2013, at 1:54 AM, 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. 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 yoshiokatsu...@gmail.com
Test-added-by: Thomas Rast tr...@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 +
[PATCH v5] 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.
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 yoshiokatsu...@gmail.com
Test-added-by: Thomas Rast tr...@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);
+
Re: [PATCH v5] diff.c: keep arrow(=) on show_stats()'s shortened filename part to make rename visible.
Yoshioka Tsuneo yoshiokatsu...@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 yoshiokatsu...@gmail.com
Test-added-by: Thomas Rast tr...@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
Re: [PATCH v5] diff.c: keep arrow(=) on show_stats()'s shortened filename part to make rename visible.
Junio C Hamano gits...@pobox.com writes:
Yoshioka Tsuneo yoshiokatsu...@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
--
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
