threads / discuss / 44906

"git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"

Subject: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"

## tl;dr

5 messages between Jan 18, 2017 and Jan 18, 2017.

replies: 4people: 3as markdown or json

Matt McCutchen· Jan 18, 2017, 02:01 UTC · lore

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· Jan 18, 2017, 11:17 UTC · re: Matt McCutchen · lore

Re: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"

On Tue, Jan 17, 2017 at 09:01:55PM -0500, Matt McCutchen wrote:
Show 6 quoted lines
> 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%
Junio C Hamano· Jan 18, 2017, 20:57 UTC · re: Jeff King · lore

Re: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"

Jeff King <peff@peff.net> 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.
Show 17 quoted lines
> +		/*
> +		 * 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--;
> +			}
> +		}
>  	}
Jeff King· Jan 18, 2017, 21:08 UTC · re: Junio C Hamano · lore

Re: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"

On Wed, Jan 18, 2017 at 12:57:12PM -0800, Junio C Hamano wrote:
Show 13 quoted lines
> Jeff King <peff@peff.net> 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· Jan 18, 2017, 22:15 UTC · re: Jeff King · lore

Re: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"

Jeff King <peff@peff.net> writes:
Show 18 quoted lines
> On Wed, Jan 18, 2017 at 12:57:12PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> 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.
Show 25 quoted lines
> 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

← back to recent threads