Re: [PATCHv3 2/3] diff: introduce --stat-lines to limit the stat lines
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 28, 2011, 04:46 UTC
- Message-ID
- <7v39jzqvny.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <5da631c64438ec3f669f0c2b7456bcfbc371e2f5.1306499600.git.git@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 9 quoted lines
> diff --git a/diff.c b/diff.c > index 4541939..6a6cbca 100644 > --- a/diff.c > +++ b/diff.c > @@ -1259,6 +1259,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) > > width = options->stat_width ? options->stat_width : 80; > name_width = options->stat_name_width ? options->stat_name_width : 50; > + count = options->stat_count ? options->stat_count : data->nr;
It is a tad hard to follow the logic of "count" here. It could be the ceiling the user gave you, or we may not be constrained at all if the full diff is smaller. But then
Show 6 quoted lines
> @@ -1275,11 +1276,12 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
> add_c = diff_get_color_opt(options, DIFF_FILE_NEW);
> del_c = diff_get_color_opt(options, DIFF_FILE_OLD);
>
> - for (i = 0; i < data->nr; i++) {
> + for (i = 0; (i < count) && (i < data->nr); i++) {Here we compare "i" which does not have much to do with how many we actually have to show (as we may be skipping in the loop) with "count" and also with data->nr. In the loop, you compensate for the skipping (i.e. increment of "i" that should not affect the outcome) by incrementing "count", so (i < count) part will correctly count towards the limit the user gave. It may be a correct math, but it is tricky and hard to read. Especially if we started with the options->stat_count that is larger than the data->nr, of course "i < count" will always hold true with or without skipping, which also is correct but even trickier.
Show 13 quoted lines
> struct diffstat_file *file = data->files[i];
> uintmax_t change = file->added + file->deleted;
> if (!data->files[i]->is_renamed &&
> (change == 0)) {
> + count++; /* not shown == room for one more */
> continue;
> }
> fill_print_name(file);
> @@ -1292,6 +1294,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
> if (max_change < change)
> max_change = change;
> }
> + count = i; /* min(count, data->nr) */This value is mechanically min(count, data->nr) but it is confusing to explain what it really means. It is the number of elements you have to look at in data->files[] array, still skipping irrelevant entries in the loop, until you show options->stat_count number of entries.
Show 6 quoted lines
> @@ -1306,7 +1309,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++) {Hence this loop is correct. You ignore everything beyond count in the array, as their lengths are immaterial to the outcome.
Show 6 quoted lines
> @@ -1373,6 +1376,19 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) > show_graph(options->file, '-', del, del_c, reset); > fprintf(options->file, "\n"); > } > + if (count < data->nr) > + fprintf(options->file, "%s ...\n", line_prefix);
At this point, count is the sum of the number of items you are going to show and the number of items you are going to skip. If that is smaller than the total in the original array, there are items that you are not showing nor skipping. But you terminated the first loop early before reaching the end of data->file[] array, so you do not know enough to say "there _are_ more to show but I am not showing", I think.
If
- data->nr is 3; - the data->file[0] is to be shown but other entries in data->file[] are not to be shown; and - opt->stat_count is 1
then your first loop fills data->file[0]->name, increments i to 1, the condition (i < count) no longer holds, and stops. At this point count is 1, data->nr is 3, but data->file[1] and [2] are not to be shown anyway, so this "there are more but I am not showing them" is not quite right, is it?
Show 11 quoted lines
> + 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;
> + }If this loop runs "adds += added" even once, then you have "there are more but I am not showing" situation, and if all the entries beyond count are skipped, then you are not limiting the output.
By the way, in this round, you are trying to show only the first N items, and if there are more to show, then showing "..." after them. I kind of liked the idea that came up during the discussion in earlier rounds to show N-1 then "..." then the last one item, but I do not feel too strongly about it.
diff.c | 6 ++++-- t/t4049-diff-stat-count.sh | 25 +++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/diff.c b/diff.c index 82789e3..542ff42 100644 --- a/diff.c +++ b/diff.c @@ -1247,6 +1247,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) int width, name_width, count; const char *reset, *add_c, *del_c; const char *line_prefix = ""; + int extra_shown = 0; struct strbuf *msg = NULL; if (data->nr == 0) @@ -1376,8 +1377,6 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) show_graph(options->file, '-', del, del_c, reset); fprintf(options->file, "\n"); } - if (count < data->nr) - fprintf(options->file, "%s ...\n", line_prefix); for (i = count; i < data->nr; i++) { uintmax_t added = data->files[i]->added; uintmax_t deleted = data->files[i]->deleted; @@ -1388,6 +1387,9 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) } adds += added; dels += deleted; + if (!extra_shown) + fprintf(options->file, "%s ...\n", line_prefix); + extra_shown = 1; } fprintf(options->file, "%s", line_prefix); fprintf(options->file, diff --git a/t/t4049-diff-stat-count.sh b/t/t4049-diff-stat-count.sh new file mode 100755 index 0000000..641e70d --- /dev/null +++ b/t/t4049-diff-stat-count.sh @@ -0,0 +1,25 @@ +#!/bin/sh +# Copyright (c) 2011, Google Inc. + +test_description='diff --stat-count' +. ./test-lib.sh + +test_expect_success setup ' + >a && + >b && + >c && + >d && + git add a b c d && + chmod +x c d && + echo a >a && + echo b >b && + cat >expect <<-\EOF + a | 1 + + b | 1 + + 2 files changed, 2 insertions(+), 0 deletions(-) + EOF + git diff --stat --stat-count=2 >actual && + test_cmp expect actual +' + +test_done