Merge pull request #967 from arrbee/diff-submodule-tests-and-fixes

Diff submodule tests and fixes

10 files changed, 229 insertions, 40 deletions

diff --git a/include/git2/diff.h b/include/git2/diff.h
index 1f7f8ab2a..121c40307 100644
--- a/include/git2/diff.h
+++ b/include/git2/diff.h
@@ -47,7 +47,8 @@ enum {
 	GIT_DIFF_INCLUDE_UNMODIFIED = (1 << 9),
 	GIT_DIFF_RECURSE_UNTRACKED_DIRS = (1 << 10),
 	GIT_DIFF_DISABLE_PATHSPEC_MATCH = (1 << 11),
-	GIT_DIFF_DELTAS_ARE_ICASE = (1 << 12)
+	GIT_DIFF_DELTAS_ARE_ICASE = (1 << 12),
+	GIT_DIFF_INCLUDE_UNTRACKED_CONTENT = (1 << 13),
 };
 
 /**
diff --git a/include/git2/submodule.h b/include/git2/submodule.h
index 28057d26f..b00fad2d4 100644
--- a/include/git2/submodule.h
+++ b/include/git2/submodule.h
@@ -120,8 +120,15 @@ typedef enum {
 } git_submodule_status_t;
 
 #define GIT_SUBMODULE_STATUS_IS_UNMODIFIED(S) \
-	(((S) & ~(GIT_SUBMODULE_STATUS_IN_HEAD | GIT_SUBMODULE_STATUS_IN_INDEX | \
-	GIT_SUBMODULE_STATUS_IN_CONFIG | GIT_SUBMODULE_STATUS_IN_WD)) == 0)
+	(((S) & ~(GIT_SUBMODULE_STATUS_IN_HEAD | \
+	GIT_SUBMODULE_STATUS_IN_INDEX | \
+	GIT_SUBMODULE_STATUS_IN_CONFIG | \
+	GIT_SUBMODULE_STATUS_IN_WD)) == 0)
+
+#define GIT_SUBMODULE_STATUS_IS_WD_DIRTY(S) \
+	(((S) & (GIT_SUBMODULE_STATUS_WD_INDEX_MODIFIED | \
+	GIT_SUBMODULE_STATUS_WD_WD_MODIFIED | \
+	GIT_SUBMODULE_STATUS_WD_UNTRACKED)) != 0)
 
 /**
  * Lookup submodule information by name or path.
diff --git a/src/diff.c b/src/diff.c
index 7a0051ae3..8ab8af3a1 100644
--- a/src/diff.c
+++ b/src/diff.c
@@ -224,7 +224,10 @@ static int diff_delta__from_one(
 	}
 
 	delta->old_file.flags |= GIT_DIFF_FILE_VALID_OID;
-	delta->new_file.flags |= GIT_DIFF_FILE_VALID_OID;
+
+	if (delta->status == GIT_DELTA_DELETED ||
+		!git_oid_iszero(&delta->new_file.oid))
+		delta->new_file.flags |= GIT_DIFF_FILE_VALID_OID;
 
 	if (git_vector_insert(&diff->deltas, delta) < 0) {
 		git__free(delta);
@@ -441,17 +444,28 @@ static int oid_for_workdir_item(
 	const git_index_entry *item,
 	git_oid *oid)
 {
-	int result;
+	int result = 0;
 	git_buf full_path = GIT_BUF_INIT;
 
-	if (git_buf_joinpath(&full_path, git_repository_workdir(repo), item->path) < 0)
+	if (git_buf_joinpath(
+		&full_path, git_repository_workdir(repo), item->path) < 0)
 		return -1;
 
-	/* calculate OID for file if possible*/
+	/* calculate OID for file if possible */
 	if (S_ISGITLINK(item->mode)) {
-		/* Don't bother to figure out an oid for a submodule. We won't use it anyway. */
-		memset(oid, 0, sizeof(*oid));
-		result = 0;
+		git_submodule *sm;
+		const git_oid *sm_oid;
+
+		if (!git_submodule_lookup(&sm, repo, item->path) &&
+			(sm_oid = git_submodule_wd_oid(sm)) != NULL)
+			git_oid_cpy(oid, sm_oid);
+		else {
+			/* if submodule lookup failed probably just in an intermediate
+			 * state where some init hasn't happened, so ignore the error
+			 */
+			giterr_clear();
+			memset(oid, 0, sizeof(*oid));
+		}
 	} else if (S_ISLNK(item->mode))
 		result = git_odb__hashlink(oid, full_path.ptr);
 	else if (!git__is_sizet(item->file_size)) {
@@ -570,6 +584,15 @@ static int maybe_modified(
 					return -1;
 				status = GIT_SUBMODULE_STATUS_IS_UNMODIFIED(sm_status)
 						 ? GIT_DELTA_UNMODIFIED : GIT_DELTA_MODIFIED;
+
+				/* grab OID while we are here */
+				if (git_oid_iszero(&nitem->oid)) {
+					const git_oid *sm_oid = git_submodule_wd_oid(sub);
+					if (sub != NULL) {
+						git_oid_cpy(&noid, sm_oid);
+						use_noid = &noid;
+					}
+				}
 			}
 		}
 	}
@@ -669,7 +692,8 @@ static int diff_from_iterators(
 
 			/* check if contained in ignored parent directory */
 			if (git_buf_len(&ignore_prefix) &&
-				ITERATOR_PREFIXCMP(*old_iter, nitem->path, git_buf_cstr(&ignore_prefix)) == 0)
+				ITERATOR_PREFIXCMP(*old_iter, nitem->path,
+					git_buf_cstr(&ignore_prefix)) == 0)
 				delta_type = GIT_DELTA_IGNORED;
 
 			if (S_ISDIR(nitem->mode)) {
@@ -677,10 +701,23 @@ static int diff_from_iterators(
 				 * it or if the user requested the contents of untracked
 				 * directories and it is not under an ignored directory.
 				 */
-				if ((oitem && ITERATOR_PREFIXCMP(*old_iter, oitem->path, nitem->path) == 0) ||
+				bool contains_tracked =
+					(oitem &&
+					 !ITERATOR_PREFIXCMP(*old_iter, oitem->path, nitem->path));
+				bool recurse_untracked =
 					(delta_type == GIT_DELTA_UNTRACKED &&
-					 (diff->opts.flags & GIT_DIFF_RECURSE_UNTRACKED_DIRS) != 0))
-				{
+					 (diff->opts.flags & GIT_DIFF_RECURSE_UNTRACKED_DIRS) != 0);
+
+				/* do not advance into directories that contain a .git file */
+				if (!contains_tracked && recurse_untracked) {
+					git_buf *full = NULL;
+					if (git_iterator_current_workdir_path(new_iter, &full) < 0)
+						goto fail;
+					if (git_path_contains_dir(full, DOT_GIT))
+						recurse_untracked = false;
+				}
+
+				if (contains_tracked || recurse_untracked) {
 					/* if this directory is ignored, remember it as the
 					 * "ignore_prefix" for processing contained items
 					 */
diff --git a/src/diff_output.c b/src/diff_output.c
index f5f6c381e..10fbd391c 100644
--- a/src/diff_output.c
+++ b/src/diff_output.c
@@ -275,30 +275,34 @@ static int get_workdir_sm_content(
 	int error = 0;
 	git_buf content = GIT_BUF_INIT;
 	git_submodule* sm = NULL;
-	const git_oid* sm_head = NULL;
 	unsigned int sm_status = 0;
 	const char* sm_status_text = "";
 	char oidstr[GIT_OID_HEXSZ+1];
 
-	if ((error = git_submodule_lookup(&sm, ctxt->repo, file->path)) < 0) {
+	if ((error = git_submodule_lookup(&sm, ctxt->repo, file->path)) < 0 ||
+		(error = git_submodule_status(&sm_status, sm)) < 0)
 		return error;
-	}
 
-	if ((sm_head = git_submodule_head_oid(sm)) == NULL) {
-		giterr_set(GITERR_SUBMODULE, "Cannot find head of submodule '%s'", file->path);
-		return -1;
-	}
+	/* update OID if we didn't have it previously */
+	if ((file->flags & GIT_DIFF_FILE_VALID_OID) == 0) {
+		const git_oid* sm_head;
 
-	if ((error = git_submodule_status(&sm_status, sm)) < 0) {
-		return -1;
+		if ((sm_head = git_submodule_wd_oid(sm)) != NULL ||
+			(sm_head = git_submodule_head_oid(sm)) != NULL)
+		{
+			git_oid_cpy(&file->oid, sm_head);
+			file->flags |= GIT_DIFF_FILE_VALID_OID;
+		}
 	}
-	if (!GIT_SUBMODULE_STATUS_IS_UNMODIFIED(sm_status)) {
+
+	git_oid_fmt(oidstr, &file->oid);
+	oidstr[GIT_OID_HEXSZ] = '\0';
+
+	if (GIT_SUBMODULE_STATUS_IS_WD_DIRTY(sm_status))
 		sm_status_text = "-dirty";
-	}
 
-	git_oid_fmt(oidstr, sm_head);
-	oidstr[GIT_OID_HEXSZ] = 0;
-	git_buf_printf(&content, "Subproject commit %s%s\n", oidstr, sm_status_text );
+	git_buf_printf(&content, "Subproject commit %s%s\n",
+				   oidstr, sm_status_text);
 
 	map->data = git_buf_detach(&content);
 	map->len = strlen(map->data);
@@ -318,9 +322,12 @@ static int get_workdir_content(
 	git_buf path = GIT_BUF_INIT;
 	const char *wd = git_repository_workdir(ctxt->repo);
 
-	if (file->mode == GIT_FILEMODE_COMMIT)
+	if (S_ISGITLINK(file->mode))
 		return get_workdir_sm_content(ctxt, file, map);
 
+	if (S_ISDIR(file->mode))
+		return 0;
+
 	if (git_buf_joinpath(&path, wd, file->path) < 0)
 		return -1;
 
@@ -535,6 +542,11 @@ static int diff_patch_load(
 		break;
 	case GIT_DELTA_MODIFIED:
 		break;
+	case GIT_DELTA_UNTRACKED:
+		delta->old_file.flags |= GIT_DIFF_FILE_NO_DATA;
+		if ((ctxt->opts->flags & GIT_DIFF_INCLUDE_UNTRACKED_CONTENT) == 0)
+			delta->new_file.flags |= GIT_DIFF_FILE_NO_DATA;
+		break;
 	default:
 		delta->new_file.flags |= GIT_DIFF_FILE_NO_DATA;
 		delta->old_file.flags |= GIT_DIFF_FILE_NO_DATA;
@@ -1070,6 +1082,9 @@ static int print_patch_file(
 
 	GIT_UNUSED(progress);
 
+	if (S_ISDIR(delta->new_file.mode))
+		return 0;
+
 	if (!oldpfx)
 		oldpfx = DIFF_OLD_PREFIX_DEFAULT;
 
@@ -1134,6 +1149,9 @@ static int print_patch_hunk(
 {
 	diff_print_info *pi = data;
 
+	if (S_ISDIR(d->new_file.mode))
+		return 0;
+
 	git_buf_clear(pi->buf);
 	if (git_buf_printf(pi->buf, "%.*s", (int)header_len, header) < 0)
 		return -1;
@@ -1158,6 +1176,9 @@ static int print_patch_line(
 {
 	diff_print_info *pi = data;
 
+	if (S_ISDIR(delta->new_file.mode))
+		return 0;
+
 	git_buf_clear(pi->buf);
 
 	if (line_origin == GIT_DIFF_LINE_ADDITION ||
diff --git a/src/iterator.c b/src/iterator.c
index e52554d4f..267687e01 100644
--- a/src/iterator.c
+++ b/src/iterator.c
@@ -905,3 +905,15 @@ int git_iterator_cmp(
 	return ITERATOR_PREFIXCMP(*iter, entry->path, path_prefix);
 }
 
+int git_iterator_current_workdir_path(git_iterator *iter, git_buf **path)
+{
+	workdir_iterator *wi = (workdir_iterator *)iter;
+
+	if (iter->type != GIT_ITERATOR_WORKDIR || !wi->entry.path)
+		*path = NULL;
+	else
+		*path = &wi->path;
+
+	return 0;
+}
+
diff --git a/src/iterator.h b/src/iterator.h
index 11cd2182c..29c8985d4 100644
--- a/src/iterator.h
+++ b/src/iterator.h
@@ -10,6 +10,7 @@
 #include "common.h"
 #include "git2/index.h"
 #include "vector.h"
+#include "buffer.h"
 
 #define ITERATOR_PREFIXCMP(ITER, STR, PREFIX)	(((ITER).ignore_case) ? \
 	git__prefixcmp_icase((STR), (PREFIX)) : \
@@ -166,4 +167,11 @@ extern int git_iterator_advance_into_directory(
 extern int git_iterator_cmp(
 	git_iterator *iter, const char *path_prefix);
 
+/**
+ * Get the full path of the current item from a workdir iterator.
+ * This will return NULL for a non-workdir iterator.
+ */
+extern int git_iterator_current_workdir_path(
+	git_iterator *iter, git_buf **path);
+
 #endif
diff --git a/tests-clar/diff/diff_helpers.c b/tests-clar/diff/diff_helpers.c
index b4c68769e..de0e7e074 100644
--- a/tests-clar/diff/diff_helpers.c
+++ b/tests-clar/diff/diff_helpers.c
@@ -30,10 +30,13 @@ int diff_file_fn(
 
 	GIT_UNUSED(progress);
 
-	if (delta->binary)
+	e->files++;
+
+	if (delta->binary) {
 		e->at_least_one_of_them_is_binary = true;
+		e->files_binary++;
+	}
 
-	e->files++;
 	switch (delta->status) {
 	case GIT_DELTA_ADDED: e->file_adds++; break;
 	case GIT_DELTA_DELETED: e->file_dels++; break;
@@ -180,3 +183,25 @@ abort:
 	giterr_clear();
 	return GIT_EUSER;
 }
+
+static int diff_print_cb(
+	void *cb_data,
+	const git_diff_delta *delta,
+	const git_diff_range *range,
+	char line_origin, /**< GIT_DIFF_LINE_... value from above */
+	const char *content,
+	size_t content_len)
+{
+	GIT_UNUSED(cb_data);
+	GIT_UNUSED(delta);
+	GIT_UNUSED(range);
+	GIT_UNUSED(line_origin);
+	GIT_UNUSED(content_len);
+	fputs(content, (FILE *)cb_data);
+	return 0;
+}
+
+void diff_print(FILE *fp, git_diff_list *diff)
+{
+	cl_git_pass(git_diff_print_patch(diff, fp ? fp : stderr, diff_print_cb));
+}
diff --git a/tests-clar/diff/diff_helpers.h b/tests-clar/diff/diff_helpers.h
index 7984294a2..629130934 100644
--- a/tests-clar/diff/diff_helpers.h
+++ b/tests-clar/diff/diff_helpers.h
@@ -6,6 +6,8 @@ extern git_tree *resolve_commit_oid_to_tree(
 
 typedef struct {
 	int files;
+	int files_binary;
+
 	int file_adds;
 	int file_dels;
 	int file_mods;
@@ -51,3 +53,5 @@ extern int diff_foreach_via_iterator(
 	git_diff_file_fn file_cb,
 	git_diff_hunk_fn hunk_cb,
 	git_diff_data_fn line_cb);
+
+extern void diff_print(FILE *fp, git_diff_list *diff);
diff --git a/tests-clar/diff/tree.c b/tests-clar/diff/tree.c
index d8af3acf3..c5a0e626e 100644
--- a/tests-clar/diff/tree.c
+++ b/tests-clar/diff/tree.c
@@ -113,16 +113,16 @@ void test_diff_tree__options(void)
 	 */
 	diff_expects test_expects[] = {
 		/* a vs b tests */
-		{ 5, 3, 0, 2, 0, 0, 0, 4, 0, 0, 51, 2, 46, 3 },
-		{ 5, 3, 0, 2, 0, 0, 0, 4, 0, 0, 53, 4, 46, 3 },
-		{ 5, 0, 3, 2, 0, 0, 0, 4, 0, 0, 52, 3, 3, 46 },
-		{ 5, 3, 0, 2, 0, 0, 0, 5, 0, 0, 54, 3, 47, 4 },
+		{ 5, 0, 3, 0, 2, 0, 0, 0, 4, 0, 0, 51, 2, 46, 3 },
+		{ 5, 0, 3, 0, 2, 0, 0, 0, 4, 0, 0, 53, 4, 46, 3 },
+		{ 5, 0, 0, 3, 2, 0, 0, 0, 4, 0, 0, 52, 3, 3, 46 },
+		{ 5, 0, 3, 0, 2, 0, 0, 0, 5, 0, 0, 54, 3, 47, 4 },
 		/* c vs d tests */
-		{ 1, 0, 0, 1, 0, 0, 0, 1, 0, 0, 22, 9, 10, 3 },
-		{ 1, 0, 0, 1, 0, 0, 0, 1, 0, 0, 19, 12, 7, 0 },
-		{ 1, 0, 0, 1, 0, 0, 0, 1, 0, 0, 20, 11, 8, 1 },
-		{ 1, 0, 0, 1, 0, 0, 0, 1, 0, 0, 20, 11, 8, 1 },
-		{ 1, 0, 0, 1, 0, 0, 0, 1, 0, 0, 18, 11, 0, 7 },
+		{ 1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 22, 9, 10, 3 },
+		{ 1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 19, 12, 7, 0 },
+		{ 1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 20, 11, 8, 1 },
+		{ 1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 20, 11, 8, 1 },
+		{ 1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 18, 11, 0, 7 },
 		{ 0 },
 	};
 	diff_expects *expected;
diff --git a/tests-clar/diff/workdir.c b/tests-clar/diff/workdir.c
index 916113178..e184c28b4 100644
--- a/tests-clar/diff/workdir.c
+++ b/tests-clar/diff/workdir.c
@@ -744,3 +744,77 @@ void test_diff_workdir__larger_hunks(void)
 	git_tree_free(a);
 	git_tree_free(b);
 }
+
+/* Set up a test that exercises this code. The easiest test using existing
+ * test data is probably to create a sandbox of submod2 and then run a
+ * git_diff_workdir_to_tree against tree
+ * 873585b94bdeabccea991ea5e3ec1a277895b698. As for what you should actually
+ * test, you can start by just checking that the number of lines of diff
+ * content matches the actual output of git diff. That will at least
+ * demonstrate that the submodule content is being used to generate somewhat
+ * comparable outputs. It is a test that would fail without this code and
+ * will succeed with it.
+ */
+
+#include "../submodule/submodule_helpers.h"
+
+void test_diff_workdir__submodules(void)
+{
+	const char *a_commit = "873585b94bdeabccea991ea5e3ec1a277895b698";
+	git_tree *a;
+	git_diff_options opts = {0};
+	git_diff_list *diff = NULL;
+	diff_expects exp;
+
+	g_repo = cl_git_sandbox_init("submod2");
+
+	cl_fixture_sandbox("submod2_target");
+	p_rename("submod2_target/.gitted", "submod2_target/.git");
+
+	rewrite_gitmodules(git_repository_workdir(g_repo));
+	p_rename("submod2/not_submodule/.gitted", "submod2/not_submodule/.git");
+
+	cl_fixture_cleanup("submod2_target");
+
+	a = resolve_commit_oid_to_tree(g_repo, a_commit);
+
+	opts.flags =
+		GIT_DIFF_INCLUDE_UNTRACKED |
+		GIT_DIFF_RECURSE_UNTRACKED_DIRS |
+		GIT_DIFF_INCLUDE_UNTRACKED_CONTENT;
+
+	cl_git_pass(git_diff_workdir_to_tree(g_repo, &opts, a, &diff));
+
+	/* diff_print(stderr, diff); */
+
+	/* essentially doing: git diff 873585b94bdeabccea991ea5e3ec1a277895b698 */
+
+	memset(&exp, 0, sizeof(exp));
+	cl_git_pass(git_diff_foreach(
+		diff, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	/* the following differs from "git diff 873585" by one "untracked" file
+	 * because the diff list includes the "not_submodule/" directory which
+	 * is not displayed in the text diff.
+	 */
+
+	cl_assert_equal_i(10, exp.files);
+
+	cl_assert_equal_i(0, exp.file_adds);
+	cl_assert_equal_i(0, exp.file_dels);
+	cl_assert_equal_i(1, exp.file_mods);
+	cl_assert_equal_i(0, exp.file_ignored);
+	cl_assert_equal_i(9, exp.file_untracked);
+
+	/* the following numbers match "git diff 873585" exactly */
+
+	cl_assert_equal_i(9, exp.hunks);
+
+	cl_assert_equal_i(33, exp.lines);
+	cl_assert_equal_i(2, exp.line_ctxt);
+	cl_assert_equal_i(30, exp.line_adds);
+	cl_assert_equal_i(1, exp.line_dels);
+
+	git_diff_list_free(diff);
+	git_tree_free(a);
+}