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

[Michael J Gruber <git@drmicha.warpmail.net>]

Junio C Hamano venit, vidit, dixit 03.05.2011 20:47:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> 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.

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

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

>> +	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 <git@drmicha.warpmail.net> writes:
> 
>> > @@ -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