Skip to content

Commit afab1ff

Browse files
hackhaslamEdward Thomson
authored and
Edward Thomson
committedMay 26, 2016
checkout: handle dirty submodules correctly
Don't generate conflicts when checking out a modified submodule and the submodule is dirty or modified in the workdir.
1 parent fdf1463 commit afab1ff

File tree

3 files changed

+104
-10
lines changed

3 files changed

+104
-10
lines changed
 

‎docs/checkout-internals.md

+28-1
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,8 @@ Key
6666
- Bi - ignored blob (WD only)
6767
- T1,T2,T3 - trees with different SHAs,
6868
- Ti - ignored tree (WD only)
69+
- S1,S2 - submodules with different SHAs
70+
- Sd - dirty submodule (WD only)
6971
- x - nothing
7072

7173
Diff with 2 non-workdir iterators
@@ -162,6 +164,27 @@ Checkout From 3 Iterators (2 not workdir, 1 workdir)
162164
| 35+ | T1 | T2 | x | update locally deleted tree (SAFE+MISSING) |
163165
| 36* | T1 | T2 | B1/Bi | update to tree with typechanged tree->blob conflict (F-1) |
164166
| 37 | T1 | T2 | T1/T2/T3 | update to existing tree (MAYBE SAFE) |
167+
| 38+ | x | S1 | x | add submodule (SAFE) |
168+
| 39 | x | S1 | S1/Sd | independently added submodule (SUBMODULE) |
169+
| 40* | x | S1 | B1 | add submodule with blob confilct (FORCEABLE) |
170+
| 41* | x | S1 | T1 | add submodule with tree conflict (FORCEABLE) |
171+
| 42 | S1 | x | S1/Sd | deleted submodule (SUBMODULE) |
172+
| 43 | S1 | x | x | independently deleted submodule (SUBMODULE) |
173+
| 44 | S1 | x | B1 | independently deleted submodule with added blob (SAFE+MISSING) |
174+
| 45 | S1 | x | T1 | independently deleted submodule with added tree (SAFE+MISSING) |
175+
| 46 | S1 | S1 | x | locally deleted submodule (SUBMODULE) |
176+
| 47+ | S1 | S2 | x | update locally deleted submodule (SAFE) |
177+
| 48 | S1 | S1 | S2 | locally updated submodule commit (SUBMODULE) |
178+
| 49 | S1 | S2 | S1 | updated submodule commit (SUBMODULE) |
179+
| 50+ | S1 | B1 | x | add blob with locally deleted submodule (SAFE+MISSING) |
180+
| 51* | S1 | B1 | S1 | typechange submodule->blob (SAFE) |
181+
| 52* | S1 | B1 | Sd | typechange dirty submodule->blob (SAFE!?!?) |
182+
| 53+ | S1 | T1 | x | add tree with locally deleted submodule (SAFE+MISSING) |
183+
| 54* | S1 | T1 | S1/Sd | typechange submodule->tree (MAYBE SAFE) |
184+
| 55+ | B1 | S1 | x | add submodule with locally deleted blob (SAFE+MISSING) |
185+
| 56* | B1 | S1 | B1 | typechange blob->submodule (SAFE) |
186+
| 57+ | T1 | S1 | x | add submodule with locally deleted tree (SAFE+MISSING) |
187+
| 58* | T1 | S1 | T1 | typechange tree->submodule (SAFE) |
165188

166189

167190
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:
176199
content, which is unknown at this point
177200
* FORCEABLE == conflict unless FORCE is given
178201
* DIRTY == no conflict but change is not applied unless FORCE
202+
* SUBMODULE == no conflict and no change is applied unless a deleted
203+
submodule dir is empty
179204

180205
Some slightly unusual circumstances:
181206

@@ -198,7 +223,9 @@ Some slightly unusual circumstances:
198223
cases, if baseline == target, we don't touch the workdir (it is
199224
either already right or is "dirty"). However, since this case also
200225
implies that a ?/B1/x case will exist as well, it can be skipped.
226+
* 41 - It's not clear how core git distinguishes this case from 39 (mode?).
227+
* 52 - Core git makes destructive changes without any warning when the
228+
submodule is dirty and the type changes to a blob.
201229

202230
Cases 3, 17, 24, 26, and 29 are all considered conflicts even though
203231
none of them will require making any updates to the working directory.
204-

‎src/checkout.c

+2-1
Original file line numberDiff line numberDiff line change
@@ -482,7 +482,8 @@ static int checkout_action_with_wd(
482482
*action = CHECKOUT_ACTION_IF(SAFE, REMOVE, NONE);
483483
break;
484484
case GIT_DELTA_MODIFIED: /* case 16, 17, 18 (or 36 but not really) */
485-
if (checkout_is_workdir_modified(data, &delta->old_file, &delta->new_file, wd))
485+
if (wd->mode != GIT_FILEMODE_COMMIT &&
486+
checkout_is_workdir_modified(data, &delta->old_file, &delta->new_file, wd))
486487
*action = CHECKOUT_ACTION_IF(FORCE, UPDATE_BLOB, CONFLICT);
487488
else
488489
*action = CHECKOUT_ACTION_IF(SAFE, UPDATE_BLOB, NONE);

‎tests/checkout/typechange.c

+74-8
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,36 @@
66

77
static git_repository *g_repo = NULL;
88

9+
/*
10+
From the test repo used for this test:
11+
--------------------------------------
12+
13+
This is a test repo for libgit2 where tree entries have type changes
14+
15+
The key types that could be found in tree entries are:
16+
17+
1 - GIT_FILEMODE_NEW = 0000000
18+
2 - GIT_FILEMODE_TREE = 0040000
19+
3 - GIT_FILEMODE_BLOB = 0100644
20+
4 - GIT_FILEMODE_BLOB_EXECUTABLE = 0100755
21+
5 - GIT_FILEMODE_LINK = 0120000
22+
6 - GIT_FILEMODE_COMMIT = 0160000
23+
24+
I will try to have every type of transition somewhere in the history
25+
of this repo.
26+
27+
Commits
28+
-------
29+
Initial commit - a(1) b(1) c(1) d(1) e(1)
30+
Create content - a(1->2) b(1->3) c(1->4) d(1->5) e(1->6)
31+
Changes #1 - a(2->3) b(3->4) c(4->5) d(5->6) e(6->2)
32+
Changes #2 - a(3->5) b(4->6) c(5->2) d(6->3) e(2->4)
33+
Changes #3 - a(5->3) b(6->4) c(2->5) d(3->6) e(4->2)
34+
Changes #4 - a(3->2) b(4->3) c(5->4) d(6->5) e(2->6)
35+
Changes #5 - a(2->1) b(3->1) c(4->1) d(5->1) e(6->1)
36+
37+
*/
38+
939
static const char *g_typechange_oids[] = {
1040
"79b9f23e85f55ea36a472a902e875bc1121a94cb",
1141
"9bdb75b73836a99e3dbeea640a81de81031fdc29",
@@ -21,6 +51,14 @@ static bool g_typechange_empty[] = {
2151
true, false, false, false, false, false, true, true
2252
};
2353

54+
static const int g_typechange_expected_conflicts[] = {
55+
1, 2, 3, 3, 2, 3, 2
56+
};
57+
58+
static const int g_typechange_expected_untracked[] = {
59+
6, 4, 3, 2, 3, 2, 5
60+
};
61+
2462
void test_checkout_typechange__initialize(void)
2563
{
2664
g_repo = cl_git_sandbox_init("typechanges");
@@ -112,12 +150,7 @@ void test_checkout_typechange__checkout_typechanges_safe(void)
112150
for (i = 0; g_typechange_oids[i] != NULL; ++i) {
113151
cl_git_pass(git_revparse_single(&obj, g_repo, g_typechange_oids[i]));
114152

115-
opts.checkout_strategy = GIT_CHECKOUT_FORCE;
116-
117-
/* There are bugs in some submodule->tree changes that prevent
118-
* SAFE from passing here, even though the following should work:
119-
*/
120-
/* !i ? GIT_CHECKOUT_FORCE : GIT_CHECKOUT_SAFE; */
153+
opts.checkout_strategy = !i ? GIT_CHECKOUT_FORCE : GIT_CHECKOUT_SAFE;
121154

122155
cl_git_pass(git_checkout_tree(g_repo, obj, &opts));
123156

@@ -190,6 +223,35 @@ static void force_create_file(const char *file)
190223
cl_git_rewritefile(file, "yowza!!");
191224
}
192225

226+
static int make_submodule_dirty(git_submodule *sm, const char *name, void *payload)
227+
{
228+
git_buf submodulepath = GIT_BUF_INIT;
229+
git_buf dirtypath = GIT_BUF_INIT;
230+
git_repository *submodule_repo;
231+
232+
/* remove submodule directory in preparation for init and repo_init */
233+
cl_git_pass(git_buf_joinpath(
234+
&submodulepath,
235+
git_repository_workdir(g_repo),
236+
git_submodule_path(sm)
237+
));
238+
git_futils_rmdir_r(git_buf_cstr(&submodulepath), NULL, GIT_RMDIR_REMOVE_FILES);
239+
240+
/* initialize submodule and its repository */
241+
cl_git_pass(git_submodule_init(sm, 1));
242+
cl_git_pass(git_submodule_repo_init(&submodule_repo, sm, 0));
243+
244+
/* create a file in the submodule workdir to make it dirty */
245+
cl_git_pass(
246+
git_buf_joinpath(&dirtypath, git_repository_workdir(submodule_repo), "dirty"));
247+
force_create_file(git_buf_cstr(&dirtypath));
248+
249+
git_buf_free(&dirtypath);
250+
git_buf_free(&submodulepath);
251+
252+
return 0;
253+
}
254+
193255
void test_checkout_typechange__checkout_with_conflicts(void)
194256
{
195257
int i;
@@ -211,13 +273,17 @@ void test_checkout_typechange__checkout_with_conflicts(void)
211273
git_futils_rmdir_r("typechanges/d", NULL, GIT_RMDIR_REMOVE_FILES);
212274
p_mkdir("typechanges/d", 0777); /* intentionally empty dir */
213275
force_create_file("typechanges/untracked");
276+
cl_git_pass(git_submodule_foreach(g_repo, make_submodule_dirty, NULL));
214277

215278
opts.checkout_strategy = GIT_CHECKOUT_SAFE;
216279
memset(&cts, 0, sizeof(cts));
217280

218281
cl_git_fail(git_checkout_tree(g_repo, obj, &opts));
219-
cl_assert(cts.conflicts > 0);
220-
cl_assert(cts.untracked > 0);
282+
cl_assert_equal_i(cts.conflicts, g_typechange_expected_conflicts[i]);
283+
cl_assert_equal_i(cts.untracked, g_typechange_expected_untracked[i]);
284+
cl_assert_equal_i(cts.dirty, 0);
285+
cl_assert_equal_i(cts.updates, 0);
286+
cl_assert_equal_i(cts.ignored, 0);
221287

222288
opts.checkout_strategy =
223289
GIT_CHECKOUT_FORCE | GIT_CHECKOUT_REMOVE_UNTRACKED;

0 commit comments

Comments
 (0)
Please sign in to comment.