run_diff_files: do not look at uninitialized stat datajk/diff-files-assume-unchanged

If we try to diff an index entry marked CE_VALID (because it
was marked with --assume-unchanged), we do not bother even
running stat() on the file to see if it was removed. This
started long ago with 540e694 (Prevent diff machinery from
examining assume-unchanged entries on worktree, 2009-08-11).

However, the subsequent code may look at our "struct stat"
and expect to find actual data; currently it will find
whatever cruft was left on the stack. This can cause
problems in two situations:

  1. We call match_stat_with_submodule with the stat data,
     so a submodule may be erroneously marked as changed.

  2. If --find-copies-harder is in effect, we pass all
     entries, even unchanged ones, to diff_change, so it can
     list them as rename/copy sources. Since we found no
     change, we assume that function will realize it and not
     actually display any diff output. However, we end up
     feeding it a bogus mode, leading it to sometimes claim
     there was a mode change.

We can fix both by splitting the CE_VALID and regular code
paths, and making sure only to look at the stat information
in the latter. Furthermore, we push the declaration of our
"struct stat" down into the code paths that actually set it,
so we cannot accidentally access it uninitialized in future
code.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

1 files changed, 21 insertions, 12 deletions

diff --git a/diff-lib.c b/diff-lib.c
index 346cac651d..73eedca9c8 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -99,7 +99,6 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 		diff_unmerged_stage = 2;
 	entries = active_nr;
 	for (i = 0; i < entries; i++) {
-		struct stat st;
 		unsigned int oldmode, newmode;
 		struct cache_entry *ce = active_cache[i];
 		int changed;
@@ -117,6 +116,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			unsigned int wt_mode = 0;
 			int num_compare_stages = 0;
 			size_t path_len;
+			struct stat st;
 
 			path_len = ce_namelen(ce);
 
@@ -198,26 +198,35 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			continue;
 
 		/* If CE_VALID is set, don't look at workdir for file removal */
-		changed = (ce->ce_flags & CE_VALID) ? 0 : check_removed(ce, &st);
-		if (changed) {
-			if (changed < 0) {
-				perror(ce->name);
+		if (ce->ce_flags & CE_VALID) {
+			changed = 0;
+			newmode = ce->ce_mode;
+		} else {
+			struct stat st;
+
+			changed = check_removed(ce, &st);
+			if (changed) {
+				if (changed < 0) {
+					perror(ce->name);
+					continue;
+				}
+				diff_addremove(&revs->diffopt, '-', ce->ce_mode,
+					       ce->sha1, !is_null_sha1(ce->sha1),
+					       ce->name, 0);
 				continue;
 			}
-			diff_addremove(&revs->diffopt, '-', ce->ce_mode,
-				       ce->sha1, !is_null_sha1(ce->sha1),
-				       ce->name, 0);
-			continue;
+
+			changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
+							    ce_option, &dirty_submodule);
+			newmode = ce_mode_from_stat(ce, st.st_mode);
 		}
-		changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
-						    ce_option, &dirty_submodule);
+
 		if (!changed && !dirty_submodule) {
 			ce_mark_uptodate(ce);
 			if (!DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))
 				continue;
 		}
 		oldmode = ce->ce_mode;
-		newmode = ce_mode_from_stat(ce, st.st_mode);
 		diff_change(&revs->diffopt, oldmode, newmode,
 			    ce->sha1, (changed ? null_sha1 : ce->sha1),
 			    !is_null_sha1(ce->sha1), (changed ? 0 : !is_null_sha1(ce->sha1)),