Merge branch 'checkout_submodules'

Author: Edward Thomson

docs/checkout-internals.md

@@ -66,6 +66,8 @@ Key
 - Bi       - ignored blob (WD only)
 - T1,T2,T3 - trees with different SHAs,
 - Ti       - ignored tree (WD only)
+- S1,S2    - submodules with different SHAs
+- Sd       - dirty submodule (WD only)
 - x        - nothing
 
 Diff with 2 non-workdir iterators
@@ -162,6 +164,27 @@ Checkout From 3 Iterators (2 not workdir, 1 workdir)
 | 35+ |  T1 |     T2 |      x         | update locally deleted tree (SAFE+MISSING)                         |
 | 36* |  T1 |     T2 |  B1/Bi         | update to tree with typechanged tree->blob conflict (F-1)          |
 | 37  |  T1 |     T2 | T1/T2/T3       | update to existing tree (MAYBE SAFE)                               |
+| 38+ |   x |     S1 |      x         | add submodule (SAFE)                                               |
+| 39  |   x |     S1 |  S1/Sd         | independently added submodule (SUBMODULE)                          |
+| 40* |   x |     S1 |     B1         | add submodule with blob confilct (FORCEABLE)                       |
+| 41* |   x |     S1 |     T1         | add submodule with tree conflict (FORCEABLE)                       |
+| 42  |  S1 |      x |  S1/Sd         | deleted submodule (SUBMODULE)                                      |
+| 43  |  S1 |      x |      x         | independently deleted submodule (SUBMODULE)                        |
+| 44  |  S1 |      x |     B1         | independently deleted submodule with added blob (SAFE+MISSING)     |
+| 45  |  S1 |      x |     T1         | independently deleted submodule with added tree (SAFE+MISSING)     |
+| 46  |  S1 |     S1 |      x         | locally deleted submodule (SUBMODULE)                              |
+| 47+ |  S1 |     S2 |      x         | update locally deleted submodule (SAFE)                            |
+| 48  |  S1 |     S1 |     S2         | locally updated submodule commit (SUBMODULE)                       |
+| 49  |  S1 |     S2 |     S1         | updated submodule commit (SUBMODULE)                               |
+| 50+ |  S1 |     B1 |      x         | add blob with locally deleted submodule (SAFE+MISSING)             |
+| 51* |  S1 |     B1 |     S1         | typechange submodule->blob (SAFE)                                  |
+| 52* |  S1 |     B1 |     Sd         | typechange dirty submodule->blob (SAFE!?!?)                        |
+| 53+ |  S1 |     T1 |      x         | add tree with locally deleted submodule (SAFE+MISSING)             |
+| 54* |  S1 |     T1 |  S1/Sd         | typechange submodule->tree (MAYBE SAFE)                            |
+| 55+ |  B1 |     S1 |      x         | add submodule with locally deleted blob (SAFE+MISSING)             |
+| 56* |  B1 |     S1 |     B1         | typechange blob->submodule (SAFE)                                  |
+| 57+ |  T1 |     S1 |      x         | add submodule with locally deleted tree (SAFE+MISSING)             |
+| 58* |  T1 |     S1 |     T1         | typechange tree->submodule (SAFE)                                  |
 
 
 The number is followed by ' ' if no change is needed or '+' if the case
@@ -176,6 +199,8 @@ There are four tiers of safe cases:
                   content, which is unknown at this point
 * FORCEABLE == conflict unless FORCE is given
 * DIRTY     == no conflict but change is not applied unless FORCE
+* SUBMODULE == no conflict and no change is applied unless a deleted
+               submodule dir is empty
 
 Some slightly unusual circumstances:
 
@@ -198,7 +223,9 @@ Some slightly unusual circumstances:
     cases, if baseline == target, we don't touch the workdir (it is
     either already right or is "dirty").  However, since this case also
     implies that a ?/B1/x case will exist as well, it can be skipped.
+* 41 - It's not clear how core git distinguishes this case from 39 (mode?).
+* 52 - Core git makes destructive changes without any warning when the
+    submodule is dirty and the type changes to a blob.
 
 Cases 3, 17, 24, 26, and 29 are all considered conflicts even though
 none of them will require making any updates to the working directory.
-

src/checkout.c

@@ -482,7 +482,8 @@ static int checkout_action_with_wd(
 			*action = CHECKOUT_ACTION_IF(SAFE, REMOVE, NONE);
 		break;
 	case GIT_DELTA_MODIFIED: /* case 16, 17, 18 (or 36 but not really) */
-		if (checkout_is_workdir_modified(data, &delta->old_file, &delta->new_file, wd))
+		if (wd->mode != GIT_FILEMODE_COMMIT &&
+			checkout_is_workdir_modified(data, &delta->old_file, &delta->new_file, wd))
 			*action = CHECKOUT_ACTION_IF(FORCE, UPDATE_BLOB, CONFLICT);
 		else
 			*action = CHECKOUT_ACTION_IF(SAFE, UPDATE_BLOB, NONE);

src/merge.c

@@ -2827,6 +2827,7 @@ static int merge_check_workdir(size_t *conflicts, git_repository *repo, git_inde
 	opts.flags |= GIT_DIFF_DISABLE_PATHSPEC_MATCH;
 	opts.pathspec.count = merged_paths->length;
 	opts.pathspec.strings = (char **)merged_paths->contents;
+	opts.ignore_submodules = GIT_SUBMODULE_IGNORE_ALL;
 
 	if ((error = git_diff_index_to_workdir(&wd_diff_list, repo, NULL, &opts)) < 0)
 		goto done;

tests/checkout/typechange.c

@@ -6,6 +6,36 @@
 
 static git_repository *g_repo = NULL;
 
+/*
+From the test repo used for this test:
+--------------------------------------
+
+This is a test repo for libgit2 where tree entries have type changes
+
+The key types that could be found in tree entries are:
+
+1 - GIT_FILEMODE_NEW             = 0000000
+2 - GIT_FILEMODE_TREE            = 0040000
+3 - GIT_FILEMODE_BLOB            = 0100644
+4 - GIT_FILEMODE_BLOB_EXECUTABLE = 0100755
+5 - GIT_FILEMODE_LINK            = 0120000
+6 - GIT_FILEMODE_COMMIT          = 0160000
+
+I will try to have every type of transition somewhere in the history
+of this repo.
+
+Commits
+-------
+Initial commit - a(1)    b(1)    c(1)    d(1)    e(1)
+Create content - a(1->2) b(1->3) c(1->4) d(1->5) e(1->6)
+Changes #1     - a(2->3) b(3->4) c(4->5) d(5->6) e(6->2)
+Changes #2     - a(3->5) b(4->6) c(5->2) d(6->3) e(2->4)
+Changes #3     - a(5->3) b(6->4) c(2->5) d(3->6) e(4->2)
+Changes #4     - a(3->2) b(4->3) c(5->4) d(6->5) e(2->6)
+Changes #5     - a(2->1) b(3->1) c(4->1) d(5->1) e(6->1)
+
+*/
+
 static const char *g_typechange_oids[] = {
 	"79b9f23e85f55ea36a472a902e875bc1121a94cb",
 	"9bdb75b73836a99e3dbeea640a81de81031fdc29",
@@ -21,6 +51,14 @@ static bool g_typechange_empty[] = {
 	true, false, false, false, false, false, true, true
 };
 
+static const int g_typechange_expected_conflicts[] = {
+	1, 2, 3, 3, 2, 3, 2
+};
+
+static const int g_typechange_expected_untracked[] = {
+	6, 4, 3, 2, 3, 2, 5
+};
+
 void test_checkout_typechange__initialize(void)
 {
 	g_repo = cl_git_sandbox_init("typechanges");
@@ -112,12 +150,7 @@ void test_checkout_typechange__checkout_typechanges_safe(void)
 	for (i = 0; g_typechange_oids[i] != NULL; ++i) {
 		cl_git_pass(git_revparse_single(&obj, g_repo, g_typechange_oids[i]));
 
-		opts.checkout_strategy = GIT_CHECKOUT_FORCE;
-
-		/* There are bugs in some submodule->tree changes that prevent
-		 * SAFE from passing here, even though the following should work:
-		 */
-		/* !i ? GIT_CHECKOUT_FORCE : GIT_CHECKOUT_SAFE; */
+		opts.checkout_strategy = !i ? GIT_CHECKOUT_FORCE : GIT_CHECKOUT_SAFE;
 
 		cl_git_pass(git_checkout_tree(g_repo, obj, &opts));
 
@@ -190,6 +223,35 @@ static void force_create_file(const char *file)
 	cl_git_rewritefile(file, "yowza!!");
 }
 
+static int make_submodule_dirty(git_submodule *sm, const char *name, void *payload)
+{
+	git_buf submodulepath = GIT_BUF_INIT;
+	git_buf dirtypath = GIT_BUF_INIT;
+	git_repository *submodule_repo;
+
+	/* remove submodule directory in preparation for init and repo_init */
+	cl_git_pass(git_buf_joinpath(
+		&submodulepath,
+		git_repository_workdir(g_repo),
+		git_submodule_path(sm)
+	));
+	git_futils_rmdir_r(git_buf_cstr(&submodulepath), NULL, GIT_RMDIR_REMOVE_FILES);
+
+	/* initialize submodule and its repository */
+	cl_git_pass(git_submodule_init(sm, 1));
+	cl_git_pass(git_submodule_repo_init(&submodule_repo, sm, 0));
+
+	/* create a file in the submodule workdir to make it dirty */
+	cl_git_pass(
+		git_buf_joinpath(&dirtypath, git_repository_workdir(submodule_repo), "dirty"));
+	force_create_file(git_buf_cstr(&dirtypath));
+
+	git_buf_free(&dirtypath);
+	git_buf_free(&submodulepath);
+
+	return 0;
+}
+
 void test_checkout_typechange__checkout_with_conflicts(void)
 {
 	int i;
@@ -211,13 +273,17 @@ void test_checkout_typechange__checkout_with_conflicts(void)
 		git_futils_rmdir_r("typechanges/d", NULL, GIT_RMDIR_REMOVE_FILES);
 		p_mkdir("typechanges/d", 0777); /* intentionally empty dir */
 		force_create_file("typechanges/untracked");
+		cl_git_pass(git_submodule_foreach(g_repo, make_submodule_dirty, NULL));
 
 		opts.checkout_strategy = GIT_CHECKOUT_SAFE;
 		memset(&cts, 0, sizeof(cts));
 
 		cl_git_fail(git_checkout_tree(g_repo, obj, &opts));
-		cl_assert(cts.conflicts > 0);
-		cl_assert(cts.untracked > 0);
+		cl_assert_equal_i(cts.conflicts, g_typechange_expected_conflicts[i]);
+		cl_assert_equal_i(cts.untracked, g_typechange_expected_untracked[i]);
+		cl_assert_equal_i(cts.dirty, 0);
+		cl_assert_equal_i(cts.updates, 0);
+		cl_assert_equal_i(cts.ignored, 0);
 
 		opts.checkout_strategy =
 			GIT_CHECKOUT_FORCE | GIT_CHECKOUT_REMOVE_UNTRACKED;