From 74faaa16f016af9fc429770ba701f2aa598d9f21 Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Wed, 17 Oct 2012 10:00:37 -0700 Subject: Fix "git diff --stat" for interesting - but empty - file changes The behavior of "git diff --stat" is rather odd for files that have zero lines of changes: it will discount them entirely unless they were renames. Which means that the stat output will simply not show files that only had "other" changes: they were created or deleted, or their mode was changed. Now, those changes do show up in the summary, but so do renames, so the diffstat logic is inconsistent. Why does it show renames with zero lines changed, but not mode changes or added files with zero lines changed? So change the logic to not check for "is_renamed", but for "is_interesting" instead, where "interesting" is judged to be any action but a pure data change (because a pure data change with zero data changed really isn't worth showing, if we ever get one in our diffpairs). So if you did chmod +x Makefile git diff --stat before, it would show empty (" 0 files changed"), with this it shows Makefile | 0 1 file changed, 0 insertions(+), 0 deletions(-) which I think is a more correct diffstat (and then with "--summary" it shows *what* the metadata change to Makefile was - this is completely consistent with our handling of renamed files). Side note: the old behavior was *really* odd. With no changes at all, "git diff --stat" output was empty. With just a chmod, it said "0 files changed". No way is our legacy behavior sane. Signed-off-by: Linus Torvalds Signed-off-by: Junio C Hamano --- diff.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) (limited to 'diff.c') diff --git a/diff.c b/diff.c index 35d3f07385..95bbad66c6 100644 --- a/diff.c +++ b/diff.c @@ -1300,6 +1300,7 @@ struct diffstat_t { unsigned is_unmerged:1; unsigned is_binary:1; unsigned is_renamed:1; + unsigned is_interesting:1; uintmax_t added, deleted; } **files; }; @@ -1469,7 +1470,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) for (i = 0; (i < count) && (i < data->nr); i++) { struct diffstat_file *file = data->files[i]; uintmax_t change = file->added + file->deleted; - if (!data->files[i]->is_renamed && + if (!data->files[i]->is_interesting && (change == 0)) { count++; /* not shown == room for one more */ continue; @@ -1590,7 +1591,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) uintmax_t deleted = data->files[i]->deleted; int name_len; - if (!data->files[i]->is_renamed && + if (!data->files[i]->is_interesting && (added + deleted == 0)) { total_files--; continue; @@ -1669,7 +1670,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) 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 && + if (!data->files[i]->is_interesting && (added + deleted == 0)) { total_files--; continue; @@ -1697,7 +1698,7 @@ static void show_shortstats(struct diffstat_t *data, struct diff_options *option if (data->files[i]->is_unmerged) continue; - if (!data->files[i]->is_renamed && (added + deleted == 0)) { + if (!data->files[i]->is_interesting && (added + deleted == 0)) { total_files--; } else if (!data->files[i]->is_binary) { /* don't count bytes */ adds += added; @@ -2397,13 +2398,20 @@ static void builtin_diffstat(const char *name_a, const char *name_b, struct diff_filespec *two, struct diffstat_t *diffstat, struct diff_options *o, - int complete_rewrite) + struct diff_filepair *p) { mmfile_t mf1, mf2; struct diffstat_file *data; int same_contents; + int complete_rewrite = 0; + + if (!DIFF_PAIR_UNMERGED(p)) { + if (p->status == DIFF_STATUS_MODIFIED && p->score) + complete_rewrite = 1; + } data = diffstat_add(diffstat, name_a, name_b); + data->is_interesting = p->status != 0; if (!one || !two) { data->is_unmerged = 1; @@ -3114,11 +3122,10 @@ static void run_diffstat(struct diff_filepair *p, struct diff_options *o, { const char *name; const char *other; - int complete_rewrite = 0; if (DIFF_PAIR_UNMERGED(p)) { /* unmerged */ - builtin_diffstat(p->one->path, NULL, NULL, NULL, diffstat, o, 0); + builtin_diffstat(p->one->path, NULL, NULL, NULL, diffstat, o, p); return; } @@ -3131,9 +3138,7 @@ static void run_diffstat(struct diff_filepair *p, struct diff_options *o, diff_fill_sha1_info(p->one); diff_fill_sha1_info(p->two); - if (p->status == DIFF_STATUS_MODIFIED && p->score) - complete_rewrite = 1; - builtin_diffstat(name, other, p->one, p->two, diffstat, o, complete_rewrite); + builtin_diffstat(name, other, p->one, p->two, diffstat, o, p); } static void run_checkdiff(struct diff_filepair *p, struct diff_options *o) -- cgit v1.2.3 From 99bfd407000302ff27f2809d9d556c97427a9e3b Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 27 Nov 2012 11:17:14 -0800 Subject: diff --stat: status of unmodified pair in diff-q is not zero It is spelled DIFF_STATUS_UNKNOWN these days, and is different from zero. Signed-off-by: Junio C Hamano --- diff.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'diff.c') diff --git a/diff.c b/diff.c index 95bbad66c6..ce6baa4c69 100644 --- a/diff.c +++ b/diff.c @@ -2411,7 +2411,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b, } data = diffstat_add(diffstat, name_a, name_b); - data->is_interesting = p->status != 0; + data->is_interesting = p->status != DIFF_STATUS_UNKNOWN; if (!one || !two) { data->is_unmerged = 1; -- cgit v1.2.3 From af0ed819c59248bc98f0a572bd4cdcb394f185c7 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 27 Nov 2012 11:24:54 -0800 Subject: diff --stat: use "file" temporary variable to refer to data->files[i] The generated code shouldn't change but it is easier to read. Signed-off-by: Junio C Hamano --- diff.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) (limited to 'diff.c') diff --git a/diff.c b/diff.c index ce6baa4c69..e4e70e55fe 100644 --- a/diff.c +++ b/diff.c @@ -1470,8 +1470,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) for (i = 0; (i < count) && (i < data->nr); i++) { struct diffstat_file *file = data->files[i]; uintmax_t change = file->added + file->deleted; - if (!data->files[i]->is_interesting && - (change == 0)) { + + if (!file->is_interesting && (change == 0)) { count++; /* not shown == room for one more */ continue; } @@ -1586,13 +1586,13 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) */ for (i = 0; i < count; i++) { const char *prefix = ""; - char *name = data->files[i]->print_name; - uintmax_t added = data->files[i]->added; - uintmax_t deleted = data->files[i]->deleted; + struct diffstat_file *file = data->files[i]; + char *name = file->print_name; + uintmax_t added = file->added; + uintmax_t deleted = file->deleted; int name_len; - if (!data->files[i]->is_interesting && - (added + deleted == 0)) { + if (!file->is_interesting && (added + deleted == 0)) { total_files--; continue; } @@ -1611,7 +1611,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) name = slash; } - if (data->files[i]->is_binary) { + if (file->is_binary) { fprintf(options->file, "%s", line_prefix); show_name(options->file, prefix, name, len); fprintf(options->file, " %*s", number_width, "Bin"); @@ -1628,7 +1628,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) fprintf(options->file, "\n"); continue; } - else if (data->files[i]->is_unmerged) { + else if (file->is_unmerged) { fprintf(options->file, "%s", line_prefix); show_name(options->file, prefix, name, len); fprintf(options->file, " Unmerged\n"); @@ -1668,10 +1668,10 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) fprintf(options->file, "\n"); } 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_interesting && - (added + deleted == 0)) { + struct diffstat_file *file = data->files[i]; + uintmax_t added = file->added; + uintmax_t deleted = file->deleted; + if (!file->is_interesting && (added + deleted == 0)) { total_files--; continue; } -- cgit v1.2.3 From a20d3c0de1f107ddd719a4a9cec3addd56e8444f Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 27 Nov 2012 11:47:46 -0800 Subject: diff --stat: move the "total count" logic to the last loop The diffstat generation logic, with --stat-count limit, is implemented as three loops. - The first counts the width necessary to show stats up to specified number of entries, and notes up to how many entries in the data we need to iterate to show the graph; - The second iterates that many times to draw the graph, adjusts the number of "total modified files", and counts the total added/deleted lines for the part that was shown in the graph; - The third iterates over the remainder and only does the part to count "total added/deleted lines" and to adjust "total modified files" without drawing anything. Move the logic to count added/deleted lines and modified files from the second loop to the third loop. This incidentally fixes a bug. The third loop was not filtering binary changes (counted in bytes) from the total added/deleted as it should. The second loop implemented this correctly, so if a binary change appeared earlier than the --stat-count cutoff, the code counted number of added/deleted lines correctly, but if it appeared beyond the cutoff, the number of lines would have mixed with the byte count in the buggy third loop. Signed-off-by: Junio C Hamano --- diff.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) (limited to 'diff.c') diff --git a/diff.c b/diff.c index e4e70e55fe..4105260122 100644 --- a/diff.c +++ b/diff.c @@ -1498,7 +1498,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) */ + count = i; /* where we can stop scanning in data->files[] */ /* * We have width = stat_width or term_columns() columns total. @@ -1592,10 +1592,9 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) uintmax_t deleted = file->deleted; int name_len; - if (!file->is_interesting && (added + deleted == 0)) { - total_files--; + if (!file->is_interesting && (added + deleted == 0)) continue; - } + /* * "scale" the filename */ @@ -1640,8 +1639,6 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) */ add = added; del = deleted; - adds += add; - dels += del; if (graph_width <= max_change) { int total = add + del; @@ -1667,7 +1664,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) show_graph(options->file, '-', del, del_c, reset); fprintf(options->file, "\n"); } - for (i = count; i < data->nr; i++) { + + for (i = 0; i < data->nr; i++) { struct diffstat_file *file = data->files[i]; uintmax_t added = file->added; uintmax_t deleted = file->deleted; @@ -1675,8 +1673,13 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) total_files--; continue; } - adds += added; - dels += deleted; + + if (!file->is_binary && !file->is_unmerged) { + adds += added; + dels += deleted; + } + if (i < count) + continue; if (!extra_shown) fprintf(options->file, "%s ...\n", line_prefix); extra_shown = 1; -- cgit v1.2.3 From 82dfc2c44ecda8a7afe417086c704b141a11cd58 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 27 Nov 2012 12:05:10 -0800 Subject: diff --stat: do not count "unmerged" entries Even though we show a separate *UNMERGED* entry in the patch and diffstat output (or in the --raw format, for that matter) in addition to and separately from the diff against the specified stage (defaulting to #2) for unmerged paths, they should not be counted in the total number of files affected---that would lead to counting the same path twice. The separation done by the previous step makes this fix simple and straightforward. Among the filepairs in diff_queue, paths that weren't modified, and the extra "unmerged" entries do not count as total number of files. Signed-off-by: Junio C Hamano --- diff.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'diff.c') diff --git a/diff.c b/diff.c index 4105260122..26ede82a7b 100644 --- a/diff.c +++ b/diff.c @@ -1669,12 +1669,14 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options) struct diffstat_file *file = data->files[i]; uintmax_t added = file->added; uintmax_t deleted = file->deleted; - if (!file->is_interesting && (added + deleted == 0)) { + + if (file->is_unmerged || + (!file->is_interesting && (added + deleted == 0))) { total_files--; continue; } - if (!file->is_binary && !file->is_unmerged) { + if (!file->is_binary) { adds += added; dels += deleted; } -- cgit v1.2.3 From 20c8cde45656fc187e06088ef5baf0bbdfdf70bd Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 27 Nov 2012 14:19:36 -0800 Subject: diff --shortstat: do not count "unmerged" entries Fix the same issue as the previous one for "git diff --stat"; unmerged entries was doubly-counted. Signed-off-by: Junio C Hamano --- diff.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'diff.c') diff --git a/diff.c b/diff.c index 26ede82a7b..374b2354f3 100644 --- a/diff.c +++ b/diff.c @@ -1701,9 +1701,8 @@ static void show_shortstats(struct diffstat_t *data, struct diff_options *option int added = data->files[i]->added; int deleted= data->files[i]->deleted; - if (data->files[i]->is_unmerged) - continue; - if (!data->files[i]->is_interesting && (added + deleted == 0)) { + if (data->files[i]->is_unmerged || + (!data->files[i]->is_interesting && (added + deleted == 0))) { total_files--; } else if (!data->files[i]->is_binary) { /* don't count bytes */ adds += added; -- cgit v1.2.3