From 8583c9dcbc7d362250c0310e4cee771ec5003327 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 30 Apr 2025 14:44:57 +0200 Subject: [PATCH 1/2] 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 Signed-off-by: Junio C Hamano --- builtin/mv.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++-- t/t7001-mv.sh | 24 ++++++++++++++++---- 2 files changed, 79 insertions(+), 6 deletions(-) diff --git a/builtin/mv.c b/builtin/mv.c index 55a7d471dc..c17e14cee6 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -37,6 +37,13 @@ enum update_mode { INDEX = (1 << 2), SPARSE = (1 << 3), SKIP_WORKTREE_DIR = (1 << 4), + /* + * A file gets moved implicitly via a move of one of its parent + * directories. This flag causes us to skip the check that we don't try + * to move a file and any of its parent directories at the same point + * in time. + */ + MOVE_VIA_PARENT_DIR = (1 << 5), }; #define DUP_BASENAME 1 @@ -181,6 +188,21 @@ static void remove_empty_src_dirs(const char **src_dir, size_t src_dir_nr) strbuf_release(&a_src_dir); } +struct pathmap_entry { + struct hashmap_entry ent; + const char *path; +}; + +static int pathmap_cmp(const void *cmp_data UNUSED, + const struct hashmap_entry *a, + const struct hashmap_entry *b, + const void *key UNUSED) +{ + const struct pathmap_entry *e1 = container_of(a, struct pathmap_entry, ent); + const struct pathmap_entry *e2 = container_of(b, struct pathmap_entry, ent); + return fspathcmp(e1->path, e2->path); +} + int cmd_mv(int argc, const char **argv, const char *prefix, @@ -211,6 +233,8 @@ int cmd_mv(int argc, struct cache_entry *ce; struct string_list only_match_skip_worktree = STRING_LIST_INIT_DUP; struct string_list dirty_paths = STRING_LIST_INIT_DUP; + struct hashmap moved_dirs = HASHMAP_INIT(pathmap_cmp, NULL); + struct strbuf pathbuf = STRBUF_INIT; int ret; git_config(git_default_config, NULL); @@ -329,6 +353,7 @@ int cmd_mv(int argc, dir_check: if (S_ISDIR(st.st_mode)) { + struct pathmap_entry *entry; char *dst_with_slash; size_t dst_with_slash_len; int j, n; @@ -346,6 +371,11 @@ dir_check: goto act_on_entry; } + entry = xmalloc(sizeof(*entry)); + entry->path = src; + hashmap_entry_init(&entry->ent, fspathhash(src)); + hashmap_add(&moved_dirs, &entry->ent); + /* last - first >= 1 */ modes[i] |= WORKING_DIRECTORY; @@ -366,8 +396,7 @@ dir_check: strvec_push(&sources, path); strvec_push(&destinations, prefixed_path); - memset(modes + argc + j, 0, sizeof(enum update_mode)); - modes[argc + j] |= ce_skip_worktree(ce) ? SPARSE : INDEX; + modes[argc + j] = MOVE_VIA_PARENT_DIR | (ce_skip_worktree(ce) ? SPARSE : INDEX); submodule_gitfiles[argc + j] = NULL; free(prefixed_path); @@ -463,6 +492,32 @@ remove_entry: } } + for (i = 0; i < argc; i++) { + const char *slash_pos; + + if (modes[i] & MOVE_VIA_PARENT_DIR) + continue; + + strbuf_reset(&pathbuf); + strbuf_addstr(&pathbuf, sources.v[i]); + + slash_pos = strrchr(pathbuf.buf, '/'); + while (slash_pos > pathbuf.buf) { + struct pathmap_entry needle; + + strbuf_setlen(&pathbuf, slash_pos - pathbuf.buf); + + needle.path = pathbuf.buf; + hashmap_entry_init(&needle.ent, fspathhash(pathbuf.buf)); + + if (hashmap_get_entry(&moved_dirs, &needle, ent, NULL)) + die(_("cannot move both '%s' and its parent directory '%s'"), + sources.v[i], pathbuf.buf); + + slash_pos = strrchr(pathbuf.buf, '/'); + } + } + if (only_match_skip_worktree.nr) { advise_on_updating_sparse_paths(&only_match_skip_worktree); if (!ignore_errors) { @@ -587,6 +642,8 @@ out: strvec_clear(&dest_paths); strvec_clear(&destinations); strvec_clear(&submodule_gitfiles_to_free); + hashmap_clear_and_free(&moved_dirs, struct pathmap_entry, ent); + strbuf_release(&pathbuf); free(submodule_gitfiles); free(modes); return ret; diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh index 25334b5062..920479e925 100755 --- a/t/t7001-mv.sh +++ b/t/t7001-mv.sh @@ -550,16 +550,32 @@ test_expect_success 'moving nested submodules' ' git status ' -test_expect_failure 'nonsense mv triggers assertion failure and partially updated index' ' +test_expect_success 'moving file and its parent directory at the same time fails' ' test_when_finished git reset --hard HEAD && git reset --hard HEAD && mkdir -p a && mkdir -p b && >a/a.txt && git add a/a.txt && - test_must_fail git mv a/a.txt a b && - git status --porcelain >actual && - grep "^A[ ]*a/a.txt$" actual + cat >expect <<-EOF && + fatal: cannot move both ${SQ}a/a.txt${SQ} and its parent directory ${SQ}a${SQ} + EOF + test_must_fail git mv a/a.txt a b 2>err && + test_cmp expect err +' + +test_expect_success 'moving nested directory and its parent directory at the same time fails' ' + test_when_finished git reset --hard HEAD && + git reset --hard HEAD && + mkdir -p a/b/c && + >a/b/c/file.txt && + git add a && + mkdir target && + cat >expect <<-EOF && + fatal: cannot move both ${SQ}a/b/c${SQ} and its parent directory ${SQ}a${SQ} + EOF + test_must_fail git mv a/b/c a target 2>err && + test_cmp expect err ' test_done From 974f0d46645604ac45b8a5ce0b90e2b2a56ca764 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 30 Apr 2025 14:44:58 +0200 Subject: [PATCH 2/2] builtin/mv: convert assert(3p) into `BUG()` The use of asserts is discouraged in our codebase because they lead to different behaviour depending on how Git is built. When being unsure enough whether a condition always holds so that one adds the assert, then the assert should probably trigger regardless of how Git is being built. Drop the call to assert(3p) in git-mv(1) and instead use `BUG()`. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/mv.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/builtin/mv.c b/builtin/mv.c index c17e14cee6..4b4e1fce2e 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -560,7 +560,8 @@ remove_entry: continue; pos = index_name_pos(the_repository->index, src, strlen(src)); - assert(pos >= 0); + if (pos < 0) + BUG("could not find source in index: '%s'", src); if (!(mode & SPARSE) && !lstat(src, &st)) sparse_and_dirty = ie_modified(the_repository->index, the_repository->index->cache[pos],