[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 <ps@pks.im>
---
builtin/mv.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
t/t7001-mv.sh | 24 +++++++++++++++++++----
2 files changed, 79 insertions(+), 6 deletions(-)
Show changes to 2 files +79 −6
builtin/mv.c, t/t7001-mv.sh
diff --git a/builtin/mv.c b/builtin/mv.c
index 54b323fff72..edb854677d9 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -39,6 +39,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
@@ -183,6 +190,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,
@@ -213,6 +235,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);
@@ -331,6 +355,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;
@@ -348,6 +373,11 @@ int cmd_mv(int argc,
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;
@@ -368,8 +398,7 @@ int cmd_mv(int argc,
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);
@@ -465,6 +494,32 @@ int cmd_mv(int argc,
}
}
+ 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) {
@@ -589,6 +644,8 @@ int cmd_mv(int argc,
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 25334b50622..920479e9256 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
--
2.49.0.987.g0cc8ee98dc.dirty
Patrick Steinhardt
