From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 03 May 2011 18:47:33 GMT
Subject: Re: [PATCHv2 1/2] diff: introduce --stat-lines to limit the stat lines
Message-ID: <7vfwoviptm.fsf@alter.siamese.dyndns.org>
In-Reply-To: <102e117f7a9f7fd6e652f23d5630db973e8e52b8.1304419588.git.git@drmicha.warpmail.net>

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

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

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

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?