Merge pull request #1460 from arrbee/fix-dirty-submodules-diff

Submodule diff tests and fixes

examples/network/fetch.c

@@ -16,7 +16,7 @@ struct dl_data {
 
 static void progress_cb(const char *str, int len, void *data)
 {
-	data = data;
+	(void)data;
 	printf("remote: %.*s", len, str);
 	fflush(stdout); /* We don't have the \n to force the flush */
 }
@@ -50,7 +50,7 @@ exit:
 static int update_cb(const char *refname, const git_oid *a, const git_oid *b, void *data)
 {
 	char a_str[GIT_OID_HEXSZ+1], b_str[GIT_OID_HEXSZ+1];
-	data = data;
+	(void)data;
 
 	git_oid_fmt(b_str, b);
 	b_str[GIT_OID_HEXSZ] = '\0';
@@ -76,7 +76,11 @@ int fetch(git_repository *repo, int argc, char **argv)
 	pthread_t worker;
 #endif
 
-	argc = argc;
+	if (argc < 2) {
+		fprintf(stderr, "usage: %s fetch <repo>\n", argv[-1]);
+		return EXIT_FAILURE;
+	}
+
 	// Figure out whether it's a named remote or a URL
 	printf("Fetching %s for repo %p\n", argv[1], repo);
 	if (git_remote_load(&remote, repo, argv[1]) < 0) {

examples/network/index-pack.c

@@ -23,7 +23,7 @@
 // the indexing to finish in a worker thread
 static int index_cb(const git_transfer_progress *stats, void *data)
 {
-	data = data;
+	(void)data;
 	printf("\rProcessing %d of %d", stats->indexed_objects, stats->total_objects);
 
 	return 0;
@@ -39,9 +39,10 @@ int index_pack(git_repository *repo, int argc, char **argv)
 	ssize_t read_bytes;
 	char buf[512];
 
-	repo = repo;
+	(void)repo;
+
 	if (argc < 2) {
-		fprintf(stderr, "I need a packfile\n");
+		fprintf(stderr, "usage: %s index-pack <packfile>\n", argv[-1]);
 		return EXIT_FAILURE;
 	}
 

examples/network/ls-remote.c

@@ -8,7 +8,7 @@ static int show_ref__cb(git_remote_head *head, void *payload)
 {
 	char oid[GIT_OID_HEXSZ + 1] = {0};
 
-	payload = payload;
+	(void)payload;
 	git_oid_fmt(oid, &head->oid);
 	printf("%s\t%s\n", oid, head->name);
 	return 0;
@@ -67,7 +67,11 @@ int ls_remote(git_repository *repo, int argc, char **argv)
 {
 	int error;
 
-	argc = argc;
+	if (argc < 2) {
+		fprintf(stderr, "usage: %s ls-remote <remote>\n", argv[-1]);
+		return EXIT_FAILURE;
+	}
+
 	/* If there's a ':' in the name, assume it's an URL */
 	if (strchr(argv[1], ':') != NULL) {
 		error = use_unnamed(repo, argv[1]);

include/git2/submodule.h

@@ -119,11 +119,28 @@ typedef enum {
 	 GIT_SUBMODULE_STATUS_WD_UNTRACKED      = (1u << 13),
 } git_submodule_status_t;
 
-#define GIT_SUBMODULE_STATUS_IS_UNMODIFIED(S) \
-	(((S) & ~(GIT_SUBMODULE_STATUS_IN_HEAD | \
+#define GIT_SUBMODULE_STATUS__IN_FLAGS \
+	(GIT_SUBMODULE_STATUS_IN_HEAD | \
 	GIT_SUBMODULE_STATUS_IN_INDEX | \
 	GIT_SUBMODULE_STATUS_IN_CONFIG | \
-	GIT_SUBMODULE_STATUS_IN_WD)) == 0)
+	GIT_SUBMODULE_STATUS_IN_WD)
+
+#define GIT_SUBMODULE_STATUS__INDEX_FLAGS \
+	(GIT_SUBMODULE_STATUS_INDEX_ADDED | \
+	GIT_SUBMODULE_STATUS_INDEX_DELETED | \
+	GIT_SUBMODULE_STATUS_INDEX_MODIFIED)
+
+#define GIT_SUBMODULE_STATUS__WD_FLAGS \
+	~(GIT_SUBMODULE_STATUS__IN_FLAGS | GIT_SUBMODULE_STATUS__INDEX_FLAGS)
+
+#define GIT_SUBMODULE_STATUS_IS_UNMODIFIED(S) \
+	(((S) & ~GIT_SUBMODULE_STATUS__IN_FLAGS) == 0)
+
+#define GIT_SUBMODULE_STATUS_IS_INDEX_UNMODIFIED(S) \
+	(((S) & GIT_SUBMODULE_STATUS__INDEX_FLAGS) == 0)
+
+#define GIT_SUBMODULE_STATUS_IS_WD_UNMODIFIED(S) \
+	(((S) & GIT_SUBMODULE_STATUS__WD_FLAGS) == 0)
 
 #define GIT_SUBMODULE_STATUS_IS_WD_DIRTY(S) \
 	(((S) & (GIT_SUBMODULE_STATUS_WD_INDEX_MODIFIED | \

src/date.c

@@ -681,8 +681,8 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm
 	const char *end = date;
 	int i;
 
-	while (isalpha(*++end));
-		;
+	while (isalpha(*++end))
+		/* scan to non-alpha */;
 
 	for (i = 0; i < 12; i++) {
 		size_t match = match_string(date, month_names[i]);

src/diff.c

@@ -542,7 +542,11 @@ static int maybe_modified(
 				unsigned int sm_status = 0;
 				if (git_submodule_status(&sm_status, sub) < 0)
 					return -1;
-				status = GIT_SUBMODULE_STATUS_IS_UNMODIFIED(sm_status)
+
+				/* check IS_WD_UNMODIFIED because this case is only used
+				 * when the new side of the diff is the working directory
+				 */
+				status = GIT_SUBMODULE_STATUS_IS_WD_UNMODIFIED(sm_status)
 						 ? GIT_DELTA_UNMODIFIED : GIT_DELTA_MODIFIED;
 
 				/* grab OID while we are here */

src/diff_output.c

@@ -675,10 +675,14 @@ cleanup:
 	if (!error) {
 		patch->flags |= GIT_DIFF_PATCH_LOADED;
 
+		/* patch is diffable only for non-binary, modified files where at
+		 * least one side has data and there is actual change in the data
+		 */
 		if ((delta->flags & GIT_DIFF_FLAG_BINARY) == 0 &&
 			delta->status != GIT_DELTA_UNMODIFIED &&
 			(patch->old_data.len || patch->new_data.len) &&
-			!git_oid_equal(&delta->old_file.oid, &delta->new_file.oid))
+			(patch->old_data.len != patch->new_data.len ||
+			 !git_oid_equal(&delta->old_file.oid, &delta->new_file.oid)))
 			patch->flags |= GIT_DIFF_PATCH_DIFFABLE;
 	}
 
@@ -1149,7 +1153,11 @@ static int print_patch_file(
 
 	GIT_UNUSED(progress);
 
-	if (S_ISDIR(delta->new_file.mode))
+	if (S_ISDIR(delta->new_file.mode) ||
+		delta->status == GIT_DELTA_UNMODIFIED ||
+		delta->status == GIT_DELTA_IGNORED ||
+		(delta->status == GIT_DELTA_UNTRACKED &&
+		 (pi->diff->opts.flags & GIT_DIFF_INCLUDE_UNTRACKED_CONTENT) == 0))
 		return 0;
 
 	if (!oldpfx)

tests-clar/diff/submodules.c

@@ -0,0 +1,168 @@
+#include "clar_libgit2.h"
+#include "repository.h"
+#include "../submodule/submodule_helpers.h"
+
+static git_repository *g_repo = NULL;
+
+static void setup_submodules(void)
+{
+	g_repo = cl_git_sandbox_init("submodules");
+	cl_fixture_sandbox("testrepo.git");
+	rewrite_gitmodules(git_repository_workdir(g_repo));
+	p_rename("submodules/testrepo/.gitted", "submodules/testrepo/.git");
+}
+
+static void setup_submodules2(void)
+{
+	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");
+	p_rename("submod2/not/.gitted", "submod2/not/.git");
+}
+
+void test_diff_submodules__initialize(void)
+{
+}
+
+void test_diff_submodules__cleanup(void)
+{
+	cl_git_sandbox_cleanup();
+
+	cl_fixture_cleanup("testrepo.git");
+	cl_fixture_cleanup("submod2_target");
+}
+
+static void check_diff_patches(git_diff_list *diff, const char **expected)
+{
+	const git_diff_delta *delta;
+	git_diff_patch *patch = NULL;
+	size_t d, num_d = git_diff_num_deltas(diff);
+	char *patch_text;
+
+	for (d = 0; d < num_d; ++d, git_diff_patch_free(patch)) {
+		cl_git_pass(git_diff_get_patch(&patch, &delta, diff, d));
+
+		if (delta->status == GIT_DELTA_UNMODIFIED)
+			continue;
+
+		if (expected[d] && !strcmp(expected[d], "<SKIP>"))
+			continue;
+		if (expected[d] && !strcmp(expected[d], "<END>"))
+			cl_assert(0);
+
+		cl_git_pass(git_diff_patch_to_str(&patch_text, patch));
+
+		cl_assert_equal_s(expected[d], patch_text);
+		git__free(patch_text);
+	}
+
+	cl_assert(expected[d] && !strcmp(expected[d], "<END>"));
+}
+
+void test_diff_submodules__unmodified_submodule(void)
+{
+	git_diff_options opts = GIT_DIFF_OPTIONS_INIT;
+	git_diff_list *diff = NULL;
+	static const char *expected[] = {
+		"<SKIP>", /* .gitmodules */
+		NULL, /* added */
+		NULL, /* ignored */
+		"diff --git a/modified b/modified\nindex 092bfb9..452216e 100644\n--- a/modified\n+++ b/modified\n@@ -1 +1,2 @@\n-yo\n+changed\n+\n", /* modified */
+		NULL, /* testrepo.git */
+		NULL, /* unmodified */
+		NULL, /* untracked */
+		"<END>"
+	};
+
+	setup_submodules();
+
+	opts.flags = GIT_DIFF_INCLUDE_IGNORED |
+		GIT_DIFF_INCLUDE_UNTRACKED |
+		GIT_DIFF_INCLUDE_UNMODIFIED;
+
+	cl_git_pass(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts));
+	check_diff_patches(diff, expected);
+	git_diff_list_free(diff);
+}
+
+void test_diff_submodules__dirty_submodule(void)
+{
+	git_diff_options opts = GIT_DIFF_OPTIONS_INIT;
+	git_diff_list *diff = NULL;
+	static const char *expected[] = {
+		"<SKIP>", /* .gitmodules */
+		NULL, /* added */
+		NULL, /* ignored */
+		"diff --git a/modified b/modified\nindex 092bfb9..452216e 100644\n--- a/modified\n+++ b/modified\n@@ -1 +1,2 @@\n-yo\n+changed\n+\n", /* modified */
+		"diff --git a/testrepo b/testrepo\nindex a65fedf..a65fedf 160000\n--- a/testrepo\n+++ b/testrepo\n@@ -1 +1 @@\n-Subproject commit a65fedf39aefe402d3bb6e24df4d4f5fe4547750\n+Subproject commit a65fedf39aefe402d3bb6e24df4d4f5fe4547750-dirty\n", /* testrepo.git */
+		NULL, /* unmodified */
+		NULL, /* untracked */
+		"<END>"
+	};
+
+	setup_submodules();
+
+	cl_git_rewritefile("submodules/testrepo/README", "heyheyhey");
+	cl_git_mkfile("submodules/testrepo/all_new.txt", "never seen before");
+
+	opts.flags = GIT_DIFF_INCLUDE_IGNORED |
+		GIT_DIFF_INCLUDE_UNTRACKED |
+		GIT_DIFF_INCLUDE_UNMODIFIED;
+
+	cl_git_pass(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts));
+	check_diff_patches(diff, expected);
+	git_diff_list_free(diff);
+}
+
+void test_diff_submodules__submod2_index_to_wd(void)
+{
+	git_diff_options opts = GIT_DIFF_OPTIONS_INIT;
+	git_diff_list *diff = NULL;
+	static const char *expected[] = {
+		"<SKIP>", /* .gitmodules */
+		NULL, /* not-submodule */
+		NULL, /* not */
+		"diff --git a/sm_changed_file b/sm_changed_file\nindex 4800958..4800958 160000\n--- a/sm_changed_file\n+++ b/sm_changed_file\n@@ -1 +1 @@\n-Subproject commit 480095882d281ed676fe5b863569520e54a7d5c0\n+Subproject commit 480095882d281ed676fe5b863569520e54a7d5c0-dirty\n", /* sm_changed_file */
+		"diff --git a/sm_changed_head b/sm_changed_head\nindex 4800958..3d9386c 160000\n--- a/sm_changed_head\n+++ b/sm_changed_head\n@@ -1 +1 @@\n-Subproject commit 480095882d281ed676fe5b863569520e54a7d5c0\n+Subproject commit 3d9386c507f6b093471a3e324085657a3c2b4247\n", /* sm_changed_head */
+		"diff --git a/sm_changed_index b/sm_changed_index\nindex 4800958..4800958 160000\n--- a/sm_changed_index\n+++ b/sm_changed_index\n@@ -1 +1 @@\n-Subproject commit 480095882d281ed676fe5b863569520e54a7d5c0\n+Subproject commit 480095882d281ed676fe5b863569520e54a7d5c0-dirty\n", /* sm_changed_index */
+		"diff --git a/sm_changed_untracked_file b/sm_changed_untracked_file\nindex 4800958..4800958 160000\n--- a/sm_changed_untracked_file\n+++ b/sm_changed_untracked_file\n@@ -1 +1 @@\n-Subproject commit 480095882d281ed676fe5b863569520e54a7d5c0\n+Subproject commit 480095882d281ed676fe5b863569520e54a7d5c0-dirty\n", /* sm_changed_untracked_file */
+		"diff --git a/sm_missing_commits b/sm_missing_commits\nindex 4800958..5e49635 160000\n--- a/sm_missing_commits\n+++ b/sm_missing_commits\n@@ -1 +1 @@\n-Subproject commit 480095882d281ed676fe5b863569520e54a7d5c0\n+Subproject commit 5e4963595a9774b90524d35a807169049de8ccad\n", /* sm_missing_commits */
+		"<END>"
+	};
+
+	setup_submodules2();
+
+	opts.flags = GIT_DIFF_INCLUDE_UNTRACKED;
+
+	cl_git_pass(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts));
+	check_diff_patches(diff, expected);
+	git_diff_list_free(diff);
+}
+
+void test_diff_submodules__submod2_head_to_index(void)
+{
+	git_diff_options opts = GIT_DIFF_OPTIONS_INIT;
+	git_tree *head;
+	git_diff_list *diff = NULL;
+	static const char *expected[] = {
+		"<SKIP>", /* .gitmodules */
+		"diff --git a/sm_added_and_uncommited b/sm_added_and_uncommited\nnew file mode 160000\nindex 0000000..4800958\n--- /dev/null\n+++ b/sm_added_and_uncommited\n@@ -0,0 +1 @@\n+Subproject commit 480095882d281ed676fe5b863569520e54a7d5c0\n", /* sm_added_and_uncommited */
+		"<END>"
+	};
+
+	setup_submodules2();
+
+	cl_git_pass(git_repository_head_tree(&head, g_repo));
+
+	opts.flags = GIT_DIFF_INCLUDE_UNTRACKED;
+
+	cl_git_pass(git_diff_tree_to_index(&diff, g_repo, head, NULL, &opts));
+	check_diff_patches(diff, expected);
+	git_diff_list_free(diff);
+
+	git_tree_free(head);
+}

tests-clar/status/submodules.c

@@ -165,7 +165,7 @@ void test_status_submodules__moved_head(void)
 	cl_git_pass(
 		git_status_foreach_ext(g_repo, &opts, cb_status__match, &counts));
 	cl_assert_equal_i(6, counts.entry_count);
-	
+
 	git_repository_free(smrepo);
 }
 
@@ -219,3 +219,4 @@ void test_status_submodules__dirty_workdir_only(void)
 		git_status_foreach_ext(g_repo, &opts, cb_status__match, &counts));
 	cl_assert_equal_i(6, counts.entry_count);
 }
+