Fix filemode comparison in diffs

File modes were both not being ignored properly on platforms
where they should be ignored, nor be diffed consistently on
platforms where they are supported.

This change adds a number of diff and status filemode change
tests.  This also makes sure that filemode-only changes are
included in the diff output when they occur and that filemode
changes are ignored successfully when core.filemode is false.

There is no code that automatically toggles core.filemode
based on the capabilities of the current platform, so the user
still needs to be careful in their .git/config file.

3 files changed, 40 insertions, 19 deletions

diff --git a/src/diff.c b/src/diff.c
index 90baa9588..e3167b90e 100644
--- a/src/diff.c
+++ b/src/diff.c
@@ -214,7 +214,9 @@ static int diff_delta__from_two(
 	git_diff_list *diff,
 	git_delta_t   status,
 	const git_index_entry *old_entry,
+	uint32_t old_mode,
 	const git_index_entry *new_entry,
+	uint32_t new_mode,
 	git_oid *new_oid)
 {
 	git_diff_delta *delta;
@@ -224,19 +226,22 @@ static int diff_delta__from_two(
 		return 0;
 
 	if ((diff->opts.flags & GIT_DIFF_REVERSE) != 0) {
-		const git_index_entry *temp = old_entry;
+		uint32_t temp_mode = old_mode;
+		const git_index_entry *temp_entry = old_entry;
 		old_entry = new_entry;
-		new_entry = temp;
+		new_entry = temp_entry;
+		old_mode = new_mode;
+		new_mode = temp_mode;
 	}
 
 	delta = diff_delta__alloc(diff, status, old_entry->path);
 	GITERR_CHECK_ALLOC(delta);
 
-	delta->old_file.mode = old_entry->mode;
+	delta->old_file.mode = old_mode;
 	git_oid_cpy(&delta->old_file.oid, &old_entry->oid);
 	delta->old_file.flags |= GIT_DIFF_FILE_VALID_OID;
 
-	delta->new_file.mode = new_entry->mode;
+	delta->new_file.mode = new_mode;
 	git_oid_cpy(&delta->new_file.oid, new_oid ? new_oid : &new_entry->oid);
 	if (new_oid || !git_oid_iszero(&new_entry->oid))
 		delta->new_file.flags |= GIT_DIFF_FILE_VALID_OID;
@@ -300,7 +305,7 @@ static git_diff_list *git_diff_list_alloc(
 	if (config_bool(cfg, "core.ignorestat", 0))
 		diff->diffcaps = diff->diffcaps | GIT_DIFFCAPS_ASSUME_UNCHANGED;
 	if (config_bool(cfg, "core.filemode", 1))
-		diff->diffcaps = diff->diffcaps | GIT_DIFFCAPS_TRUST_EXEC_BIT;
+		diff->diffcaps = diff->diffcaps | GIT_DIFFCAPS_TRUST_MODE_BITS;
 	if (config_bool(cfg, "core.trustctime", 1))
 		diff->diffcaps = diff->diffcaps | GIT_DIFFCAPS_TRUST_CTIME;
 	/* Don't set GIT_DIFFCAPS_USE_DEV - compile time option in core git */
@@ -419,7 +424,7 @@ static int oid_for_workdir_item(
 	return result;
 }
 
-#define EXEC_BIT_MASK 0000111
+#define MODE_BITS_MASK 0000777
 
 static int maybe_modified(
 	git_iterator *old_iter,
@@ -443,13 +448,13 @@ static int maybe_modified(
 		!(diff->diffcaps & GIT_DIFFCAPS_HAS_SYMLINKS))
 		nmode = GIT_MODE_TYPE(omode) | (nmode & GIT_MODE_PERMS_MASK);
 
-	/* on platforms with no execmode, clear exec bit from comparisons */
-	if (!(diff->diffcaps & GIT_DIFFCAPS_TRUST_EXEC_BIT)) {
-		omode = omode & ~EXEC_BIT_MASK;
-		nmode = nmode & ~EXEC_BIT_MASK;
-	}
+	/* on platforms with no execmode, just preserve old mode */
+	if (!(diff->diffcaps & GIT_DIFFCAPS_TRUST_MODE_BITS) &&
+		(nmode & MODE_BITS_MASK) != (omode & MODE_BITS_MASK) &&
+		new_iter->type == GIT_ITERATOR_WORKDIR)
+		nmode = (nmode & ~MODE_BITS_MASK) | (omode & MODE_BITS_MASK);
 
-	/* support "assume unchanged" (badly, b/c we still stat everything) */
+	/* support "assume unchanged" (poorly, b/c we still stat everything) */
 	if ((diff->diffcaps & GIT_DIFFCAPS_ASSUME_UNCHANGED) != 0)
 		status = (oitem->flags_extended & GIT_IDXENTRY_INTENT_TO_ADD) ?
 			GIT_DELTA_MODIFIED : GIT_DELTA_UNMODIFIED;
@@ -471,8 +476,13 @@ static int maybe_modified(
 			 omode == nmode)
 		status = GIT_DELTA_UNMODIFIED;
 
-	/* if we have a workdir item with an unknown oid, check deeper */
-	else if (git_oid_iszero(&nitem->oid) && new_iter->type == GIT_ITERATOR_WORKDIR) {
+	/* if modes match and we have an unknown OID and a workdir iterator,
+	 * then check deeper for matching
+	 */
+	else if (omode == nmode &&
+		git_oid_iszero(&nitem->oid) &&
+		new_iter->type == GIT_ITERATOR_WORKDIR)
+	{
 		/* TODO: add check against index file st_mtime to avoid racy-git */
 
 		/* if they files look exactly alike, then we'll assume the same */
@@ -517,7 +527,8 @@ static int maybe_modified(
 		use_noid = &noid;
 	}
 
-	return diff_delta__from_two(diff, status, oitem, nitem, use_noid);
+	return diff_delta__from_two(
+		diff, status, oitem, omode, nitem, nmode, use_noid);
 }
 
 static int diff_from_iterators(
diff --git a/src/diff.h b/src/diff.h
index ac2457956..6cc854fbd 100644
--- a/src/diff.h
+++ b/src/diff.h
@@ -20,7 +20,7 @@
 enum {
 	GIT_DIFFCAPS_HAS_SYMLINKS     = (1 << 0), /* symlinks on platform? */
 	GIT_DIFFCAPS_ASSUME_UNCHANGED = (1 << 1), /* use stat? */
-	GIT_DIFFCAPS_TRUST_EXEC_BIT   = (1 << 2), /* use st_mode exec bit? */
+	GIT_DIFFCAPS_TRUST_MODE_BITS  = (1 << 2), /* use st_mode? */
 	GIT_DIFFCAPS_TRUST_CTIME      = (1 << 3), /* use st_ctime? */
 	GIT_DIFFCAPS_USE_DEV          = (1 << 4), /* use st_dev? */
 };
@@ -36,5 +36,8 @@ struct git_diff_list {
 	uint32_t diffcaps;
 };
 
+extern void git_diff__cleanup_modes(
+	uint32_t diffcaps, uint32_t *omode, uint32_t *nmode);
+
 #endif
 
diff --git a/src/diff_output.c b/src/diff_output.c
index 1c65e1bb8..d1aa910b3 100644
--- a/src/diff_output.c
+++ b/src/diff_output.c
@@ -359,7 +359,7 @@ int git_diff_foreach(
 
 		/* map files */
 		if (delta->binary != 1 &&
-			(hunk_cb || line_cb) &&
+			(hunk_cb || line_cb || git_oid_iszero(&delta->old_file.oid)) &&
 			(delta->status == GIT_DELTA_DELETED ||
 			 delta->status == GIT_DELTA_MODIFIED))
 		{
@@ -397,7 +397,9 @@ int git_diff_foreach(
 				/* since we did not have the definitive oid, we may have
 				 * incorrect status and need to skip this item.
 				 */
-				if (git_oid_cmp(&delta->old_file.oid, &delta->new_file.oid) == 0) {
+				if (delta->old_file.mode == delta->new_file.mode &&
+					!git_oid_cmp(&delta->old_file.oid, &delta->new_file.oid))
+				{
 					delta->status = GIT_DELTA_UNMODIFIED;
 					if ((diff->opts.flags & GIT_DIFF_INCLUDE_UNMODIFIED) == 0)
 						goto cleanup;
@@ -420,7 +422,8 @@ int git_diff_foreach(
 		 */
 
 		if (file_cb != NULL) {
-			error = file_cb(data, delta, (float)info.index / diff->deltas.length);
+			error = file_cb(
+				data, delta, (float)info.index / diff->deltas.length);
 			if (error < 0)
 				goto cleanup;
 		}
@@ -433,6 +436,10 @@ int git_diff_foreach(
 		if (!old_data.len && !new_data.len)
 			goto cleanup;
 
+		/* nothing to do if only diff was a mode change */
+		if (!git_oid_cmp(&delta->old_file.oid, &delta->new_file.oid))
+			goto cleanup;
+
 		assert(hunk_cb || line_cb);
 
 		info.delta = delta;