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

From

Patrick Steinhardt <ps@pks.im>

Date

Apr 29, 2025, 12:20 UTC

Message-ID

<aBDD-NeN2YoQbU9S@pks.im>

In-Reply-To

<xmqq8qnkdxu9.fsf@gitster.g>

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/

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

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;