freshlybakedca.ke / git
Git fork
fork atom

builtin/mv: bail out when trying to move child and its parent

We have a known issue in git-mv(1) where moving both a child and any of
its parents causes an assert to trigger because the child cannot be
found anymore in the index. We have added a test for this in commit
0fcd473fdd3 (t7001: add failure test which triggers assertion,
2024-10-22) without addressing the issue, which is why the test itself
is marked as `test_expect_failure`.

The behaviour of that test relies on a call to assert(3p) though, which
may or may not be compiled into the resulting binary depending on
whether or not we pass `-DNDEBUG`. When these asserts are compiled into
Git this may cause our CI to hang on Windows though, because asserts may
cause a modal window to be shown.

While we could work around the issue by converting this into a call to
`BUG()`, let's rather address the root cause of the issue by bailing out
in case we see that both a child and any of its parents are being moved
in the same command.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

authored by

Patrick Steinhardt and committed by
Junio C Hamano 8583c9dc f93ff170

+79 -6
unified split
+59 -2
builtin/mv.c
··· 37 37 INDEX = (1 << 2), 38 38 SPARSE = (1 << 3), 39 39 SKIP_WORKTREE_DIR = (1 << 4), 40 + /* 41 + * A file gets moved implicitly via a move of one of its parent 42 + * directories. This flag causes us to skip the check that we don't try 43 + * to move a file and any of its parent directories at the same point 44 + * in time. 45 + */ 46 + MOVE_VIA_PARENT_DIR = (1 << 5), 40 47 }; 41 48 42 49 #define DUP_BASENAME 1 ··· 181 188 strbuf_release(&a_src_dir); 182 189 } 183 190 191 + struct pathmap_entry { 192 + struct hashmap_entry ent; 193 + const char *path; 194 + }; 195 + 196 + static int pathmap_cmp(const void *cmp_data UNUSED, 197 + const struct hashmap_entry *a, 198 + const struct hashmap_entry *b, 199 + const void *key UNUSED) 200 + { 201 + const struct pathmap_entry *e1 = container_of(a, struct pathmap_entry, ent); 202 + const struct pathmap_entry *e2 = container_of(b, struct pathmap_entry, ent); 203 + return fspathcmp(e1->path, e2->path); 204 + } 205 + 184 206 int cmd_mv(int argc, 185 207 const char **argv, 186 208 const char *prefix, ··· 211 233 struct cache_entry *ce; 212 234 struct string_list only_match_skip_worktree = STRING_LIST_INIT_DUP; 213 235 struct string_list dirty_paths = STRING_LIST_INIT_DUP; 236 + struct hashmap moved_dirs = HASHMAP_INIT(pathmap_cmp, NULL); 237 + struct strbuf pathbuf = STRBUF_INIT; 214 238 int ret; 215 239 216 240 git_config(git_default_config, NULL); ··· 329 353 330 354 dir_check: 331 355 if (S_ISDIR(st.st_mode)) { 356 + struct pathmap_entry *entry; 332 357 char *dst_with_slash; 333 358 size_t dst_with_slash_len; 334 359 int j, n; ··· 346 371 goto act_on_entry; 347 372 } 348 373 374 + entry = xmalloc(sizeof(*entry)); 375 + entry->path = src; 376 + hashmap_entry_init(&entry->ent, fspathhash(src)); 377 + hashmap_add(&moved_dirs, &entry->ent); 378 + 349 379 /* last - first >= 1 */ 350 380 modes[i] |= WORKING_DIRECTORY; 351 381 ··· 366 396 strvec_push(&sources, path); 367 397 strvec_push(&destinations, prefixed_path); 368 398 369 - memset(modes + argc + j, 0, sizeof(enum update_mode)); 370 - modes[argc + j] |= ce_skip_worktree(ce) ? SPARSE : INDEX; 399 + modes[argc + j] = MOVE_VIA_PARENT_DIR | (ce_skip_worktree(ce) ? SPARSE : INDEX); 371 400 submodule_gitfiles[argc + j] = NULL; 372 401 373 402 free(prefixed_path); ··· 460 489 MOVE_ARRAY(submodule_gitfiles + i, 461 490 submodule_gitfiles + i + 1, n); 462 491 i--; 492 + } 493 + } 494 + 495 + for (i = 0; i < argc; i++) { 496 + const char *slash_pos; 497 + 498 + if (modes[i] & MOVE_VIA_PARENT_DIR) 499 + continue; 500 + 501 + strbuf_reset(&pathbuf); 502 + strbuf_addstr(&pathbuf, sources.v[i]); 503 + 504 + slash_pos = strrchr(pathbuf.buf, '/'); 505 + while (slash_pos > pathbuf.buf) { 506 + struct pathmap_entry needle; 507 + 508 + strbuf_setlen(&pathbuf, slash_pos - pathbuf.buf); 509 + 510 + needle.path = pathbuf.buf; 511 + hashmap_entry_init(&needle.ent, fspathhash(pathbuf.buf)); 512 + 513 + if (hashmap_get_entry(&moved_dirs, &needle, ent, NULL)) 514 + die(_("cannot move both '%s' and its parent directory '%s'"), 515 + sources.v[i], pathbuf.buf); 516 + 517 + slash_pos = strrchr(pathbuf.buf, '/'); 463 518 } 464 519 } 465 520 ··· 587 642 strvec_clear(&dest_paths); 588 643 strvec_clear(&destinations); 589 644 strvec_clear(&submodule_gitfiles_to_free); 645 + hashmap_clear_and_free(&moved_dirs, struct pathmap_entry, ent); 646 + strbuf_release(&pathbuf); 590 647 free(submodule_gitfiles); 591 648 free(modes); 592 649 return ret;
+20 -4
t/t7001-mv.sh
··· 550 550 git status 551 551 ' 552 552 553 - test_expect_failure 'nonsense mv triggers assertion failure and partially updated index' ' 553 + test_expect_success 'moving file and its parent directory at the same time fails' ' 554 554 test_when_finished git reset --hard HEAD && 555 555 git reset --hard HEAD && 556 556 mkdir -p a && 557 557 mkdir -p b && 558 558 >a/a.txt && 559 559 git add a/a.txt && 560 - test_must_fail git mv a/a.txt a b && 561 - git status --porcelain >actual && 562 - grep "^A[ ]*a/a.txt$" actual 560 + cat >expect <<-EOF && 561 + fatal: cannot move both ${SQ}a/a.txt${SQ} and its parent directory ${SQ}a${SQ} 562 + EOF 563 + test_must_fail git mv a/a.txt a b 2>err && 564 + test_cmp expect err 565 + ' 566 + 567 + test_expect_success 'moving nested directory and its parent directory at the same time fails' ' 568 + test_when_finished git reset --hard HEAD && 569 + git reset --hard HEAD && 570 + mkdir -p a/b/c && 571 + >a/b/c/file.txt && 572 + git add a && 573 + mkdir target && 574 + cat >expect <<-EOF && 575 + fatal: cannot move both ${SQ}a/b/c${SQ} and its parent directory ${SQ}a${SQ} 576 + EOF 577 + test_must_fail git mv a/b/c a target 2>err && 578 + test_cmp expect err 563 579 ' 564 580 565 581 test_done