# "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed" 5 messages from 2017-01-18 to 2017-01-18. Participants: Matt McCutchen, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/44906 ## Matt McCutchen, 2017-01-18 02:01 Subject: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed" Message-ID: <1484704915.2096.16.camel@mattmccutchen.net> URL: https://gitlist.dev/e/1484704915.2096.16.camel%40mattmccutchen.net ``` 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: echo ' ' >test1 && echo '  ' >test2 && git diff --stat --no-index --ignore-space-change test1 test2 This caused me some inconvenience in the following scenario: I was reading a commit diff that had a bulk license change in all files combined with code changes. I attempted to revert the bulk license change locally using "sed" to more easily read the code diff, but my reversion left some whitespace diffs where the original files had inconsistent whitespace. So the diffstat after my reversion was cluttered with these "0" entries. Regards, Matt ``` ## Jeff King, 2017-01-18 11:17 Subject: Re: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed" Message-ID: <20170118111705.6bqzkklluikda3r5@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170118111705.6bqzkklluikda3r5%40sigill.intra.peff.net 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 <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 <expect diff --git a/x b/z similarity index NUM% ``` ## Jeff King, 2017-01-18 21:08 Subject: Re: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed" Message-ID: <20170118210821.xugr6jnvzgoxpynb@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170118210821.xugr6jnvzgoxpynb%40sigill.intra.peff.net In-Reply-To: ``` On Wed, Jan 18, 2017 at 12:57:12PM -0800, Junio C Hamano wrote: > Jeff King writes: > > > So I dunno. A sensible rule to me is "iff -p would show a diff header, > > then --stat should mention it". > > True but tricky (you need a better definition of "a diff header"). > > In addition to a new and deleted file, does a file whose executable > bit was flipped need mention? If so, then "diff --git" is the diff > header in the above. Otherwise "@@ ... @@", iow, "iff -p would show > any hunk". > > I think the patch implements the latter, which I think is sensible. I would think the former is more sensible (and is what my patch is working towards). Doing: >empty git add empty git diff --cached shows a "diff --git" header, but no hunk. I think it should show a diffstat (and does with my patch). I was thinking the rule should be something like: if (p->status == DIFF_STATUS_MODIFIED && !file->added && !file->deleted)) and otherwise include the entry, since it would be an add, delete, rename, etc, which carries useful information. Though a pure rename would not hit this code path at all, I would think (it would not trigger "!same_contents"). And a rename where there was a whitespace only change probably _should_ be omitted from "-b". Ditto for a pure mode change, I think. We do not run the contents through diff at all, so it does not hit this code path. -Peff ``` ## Junio C Hamano, 2017-01-18 20:57 Subject: Re: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed" Message-ID: URL: https://gitlist.dev/e/xmqqvatc3x3r.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20170118111705.6bqzkklluikda3r5@sigill.intra.peff.net> ``` Jeff King writes: > So I dunno. A sensible rule to me is "iff -p would show a diff header, > then --stat should mention it". True but tricky (you need a better definition of "a diff header"). In addition to a new and deleted file, does a file whose executable bit was flipped need mention? If so, then "diff --git" is the diff header in the above. Otherwise "@@ ... @@", iow, "iff -p would show any hunk". I think the patch implements the latter, which I think is sensible. > + /* > + * 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--; > + } > + } > } ``` ## Junio C Hamano, 2017-01-18 22:15 Subject: Re: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed" Message-ID: URL: https://gitlist.dev/e/xmqqtw8w2ewj.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20170118210821.xugr6jnvzgoxpynb@sigill.intra.peff.net> ``` Jeff King writes: > On Wed, Jan 18, 2017 at 12:57:12PM -0800, Junio C Hamano wrote: > >> Jeff King writes: >> >> > So I dunno. A sensible rule to me is "iff -p would show a diff header, >> > then --stat should mention it". >> >> True but tricky (you need a better definition of "a diff header"). >> >> In addition to a new and deleted file, does a file whose executable >> bit was flipped need mention? If so, then "diff --git" is the diff >> header in the above. Otherwise "@@ ... @@", iow, "iff -p would show >> any hunk". >> >> I think the patch implements the latter, which I think is sensible. > > I would think the former is more sensible (and is what my patch is > working towards). Doh (yes, "diff --git" was what I meant). As a mode-flipping patch does not have hunk but does show the header, it wants to be included in "git diff --stat" output, I would think, independent of -b issue. In fact chmod +x README.md git diff --stat does show a 0-line diffstat. > Doing: > > >empty > git add empty > git diff --cached > > shows a "diff --git" header, but no hunk. I think it should show a > diffstat (and does with my patch). > > I was thinking the rule should be something like: > > if (p->status == DIFF_STATUS_MODIFIED && > !file->added && !file->deleted)) > > and otherwise include the entry, since it would be an add, delete, > rename, etc, which carries useful information. > > Though a pure rename would not hit this code path at all, I would think > (it would not trigger "!same_contents"). And a rename where there was a > whitespace only change probably _should_ be omitted from "-b". > > Ditto for a pure mode change, I think. We do not run the contents > through diff at all, so it does not hit this code path. > > -Peff ```