Re: [PATCH] diff: teach --stat to ignore uninteresting modifications

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 2, 2020, 00:31 UTC
Message-ID: <xmqqv9i128iq.fsf@gitster.c.googlers.com>
In-Reply-To: <pull.689.git.1596324796918.gitgitgadget@gmail.com>

"Matthew Rogers via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 21 quoted lines

> +		if (DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two)) {
> +			struct diffstat_file *file = 
> +				diffstat->files[diffstat->nr - 1];
> +			/*
> +			 * Omit diffstats of modified files 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 (p->status == DIFF_STATUS_MODIFIED 
> +				&& !file->added
> +				&& !file->deleted) {
> +				free_diffstat_file(file);
> +				diffstat->nr--;
> +			}
> +		}
>  	}
> 
>  	diff_free_filespec_data(one);

There are some "trailing whitespace" errors detected by "git am/apply".

Show 23 quoted lines

> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
> index 88d3026894..32c1b967f9 100755
> --- a/t/t4015-diff-whitespace.sh
> +++ b/t/t4015-diff-whitespace.sh
> @@ -789,7 +789,7 @@ test_expect_success 'checkdiff allows new blank lines' '
>  	git diff --check
>  '
>  
> -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 &&
> @@ -799,6 +799,12 @@ test_expect_success 'whitespace-only changes not reported' '
>  	test_must_be_empty actual
>  '
>  
> +test_expect_success 'whitespace-only changes not reported (diffstat)' '
> +	# reuse state from previous test
> +	git diff --stat -b >actual &&
> +	test_must_be_empty actual
> +'
> +

This is a "let's show off our shiny new toy" test, which shows that the code change covered the case you are interested in changing. We'd also need tests that makes sure that the effect of the code change is not seen when it should not trigger.

For example, if we further change mode bits of file 'x' (which has a whitespace-only changes applied in the test in the previous hunk), e.g.

	git update-index --chmod=+x x &&
	git diff --stat -b --cached >actual

should that be counted as a file with 0-line change that is worth reporting, or is it hidden? I _think_ the new code will do a wrong thing here. That is,

 - If the change truly is only mode bits and one and two have
   "same_contents", the new code is bypassed, and we'll continue to
   show "0 lines changed, but the file is worth reporting".

 - If the change is whitespace-only change plus mode bits, i.e. one
   and two do not have "same_contents", the new code triggers and
   the stat output is suppressed for the path.