Junio C Hamano venit, vidit, dixit 03.05.2011 20:47:
> Michael J Gruber <git@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html