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>

diff --git a/builtin/mv.c b/builtin/mv.c
index 55a7d471dca0126c86add94f4577f0c9795fb0d6..c17e14cee6e9b2593435304c7dacacd684801571 100644 (file)
--- 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 25334b506228f0f9d613e43d3c5be7c1caf2138b..920479e925620a08c5bbc99d28757ce335c5284f 100755 (executable)
--- 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