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

From: Johannes Schindelin <[email protected]>

Since switching to --vsenv, the t7001-mv test consistently times out after six hours in the CI builds on GitHub. This kind of waste is inconsistent with my values.

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.

The solution is straight-forward: Just like the win+VS job already did in forever, build in Release mode (where that modal assertion dialog is never shown).

Signed-off-by Johannes Schindelin <[email protected]>

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

I was surprised to find this issue today, and that this had not been addressed yet.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1908%2Fdscho%2Fdont-let-win%2BMeson-hang-in-t7001-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1908/dscho/dont-let-win+Meson-hang-in-t7001-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1908

 .github/workflows/main.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

.github/workflows/main.yml +1 -1

index 83ca8e4182b..275240be5dc 100644
@@ -265,7 +265,7 @@ jobs:
       run: pip install meson ninja
     - name: Setup
       shell: pwsh
-      run: meson setup build --vsenv -Dperl=disabled -Dcredential_helpers=wincred
+      run: meson setup build --vsenv -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred
     - name: Compile
       shell: pwsh
       run: meson compile -C build

base-commit: f65182a99e545d2f2bc22e6c1c2da192133b16a3

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 5 lines · writes:

From: Johannes Schindelin <[email protected]>

Since switching to --vsenv, the t7001-mv test consistently times out after six hours in the CI builds on GitHub. This kind of waste is inconsistent with my values.

With mine too and I would presume everybody else's. I've been annoyed for a long time by one of those sharded Meson-Win test jobs that hang around until timeout.

Thank you very much for addressing the issue.

Quoted 8 lines

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?

Quoted 5 lines

.github/workflows/main.yml +0 -0

index 83ca8e4182b..275240be5dc 100644
@@ -265,7 +265,7 @@ jobs:

.github/workflows/main.yml jobs: · line 271

Quoted 8 lines

.github/workflows/main.yml +1 -1

       run: pip install meson ninja
     - name: Setup
       shell: pwsh
-      run: meson setup build --vsenv -Dperl=disabled -Dcredential_helpers=wincred
+      run: meson setup build --vsenv -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred
     - name: Compile
       shell: pwsh
       run: meson compile -C build

Quoted 11 lines

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 11 lines · Patrick Steinhardt

Quoted 11 lines

Quoted 8 lines

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.

Quoted 7 lines

Quoted 4 lines

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/[email protected]/

builtin/mv.c +3 -0

@@ -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;

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.

Quoted 14 lines · Patrick Steinhardt

builtin/mv.c +3 -0

@@ -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;

builtin/mv.c int cmd_mv(int argc, · line 361

Quoted 10 lines

builtin/mv.c +5 -0

 			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);
+

OK, this collects in moved_dirs the directories that will get moved. And then a separate loop, ...

builtin/mv.c int cmd_mv(int argc, · line 494

Quoted 4 lines

builtin/mv.c +4 -0

+	for (i = 0; i < argc; i++) {
+		const char *slash_pos;
+
+		strbuf_addstr(&pathbuf, sources.v[i]);

Shouldn't there be a call to strbuf_reset(&pathbuf) before doing this?

builtin/mv.c int cmd_mv(int argc, · line 496

Quoted 1 line

builtin/mv.c +1 -0

+		slash_pos = strrchr(pathbuf.buf, '/');

And start from the deepest directory, going one level up per iteration, ...

builtin/mv.c int cmd_mv(int argc, · line 503

Quoted 7 lines

builtin/mv.c +7 -0

+		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));

... see if the path being moved falls within that subdirectory.

builtin/mv.c int cmd_mv(int argc, · line 506

Quoted 2 lines

builtin/mv.c +2 -0

+			if (!hashmap_get_entry(&moved_dirs, &needle, ent, NULL))
+				continue;

If there is no overlap, we need to do anything special.

builtin/mv.c int cmd_mv(int argc, · line 510

Quoted 3 lines

builtin/mv.c +3 -0

+			if (!ignore_errors)
+				die(_("cannot move both parent directory '%s' and its child '%s'"),
+				    pathbuf.buf, sources.v[i]);

Otherwise we are in trouble.

builtin/mv.c int cmd_mv(int argc, · line 521

Quoted 10 lines

builtin/mv.c +10 -0

+			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;
+			}

So with

	$ git mv a/ a/b x y z/

then a/ is left in the argv[]/sources[]/destinations[] arrays, and upon inspecting a/b, we come here and in order to ignore a/b, we shift it out; the resulting arrays would have a/, x, and y being moved to z/.

It somehow feels troubling that it would lead to a different result if I give a morally equivalent arguments, i.e.

	$ git mv a/b a/ x y z/

where a/b survives and a/ gets omitted.

One thing that came to my mind (without concrete "here is the right way to solve it" that I am myself convinced) is this.

* Should this code path even have its own ignore-errors handling? "git mv a b z/", when 'a' does not exist, may ignore 'a' and move only 'b', which may make sense. But the original command line in that case is a plausibly correct one if there weren't missing or unmovable paths. The command line "git mv a/ a/b z/" seems to fall into a different category (aka "total nonsense"); no matter how you fix the items in your working tree files, you cannot make it plausibly correct.

a totally unrelated tangent that made me scratch my head while reading the original ocde is the dest_paths variable. It is never used as a collection to hold potentially multiple paths; it is a strvec only to be able to call internel_prefix_pathspec() with, and used only once with only one element in the vector. At least it should lose the plural 's' suffix to unconfuse its readers, I would think.

Quoted 30 lines · Junio C Hamano

Quoted 14 lines · Patrick Steinhardt

builtin/mv.c +3 -0

@@ -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;

builtin/mv.c int cmd_mv(int argc, · line 361

Quoted 10 lines

builtin/mv.c +5 -0

 			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);
+

OK, this collects in moved_dirs the directories that will get moved. And then a separate loop, ...

Quoted 7 lines

builtin/mv.c int cmd_mv(int argc, · line 494

Quoted 4 lines

builtin/mv.c +4 -0

+	for (i = 0; i < argc; i++) {
+		const char *slash_pos;
+
+		strbuf_addstr(&pathbuf, sources.v[i]);

Shouldn't there be a call to strbuf_reset(&pathbuf) before doing this?

Yup, indeed.

Quoted 5 lines

builtin/mv.c int cmd_mv(int argc, · line 496

Quoted 1 line

builtin/mv.c +1 -0

+		slash_pos = strrchr(pathbuf.buf, '/');

And start from the deepest directory, going one level up per iteration, ...

Quoted 9 lines

builtin/mv.c int cmd_mv(int argc, · line 503

Quoted 7 lines

builtin/mv.c +7 -0

+		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));

... see if the path being moved falls within that subdirectory.

Ah, there's another gotcha here: when moving a directory, we also add all of its children to argc. So this would now always fail when we move directories around.

I guess we can handle this by introducing another MOVE_VIA_PARENT_DIR mode -- we'd then skip the verification for any entry marked like this.

Quoted 31 lines

builtin/mv.c int cmd_mv(int argc, · line 506

Quoted 2 lines

builtin/mv.c +2 -0

+			if (!hashmap_get_entry(&moved_dirs, &needle, ent, NULL))
+				continue;

If there is no overlap, we need to do anything special.

builtin/mv.c int cmd_mv(int argc, · line 510

Quoted 3 lines

builtin/mv.c +3 -0

+			if (!ignore_errors)
+				die(_("cannot move both parent directory '%s' and its child '%s'"),
+				    pathbuf.buf, sources.v[i]);

Otherwise we are in trouble.

builtin/mv.c int cmd_mv(int argc, · line 521

Quoted 10 lines

builtin/mv.c +10 -0

+			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;
+			}

So with

	$ git mv a/ a/b x y z/

then a/ is left in the argv[]/sources[]/destinations[] arrays, and upon inspecting a/b, we come here and in order to ignore a/b, we shift it out; the resulting arrays would have a/, x, and y being moved to z/.

Quoted 6 lines

It somehow feels troubling that it would lead to a different result if I give a morally equivalent arguments, i.e.

	$ git mv a/b a/ x y z/

where a/b survives and a/ gets omitted.

Fully agreed. I was quite surprised to see that git-mv(1) already behaves like this with a couple of other error conditions. So I simply continued to build on top of this behaviour, but I'm not a fan of it at all.

Note that this behaviour doesn't trigger by default though. So your above command would cause us to die without doing any change at all. You explicitly have to git mv -k (whatever 'k' is supposed to mean -- maybe "keep going"?) to opt into this weird behaviour. Which makes this overall a bit less awful.

Quoted 11 lines

One thing that came to my mind (without concrete "here is the right way to solve it" that I am myself convinced) is this.

* Should this code path even have its own ignore-errors handling? "git mv a b z/", when 'a' does not exist, may ignore 'a' and move only 'b', which may make sense. But the original command line in that case is a plausibly correct one if there weren't missing or unmovable paths. The command line "git mv a/ a/b z/" seems to fall into a different category (aka "total nonsense"); no matter how you fix the items in your working tree files, you cannot make it plausibly correct.

Fair. I guess the intent of '-k' is about handling the case where a subset of files might be missing, not the case where the original request didn't make any sense at all. I certainly wouldn't mind to tighten this code.

Quoted 7 lines

a totally unrelated tangent that made me scratch my head while reading the original ocde is the dest_paths variable. It is never used as a collection to hold potentially multiple paths; it is a strvec only to be able to call internel_prefix_pathspec() with, and used only once with only one element in the vector. At least it should lose the plural 's' suffix to unconfuse its readers, I would think.

Yeah. From my point of view this isn't the only confusing part about this code.

Patrick

From: Johannes Schindelin <[email protected]>
Changes since v1:

* Rewrote the commit message to reflect that this patch is still needed, even if the symptom that originally motivated the patch was addressed in a different manner, because it was merely a symptom of the underlying root cause that CI builds should not let Visual C build Git in debug mode.

Quoted 8 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>
Changes since v1:

* Rewrote the commit message to reflect that this patch is still needed, even if the symptom that originally motivated the patch was addressed in a different manner, because it was merely a symptom of the underlying root cause that CI builds should not let Visual C build Git in debug mode.

Ok, makes sense. I think we should ideally address this issue strategically, e.g. by getting rid of asserts completely in our codebase. But for now I agree that we should just build Git in release mode on Windows.

Hi Patrick,

Quoted 10 lines · Patrick Steinhardt

Quoted 8 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>
Changes since v1:

* Rewrote the commit message to reflect that this patch is still needed, even if the symptom that originally motivated the patch was addressed in a different manner, because it was merely a symptom of the underlying root cause that CI builds should not let Visual C build Git in debug mode.

Quoted 4 lines

Ok, makes sense. I think we should ideally address this issue strategically, e.g. by getting rid of asserts completely in our codebase. But for now I agree that we should just build Git in release mode on Windows.

I am afraid that getting rid of asserts in Git's codebase won't ever be able to address the challenge that Git -- despite much reluctance -- relies on a couple of external dependencies that might at any point in time cause assert() to be called, e.g. due to unexpected changes in the CI runner images.

In essence, we will need both: CI builds in release mode _and_ converting assert()s to BUG() calls (the latter, however, for different reasons than to proactively address CI hangs: Git can convey better information in BUG() calls than in assert() calls).

There are more reasons to use release mode builds in CI, too, but we already have a compelling reason to do so, therefore there is no need for me to spend more time assembling an exhaustive list.

Quoted 5 lines

I am afraid that getting rid of asserts in Git's codebase won't ever be able to address the challenge that Git -- despite much reluctance -- relies on a couple of external dependencies that might at any point in time cause assert() to be called, e.g. due to unexpected changes in the CI runner images.

Good point indeed, I haven't considered this.

Quoted 7 lines · Patrick Steinhardt

Quoted 5 lines

I am afraid that getting rid of asserts in Git's codebase won't ever be able to address the challenge that Git -- despite much reluctance -- relies on a couple of external dependencies that might at any point in time cause assert() to be called, e.g. due to unexpected changes in the CI runner images.

Good point indeed, I haven't considered this.

Thanks both for a discussion. Let's replace and queue this, and fast track it down to 'maint'.

Here is a range-diff for my tentative rebasing the patch on 'maint'; I'll make sure merging it up to 'master' would match exactly the result of applying the original patch directly to 'master' before queuing.

Thanks!

1: f3ae94b175 ! 1: 184abdcf05 ci(win+Meson): build in Release mode
@@ Commit message
patch is still needed.

Signed-off-by Johannes Schindelin <[email protected]>

+ Acked-by: Patrick Steinhardt <[email protected]>
+ [jc: rebased on 'maint' to enable fast-tracking the change down]

Signed-off-by Junio C Hamano <[email protected]>

## .github/workflows/main.yml ##
@@ .github/workflows/main.yml: jobs:
run: pip install meson ninja
- name: Setup
shell: pwsh
-- run: meson setup build --vsenv -Dperl=disabled -Dcredential_helpers=wincred
-+ run: meson setup build --vsenv -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred
+- run: meson setup build -Dperl=disabled -Dcredential_helpers=wincred
++ run: meson setup build -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred
- name: Compile
shell: pwsh
run: meson compile -C build

Quoted 30 lines · Junio C Hamano

Quoted 7 lines · Patrick Steinhardt

Quoted 5 lines

I am afraid that getting rid of asserts in Git's codebase won't ever be able to address the challenge that Git -- despite much reluctance -- relies on a couple of external dependencies that might at any point in time cause assert() to be called, e.g. due to unexpected changes in the CI runner images.

Good point indeed, I haven't considered this.

Thanks both for a discussion. Let's replace and queue this, and fast track it down to 'maint'.

Here is a range-diff for my tentative rebasing the patch on 'maint'; I'll make sure merging it up to 'master' would match exactly the result of applying the original patch directly to 'master' before queuing.

Thanks!

1: f3ae94b175 ! 1: 184abdcf05 ci(win+Meson): build in Release mode
@@ Commit message
patch is still needed.

Signed-off-by Johannes Schindelin <[email protected]>

+ Acked-by: Patrick Steinhardt <[email protected]>
+ [jc: rebased on 'maint' to enable fast-tracking the change down]

Signed-off-by Junio C Hamano <[email protected]>

Quoted 12 lines

## .github/workflows/main.yml ##
@@ .github/workflows/main.yml: jobs:
run: pip install meson ninja
- name: Setup
shell: pwsh
-- run: meson setup build --vsenv -Dperl=disabled -Dcredential_helpers=wincred
-+ run: meson setup build --vsenv -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred
+- run: meson setup build -Dperl=disabled -Dcredential_helpers=wincred
++ run: meson setup build -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred
- name: Compile
shell: pwsh
run: meson compile -C build

Am I reading this diff correctly that we drop the --vsenv flag?

I think we should keep it around as it was a bug in the first place that we didn't have it.

I guess this is done because we didn't have the flag in "maint" yet.

But I'm not even sure whether the fix would be needed in case we don't build with Visual Studio, as the MinGW toolchain probably doesn't have the same behaviour with asserts (but please correct me if I'm wrong, Dscho). So maybe we should also cherry-pick 85e1d6819fb (ci: use Visual Studio for win+meson job on GitHub Workflows, 2025-03-31) at the same time.

Indeed. If we keep both bugs unfixed in maint (the bug that win+Meson does not build with Visual Studio, plus the bug that win+Meson builds in Debug mode), the CI will continue to pass... just not for the intended reasons.