Re: [PATCH] ci(win+Meson): build in Release mode, avoiding t7001-mv hangs

[Patrick Steinhardt <ps@pks.im>]

On Mon, Apr 28, 2025 at 10:01:34AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> > The reason for this timeout is the test case 'nonsense mv triggers
> >> > assertion failure and partially updated index' in t7001-mv (which is
> >> > not even a regression test, but instead merely demonstrates a bug that
> >> > someone thought someone else should fix at some time). As the name
> >> > suggests, it triggers an assertion. The problem with this is that an
> >> > assertion on Windows, at least when run in Debug mode, will open a modal
> >> > dialog that patiently awaits some buttons to be clicked. Which never
> >> > happens in automated builds.
> >> 
> >> Interesting.
> >> 
> >> So another viable fix (no, I am not suggesting a counter-proposal,
> >> but asking a pure question to see if I understand the issue
> >> correctly) is to rewrite "assert(cond)" to "if (cond) BUG(...)"
> >> or something like that, so that it truly fails?
> >
> > On the surface this sounds like a reasonable thing to do, but I don't
> > have enough context to be really able to tell.
> 
> Interesting again ;-) I didn't realize that it was a fairly recent
> development.  0fcd473f (t7001: add failure test which triggers
> assertion, 2024-10-22) is what adds the questionable test.
> 
> And I do agree with Dscho's assessment that this is "show a bug
> without bothering to fix it", which is not what we usually take
> without first exploring how involved the necessary fix would be.
> 
> I wonder in what bad status would a production build that simply
> disabled the assert() is leaving the resulting repository.
> 
> Quoting from the last part of my response [*] to the initial report
> that eventually turned into the test after 9 months:
> 
>  [*] https://lore.kernel.org/git/xmqqil47obnw.fsf@gitster.g/

Hm. So the issue here is that we're trying to move both a parent
directory as well as a child thereof at the same point in time? I guess
that'd need something like the below patch to restrict such requests.
This leads to:

    $ ./git mv ../builtin/add.c ../builtin ../t
    fatal: cannot move both 'builtin/add.c' and its parent directory 'builtin'

The idea of the patch is that we're tracking all parent directories in a
hashmap. After we have processed all arguments, we then perform a second
pass to check that none of the source files have any of their parent
directories moved at the same time.

I only had a brief look, so this assessment may very well be completely
wrong, and the change doesn't even pass all tests right now. But if this
is indeed the issue then I'm happy to polish this into a proper patch.

Patrick

-- >8 --

diff --git a/builtin/mv.c b/builtin/mv.c
index 54b323fff72..10b2cb56ec7 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -183,6 +183,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 +228,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,11 +348,17 @@ 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;
 			int first = index_name_pos(the_repository->index, src, length), last;
 
+			entry = xmalloc(sizeof(*entry));
+			entry->path = src;
+			hashmap_entry_init(&entry->ent, fspathhash(src));
+			hashmap_add(&moved_dirs, &entry->ent);
+
 			if (first >= 0) {
 				const char *path = submodule_gitfile_path(src, first);
 				if (path != SUBMODULE_WITH_GITDIR)
@@ -465,6 +488,40 @@ int cmd_mv(int argc,
 		}
 	}
 
+	for (i = 0; i < argc; i++) {
+		const char *slash_pos;
+
+		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))
+				continue;
+
+			if (!ignore_errors)
+				die(_("cannot move both parent directory '%s' and its child '%s'"),
+				    pathbuf.buf, sources.v[i]);
+
+			if (--argc > 0) {
+				int n = argc - i;
+				strvec_remove(&sources, i);
+				strvec_remove(&destinations, i);
+				MOVE_ARRAY(modes + i, modes + i + 1, n);
+				MOVE_ARRAY(submodule_gitfiles + i,
+					   submodule_gitfiles + i + 1, n);
+				i--;
+				break;
+			}
+		}
+	}
+
 	if (only_match_skip_worktree.nr) {
 		advise_on_updating_sparse_paths(&only_match_skip_worktree);
 		if (!ignore_errors) {
@@ -589,6 +646,8 @@ int cmd_mv(int argc,
 	strvec_clear(&dest_paths);
 	strvec_clear(&destinations);
 	strvec_clear(&submodule_gitfiles_to_free);
+	hashmap_clear(&moved_dirs);
+	strbuf_release(&pathbuf);
 	free(submodule_gitfiles);
 	free(modes);
 	return ret;