Re: [PATCHv2 1/2] diff: introduce --stat-lines to limit the stat lines

Junio C Hamano venit, vidit, dixit 03.05.2011 20:47:

Michael J Gruber [off-list ref] writes:

quoted

In the case with <count>+1 items one may argue whether it makes more sense to
ignore the user wish and output all <count>+1 lines, or <count> lines (as
requested) plus the "..." line.

I think that is a must if we care about consistency. fmt-merge-msg should

I assume you mean the part before the "or" by "this".

already do this (I remember being careful about this particular case when
I wrote its first version, but I do not know it has regressed as I do not
remember writing tests for this boundary case---my bad ;-).

I'll recheck with fmt-merge-msg. Seems this series needs more work than
I expected... and may take some time.

quoted

(I saw the suggestion about N-2...2 just now. Would work also, but I guess
we would do this in more cases then, as Junio indicated.)

It is not clear what you mean by N-2...2 but if you are referring to my
"first N-1 entries, dots and the last one, to make the total N+1 lines
that show N entries", then yes I think it would make sense to do that also
in fmt-merge-msg.c::shortlog() as well as here.  But that would be a
separate topic.

Yes, N-2, then the remaining 2 or "..." plus the last one. Sorry for the
confusion.

quoted

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 34f0145..000eae0 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt

@@ -48,11 +48,14 @@ endif::git-format-patch[]
 --patience::
 	Generate a diff using the "patience diff" algorithm.
 
---stat[=<width>[,<name-width>]]::
+--stat[=<width>[,<name-width>[,<count>]]]::
 	Generate a diffstat.  You can override the default
 	output width for 80-column terminal by `--stat=<width>`.
 	The width of the filename part can be controlled by
 	giving another width to it separated by a comma.
+	By giving a third parameter `<count>`, you can limit the
+	output to the first `<count>` lines, followed by
+	`...` if there are more.

Does an empty-string <count> mean "use default" (currently "no limit")?
This matters when we teach a new parameter to --stat and make the above:

	--stat=[=<width>[,<name-width>[,<count>[,<nitfol>]]]]

Yes, strtoul("") is 0, and 0 denotes default, which is unlimited. By
design, but maybe I should mention it somewhere, probably as part of
2/2, because that same effect is true and undocumented for the 1st 2
slots, i.e. --stat=",,5" sets the count argument to 5 and the others to
default.

quoted

+	for (i = count; i < data->nr; i++) {
+		uintmax_t added = data->files[i]->added;
+		uintmax_t deleted = data->files[i]->deleted;
+		if (!data->files[i]->is_renamed &&
+			 (added + deleted == 0)) {
+			total_files--;
+			continue;
+		}
+		adds += added;
+		dels += deleted;
+	}
 	fprintf(options->file, "%s", line_prefix);
 	fprintf(options->file,
 	       " %d files changed, %d insertions(+), %d deletions(-)\n",

This is culling the output of what is in struct diffstat that we have
already spent cycles to possibly fill thousands of entries.  I first
thought it may make sense to also tweak the loop in diff_flush() that runs
diff_flush_stat() to all filepairs to run it only on the first <count>
(and later the first <count-1> and the last) filepairs, but we have to
show the short-stat summary at the end, so this cannot be avoided.

Yeah, in my 0th version that stat was for the shown lines only...

What happens when I say "diff --numstat --stat-count=4"?

Should it error out upon seeing a limit that is not infinite, or should it
also elide the lines in its output?

None of the --stat-* options affects --numstat, again by design. All of
width, name-width and count would make sense for --numstat (and
--dirstat) also, but that would require some restructuring.

Michael J Gruber [off-list ref] writes:

quoted

quoted

@@ -1302,7 +1304,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 	else
 		width = max_change;
 
-	for (i = 0; i < data->nr; i++) {
+	for (i = 0; i < count; i++) {
 		const char *prefix = "";
 		char *name = data->files[i]->print_name;
 		uintmax_t added = data->files[i]->added;

This first loop can omit a "struct diffstat_file" that is not a rename and
does not add nor delete any lines (look for "total_files--"), but you do
not seem to compensate for it. If you have such a record in the earlier
part of the result for whatever reason, you would end up showing fewer
entries than what "count" indicates in this loop.

Uh, sorry and thanks for noticing. I wasn't aware that we may skip a
pair completely.

I wanted to make the design optimal in the sense that I introduce as few
additional conditionals within the two loops there. I guess I'll have to
sacrifice that in the first loop.

Michael