From: Jeff King <peff@peff.net>
To: Matt McCutchen <matt@mattmccutchen.net>
Cc: git@vger.kernel.org
Subject: Re: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"
Date: Wed, 18 Jan 2017 06:17:05 -0500 [thread overview]
Message-ID: <20170118111705.6bqzkklluikda3r5@sigill.intra.peff.net> (raw)
In-Reply-To: <1484704915.2096.16.camel@mattmccutchen.net>
On Tue, Jan 17, 2017 at 09:01:55PM -0500, Matt McCutchen wrote:
> A bug report: I noticed that "git diff --ignore-space-change --stat"
> lists files with only whitespace differences as having changed with 0
> differing lines. This is inconsistent with the behavior without --
> stat, which doesn't list such files at all. (Same behavior with all
> the --ignore*space* flags.) I can reproduce this with the current
> "next", af746e4. Quick test case:
Hmm. This is pretty easy to do naively, but the special-casing for
addition/deletion (which I think we _do_ need, and which certainly we
fail t4205 without) makes me feel dirty. I'd worry there are other
cases, too (perhaps renames?). And I also notice that the
binary-diffstat code path just above my changes explicitly creates 0/0
diffstats, but I'm not even sure how one would trigger that.
So I dunno. A sensible rule to me is "iff -p would show a diff header,
then --stat should mention it". I think we'd want to somehow extract the
logic from builtin_diff() and reuse it.
---
diff --git a/diff.c b/diff.c
index e2eb6d66a..57ff5c1dc 100644
--- a/diff.c
+++ b/diff.c
@@ -2105,17 +2105,20 @@ static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *o
gather_dirstat(options, &dir, changed, "", 0);
}
+static void free_diffstat_file(struct diffstat_file *f)
+{
+ if (f->name != f->print_name)
+ free(f->print_name);
+ free(f->name);
+ free(f->from_name);
+ free(f);
+}
+
static void free_diffstat_info(struct diffstat_t *diffstat)
{
int i;
- for (i = 0; i < diffstat->nr; i++) {
- struct diffstat_file *f = diffstat->files[i];
- if (f->name != f->print_name)
- free(f->print_name);
- free(f->name);
- free(f->from_name);
- free(f);
- }
+ for (i = 0; i < diffstat->nr; i++)
+ free_diffstat_file(diffstat->files[i]);
free(diffstat->files);
}
@@ -2603,6 +2606,23 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
if (xdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,
&xpp, &xecfg))
die("unable to generate diffstat for %s", one->path);
+
+ /*
+ * Omit diffstats where nothing changed. Even if
+ * !same_contents, this might be the case due to ignoring
+ * whitespace changes, etc.
+ *
+ * But note that we special-case additions and deletions,
+ * as adding an empty file, for example, is still of interest.
+ */
+ if (DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two)) {
+ struct diffstat_file *file =
+ diffstat->files[diffstat->nr - 1];
+ if (!file->added && !file->deleted) {
+ free_diffstat_file(file);
+ diffstat->nr--;
+ }
+ }
}
diff_free_filespec_data(one);
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 289806d0c..2805db411 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -736,7 +736,7 @@ test_expect_success 'checkdiff allows new blank lines' '
cat <<EOF >expect
EOF
-test_expect_success 'whitespace-only changes not reported' '
+test_expect_success 'whitespace-only changes not reported (diff)' '
git reset --hard &&
echo >x "hello world" &&
git add x &&
@@ -746,6 +746,12 @@ test_expect_success 'whitespace-only changes not reported' '
test_cmp expect actual
'
+test_expect_success 'whitespace-only changes not reported (diffstat)' '
+ # reuse state from previous test
+ git diff --stat -b >actual &&
+ test_cmp expect actual
+'
+
cat <<EOF >expect
diff --git a/x b/z
similarity index NUM%
next prev parent reply other threads:[~2017-01-18 11:19 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-18 2:01 "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed" Matt McCutchen
2017-01-18 11:17 ` Jeff King [this message]
2017-01-18 20:57 ` Junio C Hamano
2017-01-18 21:08 ` Jeff King
2017-01-18 22:15 ` 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=20170118111705.6bqzkklluikda3r5@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=matt@mattmccutchen.net \
/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).