git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Michael J Gruber <git@drmicha.warpmail.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCHv2 1/2] diff: introduce --stat-lines to limit the stat lines
Date: Wed, 04 May 2011 09:16:13 +0200	[thread overview]
Message-ID: <4DC0FD3D.9010004@drmicha.warpmail.net> (raw)
In-Reply-To: <7vfwoviptm.fsf@alter.siamese.dyndns.org>

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

  reply	other threads:[~2011-05-04  7:16 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-04-29 14:57 [PATCH 1/2] diff: introduce --stat-lines to limit the stat lines Michael J Gruber
2011-04-29 14:57 ` [PATCH 2/2] diff-options.txt: describe --stat-{width,name-width,lines} Michael J Gruber
2011-04-29 15:12 ` [PATCH 1/2] diff: introduce --stat-lines to limit the stat lines Michael J Gruber
2011-04-29 16:15 ` Junio C Hamano
2011-05-01  8:27   ` Michael J Gruber
2011-05-01 18:33     ` Junio C Hamano
2011-05-03 10:46       ` [PATCHv2 " Michael J Gruber
2011-05-03 10:46         ` [PATCHv2 2/2] diff-options.txt: describe --stat-{width,name-width,count} Michael J Gruber
2011-05-03 18:47         ` [PATCHv2 1/2] diff: introduce --stat-lines to limit the stat lines Junio C Hamano
2011-05-04  7:16           ` Michael J Gruber [this message]
2011-05-27 12:36             ` [PATCHv3 0/3] diff.c: --stat-count=<n> Michael J Gruber
2011-05-27 12:36               ` [PATCHv3 1/3] diff.c: omit hidden entries from namelen calculation with --stat Michael J Gruber
2011-05-27 17:43                 ` Junio C Hamano
2011-05-27 20:19                   ` Michael J Gruber
2011-05-27 22:45                     ` Junio C Hamano
2011-05-27 12:36               ` [PATCHv3 2/3] diff: introduce --stat-lines to limit the stat lines Michael J Gruber
2011-05-28  4:46                 ` Junio C Hamano
2011-05-30  6:40                   ` Michael J Gruber
2011-05-30  7:36                     ` Junio C Hamano
2011-05-27 12:36               ` [PATCHv3 3/3] diff-options.txt: describe --stat-{width,name-width,count} Michael J Gruber
2011-05-04  4:37         ` [PATCHv2 1/2] diff: introduce --stat-lines to limit the stat lines Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4DC0FD3D.9010004@drmicha.warpmail.net \
    --to=git@drmicha.warpmail.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).