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
next prev parent 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).