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.

include/git2/status.h

@@ -102,7 +102,7 @@ enum {
 	GIT_STATUS_OPT_INCLUDE_UNTRACKED = (1 << 0),
 	GIT_STATUS_OPT_INCLUDE_IGNORED = (1 << 1),
 	GIT_STATUS_OPT_INCLUDE_UNMODIFIED = (1 << 2),
-	GIT_STATUS_OPT_EXCLUDE_SUBMODULED = (1 << 3),
+	GIT_STATUS_OPT_EXCLUDE_SUBMODULES = (1 << 3),
 	GIT_STATUS_OPT_RECURSE_UNTRACKED_DIRS = (1 << 4),
 };
 

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(

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
 

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;

tests-clar/clar_helpers.c

@@ -155,3 +155,27 @@ void cl_git_sandbox_cleanup(void)
 	}
 }
 
+bool cl_toggle_filemode(const char *filename)
+{
+	struct stat st1, st2;
+
+	cl_must_pass(p_stat(filename, &st1));
+	cl_must_pass(p_chmod(filename, st1.st_mode ^ 0100));
+	cl_must_pass(p_stat(filename, &st2));
+
+	return (st1.st_mode != st2.st_mode);
+}
+
+bool cl_is_chmod_supported(void)
+{
+	static int _is_supported = -1;
+
+	if (_is_supported < 0) {
+		cl_git_mkfile("filemode.t", "Test if filemode can be modified");
+		_is_supported = cl_toggle_filemode("filemode.t");
+		cl_must_pass(p_unlink("filemode.t"));
+	}
+
+	return _is_supported;
+}
+

tests-clar/clar_libgit2.h

@@ -40,6 +40,9 @@ void cl_git_append2file(const char *filename, const char *new_content);
 void cl_git_rewritefile(const char *filename, const char *new_content);
 void cl_git_write2file(const char *filename, const char *new_content, int mode);
 
+bool cl_toggle_filemode(const char *filename);
+bool cl_is_chmod_supported(void);
+
 /* Environment wrappers */
 char *cl_getenv(const char *name);
 int cl_setenv(const char *name, const char *value);

tests-clar/diff/workdir.c

@@ -5,7 +5,6 @@ static git_repository *g_repo = NULL;
 
 void test_diff_workdir__initialize(void)
 {
-	g_repo = cl_git_sandbox_init("status");
 }
 
 void test_diff_workdir__cleanup(void)
@@ -19,6 +18,8 @@ void test_diff_workdir__to_index(void)
 	git_diff_list *diff = NULL;
 	diff_expects exp;
 
+	g_repo = cl_git_sandbox_init("status");
+
 	opts.context_lines = 3;
 	opts.interhunk_lines = 1;
 	opts.flags |= GIT_DIFF_INCLUDE_IGNORED | GIT_DIFF_INCLUDE_UNTRACKED;
@@ -59,13 +60,17 @@ void test_diff_workdir__to_tree(void)
 	/* grabbed a couple of commit oids from the history of the attr repo */
 	const char *a_commit = "26a125ee1bf"; /* the current HEAD */
 	const char *b_commit = "0017bd4ab1ec3"; /* the start */
-	git_tree *a = resolve_commit_oid_to_tree(g_repo, a_commit);
-	git_tree *b = resolve_commit_oid_to_tree(g_repo, b_commit);
+	git_tree *a, *b;
 	git_diff_options opts = {0};
 	git_diff_list *diff = NULL;
 	git_diff_list *diff2 = NULL;
 	diff_expects exp;
 
+	g_repo = cl_git_sandbox_init("status");
+
+	a = resolve_commit_oid_to_tree(g_repo, a_commit);
+	b = resolve_commit_oid_to_tree(g_repo, b_commit);
+
 	opts.context_lines = 3;
 	opts.interhunk_lines = 1;
 	opts.flags |= GIT_DIFF_INCLUDE_IGNORED | GIT_DIFF_INCLUDE_UNTRACKED;
@@ -171,6 +176,8 @@ void test_diff_workdir__to_index_with_pathspec(void)
 	diff_expects exp;
 	char *pathspec = NULL;
 
+	g_repo = cl_git_sandbox_init("status");
+
 	opts.context_lines = 3;
 	opts.interhunk_lines = 1;
 	opts.flags |= GIT_DIFF_INCLUDE_IGNORED | GIT_DIFF_INCLUDE_UNTRACKED;
@@ -237,6 +244,102 @@ void test_diff_workdir__to_index_with_pathspec(void)
 	git_diff_list_free(diff);
 }
 
+void test_diff_workdir__filemode_changes(void)
+{
+	git_config *cfg;
+	git_diff_list *diff = NULL;
+	diff_expects exp;
+
+	if (!cl_is_chmod_supported())
+		return;
+
+	g_repo = cl_git_sandbox_init("issue_592");
+
+	cl_git_pass(git_repository_config(&cfg, g_repo));
+	cl_git_pass(git_config_set_bool(cfg, "core.filemode", true));
+
+	/* test once with no mods */
+
+	cl_git_pass(git_diff_workdir_to_index(g_repo, NULL, &diff));
+
+	memset(&exp, 0, sizeof(exp));
+	cl_git_pass(git_diff_foreach(
+		diff, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	cl_assert_equal_i(0, exp.files);
+	cl_assert_equal_i(0, exp.file_mods);
+	cl_assert_equal_i(0, exp.hunks);
+
+	git_diff_list_free(diff);
+
+	/* chmod file and test again */
+
+	cl_assert(cl_toggle_filemode("issue_592/a.txt"));
+
+	cl_git_pass(git_diff_workdir_to_index(g_repo, NULL, &diff));
+
+	memset(&exp, 0, sizeof(exp));
+	cl_git_pass(git_diff_foreach(
+		diff, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	cl_assert_equal_i(1, exp.files);
+	cl_assert_equal_i(1, exp.file_mods);
+	cl_assert_equal_i(0, exp.hunks);
+
+	git_diff_list_free(diff);
+
+	cl_assert(cl_toggle_filemode("issue_592/a.txt"));
+	git_config_free(cfg);
+}
+
+void test_diff_workdir__filemode_changes_with_filemode_false(void)
+{
+	git_config *cfg;
+	git_diff_list *diff = NULL;
+	diff_expects exp;
+
+	if (!cl_is_chmod_supported())
+		return;
+
+	g_repo = cl_git_sandbox_init("issue_592");
+
+	cl_git_pass(git_repository_config(&cfg, g_repo));
+	cl_git_pass(git_config_set_bool(cfg, "core.filemode", false));
+
+	/* test once with no mods */
+
+	cl_git_pass(git_diff_workdir_to_index(g_repo, NULL, &diff));
+
+	memset(&exp, 0, sizeof(exp));
+	cl_git_pass(git_diff_foreach(
+		diff, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	cl_assert_equal_i(0, exp.files);
+	cl_assert_equal_i(0, exp.file_mods);
+	cl_assert_equal_i(0, exp.hunks);
+
+	git_diff_list_free(diff);
+
+	/* chmod file and test again */
+
+	cl_assert(cl_toggle_filemode("issue_592/a.txt"));
+
+	cl_git_pass(git_diff_workdir_to_index(g_repo, NULL, &diff));
+
+	memset(&exp, 0, sizeof(exp));
+	cl_git_pass(git_diff_foreach(
+		diff, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	cl_assert_equal_i(0, exp.files);
+	cl_assert_equal_i(0, exp.file_mods);
+	cl_assert_equal_i(0, exp.hunks);
+
+	git_diff_list_free(diff);
+
+	cl_assert(cl_toggle_filemode("issue_592/a.txt"));
+	git_config_free(cfg);
+}
+
 /* PREPARATION OF TEST DATA
  *
  * Since there is no command line equivalent of git_diff_workdir_to_tree,

tests-clar/resources/filemodes/.gitted/HEAD

@@ -0,0 +1 @@
+ref: refs/heads/master

tests-clar/resources/filemodes/.gitted/config

@@ -0,0 +1,6 @@
+[core]
+	repositoryformatversion = 0
+	filemode = true
+	bare = false
+	logallrefupdates = true
+	ignorecase = true

tests-clar/resources/filemodes/.gitted/description

@@ -0,0 +1 @@
+Unnamed repository; edit this file 'description' to name the repository.

tests-clar/resources/filemodes/.gitted/hooks/commit-msg.sample

@@ -0,0 +1,24 @@
+#!/bin/sh
+#
+# An example hook script to check the commit log message.
+# Called by "git commit" with one argument, the name of the file
+# that has the commit message.  The hook should exit with non-zero
+# status after issuing an appropriate message if it wants to stop the
+# commit.  The hook is allowed to edit the commit message file.
+#
+# To enable this hook, rename this file to "commit-msg".
+
+# Uncomment the below to add a Signed-off-by line to the message.
+# Doing this in a hook is a bad idea in general, but the prepare-commit-msg
+# hook is more suited to it.
+#
+# SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
+# grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"
+
+# This example catches duplicate Signed-off-by lines.
+
+test "" = "$(grep '^Signed-off-by: ' "$1" |
+	 sort | uniq -c | sed -e '/^[ 	]*1[ 	]/d')" || {
+	echo >&2 Duplicate Signed-off-by lines.
+	exit 1
+}

tests-clar/resources/filemodes/.gitted/info/exclude

@@ -0,0 +1,6 @@
+# git ls-files --others --exclude-from=.git/info/exclude
+# Lines that start with '#' are comments.
+# For a project mostly in C, the following would be a good set of
+# exclude patterns (uncomment them if you want to use them):
+# *.[oa]
+# *~

tests-clar/resources/filemodes/.gitted/logs/HEAD

@@ -0,0 +1 @@
+0000000000000000000000000000000000000000 9962c8453ba6f0cf8dac7c5dcc2fa2897fa9964a Russell Belfer <rb@github.com> 1338847682 -0700	commit (initial): Initial commit of test data

tests-clar/resources/filemodes/.gitted/logs/refs/heads/master

@@ -0,0 +1 @@
+0000000000000000000000000000000000000000 9962c8453ba6f0cf8dac7c5dcc2fa2897fa9964a Russell Belfer <rb@github.com> 1338847682 -0700	commit (initial): Initial commit of test data

tests-clar/resources/filemodes/.gitted/refs/heads/master

@@ -0,0 +1 @@
+9962c8453ba6f0cf8dac7c5dcc2fa2897fa9964a

tests-clar/resources/filemodes/exec_off

@@ -0,0 +1 @@
+Howdy

tests-clar/resources/filemodes/exec_off2on_staged

@@ -0,0 +1 @@
+Howdy

tests-clar/resources/filemodes/exec_off2on_workdir

@@ -0,0 +1 @@
+Howdy

tests-clar/resources/filemodes/exec_off_untracked

@@ -0,0 +1 @@
+Howdy

tests-clar/resources/filemodes/exec_on

@@ -0,0 +1 @@
+Howdy

tests-clar/resources/filemodes/exec_on2off_staged

@@ -0,0 +1 @@
+Howdy

tests-clar/resources/filemodes/exec_on2off_workdir

@@ -0,0 +1 @@
+Howdy

tests-clar/resources/filemodes/exec_on_untracked

@@ -0,0 +1 @@
+Howdy

tests-clar/status/worktree.c

@@ -581,3 +581,69 @@ void test_status_worktree__space_in_filename(void)
 	git_index_free(index);
 	git_repository_free(repo);
 }
+
+static const char *filemode_paths[] = {
+	"exec_off",
+	"exec_off2on_staged",
+	"exec_off2on_workdir",
+	"exec_off_untracked",
+	"exec_on",
+	"exec_on2off_staged",
+	"exec_on2off_workdir",
+	"exec_on_untracked",
+};
+
+static unsigned int filemode_statuses[] = {
+	GIT_STATUS_CURRENT,
+	GIT_STATUS_INDEX_MODIFIED,
+	GIT_STATUS_WT_MODIFIED,
+	GIT_STATUS_WT_NEW,
+	GIT_STATUS_CURRENT,
+	GIT_STATUS_INDEX_MODIFIED,
+	GIT_STATUS_WT_MODIFIED,
+	GIT_STATUS_WT_NEW
+};
+
+static const size_t filemode_count = 8;
+
+void test_status_worktree__filemode_changes(void)
+{
+	git_repository *repo = cl_git_sandbox_init("filemodes");
+	status_entry_counts counts;
+	git_status_options opts;
+	git_config *cfg;
+
+	/* overwrite stored filemode with platform appropriate value */
+	cl_git_pass(git_repository_config(&cfg, repo));
+	if (cl_is_chmod_supported())
+		cl_git_pass(git_config_set_bool(cfg, "core.filemode", true));
+	else {
+		unsigned int i;
+		cl_git_pass(git_config_set_bool(cfg, "core.filemode", false));
+
+		/* won't trust filesystem mode diffs, so these will appear unchanged */
+		for (i = 0; i < filemode_count; ++i)
+			if (filemode_statuses[i] == GIT_STATUS_WT_MODIFIED)
+				filemode_statuses[i] = GIT_STATUS_CURRENT;
+	}
+
+	memset(&opts, 0, sizeof(opts));
+	opts.flags = GIT_STATUS_OPT_INCLUDE_UNTRACKED |
+		GIT_STATUS_OPT_INCLUDE_IGNORED |
+		GIT_STATUS_OPT_INCLUDE_UNMODIFIED;
+
+	memset(&counts, 0, sizeof(counts));
+	counts.expected_entry_count = filemode_count;
+	counts.expected_paths = filemode_paths;
+	counts.expected_statuses = filemode_statuses;
+
+	cl_git_pass(
+		git_status_foreach_ext(repo, &opts, cb_status__normal, &counts)
+	);
+
+	cl_assert_equal_i(counts.expected_entry_count, counts.entry_count);
+	cl_assert_equal_i(0, counts.wrong_status_flags_count);
+	cl_assert_equal_i(0, counts.wrong_sorted_path);
+
+	git_config_free(cfg);
+}