[PATCH v3 0/2] diff: add --stat-with-summary (was --compact-summary)

[PATCH v3 0/2] diff: add --stat-with-summary (was --compact-summary)

[Nguyễn Thái Ngọc Duy]

Changes since v2 [1]:

- goes back to my original version (yay!) where the extra info
  is appended after the path name. More is described in 2/2
- --compact-summary is now renamed --stat-with-summary and implies
  --stat
- 1/2 is just a cleanup patch to make it easier to add 2/2

[1] https://public-inbox.org/git/20180118100546.32251-1-pclouds@gmail.com/

Nguyễn Thái Ngọc Duy (2):
  diff.c: refactor pprint_rename() to use strbuf
  diff: add --stat-with-summary

 Documentation/diff-options.txt                     |   8 ++
 diff.c                                             | 101 ++++++++++++++-------
 diff.h                                             |   1 +
 t/t4013-diff-various.sh                            |   5 +
 ...pretty_--root_--stat-with-summary_initial (new) |  12 +++
 ...tty_-R_--root_--stat-with-summary_initial (new) |  12 +++
 ...iff-tree_--stat-with-summary_initial_mode (new) |   4 +
 ...-tree_-R_--stat-with-summary_initial_mode (new) |   4 +
 8 files changed, 113 insertions(+), 34 deletions(-)

-- 
2.16.1.75.ga05e3333b4

[PATCH v3 1/2] diff.c: refactor pprint_rename() to use strbuf

[Nguyễn Thái Ngọc Duy]

Instead of passing char* around, let function handle strbuf
directly. All callers already use strbuf internally.

This helps kill the "not free" exception in free_diffstat_info(). I
don't think this code is so critical that we need to avoid some free()
calls.

The other benefit comes in the next patch, where we append something
in pname before returning from fill_print_name(). With strbuf, it's
very simple. With "char *" we may have to resort to explicit
reallocation and stuff.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 59 ++++++++++++++++++++++++++--------------------------------
 1 file changed, 26 insertions(+), 33 deletions(-)

diff --git a/diff.c b/diff.c
index 0a9a0cdf18..9d874a670f 100644
--- a/diff.c
+++ b/diff.c
@@ -2045,11 +2045,10 @@ 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(struct strbuf *name, const char *a, const char *b)
 {
 	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);
@@ -2059,10 +2058,10 @@ 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, name, NULL, 0);
+		strbuf_addstr(name, " => ");
+		quote_c_style(b, name, NULL, 0);
+		return;
 	}
 
 	/* Find common prefix */
@@ -2109,19 +2108,18 @@ 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);
+	strbuf_grow(name, pfx_length + a_midlen + b_midlen + sfx_length + 7);
 	if (pfx_length + sfx_length) {
-		strbuf_add(&name, a, pfx_length);
-		strbuf_addch(&name, '{');
+		strbuf_add(name, a, pfx_length);
+		strbuf_addch(name, '{');
 	}
-	strbuf_add(&name, a + pfx_length, a_midlen);
-	strbuf_addstr(&name, " => ");
-	strbuf_add(&name, b + pfx_length, b_midlen);
+	strbuf_add(name, a + pfx_length, a_midlen);
+	strbuf_addstr(name, " => ");
+	strbuf_add(name, b + pfx_length, b_midlen);
 	if (pfx_length + sfx_length) {
-		strbuf_addch(&name, '}');
-		strbuf_add(&name, a + len_a - sfx_length, sfx_length);
+		strbuf_addch(name, '}');
+		strbuf_add(name, a + len_a - sfx_length, sfx_length);
 	}
-	return strbuf_detach(&name, NULL);
 }
 
 struct diffstat_t {
@@ -2197,23 +2195,17 @@ static void show_graph(struct strbuf *out, char ch, int cnt,
 
 static void fill_print_name(struct diffstat_file *file)
 {
-	char *pname;
+	struct strbuf pname = STRBUF_INIT;
 
 	if (file->print_name)
 		return;
 
-	if (!file->is_renamed) {
-		struct strbuf buf = STRBUF_INIT;
-		if (quote_c_style(file->name, &buf, NULL, 0)) {
-			pname = strbuf_detach(&buf, NULL);
-		} else {
-			pname = file->name;
-			strbuf_release(&buf);
-		}
-	} else {
-		pname = pprint_rename(file->from_name, file->name);
-	}
-	file->print_name = pname;
+	if (file->is_renamed)
+		pprint_rename(&pname, file->from_name, file->name);
+	else
+		quote_c_style(file->name, &pname, NULL, 0);
+
+	file->print_name = strbuf_detach(&pname, NULL);
 }
 
 static void print_stat_summary_inserts_deletes(struct diff_options *options,
@@ -2797,8 +2789,7 @@ static void free_diffstat_info(struct diffstat_t *diffstat)
 	int i;
 	for (i = 0; i < diffstat->nr; i++) {
 		struct diffstat_file *f = diffstat->files[i];
-		if (f->name != f->print_name)
-			free(f->print_name);
+		free(f->print_name);
 		free(f->name);
 		free(f->from_name);
 		free(f);
@@ -5241,10 +5232,12 @@ static void show_rename_copy(struct diff_options *opt, const char *renamecopy,
 		struct diff_filepair *p)
 {
 	struct strbuf sb = STRBUF_INIT;
-	char *names = pprint_rename(p->one->path, p->two->path);
+	struct strbuf names = STRBUF_INIT;
+
+	pprint_rename(&names, p->one->path, p->two->path);
 	strbuf_addf(&sb, " %s %s (%d%%)\n",
-			renamecopy, names, similarity_index(p));
-	free(names);
+		    renamecopy, names.buf, similarity_index(p));
+	strbuf_release(&names);
 	emit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY,
 				 sb.buf, sb.len, 0);
 	show_mode_change(opt, p, 0);
-- 
2.16.1.75.ga05e3333b4

[PATCH v3 2/2] diff: add --stat-with-summary

[Nguyễn Thái Ngọc Duy]

Certain information is currently shown with --summary, but when used
in combination with --stat it's a bit hard to read since info of the
same file is in two places (--stat and --summary).

On top of that, commits that add or remove files double the number of
display lines, which could be a lot if you add or remove a lot of
files.

--stat-with-summary embeds most of --summary back in --stat in the
little space between the file name part and the graph line, e.g. with
commit 0433d533f1:

   Documentation/merge-config.txt         |  4 +
   builtin/merge.c                        |  2 +
   ...-pull-verify-signatures.sh (new +x) | 81 ++++++++++++++
   t/t7612-merge-verify-signatures.sh     | 45 ++++++++
   4 files changed, 132 insertions(+)

It helps both condensing information and saving some text
space. What's new in diffstat is:

- A new 0644 file is shown as (new)
- A new 0755 file is shown as (new +x)
- A new symlink is shown as (new +l)
- A deleted file is shown as (gone)
- A mode change adding executable bit is shown as (mode +x)
- A mode change removing it is shown as (mode -x)

Note that --stat-with-summary does not contain all the information
--summary provides. Rewrite percentage is not shown but it could be
added later, like R50% or C20%.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/diff-options.txt                |  8 ++++
 diff.c                                        | 42 ++++++++++++++++++-
 diff.h                                        |  1 +
 t/t4013-diff-various.sh                       |  5 +++
 ...-pretty_--root_--stat-with-summary_initial | 12 ++++++
 ...etty_-R_--root_--stat-with-summary_initial | 12 ++++++
 ...diff-tree_--stat-with-summary_initial_mode |  4 ++
 ...f-tree_-R_--stat-with-summary_initial_mode |  4 ++
 8 files changed, 87 insertions(+), 1 deletion(-)
 create mode 100644 t/t4013/diff.diff-tree_--pretty_--root_--stat-with-summary_initial
 create mode 100644 t/t4013/diff.diff-tree_--pretty_-R_--root_--stat-with-summary_initial
 create mode 100644 t/t4013/diff.diff-tree_--stat-with-summary_initial_mode
 create mode 100644 t/t4013/diff.diff-tree_-R_--stat-with-summary_initial_mode

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index c330c01ff0..595e4cd548 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -128,6 +128,14 @@ have to use `--diff-algorithm=default` option.
 These parameters can also be set individually with `--stat-width=<width>`,
 `--stat-name-width=<name-width>` and `--stat-count=<count>`.
 
+--stat-with-summary::
+	Output a condensed summary of extended header information such
+	as file creations or deletions ("new" or "gone", optionally "+l"
+	if it's a symlink) and mode changes ("+x" or "-x" for adding
+	or removing executable bit respectively) in diffstat. The
+	information is put betwen the filename part and the graph
+	part. Implies `--stat`.
+
 --numstat::
 	Similar to `--stat`, but shows number of added and
 	deleted lines in decimal notation and pathname without
diff --git a/diff.c b/diff.c
index 9d874a670f..6bf9867388 100644
--- a/diff.c
+++ b/diff.c
@@ -2129,6 +2129,7 @@ struct diffstat_t {
 		char *from_name;
 		char *name;
 		char *print_name;
+		const char *comments;
 		unsigned is_unmerged:1;
 		unsigned is_binary:1;
 		unsigned is_renamed:1;
@@ -2205,6 +2206,9 @@ static void fill_print_name(struct diffstat_file *file)
 	else
 		quote_c_style(file->name, &pname, NULL, 0);
 
+	if (file->comments)
+		strbuf_addf(&pname, " (%s)", file->comments);
+
 	file->print_name = strbuf_detach(&pname, NULL);
 }
 
@@ -3239,6 +3243,32 @@ static void builtin_diff(const char *name_a,
 	return;
 }
 
+static char *get_compact_summary(const struct diff_filepair *p, int is_renamed)
+{
+	if (!is_renamed) {
+		if (p->status == DIFF_STATUS_ADDED) {
+			if (S_ISLNK(p->two->mode))
+				return "new +l";
+			else if ((p->two->mode & 0777) == 0755)
+				return "new +x";
+			else
+				return "new";
+		} else if (p->status == DIFF_STATUS_DELETED)
+			return "gone";
+	}
+	if (S_ISLNK(p->one->mode) && !S_ISLNK(p->two->mode))
+		return "mode -l";
+	else if (!S_ISLNK(p->one->mode) && S_ISLNK(p->two->mode))
+		return "mode +l";
+	else if ((p->one->mode & 0777) == 0644 &&
+		 (p->two->mode & 0777) == 0755)
+		return "mode +x";
+	else if ((p->one->mode & 0777) == 0755 &&
+		 (p->two->mode & 0777) == 0644)
+		return "mode -x";
+	return NULL;
+}
+
 static void builtin_diffstat(const char *name_a, const char *name_b,
 			     struct diff_filespec *one,
 			     struct diff_filespec *two,
@@ -3258,6 +3288,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
 
 	data = diffstat_add(diffstat, name_a, name_b);
 	data->is_interesting = p->status != DIFF_STATUS_UNKNOWN;
+	if (o->flags.stat_with_summary)
+		data->comments = get_compact_summary(p, data->is_renamed);
 
 	if (!one || !two) {
 		data->is_unmerged = 1;
@@ -4300,6 +4332,7 @@ static int stat_opt(struct diff_options *options, const char **av)
 	int graph_width = options->stat_graph_width;
 	int count = options->stat_count;
 	int argcount = 1;
+	unsigned with_summary = options->flags.stat_with_summary;
 
 	if (!skip_prefix(arg, "--stat", &arg))
 		die("BUG: stat option does not begin with --stat: %s", arg);
@@ -4343,6 +4376,9 @@ static int stat_opt(struct diff_options *options, const char **av)
 				count = strtoul(av[1], &end, 10);
 				argcount = 2;
 			}
+		} else if (skip_prefix(arg, "-with-summary", &arg)) {
+			with_summary = 1;
+			end = (char*)arg;
 		}
 		break;
 	case '=':
@@ -4361,6 +4397,7 @@ static int stat_opt(struct diff_options *options, const char **av)
 	options->stat_graph_width = graph_width;
 	options->stat_width = width;
 	options->stat_count = count;
+	options->flags.stat_with_summary = with_summary;
 	return argcount;
 }
 
@@ -4542,7 +4579,10 @@ int diff_opt_parse(struct diff_options *options,
 	else if (!strcmp(arg, "-s") || !strcmp(arg, "--no-patch"))
 		options->output_format |= DIFF_FORMAT_NO_OUTPUT;
 	else if (starts_with(arg, "--stat"))
-		/* --stat, --stat-width, --stat-name-width, or --stat-count */
+		/*
+		 * --stat, --stat-width, --stat-name-width,
+		 * --stat-count or --stat-with-summary.
+		 */
 		return stat_opt(options, av);
 
 	/* renames options */
diff --git a/diff.h b/diff.h
index 6bd278aac1..d29560f822 100644
--- a/diff.h
+++ b/diff.h
@@ -93,6 +93,7 @@ struct diff_flags {
 	unsigned dirstat_by_line:1;
 	unsigned funccontext:1;
 	unsigned default_follow_renames:1;
+	unsigned stat_with_summary:1;
 };
 
 static inline void diff_flags_or(struct diff_flags *a,
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index f10798b2df..aa6f5da21c 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -361,6 +361,11 @@ diff --no-index --raw dir2 dir
 diff --no-index --raw --abbrev=4 dir2 dir
 :noellipses diff --no-index --raw --abbrev=4 dir2 dir
 diff --no-index --raw --no-abbrev dir2 dir
+
+diff-tree --pretty --root --stat-with-summary initial
+diff-tree --pretty -R --root --stat-with-summary initial
+diff-tree --stat-with-summary initial mode
+diff-tree -R --stat-with-summary initial mode
 EOF
 
 test_expect_success 'log -S requires an argument' '
diff --git a/t/t4013/diff.diff-tree_--pretty_--root_--stat-with-summary_initial b/t/t4013/diff.diff-tree_--pretty_--root_--stat-with-summary_initial
new file mode 100644
index 0000000000..105f29a92d
--- /dev/null
+++ b/t/t4013/diff.diff-tree_--pretty_--root_--stat-with-summary_initial
@@ -0,0 +1,12 @@
+$ git diff-tree --pretty --root --stat-with-summary initial
+commit 444ac553ac7612cc88969031b02b3767fb8a353a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:00:00 2006 +0000
+
+    Initial
+
+ dir/sub (new) | 2 ++
+ file0 (new)   | 3 +++
+ file2 (new)   | 3 +++
+ 3 files changed, 8 insertions(+)
+$
diff --git a/t/t4013/diff.diff-tree_--pretty_-R_--root_--stat-with-summary_initial b/t/t4013/diff.diff-tree_--pretty_-R_--root_--stat-with-summary_initial
new file mode 100644
index 0000000000..45008d09fc
--- /dev/null
+++ b/t/t4013/diff.diff-tree_--pretty_-R_--root_--stat-with-summary_initial
@@ -0,0 +1,12 @@
+$ git diff-tree --pretty -R --root --stat-with-summary initial
+commit 444ac553ac7612cc88969031b02b3767fb8a353a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:00:00 2006 +0000
+
+    Initial
+
+ dir/sub (gone) | 2 --
+ file0 (gone)   | 3 ---
+ file2 (gone)   | 3 ---
+ 3 files changed, 8 deletions(-)
+$
diff --git a/t/t4013/diff.diff-tree_--stat-with-summary_initial_mode b/t/t4013/diff.diff-tree_--stat-with-summary_initial_mode
new file mode 100644
index 0000000000..f99fcdc101
--- /dev/null
+++ b/t/t4013/diff.diff-tree_--stat-with-summary_initial_mode
@@ -0,0 +1,4 @@
+$ git diff-tree --stat-with-summary initial mode
+ file0 (mode +x) | 0
+ 1 file changed, 0 insertions(+), 0 deletions(-)
+$
diff --git a/t/t4013/diff.diff-tree_-R_--stat-with-summary_initial_mode b/t/t4013/diff.diff-tree_-R_--stat-with-summary_initial_mode
new file mode 100644
index 0000000000..8dc8f3fe95
--- /dev/null
+++ b/t/t4013/diff.diff-tree_-R_--stat-with-summary_initial_mode
@@ -0,0 +1,4 @@
+$ git diff-tree -R --stat-with-summary initial mode
+ file0 (mode -x) | 0
+ 1 file changed, 0 insertions(+), 0 deletions(-)
+$
-- 
2.16.1.75.ga05e3333b4

Re: [PATCH v3 0/2] diff: add --stat-with-summary (was --compact-summary)

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Changes since v2 [1]:
>
> - goes back to my original version (yay!) where the extra info
>   is appended after the path name. More is described in 2/2
> - --compact-summary is now renamed --stat-with-summary and implies
>   --stat
> - 1/2 is just a cleanup patch to make it easier to add 2/2

It may be just me and other old timers, but --X-with-Y naming means
quite different thing around these commands, and --stat-with-summary
would hint, at least to us, that it would behave as if the two
options "--stat --summary" are given at the same time.

And from that point of view, the new name is a bit confusing one.

The patches themselves look excellent.  Thanks.

Re: [PATCH v3 0/2] diff: add --stat-with-summary (was --compact-summary)

[Duy Nguyen]

On Sat, Feb 3, 2018 at 2:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Changes since v2 [1]:
>>
>> - goes back to my original version (yay!) where the extra info
>>   is appended after the path name. More is described in 2/2
>> - --compact-summary is now renamed --stat-with-summary and implies
>>   --stat
>> - 1/2 is just a cleanup patch to make it easier to add 2/2
>
> It may be just me and other old timers, but --X-with-Y naming means
> quite different thing around these commands, and --stat-with-summary
> would hint, at least to us, that it would behave as if the two
> options "--stat --summary" are given at the same time.
>
> And from that point of view, the new name is a bit confusing one.

I don't have any good alternative name to be honest. It's kinda hard
to come up with another word that says "extended header information
such as creations, renames and mode changes", except maybe the vague
name --stat-extended?
-- 
Duy

Re: [PATCH v3 0/2] diff: add --stat-with-summary (was --compact-summary)

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Sat, Feb 3, 2018 at 2:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>
>>> Changes since v2 [1]:
>>>
>>> - goes back to my original version (yay!) where the extra info
>>>   is appended after the path name. More is described in 2/2
>>> - --compact-summary is now renamed --stat-with-summary and implies
>>>   --stat
>>> - 1/2 is just a cleanup patch to make it easier to add 2/2
>>
>> It may be just me and other old timers, but --X-with-Y naming means
>> quite different thing around these commands, and --stat-with-summary
>> would hint, at least to us, that it would behave as if the two
>> options "--stat --summary" are given at the same time.
>>
>> And from that point of view, the new name is a bit confusing one.
>
> I don't have any good alternative name to be honest. It's kinda hard
> to come up with another word that says "extended header information
> such as creations, renames and mode changes", except maybe the vague
> name --stat-extended?

I actually think compact-summary was a good way to phrase it.

Personally, I think it was a UI mistake that --summary can be given
independently with or without --stat (instead, there shouldn't have
been the --summary option, and instead when it was added, --stat
just should have gained an extra kind of output).  A single option
that can give both kinds of info may be a good way forward, so
another possibility may be --summary-in-stat (meaning: the info
given by summary is included in stat output).  I dunno.

Re: [PATCH v3 0/2] diff: add --stat-with-summary (was --compact-summary)

[Duy Nguyen]

On Tue, Feb 6, 2018 at 1:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Sat, Feb 3, 2018 at 2:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>>
>>>> Changes since v2 [1]:
>>>>
>>>> - goes back to my original version (yay!) where the extra info
>>>>   is appended after the path name. More is described in 2/2
>>>> - --compact-summary is now renamed --stat-with-summary and implies
>>>>   --stat
>>>> - 1/2 is just a cleanup patch to make it easier to add 2/2
>>>
>>> It may be just me and other old timers, but --X-with-Y naming means
>>> quite different thing around these commands, and --stat-with-summary
>>> would hint, at least to us, that it would behave as if the two
>>> options "--stat --summary" are given at the same time.
>>>
>>> And from that point of view, the new name is a bit confusing one.
>>
>> I don't have any good alternative name to be honest. It's kinda hard
>> to come up with another word that says "extended header information
>> such as creations, renames and mode changes", except maybe the vague
>> name --stat-extended?
>
> I actually think compact-summary was a good way to phrase it.
>
> Personally, I think it was a UI mistake that --summary can be given
> independently with or without --stat (instead, there shouldn't have
> been the --summary option, and instead when it was added, --stat
> just should have gained an extra kind of output).  A single option
> that can give both kinds of info may be a good way forward, so
> another possibility may be --summary-in-stat (meaning: the info
> given by summary is included in stat output).  I dunno.
>

+Eric maybe he has some idea (sorry I forgot to include people from
the last round).
-- 
Duy

Re: [PATCH v3 0/2] diff: add --stat-with-summary (was --compact-summary)

[Eric Sunshine]

On Tue, Feb 6, 2018 at 5:20 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Tue, Feb 6, 2018 at 1:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Duy Nguyen <pclouds@gmail.com> writes:
>> I actually think compact-summary was a good way to phrase it.
>>
>> Personally, I think it was a UI mistake that --summary can be given
>> independently with or without --stat (instead, there shouldn't have
>> been the --summary option, and instead when it was added, --stat
>> just should have gained an extra kind of output).  A single option
>> that can give both kinds of info may be a good way forward, so
>> another possibility may be --summary-in-stat (meaning: the info
>> given by summary is included in stat output).  I dunno.
>
> +Eric maybe he has some idea (sorry I forgot to include people from
> the last round).

What about the earlier suggestion[1] (and minor follow-ups[2,3]) of
making this another option to --stat= (for instance, --stat=compact)?
Did that idea get shot down or am I misunderstanding the question
here.

[1]: https://public-inbox.org/git/CAPig+cQLgs59JYxcmK30qY307ArwqJx6pNOo95Z39_jJ9+D6+g@mail.gmail.com/
[2]: https://public-inbox.org/git/CACsJy8B5qrN8T1aai3y3nfEc5baqn2Xkk6vZozMp5Lh-mPZ0VQ@mail.gmail.com/
[3]: https://public-inbox.org/git/CACsJy8CPHk+aXHr-mkHZi27s=c3+ny8D9CSuzOSO8PweviBcqQ@mail.gmail.com/

Re: [PATCH v3 0/2] diff: add --stat-with-summary (was --compact-summary)

[Duy Nguyen]

On Wed, Feb 7, 2018 at 4:52 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Feb 6, 2018 at 5:20 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Tue, Feb 6, 2018 at 1:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Duy Nguyen <pclouds@gmail.com> writes:
>>> I actually think compact-summary was a good way to phrase it.
>>>
>>> Personally, I think it was a UI mistake that --summary can be given
>>> independently with or without --stat (instead, there shouldn't have
>>> been the --summary option, and instead when it was added, --stat
>>> just should have gained an extra kind of output).  A single option
>>> that can give both kinds of info may be a good way forward, so
>>> another possibility may be --summary-in-stat (meaning: the info
>>> given by summary is included in stat output).  I dunno.
>>
>> +Eric maybe he has some idea (sorry I forgot to include people from
>> the last round).
>
> What about the earlier suggestion[1] (and minor follow-ups[2,3]) of
> making this another option to --stat= (for instance, --stat=compact)?
> Did that idea get shot down or am I misunderstanding the question
> here.

I thought that was something like
--stat[=<width>[,<name-width>[,<count>,[compact]]]] and turning on
"compact" alone would get awkward because you need to specify all
those widths and counts too. --stat=compact as a separate form, with
no combination with any other stat params, does not feel justified. We
could just do --stat-compact then. Perhaps we can allow compact to
appear anywhere in --stat=, and not just the end? The --stat= syntax
would be

--stat=[<option>[,<option>[,<option>...]]]

where option could be keyword ones like compact or anything else in
future, or <keyword>=<value> form. <option> could also be a number,
but in that case the three consecutive number options will present
width, name-width and count in this order.

Or we could simply add new --stat= syntax _without_ "<option> as
numbers". widths and counts must be specified keywords as well, e.g.
--stat=width=40,name-width=20,count=10,compact and leave the old
syntax "--stat=<width>,<name-width>,<count>" alone.

Then we still need to decide the new keyword for this feature, I feel
compact is a bit too vague (I read --stat=compact as "it's compact
stat", not "stat with compact summary"), so perhaps
--stat=compact-summary, or just --stat=summary?

> [1]: https://public-inbox.org/git/CAPig+cQLgs59JYxcmK30qY307ArwqJx6pNOo95Z39_jJ9+D6+g@mail.gmail.com/
> [2]: https://public-inbox.org/git/CACsJy8B5qrN8T1aai3y3nfEc5baqn2Xkk6vZozMp5Lh-mPZ0VQ@mail.gmail.com/
> [3]: https://public-inbox.org/git/CACsJy8CPHk+aXHr-mkHZi27s=c3+ny8D9CSuzOSO8PweviBcqQ@mail.gmail.com/
-- 
Duy

Re: [PATCH v3 0/2] diff: add --stat-with-summary (was --compact-summary)

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> ...
> Then we still need to decide the new keyword for this feature, I feel
> compact is a bit too vague (I read --stat=compact as "it's compact
> stat", not "stat with compact summary"), so perhaps
> --stat=compact-summary, or just --stat=summary?

Yup, this is about giving summary in a compact way, not about giving
a compact stat information.  I agree with all the above reasoning,
and that is why I said that your "compact-summary" was a good way to
refer to the feature.

Re: [PATCH v3 0/2] diff: add --stat-with-summary (was --compact-summary)

[Duy Nguyen]

On Thu, Feb 8, 2018 at 1:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> ...
>> Then we still need to decide the new keyword for this feature, I feel
>> compact is a bit too vague (I read --stat=compact as "it's compact
>> stat", not "stat with compact summary"), so perhaps
>> --stat=compact-summary, or just --stat=summary?
>
> Yup, this is about giving summary in a compact way, not about giving
> a compact stat information.  I agree with all the above reasoning,
> and that is why I said that your "compact-summary" was a good way to
> refer to the feature.

OK I'll wait for a few days. If there's no comment, I'll go with
--stat=compact-summary.
-- 
Duy

Re: [PATCH v3 0/2] diff: add --stat-with-summary (was --compact-summary)

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

>> Yup, this is about giving summary in a compact way, not about giving
>> a compact stat information.  I agree with all the above reasoning,
>> and that is why I said that your "compact-summary" was a good way to
>> refer to the feature.
>
> OK I'll wait for a few days. If there's no comment, I'll go with
> --stat=compact-summary.

Sorry, but that is not what I agreed with you on ;-) I meant to say
that your earlier --compact-summary made sense.  I do not think
"compact summary" as a value of "--stat" makes any sense.  It's not
like this new one is one of the ways we offer to present "stat"
differently and compared to other existing ways this is to give the
stat in a compactly summarized fashion.  If this were a value to
"--summary", then "--summary=in-stat" might make sense, in that this
is not about how we show the "stat" information but about how/where
"summary" information is shown.

[PATCH v4 0/2] diff: add --compact-summary (aka nd/diff-stat-with-summary)

[Nguyễn Thái Ngọc Duy]

v4 renames the option back to --compact-summary. I can't think of any
better name.

Interdiff

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 595e4cd548..e3a44f03cd 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -128,7 +128,7 @@ have to use `--diff-algorithm=default` option.
 These parameters can also be set individually with `--stat-width=<width>`,
 `--stat-name-width=<name-width>` and `--stat-count=<count>`.
 
---stat-with-summary::
+--compact-summary::
 	Output a condensed summary of extended header information such
 	as file creations or deletions ("new" or "gone", optionally "+l"
 	if it's a symlink) and mode changes ("+x" or "-x" for adding
diff --git a/diff.c b/diff.c
index e7ff7dceb7..62e413a80f 100644
--- a/diff.c
+++ b/diff.c
@@ -4332,7 +4332,6 @@ static int stat_opt(struct diff_options *options, const char **av)
 	int graph_width = options->stat_graph_width;
 	int count = options->stat_count;
 	int argcount = 1;
-	unsigned with_summary = options->flags.stat_with_summary;
 
 	if (!skip_prefix(arg, "--stat", &arg))
 		die("BUG: stat option does not begin with --stat: %s", arg);
@@ -4376,9 +4375,6 @@ static int stat_opt(struct diff_options *options, const char **av)
 				count = strtoul(av[1], &end, 10);
 				argcount = 2;
 			}
-		} else if (skip_prefix(arg, "-with-summary", &arg)) {
-			with_summary = 1;
-			end = (char*)arg;
 		}
 		break;
 	case '=':
@@ -4397,7 +4393,6 @@ static int stat_opt(struct diff_options *options, const char **av)
 	options->stat_graph_width = graph_width;
 	options->stat_width = width;
 	options->stat_count = count;
-	options->flags.stat_with_summary = with_summary;
 	return argcount;
 }
 
@@ -4579,11 +4574,13 @@ int diff_opt_parse(struct diff_options *options,
 	else if (!strcmp(arg, "-s") || !strcmp(arg, "--no-patch"))
 		options->output_format |= DIFF_FORMAT_NO_OUTPUT;
 	else if (starts_with(arg, "--stat"))
-		/*
-		 * --stat, --stat-width, --stat-name-width,
-		 * --stat-count or --stat-with-summary.
-		 */
+		/* --stat, --stat-width, --stat-name-width, or --stat-count */
 		return stat_opt(options, av);
+	else if (!strcmp(arg, "--compact-summary")) {
+		 options->flags.stat_with_summary = 1;
+		 options->output_format |= DIFF_FORMAT_DIFFSTAT;
+	} else if (!strcmp(arg, "--no-compact-summary"))
+		 options->flags.stat_with_summary = 0;
 
 	/* renames options */
 	else if (starts_with(arg, "-B") ||
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index aa6f5da21c..3f9a24fd56 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -362,10 +362,10 @@ diff --no-index --raw --abbrev=4 dir2 dir
 :noellipses diff --no-index --raw --abbrev=4 dir2 dir
 diff --no-index --raw --no-abbrev dir2 dir
 
-diff-tree --pretty --root --stat-with-summary initial
-diff-tree --pretty -R --root --stat-with-summary initial
-diff-tree --stat-with-summary initial mode
-diff-tree -R --stat-with-summary initial mode
+diff-tree --pretty --root --stat --compact-summary initial
+diff-tree --pretty -R --root --stat --compact-summary initial
+diff-tree --stat --compact-summary initial mode
+diff-tree -R --stat --compact-summary initial mode
 EOF
 
 test_expect_success 'log -S requires an argument' '
diff --git a/t/t4013/diff.diff-tree_--pretty_--root_--stat-with-summary_initial b/t/t4013/diff.diff-tree_--pretty_--root_--stat_--compact-summary_initial
similarity index 78%
rename from t/t4013/diff.diff-tree_--pretty_--root_--stat-with-summary_initial
rename to t/t4013/diff.diff-tree_--pretty_--root_--stat_--compact-summary_initial
index 105f29a92d..d6451ff7cc 100644
--- a/t/t4013/diff.diff-tree_--pretty_--root_--stat-with-summary_initial
+++ b/t/t4013/diff.diff-tree_--pretty_--root_--stat_--compact-summary_initial
@@ -1,4 +1,4 @@
-$ git diff-tree --pretty --root --stat-with-summary initial
+$ git diff-tree --pretty --root --stat --compact-summary initial
 commit 444ac553ac7612cc88969031b02b3767fb8a353a
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:00:00 2006 +0000
diff --git a/t/t4013/diff.diff-tree_--pretty_-R_--root_--stat-with-summary_initial b/t/t4013/diff.diff-tree_--pretty_-R_--root_--stat_--compact-summary_initial
similarity index 78%
rename from t/t4013/diff.diff-tree_--pretty_-R_--root_--stat-with-summary_initial
rename to t/t4013/diff.diff-tree_--pretty_-R_--root_--stat_--compact-summary_initial
index 45008d09fc..1989e55cd0 100644
--- a/t/t4013/diff.diff-tree_--pretty_-R_--root_--stat-with-summary_initial
+++ b/t/t4013/diff.diff-tree_--pretty_-R_--root_--stat_--compact-summary_initial
@@ -1,4 +1,4 @@
-$ git diff-tree --pretty -R --root --stat-with-summary initial
+$ git diff-tree --pretty -R --root --stat --compact-summary initial
 commit 444ac553ac7612cc88969031b02b3767fb8a353a
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:00:00 2006 +0000
diff --git a/t/t4013/diff.diff-tree_--stat-with-summary_initial_mode b/t/t4013/diff.diff-tree_--stat_--compact-summary_initial_mode
similarity index 57%
rename from t/t4013/diff.diff-tree_--stat-with-summary_initial_mode
rename to t/t4013/diff.diff-tree_--stat_--compact-summary_initial_mode
index f99fcdc101..9c7c8f63af 100644
--- a/t/t4013/diff.diff-tree_--stat-with-summary_initial_mode
+++ b/t/t4013/diff.diff-tree_--stat_--compact-summary_initial_mode
@@ -1,4 +1,4 @@
-$ git diff-tree --stat-with-summary initial mode
+$ git diff-tree --stat --compact-summary initial mode
  file0 (mode +x) | 0
  1 file changed, 0 insertions(+), 0 deletions(-)
 $
diff --git a/t/t4013/diff.diff-tree_-R_--stat-with-summary_initial_mode b/t/t4013/diff.diff-tree_-R_--stat_--compact-summary_initial_mode
similarity index 55%
rename from t/t4013/diff.diff-tree_-R_--stat-with-summary_initial_mode
rename to t/t4013/diff.diff-tree_-R_--stat_--compact-summary_initial_mode
index 8dc8f3fe95..e38f3d3bfb 100644
--- a/t/t4013/diff.diff-tree_-R_--stat-with-summary_initial_mode
+++ b/t/t4013/diff.diff-tree_-R_--stat_--compact-summary_initial_mode
@@ -1,4 +1,4 @@
-$ git diff-tree -R --stat-with-summary initial mode
+$ git diff-tree -R --stat --compact-summary initial mode
  file0 (mode -x) | 0
  1 file changed, 0 insertions(+), 0 deletions(-)
 $
-- 
2.16.1.435.g8f24da2e1a

[PATCH v4 1/2] diff.c: refactor pprint_rename() to use strbuf

[Nguyễn Thái Ngọc Duy]

Instead of passing char* around, let function handle strbuf
directly. All callers already use strbuf internally.

This helps kill the "not free" exception in free_diffstat_info(). I
don't think this code is so critical that we need to avoid some free()
calls.

The other benefit comes in the next patch, where we append something
in pname before returning from fill_print_name(). With strbuf, it's
very simple. With "char *" we may have to resort to explicit
reallocation and stuff.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 59 ++++++++++++++++++++++++++--------------------------------
 1 file changed, 26 insertions(+), 33 deletions(-)

diff --git a/diff.c b/diff.c
index 21c3838b25..e3f72de27d 100644
--- a/diff.c
+++ b/diff.c
@@ -2045,11 +2045,10 @@ 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(struct strbuf *name, const char *a, const char *b)
 {
 	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);
@@ -2059,10 +2058,10 @@ 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, name, NULL, 0);
+		strbuf_addstr(name, " => ");
+		quote_c_style(b, name, NULL, 0);
+		return;
 	}
 
 	/* Find common prefix */
@@ -2109,19 +2108,18 @@ 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);
+	strbuf_grow(name, pfx_length + a_midlen + b_midlen + sfx_length + 7);
 	if (pfx_length + sfx_length) {
-		strbuf_add(&name, a, pfx_length);
-		strbuf_addch(&name, '{');
+		strbuf_add(name, a, pfx_length);
+		strbuf_addch(name, '{');
 	}
-	strbuf_add(&name, a + pfx_length, a_midlen);
-	strbuf_addstr(&name, " => ");
-	strbuf_add(&name, b + pfx_length, b_midlen);
+	strbuf_add(name, a + pfx_length, a_midlen);
+	strbuf_addstr(name, " => ");
+	strbuf_add(name, b + pfx_length, b_midlen);
 	if (pfx_length + sfx_length) {
-		strbuf_addch(&name, '}');
-		strbuf_add(&name, a + len_a - sfx_length, sfx_length);
+		strbuf_addch(name, '}');
+		strbuf_add(name, a + len_a - sfx_length, sfx_length);
 	}
-	return strbuf_detach(&name, NULL);
 }
 
 struct diffstat_t {
@@ -2197,23 +2195,17 @@ static void show_graph(struct strbuf *out, char ch, int cnt,
 
 static void fill_print_name(struct diffstat_file *file)
 {
-	char *pname;
+	struct strbuf pname = STRBUF_INIT;
 
 	if (file->print_name)
 		return;
 
-	if (!file->is_renamed) {
-		struct strbuf buf = STRBUF_INIT;
-		if (quote_c_style(file->name, &buf, NULL, 0)) {
-			pname = strbuf_detach(&buf, NULL);
-		} else {
-			pname = file->name;
-			strbuf_release(&buf);
-		}
-	} else {
-		pname = pprint_rename(file->from_name, file->name);
-	}
-	file->print_name = pname;
+	if (file->is_renamed)
+		pprint_rename(&pname, file->from_name, file->name);
+	else
+		quote_c_style(file->name, &pname, NULL, 0);
+
+	file->print_name = strbuf_detach(&pname, NULL);
 }
 
 static void print_stat_summary_inserts_deletes(struct diff_options *options,
@@ -2797,8 +2789,7 @@ static void free_diffstat_info(struct diffstat_t *diffstat)
 	int i;
 	for (i = 0; i < diffstat->nr; i++) {
 		struct diffstat_file *f = diffstat->files[i];
-		if (f->name != f->print_name)
-			free(f->print_name);
+		free(f->print_name);
 		free(f->name);
 		free(f->from_name);
 		free(f);
@@ -5241,10 +5232,12 @@ static void show_rename_copy(struct diff_options *opt, const char *renamecopy,
 		struct diff_filepair *p)
 {
 	struct strbuf sb = STRBUF_INIT;
-	char *names = pprint_rename(p->one->path, p->two->path);
+	struct strbuf names = STRBUF_INIT;
+
+	pprint_rename(&names, p->one->path, p->two->path);
 	strbuf_addf(&sb, " %s %s (%d%%)\n",
-			renamecopy, names, similarity_index(p));
-	free(names);
+		    renamecopy, names.buf, similarity_index(p));
+	strbuf_release(&names);
 	emit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY,
 				 sb.buf, sb.len, 0);
 	show_mode_change(opt, p, 0);
-- 
2.16.1.435.g8f24da2e1a

[PATCH v4 2/2] diff: add --stat-with-summary

[Nguyễn Thái Ngọc Duy]

Certain information is currently shown with --summary, but when used
in combination with --stat it's a bit hard to read since info of the
same file is in two places (--stat and --summary).

On top of that, commits that add or remove files double the number of
display lines, which could be a lot if you add or remove a lot of
files.

--stat-with-summary embeds most of --summary back in --stat in the
little space between the file name part and the graph line, e.g. with
commit 0433d533f1:

   Documentation/merge-config.txt         |  4 +
   builtin/merge.c                        |  2 +
   ...-pull-verify-signatures.sh (new +x) | 81 ++++++++++++++
   t/t7612-merge-verify-signatures.sh     | 45 ++++++++
   4 files changed, 132 insertions(+)

It helps both condensing information and saving some text
space. What's new in diffstat is:

- A new 0644 file is shown as (new)
- A new 0755 file is shown as (new +x)
- A new symlink is shown as (new +l)
- A deleted file is shown as (gone)
- A mode change adding executable bit is shown as (mode +x)
- A mode change removing it is shown as (mode -x)

Note that --stat-with-summary does not contain all the information
--summary provides. Rewrite percentage is not shown but it could be
added later, like R50% or C20%.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/diff-options.txt                |  8 ++++
 diff.c                                        | 37 +++++++++++++++++++
 diff.h                                        |  1 +
 t/t4013-diff-various.sh                       |  5 +++
 ...ty_--root_--stat_--compact-summary_initial | 12 ++++++
 ...-R_--root_--stat_--compact-summary_initial | 12 ++++++
 ...tree_--stat_--compact-summary_initial_mode |  4 ++
 ...e_-R_--stat_--compact-summary_initial_mode |  4 ++
 8 files changed, 83 insertions(+)
 create mode 100644 t/t4013/diff.diff-tree_--pretty_--root_--stat_--compact-summary_initial
 create mode 100644 t/t4013/diff.diff-tree_--pretty_-R_--root_--stat_--compact-summary_initial
 create mode 100644 t/t4013/diff.diff-tree_--stat_--compact-summary_initial_mode
 create mode 100644 t/t4013/diff.diff-tree_-R_--stat_--compact-summary_initial_mode

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index c330c01ff0..e3a44f03cd 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -128,6 +128,14 @@ have to use `--diff-algorithm=default` option.
 These parameters can also be set individually with `--stat-width=<width>`,
 `--stat-name-width=<name-width>` and `--stat-count=<count>`.
 
+--compact-summary::
+	Output a condensed summary of extended header information such
+	as file creations or deletions ("new" or "gone", optionally "+l"
+	if it's a symlink) and mode changes ("+x" or "-x" for adding
+	or removing executable bit respectively) in diffstat. The
+	information is put betwen the filename part and the graph
+	part. Implies `--stat`.
+
 --numstat::
 	Similar to `--stat`, but shows number of added and
 	deleted lines in decimal notation and pathname without
diff --git a/diff.c b/diff.c
index e3f72de27d..62e413a80f 100644
--- a/diff.c
+++ b/diff.c
@@ -2129,6 +2129,7 @@ struct diffstat_t {
 		char *from_name;
 		char *name;
 		char *print_name;
+		const char *comments;
 		unsigned is_unmerged:1;
 		unsigned is_binary:1;
 		unsigned is_renamed:1;
@@ -2205,6 +2206,9 @@ static void fill_print_name(struct diffstat_file *file)
 	else
 		quote_c_style(file->name, &pname, NULL, 0);
 
+	if (file->comments)
+		strbuf_addf(&pname, " (%s)", file->comments);
+
 	file->print_name = strbuf_detach(&pname, NULL);
 }
 
@@ -3239,6 +3243,32 @@ static void builtin_diff(const char *name_a,
 	return;
 }
 
+static char *get_compact_summary(const struct diff_filepair *p, int is_renamed)
+{
+	if (!is_renamed) {
+		if (p->status == DIFF_STATUS_ADDED) {
+			if (S_ISLNK(p->two->mode))
+				return "new +l";
+			else if ((p->two->mode & 0777) == 0755)
+				return "new +x";
+			else
+				return "new";
+		} else if (p->status == DIFF_STATUS_DELETED)
+			return "gone";
+	}
+	if (S_ISLNK(p->one->mode) && !S_ISLNK(p->two->mode))
+		return "mode -l";
+	else if (!S_ISLNK(p->one->mode) && S_ISLNK(p->two->mode))
+		return "mode +l";
+	else if ((p->one->mode & 0777) == 0644 &&
+		 (p->two->mode & 0777) == 0755)
+		return "mode +x";
+	else if ((p->one->mode & 0777) == 0755 &&
+		 (p->two->mode & 0777) == 0644)
+		return "mode -x";
+	return NULL;
+}
+
 static void builtin_diffstat(const char *name_a, const char *name_b,
 			     struct diff_filespec *one,
 			     struct diff_filespec *two,
@@ -3258,6 +3288,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
 
 	data = diffstat_add(diffstat, name_a, name_b);
 	data->is_interesting = p->status != DIFF_STATUS_UNKNOWN;
+	if (o->flags.stat_with_summary)
+		data->comments = get_compact_summary(p, data->is_renamed);
 
 	if (!one || !two) {
 		data->is_unmerged = 1;
@@ -4544,6 +4576,11 @@ int diff_opt_parse(struct diff_options *options,
 	else if (starts_with(arg, "--stat"))
 		/* --stat, --stat-width, --stat-name-width, or --stat-count */
 		return stat_opt(options, av);
+	else if (!strcmp(arg, "--compact-summary")) {
+		 options->flags.stat_with_summary = 1;
+		 options->output_format |= DIFF_FORMAT_DIFFSTAT;
+	} else if (!strcmp(arg, "--no-compact-summary"))
+		 options->flags.stat_with_summary = 0;
 
 	/* renames options */
 	else if (starts_with(arg, "-B") ||
diff --git a/diff.h b/diff.h
index 6bd278aac1..d29560f822 100644
--- a/diff.h
+++ b/diff.h
@@ -93,6 +93,7 @@ struct diff_flags {
 	unsigned dirstat_by_line:1;
 	unsigned funccontext:1;
 	unsigned default_follow_renames:1;
+	unsigned stat_with_summary:1;
 };
 
 static inline void diff_flags_or(struct diff_flags *a,
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index f10798b2df..3f9a24fd56 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -361,6 +361,11 @@ diff --no-index --raw dir2 dir
 diff --no-index --raw --abbrev=4 dir2 dir
 :noellipses diff --no-index --raw --abbrev=4 dir2 dir
 diff --no-index --raw --no-abbrev dir2 dir
+
+diff-tree --pretty --root --stat --compact-summary initial
+diff-tree --pretty -R --root --stat --compact-summary initial
+diff-tree --stat --compact-summary initial mode
+diff-tree -R --stat --compact-summary initial mode
 EOF
 
 test_expect_success 'log -S requires an argument' '
diff --git a/t/t4013/diff.diff-tree_--pretty_--root_--stat_--compact-summary_initial b/t/t4013/diff.diff-tree_--pretty_--root_--stat_--compact-summary_initial
new file mode 100644
index 0000000000..d6451ff7cc
--- /dev/null
+++ b/t/t4013/diff.diff-tree_--pretty_--root_--stat_--compact-summary_initial
@@ -0,0 +1,12 @@
+$ git diff-tree --pretty --root --stat --compact-summary initial
+commit 444ac553ac7612cc88969031b02b3767fb8a353a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:00:00 2006 +0000
+
+    Initial
+
+ dir/sub (new) | 2 ++
+ file0 (new)   | 3 +++
+ file2 (new)   | 3 +++
+ 3 files changed, 8 insertions(+)
+$
diff --git a/t/t4013/diff.diff-tree_--pretty_-R_--root_--stat_--compact-summary_initial b/t/t4013/diff.diff-tree_--pretty_-R_--root_--stat_--compact-summary_initial
new file mode 100644
index 0000000000..1989e55cd0
--- /dev/null
+++ b/t/t4013/diff.diff-tree_--pretty_-R_--root_--stat_--compact-summary_initial
@@ -0,0 +1,12 @@
+$ git diff-tree --pretty -R --root --stat --compact-summary initial
+commit 444ac553ac7612cc88969031b02b3767fb8a353a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:00:00 2006 +0000
+
+    Initial
+
+ dir/sub (gone) | 2 --
+ file0 (gone)   | 3 ---
+ file2 (gone)   | 3 ---
+ 3 files changed, 8 deletions(-)
+$
diff --git a/t/t4013/diff.diff-tree_--stat_--compact-summary_initial_mode b/t/t4013/diff.diff-tree_--stat_--compact-summary_initial_mode
new file mode 100644
index 0000000000..9c7c8f63af
--- /dev/null
+++ b/t/t4013/diff.diff-tree_--stat_--compact-summary_initial_mode
@@ -0,0 +1,4 @@
+$ git diff-tree --stat --compact-summary initial mode
+ file0 (mode +x) | 0
+ 1 file changed, 0 insertions(+), 0 deletions(-)
+$
diff --git a/t/t4013/diff.diff-tree_-R_--stat_--compact-summary_initial_mode b/t/t4013/diff.diff-tree_-R_--stat_--compact-summary_initial_mode
new file mode 100644
index 0000000000..e38f3d3bfb
--- /dev/null
+++ b/t/t4013/diff.diff-tree_-R_--stat_--compact-summary_initial_mode
@@ -0,0 +1,4 @@
+$ git diff-tree -R --stat --compact-summary initial mode
+ file0 (mode -x) | 0
+ 1 file changed, 0 insertions(+), 0 deletions(-)
+$
-- 
2.16.1.435.g8f24da2e1a

[PATCH v5 0/2] diff: add --compact-summary (aka nd/diff-stat-with-summary)

[Nguyễn Thái Ngọc Duy]

... and v5 fixes the commit message of 2/2 where in v4 it still
mentions --stat-with-summary instead of --compact-summary. Sorry.

Nguyễn Thái Ngọc Duy (2):
  diff.c: refactor pprint_rename() to use strbuf
  diff: add --compact-summary

 Documentation/diff-options.txt                |  8 ++
 diff.c                                        | 96 ++++++++++++-------
 diff.h                                        |  1 +
 t/t4013-diff-various.sh                       |  5 +
 ...ty_--root_--stat_--compact-summary_initial | 12 +++
 ...-R_--root_--stat_--compact-summary_initial | 12 +++
 ...tree_--stat_--compact-summary_initial_mode |  4 +
 ...e_-R_--stat_--compact-summary_initial_mode |  4 +
 8 files changed, 109 insertions(+), 33 deletions(-)
 create mode 100644 t/t4013/diff.diff-tree_--pretty_--root_--stat_--compact-summary_initial
 create mode 100644 t/t4013/diff.diff-tree_--pretty_-R_--root_--stat_--compact-summary_initial
 create mode 100644 t/t4013/diff.diff-tree_--stat_--compact-summary_initial_mode
 create mode 100644 t/t4013/diff.diff-tree_-R_--stat_--compact-summary_initial_mode

-- 
2.16.1.435.g8f24da2e1a

[PATCH v5 1/2] diff.c: refactor pprint_rename() to use strbuf

[Nguyễn Thái Ngọc Duy]

Instead of passing char* around, let function handle strbuf
directly. All callers already use strbuf internally.

This helps kill the "not free" exception in free_diffstat_info(). I
don't think this code is so critical that we need to avoid some free()
calls.

The other benefit comes in the next patch, where we append something
in pname before returning from fill_print_name(). With strbuf, it's
very simple. With "char *" we may have to resort to explicit
reallocation and stuff.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 59 ++++++++++++++++++++++++++--------------------------------
 1 file changed, 26 insertions(+), 33 deletions(-)

diff --git a/diff.c b/diff.c
index 21c3838b25..e3f72de27d 100644
--- a/diff.c
+++ b/diff.c
@@ -2045,11 +2045,10 @@ 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(struct strbuf *name, const char *a, const char *b)
 {
 	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);
@@ -2059,10 +2058,10 @@ 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, name, NULL, 0);
+		strbuf_addstr(name, " => ");
+		quote_c_style(b, name, NULL, 0);
+		return;
 	}
 
 	/* Find common prefix */
@@ -2109,19 +2108,18 @@ 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);
+	strbuf_grow(name, pfx_length + a_midlen + b_midlen + sfx_length + 7);
 	if (pfx_length + sfx_length) {
-		strbuf_add(&name, a, pfx_length);
-		strbuf_addch(&name, '{');
+		strbuf_add(name, a, pfx_length);
+		strbuf_addch(name, '{');
 	}
-	strbuf_add(&name, a + pfx_length, a_midlen);
-	strbuf_addstr(&name, " => ");
-	strbuf_add(&name, b + pfx_length, b_midlen);
+	strbuf_add(name, a + pfx_length, a_midlen);
+	strbuf_addstr(name, " => ");
+	strbuf_add(name, b + pfx_length, b_midlen);
 	if (pfx_length + sfx_length) {
-		strbuf_addch(&name, '}');
-		strbuf_add(&name, a + len_a - sfx_length, sfx_length);
+		strbuf_addch(name, '}');
+		strbuf_add(name, a + len_a - sfx_length, sfx_length);
 	}
-	return strbuf_detach(&name, NULL);
 }
 
 struct diffstat_t {
@@ -2197,23 +2195,17 @@ static void show_graph(struct strbuf *out, char ch, int cnt,
 
 static void fill_print_name(struct diffstat_file *file)
 {
-	char *pname;
+	struct strbuf pname = STRBUF_INIT;
 
 	if (file->print_name)
 		return;
 
-	if (!file->is_renamed) {
-		struct strbuf buf = STRBUF_INIT;
-		if (quote_c_style(file->name, &buf, NULL, 0)) {
-			pname = strbuf_detach(&buf, NULL);
-		} else {
-			pname = file->name;
-			strbuf_release(&buf);
-		}
-	} else {
-		pname = pprint_rename(file->from_name, file->name);
-	}
-	file->print_name = pname;
+	if (file->is_renamed)
+		pprint_rename(&pname, file->from_name, file->name);
+	else
+		quote_c_style(file->name, &pname, NULL, 0);
+
+	file->print_name = strbuf_detach(&pname, NULL);
 }
 
 static void print_stat_summary_inserts_deletes(struct diff_options *options,
@@ -2797,8 +2789,7 @@ static void free_diffstat_info(struct diffstat_t *diffstat)
 	int i;
 	for (i = 0; i < diffstat->nr; i++) {
 		struct diffstat_file *f = diffstat->files[i];
-		if (f->name != f->print_name)
-			free(f->print_name);
+		free(f->print_name);
 		free(f->name);
 		free(f->from_name);
 		free(f);
@@ -5241,10 +5232,12 @@ static void show_rename_copy(struct diff_options *opt, const char *renamecopy,
 		struct diff_filepair *p)
 {
 	struct strbuf sb = STRBUF_INIT;
-	char *names = pprint_rename(p->one->path, p->two->path);
+	struct strbuf names = STRBUF_INIT;
+
+	pprint_rename(&names, p->one->path, p->two->path);
 	strbuf_addf(&sb, " %s %s (%d%%)\n",
-			renamecopy, names, similarity_index(p));
-	free(names);
+		    renamecopy, names.buf, similarity_index(p));
+	strbuf_release(&names);
 	emit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY,
 				 sb.buf, sb.len, 0);
 	show_mode_change(opt, p, 0);
-- 
2.16.1.435.g8f24da2e1a

[PATCH v5 2/2] diff: add --compact-summary

[Nguyễn Thái Ngọc Duy]

Certain information is currently shown with --summary, but when used
in combination with --stat it's a bit hard to read since info of the
same file is in two places (--stat and --summary).

On top of that, commits that add or remove files double the number of
display lines, which could be a lot if you add or remove a lot of
files.

--compact-summary embeds most of --summary back in --stat in the
little space between the file name part and the graph line, e.g. with
commit 0433d533f1:

   Documentation/merge-config.txt         |  4 +
   builtin/merge.c                        |  2 +
   ...-pull-verify-signatures.sh (new +x) | 81 ++++++++++++++
   t/t7612-merge-verify-signatures.sh     | 45 ++++++++
   4 files changed, 132 insertions(+)

It helps both condensing information and saving some text
space. What's new in diffstat is:

- A new 0644 file is shown as (new)
- A new 0755 file is shown as (new +x)
- A new symlink is shown as (new +l)
- A deleted file is shown as (gone)
- A mode change adding executable bit is shown as (mode +x)
- A mode change removing it is shown as (mode -x)

Note that --compact-summary does not contain all the information
--summary provides. Rewrite percentage is not shown but it could be
added later, like R50% or C20%.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/diff-options.txt                |  8 ++++
 diff.c                                        | 37 +++++++++++++++++++
 diff.h                                        |  1 +
 t/t4013-diff-various.sh                       |  5 +++
 ...ty_--root_--stat_--compact-summary_initial | 12 ++++++
 ...-R_--root_--stat_--compact-summary_initial | 12 ++++++
 ...tree_--stat_--compact-summary_initial_mode |  4 ++
 ...e_-R_--stat_--compact-summary_initial_mode |  4 ++
 8 files changed, 83 insertions(+)
 create mode 100644 t/t4013/diff.diff-tree_--pretty_--root_--stat_--compact-summary_initial
 create mode 100644 t/t4013/diff.diff-tree_--pretty_-R_--root_--stat_--compact-summary_initial
 create mode 100644 t/t4013/diff.diff-tree_--stat_--compact-summary_initial_mode
 create mode 100644 t/t4013/diff.diff-tree_-R_--stat_--compact-summary_initial_mode

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index c330c01ff0..e3a44f03cd 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -128,6 +128,14 @@ have to use `--diff-algorithm=default` option.
 These parameters can also be set individually with `--stat-width=<width>`,
 `--stat-name-width=<name-width>` and `--stat-count=<count>`.
 
+--compact-summary::
+	Output a condensed summary of extended header information such
+	as file creations or deletions ("new" or "gone", optionally "+l"
+	if it's a symlink) and mode changes ("+x" or "-x" for adding
+	or removing executable bit respectively) in diffstat. The
+	information is put betwen the filename part and the graph
+	part. Implies `--stat`.
+
 --numstat::
 	Similar to `--stat`, but shows number of added and
 	deleted lines in decimal notation and pathname without
diff --git a/diff.c b/diff.c
index e3f72de27d..62e413a80f 100644
--- a/diff.c
+++ b/diff.c
@@ -2129,6 +2129,7 @@ struct diffstat_t {
 		char *from_name;
 		char *name;
 		char *print_name;
+		const char *comments;
 		unsigned is_unmerged:1;
 		unsigned is_binary:1;
 		unsigned is_renamed:1;
@@ -2205,6 +2206,9 @@ static void fill_print_name(struct diffstat_file *file)
 	else
 		quote_c_style(file->name, &pname, NULL, 0);
 
+	if (file->comments)
+		strbuf_addf(&pname, " (%s)", file->comments);
+
 	file->print_name = strbuf_detach(&pname, NULL);
 }
 
@@ -3239,6 +3243,32 @@ static void builtin_diff(const char *name_a,
 	return;
 }
 
+static char *get_compact_summary(const struct diff_filepair *p, int is_renamed)
+{
+	if (!is_renamed) {
+		if (p->status == DIFF_STATUS_ADDED) {
+			if (S_ISLNK(p->two->mode))
+				return "new +l";
+			else if ((p->two->mode & 0777) == 0755)
+				return "new +x";
+			else
+				return "new";
+		} else if (p->status == DIFF_STATUS_DELETED)
+			return "gone";
+	}
+	if (S_ISLNK(p->one->mode) && !S_ISLNK(p->two->mode))
+		return "mode -l";
+	else if (!S_ISLNK(p->one->mode) && S_ISLNK(p->two->mode))
+		return "mode +l";
+	else if ((p->one->mode & 0777) == 0644 &&
+		 (p->two->mode & 0777) == 0755)
+		return "mode +x";
+	else if ((p->one->mode & 0777) == 0755 &&
+		 (p->two->mode & 0777) == 0644)
+		return "mode -x";
+	return NULL;
+}
+
 static void builtin_diffstat(const char *name_a, const char *name_b,
 			     struct diff_filespec *one,
 			     struct diff_filespec *two,
@@ -3258,6 +3288,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
 
 	data = diffstat_add(diffstat, name_a, name_b);
 	data->is_interesting = p->status != DIFF_STATUS_UNKNOWN;
+	if (o->flags.stat_with_summary)
+		data->comments = get_compact_summary(p, data->is_renamed);
 
 	if (!one || !two) {
 		data->is_unmerged = 1;
@@ -4544,6 +4576,11 @@ int diff_opt_parse(struct diff_options *options,
 	else if (starts_with(arg, "--stat"))
 		/* --stat, --stat-width, --stat-name-width, or --stat-count */
 		return stat_opt(options, av);
+	else if (!strcmp(arg, "--compact-summary")) {
+		 options->flags.stat_with_summary = 1;
+		 options->output_format |= DIFF_FORMAT_DIFFSTAT;
+	} else if (!strcmp(arg, "--no-compact-summary"))
+		 options->flags.stat_with_summary = 0;
 
 	/* renames options */
 	else if (starts_with(arg, "-B") ||
diff --git a/diff.h b/diff.h
index 6bd278aac1..d29560f822 100644
--- a/diff.h
+++ b/diff.h
@@ -93,6 +93,7 @@ struct diff_flags {
 	unsigned dirstat_by_line:1;
 	unsigned funccontext:1;
 	unsigned default_follow_renames:1;
+	unsigned stat_with_summary:1;
 };
 
 static inline void diff_flags_or(struct diff_flags *a,
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index f10798b2df..3f9a24fd56 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -361,6 +361,11 @@ diff --no-index --raw dir2 dir
 diff --no-index --raw --abbrev=4 dir2 dir
 :noellipses diff --no-index --raw --abbrev=4 dir2 dir
 diff --no-index --raw --no-abbrev dir2 dir
+
+diff-tree --pretty --root --stat --compact-summary initial
+diff-tree --pretty -R --root --stat --compact-summary initial
+diff-tree --stat --compact-summary initial mode
+diff-tree -R --stat --compact-summary initial mode
 EOF
 
 test_expect_success 'log -S requires an argument' '
diff --git a/t/t4013/diff.diff-tree_--pretty_--root_--stat_--compact-summary_initial b/t/t4013/diff.diff-tree_--pretty_--root_--stat_--compact-summary_initial
new file mode 100644
index 0000000000..d6451ff7cc
--- /dev/null
+++ b/t/t4013/diff.diff-tree_--pretty_--root_--stat_--compact-summary_initial
@@ -0,0 +1,12 @@
+$ git diff-tree --pretty --root --stat --compact-summary initial
+commit 444ac553ac7612cc88969031b02b3767fb8a353a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:00:00 2006 +0000
+
+    Initial
+
+ dir/sub (new) | 2 ++
+ file0 (new)   | 3 +++
+ file2 (new)   | 3 +++
+ 3 files changed, 8 insertions(+)
+$
diff --git a/t/t4013/diff.diff-tree_--pretty_-R_--root_--stat_--compact-summary_initial b/t/t4013/diff.diff-tree_--pretty_-R_--root_--stat_--compact-summary_initial
new file mode 100644
index 0000000000..1989e55cd0
--- /dev/null
+++ b/t/t4013/diff.diff-tree_--pretty_-R_--root_--stat_--compact-summary_initial
@@ -0,0 +1,12 @@
+$ git diff-tree --pretty -R --root --stat --compact-summary initial
+commit 444ac553ac7612cc88969031b02b3767fb8a353a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:00:00 2006 +0000
+
+    Initial
+
+ dir/sub (gone) | 2 --
+ file0 (gone)   | 3 ---
+ file2 (gone)   | 3 ---
+ 3 files changed, 8 deletions(-)
+$
diff --git a/t/t4013/diff.diff-tree_--stat_--compact-summary_initial_mode b/t/t4013/diff.diff-tree_--stat_--compact-summary_initial_mode
new file mode 100644
index 0000000000..9c7c8f63af
--- /dev/null
+++ b/t/t4013/diff.diff-tree_--stat_--compact-summary_initial_mode
@@ -0,0 +1,4 @@
+$ git diff-tree --stat --compact-summary initial mode
+ file0 (mode +x) | 0
+ 1 file changed, 0 insertions(+), 0 deletions(-)
+$
diff --git a/t/t4013/diff.diff-tree_-R_--stat_--compact-summary_initial_mode b/t/t4013/diff.diff-tree_-R_--stat_--compact-summary_initial_mode
new file mode 100644
index 0000000000..e38f3d3bfb
--- /dev/null
+++ b/t/t4013/diff.diff-tree_-R_--stat_--compact-summary_initial_mode
@@ -0,0 +1,4 @@
+$ git diff-tree -R --stat --compact-summary initial mode
+ file0 (mode -x) | 0
+ 1 file changed, 0 insertions(+), 0 deletions(-)
+$
-- 
2.16.1.435.g8f24da2e1a

Re: [PATCH v5 0/2] diff: add --compact-summary (aka nd/diff-stat-with-summary)

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> ... and v5 fixes the commit message of 2/2 where in v4 it still
> mentions --stat-with-summary instead of --compact-summary. Sorry.
>
> Nguyễn Thái Ngọc Duy (2):
>   diff.c: refactor pprint_rename() to use strbuf
>   diff: add --compact-summary

Thanks, will queue.  I guess we all run out of paints of different
colours, and it's a good time to go incremental by merging the topic
to 'next'.

>
>  Documentation/diff-options.txt                |  8 ++
>  diff.c                                        | 96 ++++++++++++-------
>  diff.h                                        |  1 +
>  t/t4013-diff-various.sh                       |  5 +
>  ...ty_--root_--stat_--compact-summary_initial | 12 +++
>  ...-R_--root_--stat_--compact-summary_initial | 12 +++
>  ...tree_--stat_--compact-summary_initial_mode |  4 +
>  ...e_-R_--stat_--compact-summary_initial_mode |  4 +
>  8 files changed, 109 insertions(+), 33 deletions(-)
>  create mode 100644 t/t4013/diff.diff-tree_--pretty_--root_--stat_--compact-summary_initial
>  create mode 100644 t/t4013/diff.diff-tree_--pretty_-R_--root_--stat_--compact-summary_initial
>  create mode 100644 t/t4013/diff.diff-tree_--stat_--compact-summary_initial_mode
>  create mode 100644 t/t4013/diff.diff-tree_-R_--stat_--compact-summary_initial_mode