{"thread":{"id":"63350","subject":"[PATCH] ci(win+Meson): build in Release mode, avoiding t7001-mv hangs","startedAt":"2025-04-25T15:05:21Z","lastAt":"2025-05-08T18:19:53Z","messageCount":17,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Patrick Steinhardt","Kristoffer Haugsbakk","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"516807","messageId":"pull.1908.git.1745593515875.gitgitgadget@gmail.com","threadId":"63350","inReplyTo":null,"subject":"[PATCH] ci(win+Meson): build in Release mode, avoiding t7001-mv hangs","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-04-25T15:05:15Z","receivedAt":"2025-04-25T15:05:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nSince switching to `--vsenv`, the t7001-mv test consistently times out\nafter six hours in the CI builds on GitHub. This kind of waste is\ninconsistent with my values.\n\nThe reason for this timeout is the test case 'nonsense mv triggers\nassertion failure and partially updated index' in t7001-mv (which is\nnot even a regression test, but instead merely demonstrates a bug that\nsomeone thought someone else should fix at some time). As the name\nsuggests, it triggers an assertion. The problem with this is that an\nassertion on Windows, at least when run in Debug mode, will open a modal\ndialog that patiently awaits some buttons to be clicked. Which never\nhappens in automated builds.\n\nThe solution is straight-forward: Just like the `win+VS` job already did\nin forever, build in Release mode (where that modal assertion dialog is\nnever shown).\n\nSigned-off-by: Johannes Schindelin \n---\n ci(win+Meson): build in Release mode, avoiding t7001-mv hangs\n \n I was surprised to find this issue today, and that this had not been\n addressed yet.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1908%2Fdscho%2Fdont-let-win%2BMeson-hang-in-t7001-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1908/dscho/dont-let-win+Meson-hang-in-t7001-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1908\n\n .github/workflows/main.yml | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\nindex 83ca8e4182b..275240be5dc 100644\n--- a/.github/workflows/main.yml\n+++ b/.github/workflows/main.yml\n@@ -265,7 +265,7 @@ jobs:\n run: pip install meson ninja\n - name: Setup\n shell: pwsh\n- run: meson setup build --vsenv -Dperl=disabled -Dcredential_helpers=wincred\n+ run: meson setup build --vsenv -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred\n - name: Compile\n shell: pwsh\n run: meson compile -C build\n\nbase-commit: f65182a99e545d2f2bc22e6c1c2da192133b16a3\n-- \ngitgitgadget\n"},{"id":"516808","messageId":"xmqqmsc4uv6d.fsf@gitster.g","threadId":"63350","inReplyTo":"pull.1908.git.1745593515875.gitgitgadget@gmail.com","subject":"Re: [PATCH] ci(win+Meson): build in Release mode, avoiding t7001-mv hangs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-25T15:18:02Z","receivedAt":"2025-04-25T15:18:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> Since switching to `--vsenv`, the t7001-mv test consistently times out\n> after six hours in the CI builds on GitHub. This kind of waste is\n> inconsistent with my values.\n\nWith mine too and I would presume everybody else's. I've been\nannoyed for a long time by one of those sharded Meson-Win test jobs\nthat hang around until timeout.\n\nThank you very much for addressing the issue.\n\n> The reason for this timeout is the test case 'nonsense mv triggers\n> assertion failure and partially updated index' in t7001-mv (which is\n> not even a regression test, but instead merely demonstrates a bug that\n> someone thought someone else should fix at some time). As the name\n> suggests, it triggers an assertion. The problem with this is that an\n> assertion on Windows, at least when run in Debug mode, will open a modal\n> dialog that patiently awaits some buttons to be clicked. Which never\n> happens in automated builds.\n\nInteresting.\n\nSo another viable fix (no, I am not suggesting a counter-proposal,\nbut asking a pure question to see if I understand the issue\ncorrectly) is to rewrite \"assert(cond)\" to \"if (cond) BUG(...)\"\nor something like that, so that it truly fails?\n\n> The solution is straight-forward: Just like the `win+VS` job already did\n> in forever, build in Release mode (where that modal assertion dialog is\n> never shown).\n\nOK.\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> ci(win+Meson): build in Release mode, avoiding t7001-mv hangs\n> \n> I was surprised to find this issue today, and that this had not been\n> addressed yet.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1908%2Fdscho%2Fdont-let-win%2BMeson-hang-in-t7001-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1908/dscho/dont-let-win+Meson-hang-in-t7001-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1908\n>\n> .github/workflows/main.yml | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\n> index 83ca8e4182b..275240be5dc 100644\n> --- a/.github/workflows/main.yml\n> +++ b/.github/workflows/main.yml\n> @@ -265,7 +265,7 @@ jobs:\n> run: pip install meson ninja\n> - name: Setup\n> shell: pwsh\n> - run: meson setup build --vsenv -Dperl=disabled -Dcredential_helpers=wincred\n> + run: meson setup build --vsenv -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred\n> - name: Compile\n> shell: pwsh\n> run: meson compile -C build\n>\n> base-commit: f65182a99e545d2f2bc22e6c1c2da192133b16a3\n"},{"id":"516882","messageId":"aA8ymUzWM2t0QkFP@pks.im","threadId":"63350","inReplyTo":"xmqqmsc4uv6d.fsf@gitster.g","subject":"Re: [PATCH] ci(win+Meson): build in Release mode, avoiding t7001-mv hangs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-04-28T07:47:37Z","receivedAt":"2025-04-28T07:47:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Apr 25, 2025 at 08:18:02AM -0700, Junio C Hamano wrote:\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n> \n> > From: Johannes Schindelin \n> >\n> > Since switching to `--vsenv`, the t7001-mv test consistently times out\n> > after six hours in the CI builds on GitHub. This kind of waste is\n> > inconsistent with my values.\n> \n> With mine too and I would presume everybody else's. I've been\n> annoyed for a long time by one of those sharded Meson-Win test jobs\n> that hang around until timeout.\n> \n> Thank you very much for addressing the issue.\n\nIndeed, thanks for fixing the issue! I haven't noticed these failing\njobs yet on my end, probably because on GitLab we only execute the\nWin+Meson changes manually. Which is not great given that it makes me\nmiss issues with Meson like this.\n\nI'll send a patch to run these tests by default so that I can see the\npain myself and address any issues that come up before others are forced\nto.\n\n> > The reason for this timeout is the test case 'nonsense mv triggers\n> > assertion failure and partially updated index' in t7001-mv (which is\n> > not even a regression test, but instead merely demonstrates a bug that\n> > someone thought someone else should fix at some time). As the name\n> > suggests, it triggers an assertion. The problem with this is that an\n> > assertion on Windows, at least when run in Debug mode, will open a modal\n> > dialog that patiently awaits some buttons to be clicked. Which never\n> > happens in automated builds.\n> \n> Interesting.\n> \n> So another viable fix (no, I am not suggesting a counter-proposal,\n> but asking a pure question to see if I understand the issue\n> correctly) is to rewrite \"assert(cond)\" to \"if (cond) BUG(...)\"\n> or something like that, so that it truly fails?\n\nOn the surface this sounds like a reasonable thing to do, but I don't\nhave enough context to be really able to tell.\n\n> > diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\n> > index 83ca8e4182b..275240be5dc 100644\n> > --- a/.github/workflows/main.yml\n> > +++ b/.github/workflows/main.yml\n> > @@ -265,7 +265,7 @@ jobs:\n> > run: pip install meson ninja\n> > - name: Setup\n> > shell: pwsh\n> > - run: meson setup build --vsenv -Dperl=disabled -Dcredential_helpers=wincred\n> > + run: meson setup build --vsenv -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred\n> > - name: Compile\n> > shell: pwsh\n> > run: meson compile -C build\n\nSo I'm fine with this trivial change as a band aid for now. The diff\nlooks obviously good to me.\n\nPatrick\n"},{"id":"516899","messageId":"xmqq8qnkdxu9.fsf@gitster.g","threadId":"63350","inReplyTo":"aA8ymUzWM2t0QkFP@pks.im","subject":"Re: [PATCH] ci(win+Meson): build in Release mode, avoiding t7001-mv hangs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-28T17:01:34Z","receivedAt":"2025-04-28T17:01:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> > The reason for this timeout is the test case 'nonsense mv triggers\n>> > assertion failure and partially updated index' in t7001-mv (which is\n>> > not even a regression test, but instead merely demonstrates a bug that\n>> > someone thought someone else should fix at some time). As the name\n>> > suggests, it triggers an assertion. The problem with this is that an\n>> > assertion on Windows, at least when run in Debug mode, will open a modal\n>> > dialog that patiently awaits some buttons to be clicked. Which never\n>> > happens in automated builds.\n>> \n>> Interesting.\n>> \n>> So another viable fix (no, I am not suggesting a counter-proposal,\n>> but asking a pure question to see if I understand the issue\n>> correctly) is to rewrite \"assert(cond)\" to \"if (cond) BUG(...)\"\n>> or something like that, so that it truly fails?\n>\n> On the surface this sounds like a reasonable thing to do, but I don't\n> have enough context to be really able to tell.\n\nInteresting again ;-) I didn't realize that it was a fairly recent\ndevelopment. 0fcd473f (t7001: add failure test which triggers\nassertion, 2024-10-22) is what adds the questionable test.\n\nAnd I do agree with Dscho's assessment that this is \"show a bug\nwithout bothering to fix it\", which is not what we usually take\nwithout first exploring how involved the necessary fix would be.\n\nI wonder in what bad status would a production build that simply\ndisabled the assert() is leaving the resulting repository.\n\nQuoting from the last part of my response [*] to the initial report\nthat eventually turned into the test after 9 months:\n\n [*] https://lore.kernel.org/git/xmqqil47obnw.fsf@gitster.g/\n\n---- snip snap ----\nThanks for reporting, Kristoffer.\n\nAny takers?\n\n$ git shortlog --since=3.years -s -n -e --no-merges v2.43.0 builtin/mv.c\n 15\tShaoxuan Yuan \n 10\tElijah Newren \n 5\tÆvar Arnfjörð Bjarmason \n 2\tJunio C Hamano \n 1\tAndrzej Hunt \n 1\tCalvin Wan \n 1\tDerrick Stolee \n 1\tSebastian Thiel \n 1\tTorsten Bögershausen \n"},{"id":"516949","messageId":"aBDD-NeN2YoQbU9S@pks.im","threadId":"63350","inReplyTo":"xmqq8qnkdxu9.fsf@gitster.g","subject":"Re: [PATCH] ci(win+Meson): build in Release mode, avoiding t7001-mv hangs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-04-29T12:20:08Z","receivedAt":"2025-04-29T12:20:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Apr 28, 2025 at 10:01:34AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> >> > The reason for this timeout is the test case 'nonsense mv triggers\n> >> > assertion failure and partially updated index' in t7001-mv (which is\n> >> > not even a regression test, but instead merely demonstrates a bug that\n> >> > someone thought someone else should fix at some time). As the name\n> >> > suggests, it triggers an assertion. The problem with this is that an\n> >> > assertion on Windows, at least when run in Debug mode, will open a modal\n> >> > dialog that patiently awaits some buttons to be clicked. Which never\n> >> > happens in automated builds.\n> >> \n> >> Interesting.\n> >> \n> >> So another viable fix (no, I am not suggesting a counter-proposal,\n> >> but asking a pure question to see if I understand the issue\n> >> correctly) is to rewrite \"assert(cond)\" to \"if (cond) BUG(...)\"\n> >> or something like that, so that it truly fails?\n> >\n> > On the surface this sounds like a reasonable thing to do, but I don't\n> > have enough context to be really able to tell.\n> \n> Interesting again ;-) I didn't realize that it was a fairly recent\n> development. 0fcd473f (t7001: add failure test which triggers\n> assertion, 2024-10-22) is what adds the questionable test.\n> \n> And I do agree with Dscho's assessment that this is \"show a bug\n> without bothering to fix it\", which is not what we usually take\n> without first exploring how involved the necessary fix would be.\n> \n> I wonder in what bad status would a production build that simply\n> disabled the assert() is leaving the resulting repository.\n> \n> Quoting from the last part of my response [*] to the initial report\n> that eventually turned into the test after 9 months:\n> \n> [*] https://lore.kernel.org/git/xmqqil47obnw.fsf@gitster.g/\n\nHm. So the issue here is that we're trying to move both a parent\ndirectory as well as a child thereof at the same point in time? I guess\nthat'd need something like the below patch to restrict such requests.\nThis leads to:\n\n $ ./git mv ../builtin/add.c ../builtin ../t\n fatal: cannot move both 'builtin/add.c' and its parent directory 'builtin'\n\nThe idea of the patch is that we're tracking all parent directories in a\nhashmap. After we have processed all arguments, we then perform a second\npass to check that none of the source files have any of their parent\ndirectories moved at the same time.\n\nI only had a brief look, so this assessment may very well be completely\nwrong, and the change doesn't even pass all tests right now. But if this\nis indeed the issue then I'm happy to polish this into a proper patch.\n\nPatrick\n\n-- >8 --\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 54b323fff72..10b2cb56ec7 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -183,6 +183,21 @@ static void remove_empty_src_dirs(const char **src_dir, size_t src_dir_nr)\n \tstrbuf_release(&a_src_dir);\n }\n \n+struct pathmap_entry {\n+\tstruct hashmap_entry ent;\n+\tconst char *path;\n+};\n+\n+static int pathmap_cmp(const void *cmp_data UNUSED,\n+\t\t const struct hashmap_entry *a,\n+\t\t const struct hashmap_entry *b,\n+\t\t const void *key UNUSED)\n+{\n+\tconst struct pathmap_entry *e1 = container_of(a, struct pathmap_entry, ent);\n+\tconst struct pathmap_entry *e2 = container_of(b, struct pathmap_entry, ent);\n+\treturn fspathcmp(e1->path, e2->path);\n+}\n+\n int cmd_mv(int argc,\n \t const char **argv,\n \t const char *prefix,\n@@ -213,6 +228,8 @@ int cmd_mv(int argc,\n \tstruct cache_entry *ce;\n \tstruct string_list only_match_skip_worktree = STRING_LIST_INIT_DUP;\n \tstruct string_list dirty_paths = STRING_LIST_INIT_DUP;\n+\tstruct hashmap moved_dirs = HASHMAP_INIT(pathmap_cmp, NULL);\n+\tstruct strbuf pathbuf = STRBUF_INIT;\n \tint ret;\n \n \tgit_config(git_default_config, NULL);\n@@ -331,11 +348,17 @@ int cmd_mv(int argc,\n \n dir_check:\n \t\tif (S_ISDIR(st.st_mode)) {\n+\t\t\tstruct pathmap_entry *entry;\n \t\t\tchar *dst_with_slash;\n \t\t\tsize_t dst_with_slash_len;\n \t\t\tint j, n;\n \t\t\tint first = index_name_pos(the_repository->index, src, length), last;\n \n+\t\t\tentry = xmalloc(sizeof(*entry));\n+\t\t\tentry->path = src;\n+\t\t\thashmap_entry_init(&entry->ent, fspathhash(src));\n+\t\t\thashmap_add(&moved_dirs, &entry->ent);\n+\n \t\t\tif (first >= 0) {\n \t\t\t\tconst char *path = submodule_gitfile_path(src, first);\n \t\t\t\tif (path != SUBMODULE_WITH_GITDIR)\n@@ -465,6 +488,40 @@ int cmd_mv(int argc,\n \t\t}\n \t}\n \n+\tfor (i = 0; i < argc; i++) {\n+\t\tconst char *slash_pos;\n+\n+\t\tstrbuf_addstr(&pathbuf, sources.v[i]);\n+\n+\t\tslash_pos = strrchr(pathbuf.buf, '/');\n+\t\twhile (slash_pos > pathbuf.buf) {\n+\t\t\tstruct pathmap_entry needle;\n+\n+\t\t\tstrbuf_setlen(&pathbuf, slash_pos - pathbuf.buf);\n+\n+\t\t\tneedle.path = pathbuf.buf;\n+\t\t\thashmap_entry_init(&needle.ent, fspathhash(pathbuf.buf));\n+\n+\t\t\tif (!hashmap_get_entry(&moved_dirs, &needle, ent, NULL))\n+\t\t\t\tcontinue;\n+\n+\t\t\tif (!ignore_errors)\n+\t\t\t\tdie(_(\"cannot move both parent directory '%s' and its child '%s'\"),\n+\t\t\t\t pathbuf.buf, sources.v[i]);\n+\n+\t\t\tif (--argc > 0) {\n+\t\t\t\tint n = argc - i;\n+\t\t\t\tstrvec_remove(&sources, i);\n+\t\t\t\tstrvec_remove(&destinations, i);\n+\t\t\t\tMOVE_ARRAY(modes + i, modes + i + 1, n);\n+\t\t\t\tMOVE_ARRAY(submodule_gitfiles + i,\n+\t\t\t\t\t submodule_gitfiles + i + 1, n);\n+\t\t\t\ti--;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n \tif (only_match_skip_worktree.nr) {\n \t\tadvise_on_updating_sparse_paths(&only_match_skip_worktree);\n \t\tif (!ignore_errors) {\n@@ -589,6 +646,8 @@ int cmd_mv(int argc,\n \tstrvec_clear(&dest_paths);\n \tstrvec_clear(&destinations);\n \tstrvec_clear(&submodule_gitfiles_to_free);\n+\thashmap_clear(&moved_dirs);\n+\tstrbuf_release(&pathbuf);\n \tfree(submodule_gitfiles);\n \tfree(modes);\n \treturn ret;\n\n"},{"id":"516987","messageId":"xmqqplgu4ru5.fsf@gitster.g","threadId":"63350","inReplyTo":"aBDD-NeN2YoQbU9S@pks.im","subject":"Re: [PATCH] ci(win+Meson): build in Release mode, avoiding t7001-mv hangs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-29T20:48:18Z","receivedAt":"2025-04-29T20:48:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> @@ -213,6 +228,8 @@ int cmd_mv(int argc,\n> \tstruct cache_entry *ce;\n> \tstruct string_list only_match_skip_worktree = STRING_LIST_INIT_DUP;\n> \tstruct string_list dirty_paths = STRING_LIST_INIT_DUP;\n> +\tstruct hashmap moved_dirs = HASHMAP_INIT(pathmap_cmp, NULL);\n> +\tstruct strbuf pathbuf = STRBUF_INIT;\n> \tint ret;\n> \n> \tgit_config(git_default_config, NULL);\n> @@ -331,11 +348,17 @@ int cmd_mv(int argc,\n> \n> dir_check:\n> \t\tif (S_ISDIR(st.st_mode)) {\n> +\t\t\tstruct pathmap_entry *entry;\n> \t\t\tchar *dst_with_slash;\n> \t\t\tsize_t dst_with_slash_len;\n> \t\t\tint j, n;\n> \t\t\tint first = index_name_pos(the_repository->index, src, length), last;\n> \n> +\t\t\tentry = xmalloc(sizeof(*entry));\n> +\t\t\tentry->path = src;\n> +\t\t\thashmap_entry_init(&entry->ent, fspathhash(src));\n> +\t\t\thashmap_add(&moved_dirs, &entry->ent);\n> +\n\nOK, this collects in moved_dirs the directories that will get moved.\nAnd then a separate loop, ...\n\n> +\tfor (i = 0; i < argc; i++) {\n> +\t\tconst char *slash_pos;\n> +\n> +\t\tstrbuf_addstr(&pathbuf, sources.v[i]);\n\nShouldn't there be a call to strbuf_reset(&pathbuf) before doing\nthis?\n\n> +\t\tslash_pos = strrchr(pathbuf.buf, '/');\n\nAnd start from the deepest directory, going one level up per\niteration, ...\n\n> +\t\twhile (slash_pos > pathbuf.buf) {\n> +\t\t\tstruct pathmap_entry needle;\n> +\n> +\t\t\tstrbuf_setlen(&pathbuf, slash_pos - pathbuf.buf);\n> +\n> +\t\t\tneedle.path = pathbuf.buf;\n> +\t\t\thashmap_entry_init(&needle.ent, fspathhash(pathbuf.buf));\n\n... see if the path being moved falls within that subdirectory.\n\n> +\t\t\tif (!hashmap_get_entry(&moved_dirs, &needle, ent, NULL))\n> +\t\t\t\tcontinue;\n\nIf there is no overlap, we need to do anything special.\n\n> +\t\t\tif (!ignore_errors)\n> +\t\t\t\tdie(_(\"cannot move both parent directory '%s' and its child '%s'\"),\n> +\t\t\t\t pathbuf.buf, sources.v[i]);\n\nOtherwise we are in trouble.\n\n> +\t\t\tif (--argc > 0) {\n> +\t\t\t\tint n = argc - i;\n> +\t\t\t\tstrvec_remove(&sources, i);\n> +\t\t\t\tstrvec_remove(&destinations, i);\n> +\t\t\t\tMOVE_ARRAY(modes + i, modes + i + 1, n);\n> +\t\t\t\tMOVE_ARRAY(submodule_gitfiles + i,\n> +\t\t\t\t\t submodule_gitfiles + i + 1, n);\n> +\t\t\t\ti--;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n\nSo with\n\n\t$ git mv a/ a/b x y z/\n\nthen a/ is left in the argv[]/sources[]/destinations[] arrays, and\nupon inspecting a/b, we come here and in order to ignore a/b, we\nshift it out; the resulting arrays would have a/, x, and y being\nmoved to z/.\n\nIt somehow feels troubling that it would lead to a different result\nif I give a morally equivalent arguments, i.e.\n\n\t$ git mv a/b a/ x y z/\n\nwhere a/b survives and a/ gets omitted.\n\nOne thing that came to my mind (without concrete \"here is the right\nway to solve it\" that I am myself convinced) is this.\n\n * Should this code path even have its own ignore-errors handling?\n \"git mv a b z/\", when 'a' does not exist, may ignore 'a' and move\n only 'b', which may make sense. But the original command line in\n that case is a plausibly correct one if there weren't missing or\n unmovable paths. The command line \"git mv a/ a/b z/\" seems to\n fall into a different category (aka \"total nonsense\"); no matter\n how you fix the items in your working tree files, you cannot make\n it plausibly correct.\n\n\na totally unrelated tangent that made me scratch my head while\nreading the original ocde is the dest_paths variable. It is never\nused as a collection to hold potentially multiple paths; it is a\nstrvec only to be able to call internel_prefix_pathspec() with, and\nused only once with only one element in the vector. At least it\nshould lose the plural 's' suffix to unconfuse its readers, I would\nthink.\n"},{"id":"516988","messageId":"527d6cd3-185c-47c6-8c02-7e023b6dcba3@app.fastmail.com","threadId":"63350","inReplyTo":"pull.1908.git.1745593515875.gitgitgadget@gmail.com","subject":"Re: [PATCH] ci(win+Meson): build in Release mode, avoiding t7001-mv hangs","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2025-04-29T20:57:29Z","receivedAt":"2025-04-29T20:57:53Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Fri, Apr 25, 2025, at 17:05, Johannes Schindelin via GitGitGadget wrote:\n> The reason for this timeout is the test case 'nonsense mv triggers\n> assertion failure and partially updated index' in t7001-mv (which is\n> not even a regression test, but instead merely demonstrates a bug that\n> someone thought someone else should fix at some time). As the name\n> suggests, it triggers an assertion. The problem with this is that an\n> assertion on Windows, at least when run in Debug mode, will open a modal\n> dialog that patiently awaits some buttons to be clicked. Which never\n> happens in automated builds.\n\nSorry for the trouble.\n"},{"id":"517002","messageId":"aBHmMKqhVDEl777o@pks.im","threadId":"63350","inReplyTo":"xmqqplgu4ru5.fsf@gitster.g","subject":"Re: [PATCH] ci(win+Meson): build in Release mode, avoiding t7001-mv hangs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-04-30T08:58:24Z","receivedAt":"2025-04-30T08:58:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Apr 29, 2025 at 01:48:18PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > @@ -213,6 +228,8 @@ int cmd_mv(int argc,\n> > \tstruct cache_entry *ce;\n> > \tstruct string_list only_match_skip_worktree = STRING_LIST_INIT_DUP;\n> > \tstruct string_list dirty_paths = STRING_LIST_INIT_DUP;\n> > +\tstruct hashmap moved_dirs = HASHMAP_INIT(pathmap_cmp, NULL);\n> > +\tstruct strbuf pathbuf = STRBUF_INIT;\n> > \tint ret;\n> > \n> > \tgit_config(git_default_config, NULL);\n> > @@ -331,11 +348,17 @@ int cmd_mv(int argc,\n> > \n> > dir_check:\n> > \t\tif (S_ISDIR(st.st_mode)) {\n> > +\t\t\tstruct pathmap_entry *entry;\n> > \t\t\tchar *dst_with_slash;\n> > \t\t\tsize_t dst_with_slash_len;\n> > \t\t\tint j, n;\n> > \t\t\tint first = index_name_pos(the_repository->index, src, length), last;\n> > \n> > +\t\t\tentry = xmalloc(sizeof(*entry));\n> > +\t\t\tentry->path = src;\n> > +\t\t\thashmap_entry_init(&entry->ent, fspathhash(src));\n> > +\t\t\thashmap_add(&moved_dirs, &entry->ent);\n> > +\n> \n> OK, this collects in moved_dirs the directories that will get moved.\n> And then a separate loop, ...\n> \n> > +\tfor (i = 0; i < argc; i++) {\n> > +\t\tconst char *slash_pos;\n> > +\n> > +\t\tstrbuf_addstr(&pathbuf, sources.v[i]);\n> \n> Shouldn't there be a call to strbuf_reset(&pathbuf) before doing\n> this?\n\nYup, indeed.\n\n> > +\t\tslash_pos = strrchr(pathbuf.buf, '/');\n> \n> And start from the deepest directory, going one level up per\n> iteration, ...\n> \n> > +\t\twhile (slash_pos > pathbuf.buf) {\n> > +\t\t\tstruct pathmap_entry needle;\n> > +\n> > +\t\t\tstrbuf_setlen(&pathbuf, slash_pos - pathbuf.buf);\n> > +\n> > +\t\t\tneedle.path = pathbuf.buf;\n> > +\t\t\thashmap_entry_init(&needle.ent, fspathhash(pathbuf.buf));\n> \n> ... see if the path being moved falls within that subdirectory.\n\nAh, there's another gotcha here: when moving a directory, we also add\nall of its children to `argc`. So this would now always fail when we\nmove directories around.\n\nI guess we can handle this by introducing another `MOVE_VIA_PARENT_DIR`\nmode -- we'd then skip the verification for any entry marked like this.\n\n> > +\t\t\tif (!hashmap_get_entry(&moved_dirs, &needle, ent, NULL))\n> > +\t\t\t\tcontinue;\n> \n> If there is no overlap, we need to do anything special.\n> \n> > +\t\t\tif (!ignore_errors)\n> > +\t\t\t\tdie(_(\"cannot move both parent directory '%s' and its child '%s'\"),\n> > +\t\t\t\t pathbuf.buf, sources.v[i]);\n> \n> Otherwise we are in trouble.\n> \n> > +\t\t\tif (--argc > 0) {\n> > +\t\t\t\tint n = argc - i;\n> > +\t\t\t\tstrvec_remove(&sources, i);\n> > +\t\t\t\tstrvec_remove(&destinations, i);\n> > +\t\t\t\tMOVE_ARRAY(modes + i, modes + i + 1, n);\n> > +\t\t\t\tMOVE_ARRAY(submodule_gitfiles + i,\n> > +\t\t\t\t\t submodule_gitfiles + i + 1, n);\n> > +\t\t\t\ti--;\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n> \n> So with\n> \n> \t$ git mv a/ a/b x y z/\n> \n> then a/ is left in the argv[]/sources[]/destinations[] arrays, and\n> upon inspecting a/b, we come here and in order to ignore a/b, we\n> shift it out; the resulting arrays would have a/, x, and y being\n> moved to z/.\n> \n> It somehow feels troubling that it would lead to a different result\n> if I give a morally equivalent arguments, i.e.\n> \n> \t$ git mv a/b a/ x y z/\n> \n> where a/b survives and a/ gets omitted.\n\nFully agreed. I was quite surprised to see that git-mv(1) already\nbehaves like this with a couple of other error conditions. So I simply\ncontinued to build on top of this behaviour, but I'm not a fan of it at\nall.\n\nNote that this behaviour doesn't trigger by default though. So your\nabove command would cause us to die without doing any change at all. You\nexplicitly have to `git mv -k` (whatever 'k' is supposed to mean --\nmaybe \"keep going\"?) to opt into this weird behaviour. Which makes this\noverall a bit less awful.\n\n> One thing that came to my mind (without concrete \"here is the right\n> way to solve it\" that I am myself convinced) is this.\n> \n> * Should this code path even have its own ignore-errors handling?\n> \"git mv a b z/\", when 'a' does not exist, may ignore 'a' and move\n> only 'b', which may make sense. But the original command line in\n> that case is a plausibly correct one if there weren't missing or\n> unmovable paths. The command line \"git mv a/ a/b z/\" seems to\n> fall into a different category (aka \"total nonsense\"); no matter\n> how you fix the items in your working tree files, you cannot make\n> it plausibly correct.\n\nFair. I guess the intent of '-k' is about handling the case where a\nsubset of files might be missing, not the case where the original\nrequest didn't make any sense at all. I certainly wouldn't mind to\ntighten this code.\n\n> a totally unrelated tangent that made me scratch my head while\n> reading the original ocde is the dest_paths variable. It is never\n> used as a collection to hold potentially multiple paths; it is a\n> strvec only to be able to call internel_prefix_pathspec() with, and\n> used only once with only one element in the vector. At least it\n> should lose the plural 's' suffix to unconfuse its readers, I would\n> think.\n\nYeah. From my point of view this isn't the only confusing part about\nthis code.\n\nPatrick\n"},{"id":"517021","messageId":"aBIbtykdjseJRzwr@pks.im","threadId":"63350","inReplyTo":"aBHmMKqhVDEl777o@pks.im","subject":"Re: [PATCH] ci(win+Meson): build in Release mode, avoiding t7001-mv hangs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-04-30T12:46:47Z","receivedAt":"2025-04-30T12:46:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 30, 2025 at 10:58:24AM +0200, Patrick Steinhardt wrote:\n> On Tue, Apr 29, 2025 at 01:48:18PM -0700, Junio C Hamano wrote:\n> > Patrick Steinhardt writes:\n> > \n> > > @@ -213,6 +228,8 @@ int cmd_mv(int argc,\n> > > \tstruct cache_entry *ce;\n> > > \tstruct string_list only_match_skip_worktree = STRING_LIST_INIT_DUP;\n> > > \tstruct string_list dirty_paths = STRING_LIST_INIT_DUP;\n> > > +\tstruct hashmap moved_dirs = HASHMAP_INIT(pathmap_cmp, NULL);\n> > > +\tstruct strbuf pathbuf = STRBUF_INIT;\n> > > \tint ret;\n> > > \n> > > \tgit_config(git_default_config, NULL);\n> > > @@ -331,11 +348,17 @@ int cmd_mv(int argc,\n> > > \n> > > dir_check:\n> > > \t\tif (S_ISDIR(st.st_mode)) {\n> > > +\t\t\tstruct pathmap_entry *entry;\n> > > \t\t\tchar *dst_with_slash;\n> > > \t\t\tsize_t dst_with_slash_len;\n> > > \t\t\tint j, n;\n> > > \t\t\tint first = index_name_pos(the_repository->index, src, length), last;\n> > > \n> > > +\t\t\tentry = xmalloc(sizeof(*entry));\n> > > +\t\t\tentry->path = src;\n> > > +\t\t\thashmap_entry_init(&entry->ent, fspathhash(src));\n> > > +\t\t\thashmap_add(&moved_dirs, &entry->ent);\n> > > +\n> > \n> > OK, this collects in moved_dirs the directories that will get moved.\n> > And then a separate loop, ...\n> > \n> > > +\tfor (i = 0; i < argc; i++) {\n> > > +\t\tconst char *slash_pos;\n> > > +\n> > > +\t\tstrbuf_addstr(&pathbuf, sources.v[i]);\n> > \n> > Shouldn't there be a call to strbuf_reset(&pathbuf) before doing\n> > this?\n> \n> Yup, indeed.\n> \n> > > +\t\tslash_pos = strrchr(pathbuf.buf, '/');\n> > \n> > And start from the deepest directory, going one level up per\n> > iteration, ...\n> > \n> > > +\t\twhile (slash_pos > pathbuf.buf) {\n> > > +\t\t\tstruct pathmap_entry needle;\n> > > +\n> > > +\t\t\tstrbuf_setlen(&pathbuf, slash_pos - pathbuf.buf);\n> > > +\n> > > +\t\t\tneedle.path = pathbuf.buf;\n> > > +\t\t\thashmap_entry_init(&needle.ent, fspathhash(pathbuf.buf));\n> > \n> > ... see if the path being moved falls within that subdirectory.\n> \n> Ah, there's another gotcha here: when moving a directory, we also add\n> all of its children to `argc`. So this would now always fail when we\n> move directories around.\n> \n> I guess we can handle this by introducing another `MOVE_VIA_PARENT_DIR`\n> mode -- we'd then skip the verification for any entry marked like this.\n> \n> > > +\t\t\tif (!hashmap_get_entry(&moved_dirs, &needle, ent, NULL))\n> > > +\t\t\t\tcontinue;\n> > \n> > If there is no overlap, we need to do anything special.\n> > \n> > > +\t\t\tif (!ignore_errors)\n> > > +\t\t\t\tdie(_(\"cannot move both parent directory '%s' and its child '%s'\"),\n> > > +\t\t\t\t pathbuf.buf, sources.v[i]);\n> > \n> > Otherwise we are in trouble.\n> > \n> > > +\t\t\tif (--argc > 0) {\n> > > +\t\t\t\tint n = argc - i;\n> > > +\t\t\t\tstrvec_remove(&sources, i);\n> > > +\t\t\t\tstrvec_remove(&destinations, i);\n> > > +\t\t\t\tMOVE_ARRAY(modes + i, modes + i + 1, n);\n> > > +\t\t\t\tMOVE_ARRAY(submodule_gitfiles + i,\n> > > +\t\t\t\t\t submodule_gitfiles + i + 1, n);\n> > > +\t\t\t\ti--;\n> > > +\t\t\t\tbreak;\n> > > +\t\t\t}\n> > \n> > So with\n> > \n> > \t$ git mv a/ a/b x y z/\n> > \n> > then a/ is left in the argv[]/sources[]/destinations[] arrays, and\n> > upon inspecting a/b, we come here and in order to ignore a/b, we\n> > shift it out; the resulting arrays would have a/, x, and y being\n> > moved to z/.\n> > \n> > It somehow feels troubling that it would lead to a different result\n> > if I give a morally equivalent arguments, i.e.\n> > \n> > \t$ git mv a/b a/ x y z/\n> > \n> > where a/b survives and a/ gets omitted.\n> \n> Fully agreed. I was quite surprised to see that git-mv(1) already\n> behaves like this with a couple of other error conditions. So I simply\n> continued to build on top of this behaviour, but I'm not a fan of it at\n> all.\n> \n> Note that this behaviour doesn't trigger by default though. So your\n> above command would cause us to die without doing any change at all. You\n> explicitly have to `git mv -k` (whatever 'k' is supposed to mean --\n> maybe \"keep going\"?) to opt into this weird behaviour. Which makes this\n> overall a bit less awful.\n> \n> > One thing that came to my mind (without concrete \"here is the right\n> > way to solve it\" that I am myself convinced) is this.\n> > \n> > * Should this code path even have its own ignore-errors handling?\n> > \"git mv a b z/\", when 'a' does not exist, may ignore 'a' and move\n> > only 'b', which may make sense. But the original command line in\n> > that case is a plausibly correct one if there weren't missing or\n> > unmovable paths. The command line \"git mv a/ a/b z/\" seems to\n> > fall into a different category (aka \"total nonsense\"); no matter\n> > how you fix the items in your working tree files, you cannot make\n> > it plausibly correct.\n> \n> Fair. I guess the intent of '-k' is about handling the case where a\n> subset of files might be missing, not the case where the original\n> request didn't make any sense at all. I certainly wouldn't mind to\n> tighten this code.\n> \n> > a totally unrelated tangent that made me scratch my head while\n> > reading the original ocde is the dest_paths variable. It is never\n> > used as a collection to hold potentially multiple paths; it is a\n> > strvec only to be able to call internel_prefix_pathspec() with, and\n> > used only once with only one element in the vector. At least it\n> > should lose the plural 's' suffix to unconfuse its readers, I would\n> > think.\n> \n> Yeah. From my point of view this isn't the only confusing part about\n> this code.\n> \n> Patrick\n> \n\nI have polished this patch a bit now and sent it via [1]. Thanks!\n\nPatrick\n\n[1]: <20250430-pks-mv-parent-child-conflict-v1-0-11a87c55ffb9@pks.im>\n"},{"id":"517171","messageId":"pull.1908.v2.git.1746282346370.gitgitgadget@gmail.com","threadId":"63350","inReplyTo":"pull.1908.git.1745593515875.gitgitgadget@gmail.com","subject":"[PATCH v2] ci(win+Meson): build in Release mode","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-03T14:25:46Z","receivedAt":"2025-05-03T14:25:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen the `win+Meson` job was added to Git's CI, modeled after the\n`win+vs` job, it overlooked that the latter built the Git artifacts in\nrelease mode.\n\nThe reason for this is that there is code in `compat/mingw.c` that turns\non the modal assertion dialogs in debug mode, which are very useful when\ndebugging interactively (as they offer to attach Visual Studio's\ndebugger), but they are scarcely useful in CI builds (where that modal\ndialog would sit around, waiting for a human being to see and deal with\nit, which obviously won't ever happen).\n\nThis problem was not realized immediately because of a separate bug: the\n`win+Meson` job erroneously built using the `gcc` that is in the `PATH`\nby default on hosted GitHub Actions runners. Since that bug was fixed by\nswitching to `--vsenv`, though, the t7001-mv test consistently timed out\nafter six hours in the CI builds on GitHub, quite often, and wasting\nbuild minutes without any benefit in return.\n\nThe reason for this timeout was a symptom of aforementioned debug mode\nproblem, where the test case 'nonsense mv triggers assertion failure and\npartially updated index' in t7001-mv triggered an assertion.\n\nI originally proposed this here patch to address the timeouts in CI\nbuilds. The Git project decided to address this timeout differently,\nthough: by fixing the bug that the t7001-mv test case demonstrated. This\ndoes not address the debug mode problem, though, as an `assert()` call\ncould be triggered in other ways in CI, and it should still not cause\nthe CI build to hang but should cause Git to error out instead. To avoid\nhaving to accept this here patch, it was then proposed to replace all\n`assert()` calls in Git's code base by `BUG()` calls. This might be\nreasonable for independent reasons, but it obviously still does not\naddress the debug mode problem, as `assert()` calls could be easily\nre-introduced by mistake, and besides, Git has a couple of dependencies\nthat all may have their own `assert()` calls (which are then safely\noutside the control of the Git project to remove), therefore this here\npatch is still needed.\n\nSigned-off-by: Johannes Schindelin \n---\n ci(win+Meson): build in Release mode, avoiding t7001-mv hangs\n \n I was surprised to find this issue today, and that this had not been\n addressed yet.\n \n Changes since v1:\n \n * Rewrote the commit message to reflect that this patch is still\n needed, even if the symptom that originally motivated the patch was\n addressed in a different manner, because it was merely a symptom of\n the underlying root cause that CI builds should not let Visual C\n build Git in debug mode.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1908%2Fdscho%2Fdont-let-win%2BMeson-hang-in-t7001-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1908/dscho/dont-let-win+Meson-hang-in-t7001-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1908\n\nRange-diff vs v1:\n\n 1: 18d1d18c48a ! 1: c04b6c97b25 ci(win+Meson): build in Release mode, avoiding t7001-mv hangs\n @@ Metadata\n Author: Johannes Schindelin \n \n ## Commit message ##\n - ci(win+Meson): build in Release mode, avoiding t7001-mv hangs\n -\n - Since switching to `--vsenv`, the t7001-mv test consistently times out\n - after six hours in the CI builds on GitHub. This kind of waste is\n - inconsistent with my values.\n -\n - The reason for this timeout is the test case 'nonsense mv triggers\n - assertion failure and partially updated index' in t7001-mv (which is\n - not even a regression test, but instead merely demonstrates a bug that\n - someone thought someone else should fix at some time). As the name\n - suggests, it triggers an assertion. The problem with this is that an\n - assertion on Windows, at least when run in Debug mode, will open a modal\n - dialog that patiently awaits some buttons to be clicked. Which never\n - happens in automated builds.\n -\n - The solution is straight-forward: Just like the `win+VS` job already did\n - in forever, build in Release mode (where that modal assertion dialog is\n - never shown).\n + ci(win+Meson): build in Release mode\n +\n + When the `win+Meson` job was added to Git's CI, modeled after the\n + `win+vs` job, it overlooked that the latter built the Git artifacts in\n + release mode.\n +\n + The reason for this is that there is code in `compat/mingw.c` that turns\n + on the modal assertion dialogs in debug mode, which are very useful when\n + debugging interactively (as they offer to attach Visual Studio's\n + debugger), but they are scarcely useful in CI builds (where that modal\n + dialog would sit around, waiting for a human being to see and deal with\n + it, which obviously won't ever happen).\n +\n + This problem was not realized immediately because of a separate bug: the\n + `win+Meson` job erroneously built using the `gcc` that is in the `PATH`\n + by default on hosted GitHub Actions runners. Since that bug was fixed by\n + switching to `--vsenv`, though, the t7001-mv test consistently timed out\n + after six hours in the CI builds on GitHub, quite often, and wasting\n + build minutes without any benefit in return.\n +\n + The reason for this timeout was a symptom of aforementioned debug mode\n + problem, where the test case 'nonsense mv triggers assertion failure and\n + partially updated index' in t7001-mv triggered an assertion.\n +\n + I originally proposed this here patch to address the timeouts in CI\n + builds. The Git project decided to address this timeout differently,\n + though: by fixing the bug that the t7001-mv test case demonstrated. This\n + does not address the debug mode problem, though, as an `assert()` call\n + could be triggered in other ways in CI, and it should still not cause\n + the CI build to hang but should cause Git to error out instead. To avoid\n + having to accept this here patch, it was then proposed to replace all\n + `assert()` calls in Git's code base by `BUG()` calls. This might be\n + reasonable for independent reasons, but it obviously still does not\n + address the debug mode problem, as `assert()` calls could be easily\n + re-introduced by mistake, and besides, Git has a couple of dependencies\n + that all may have their own `assert()` calls (which are then safely\n + outside the control of the Git project to remove), therefore this here\n + patch is still needed.\n \n Signed-off-by: Johannes Schindelin \n \n\n\n .github/workflows/main.yml | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\nindex 83ca8e4182b..275240be5dc 100644\n--- a/.github/workflows/main.yml\n+++ b/.github/workflows/main.yml\n@@ -265,7 +265,7 @@ jobs:\n run: pip install meson ninja\n - name: Setup\n shell: pwsh\n- run: meson setup build --vsenv -Dperl=disabled -Dcredential_helpers=wincred\n+ run: meson setup build --vsenv -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred\n - name: Compile\n shell: pwsh\n run: meson compile -C build\n\nbase-commit: f65182a99e545d2f2bc22e6c1c2da192133b16a3\n-- \ngitgitgadget\n"},{"id":"517197","messageId":"aBhVWMabOFYRUjvD@pks.im","threadId":"63350","inReplyTo":"pull.1908.v2.git.1746282346370.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] ci(win+Meson): build in Release mode","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-05T06:06:16Z","receivedAt":"2025-05-05T06:06:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, May 03, 2025 at 02:25:46PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> Changes since v1:\n> \n> * Rewrote the commit message to reflect that this patch is still\n> needed, even if the symptom that originally motivated the patch was\n> addressed in a different manner, because it was merely a symptom of\n> the underlying root cause that CI builds should not let Visual C\n> build Git in debug mode.\n\nOk, makes sense. I think we should ideally address this issue\nstrategically, e.g. by getting rid of asserts completely in our\ncodebase. But for now I agree that we should just build Git in release\nmode on Windows.\n\nThanks!\n\nPatrick\n"},{"id":"517212","messageId":"5e2fdccc-2c52-805f-ff9c-b9e0e72614ac@gmx.de","threadId":"63350","inReplyTo":"aBhVWMabOFYRUjvD@pks.im","subject":"Re: [PATCH v2] ci(win+Meson): build in Release mode","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-05-05T07:27:10Z","receivedAt":"2025-05-05T07:27:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Mon, 5 May 2025, Patrick Steinhardt wrote:\n\n> On Sat, May 03, 2025 at 02:25:46PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> > Changes since v1:\n> > \n> > * Rewrote the commit message to reflect that this patch is still\n> > needed, even if the symptom that originally motivated the patch was\n> > addressed in a different manner, because it was merely a symptom of\n> > the underlying root cause that CI builds should not let Visual C\n> > build Git in debug mode.\n> \n> Ok, makes sense. I think we should ideally address this issue\n> strategically, e.g. by getting rid of asserts completely in our\n> codebase. But for now I agree that we should just build Git in release\n> mode on Windows.\n\nI am afraid that getting rid of asserts in Git's codebase won't ever be\nable to address the challenge that Git -- despite much reluctance --\nrelies on a couple of external dependencies that might at any point in\ntime cause `assert()` to be called, e.g. due to unexpected changes in the\nCI runner images.\n\nIn essence, we will need both: CI builds in release mode _and_ converting\n`assert()`s to `BUG()` calls (the latter, however, for different reasons\nthan to proactively address CI hangs: Git can convey better information in\n`BUG()` calls than in `assert()` calls).\n\nThere are more reasons to use release mode builds in CI, too, but we\nalready have a compelling reason to do so, therefore there is no need for\nme to spend more time assembling an exhaustive list.\n\nCiao,\nJohannes\n"},{"id":"517236","messageId":"aBiIPe7EVTBxENUp@pks.im","threadId":"63350","inReplyTo":"5e2fdccc-2c52-805f-ff9c-b9e0e72614ac@gmx.de","subject":"Re: [PATCH v2] ci(win+Meson): build in Release mode","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-05T09:43:25Z","receivedAt":"2025-05-05T09:43:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, May 05, 2025 at 09:27:10AM +0200, Johannes Schindelin wrote:\n> Hi Patrick,\n> \n> On Mon, 5 May 2025, Patrick Steinhardt wrote:\n> \n> > On Sat, May 03, 2025 at 02:25:46PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > > From: Johannes Schindelin \n> > > Changes since v1:\n> > > \n> > > * Rewrote the commit message to reflect that this patch is still\n> > > needed, even if the symptom that originally motivated the patch was\n> > > addressed in a different manner, because it was merely a symptom of\n> > > the underlying root cause that CI builds should not let Visual C\n> > > build Git in debug mode.\n> > \n> > Ok, makes sense. I think we should ideally address this issue\n> > strategically, e.g. by getting rid of asserts completely in our\n> > codebase. But for now I agree that we should just build Git in release\n> > mode on Windows.\n> \n> I am afraid that getting rid of asserts in Git's codebase won't ever be\n> able to address the challenge that Git -- despite much reluctance --\n> relies on a couple of external dependencies that might at any point in\n> time cause `assert()` to be called, e.g. due to unexpected changes in the\n> CI runner images.\n\nGood point indeed, I haven't considered this.\n\n> In essence, we will need both: CI builds in release mode _and_ converting\n> `assert()`s to `BUG()` calls (the latter, however, for different reasons\n> than to proactively address CI hangs: Git can convey better information in\n> `BUG()` calls than in `assert()` calls).\n> \n> There are more reasons to use release mode builds in CI, too, but we\n> already have a compelling reason to do so, therefore there is no need for\n> me to spend more time assembling an exhaustive list.\n\nYup.\n\nPatrick\n"},{"id":"517265","messageId":"xmqqmsbroxxs.fsf@gitster.g","threadId":"63350","inReplyTo":"aBiIPe7EVTBxENUp@pks.im","subject":"Re: [PATCH v2] ci(win+Meson): build in Release mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-05T15:54:23Z","receivedAt":"2025-05-05T15:54:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> I am afraid that getting rid of asserts in Git's codebase won't ever be\n>> able to address the challenge that Git -- despite much reluctance --\n>> relies on a couple of external dependencies that might at any point in\n>> time cause `assert()` to be called, e.g. due to unexpected changes in the\n>> CI runner images.\n>\n> Good point indeed, I haven't considered this.\n\nThanks both for a discussion. Let's replace and queue this, and\nfast track it down to 'maint'.\n\nHere is a range-diff for my tentative rebasing the patch on 'maint';\nI'll make sure merging it up to 'master' would match exactly the\nresult of applying the original patch directly to 'master' before\nqueuing.\n\nThanks!\n\n\n1: f3ae94b175 ! 1: 184abdcf05 ci(win+Meson): build in Release mode\n @@ Commit message\n patch is still needed.\n \n Signed-off-by: Johannes Schindelin \n + Acked-by: Patrick Steinhardt \n + [jc: rebased on 'maint' to enable fast-tracking the change down]\n Signed-off-by: Junio C Hamano \n \n ## .github/workflows/main.yml ##\n @@ .github/workflows/main.yml: jobs:\n run: pip install meson ninja\n - name: Setup\n shell: pwsh\n -- run: meson setup build --vsenv -Dperl=disabled -Dcredential_helpers=wincred\n -+ run: meson setup build --vsenv -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred\n +- run: meson setup build -Dperl=disabled -Dcredential_helpers=wincred\n ++ run: meson setup build -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred\n - name: Compile\n shell: pwsh\n run: meson compile -C build\n\n\n"},{"id":"517297","messageId":"aBnA3KNOfAo9EbpS@pks.im","threadId":"63350","inReplyTo":"xmqqmsbroxxs.fsf@gitster.g","subject":"Re: [PATCH v2] ci(win+Meson): build in Release mode","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-06T07:57:16Z","receivedAt":"2025-05-06T07:57:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, May 05, 2025 at 08:54:23AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> >> I am afraid that getting rid of asserts in Git's codebase won't ever be\n> >> able to address the challenge that Git -- despite much reluctance --\n> >> relies on a couple of external dependencies that might at any point in\n> >> time cause `assert()` to be called, e.g. due to unexpected changes in the\n> >> CI runner images.\n> >\n> > Good point indeed, I haven't considered this.\n> \n> Thanks both for a discussion. Let's replace and queue this, and\n> fast track it down to 'maint'.\n> \n> Here is a range-diff for my tentative rebasing the patch on 'maint';\n> I'll make sure merging it up to 'master' would match exactly the\n> result of applying the original patch directly to 'master' before\n> queuing.\n> \n> Thanks!\n> \n> \n> 1: f3ae94b175 ! 1: 184abdcf05 ci(win+Meson): build in Release mode\n> @@ Commit message\n> patch is still needed.\n> \n> Signed-off-by: Johannes Schindelin \n> + Acked-by: Patrick Steinhardt \n> + [jc: rebased on 'maint' to enable fast-tracking the change down]\n> Signed-off-by: Junio C Hamano \n> \n> ## .github/workflows/main.yml ##\n> @@ .github/workflows/main.yml: jobs:\n> run: pip install meson ninja\n> - name: Setup\n> shell: pwsh\n> -- run: meson setup build --vsenv -Dperl=disabled -Dcredential_helpers=wincred\n> -+ run: meson setup build --vsenv -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred\n> +- run: meson setup build -Dperl=disabled -Dcredential_helpers=wincred\n> ++ run: meson setup build -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred\n> - name: Compile\n> shell: pwsh\n> run: meson compile -C build\n\nAm I reading this diff correctly that we drop the `--vsenv` flag? I\nthink we should keep it around as it was a bug in the first place that\nwe didn't have it.\n\nI guess this is done because we didn't have the flag in \"maint\" yet. But\nI'm not even sure whether the fix would be needed in case we don't build\nwith Visual Studio, as the MinGW toolchain probably doesn't have the\nsame behaviour with asserts (but please correct me if I'm wrong, Dscho).\nSo maybe we should also cherry-pick 85e1d6819fb (ci: use Visual Studio\nfor win+meson job on GitHub Workflows, 2025-03-31) at the same time.\n\nPatrick\n"},{"id":"517300","messageId":"3952ab1e-dc67-c47b-a86b-88b0e8886755@gmx.de","threadId":"63350","inReplyTo":"aBnA3KNOfAo9EbpS@pks.im","subject":"Re: [PATCH v2] ci(win+Meson): build in Release mode","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-05-06T08:32:36Z","receivedAt":"2025-05-06T08:32:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Tue, 6 May 2025, Patrick Steinhardt wrote:\n\n> On Mon, May 05, 2025 at 08:54:23AM -0700, Junio C Hamano wrote:\n> > Patrick Steinhardt writes:\n> > \n> > >> I am afraid that getting rid of asserts in Git's codebase won't ever be\n> > >> able to address the challenge that Git -- despite much reluctance --\n> > >> relies on a couple of external dependencies that might at any point in\n> > >> time cause `assert()` to be called, e.g. due to unexpected changes in the\n> > >> CI runner images.\n> > >\n> > > Good point indeed, I haven't considered this.\n> > \n> > Thanks both for a discussion. Let's replace and queue this, and\n> > fast track it down to 'maint'.\n> > \n> > Here is a range-diff for my tentative rebasing the patch on 'maint';\n> > I'll make sure merging it up to 'master' would match exactly the\n> > result of applying the original patch directly to 'master' before\n> > queuing.\n> > \n> > Thanks!\n> > \n> > \n> > 1: f3ae94b175 ! 1: 184abdcf05 ci(win+Meson): build in Release mode\n> > @@ Commit message\n> > patch is still needed.\n> > \n> > Signed-off-by: Johannes Schindelin \n> > + Acked-by: Patrick Steinhardt \n> > + [jc: rebased on 'maint' to enable fast-tracking the change down]\n> > Signed-off-by: Junio C Hamano \n> > \n> > ## .github/workflows/main.yml ##\n> > @@ .github/workflows/main.yml: jobs:\n> > run: pip install meson ninja\n> > - name: Setup\n> > shell: pwsh\n> > -- run: meson setup build --vsenv -Dperl=disabled -Dcredential_helpers=wincred\n> > -+ run: meson setup build --vsenv -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred\n> > +- run: meson setup build -Dperl=disabled -Dcredential_helpers=wincred\n> > ++ run: meson setup build -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred\n> > - name: Compile\n> > shell: pwsh\n> > run: meson compile -C build\n> \n> Am I reading this diff correctly that we drop the `--vsenv` flag?\n\nThat's similar to my reading as well, but...\n\n> I think we should keep it around as it was a bug in the first place that\n> we didn't have it.\n\nCorrect.\n\n> I guess this is done because we didn't have the flag in \"maint\" yet.\n\n... I think this is the reason. It would probably make more sense to\nbackport the `--vsenv` patch first, seeing as having `win+Meson` without\nthat flag defeats the purpose of `win+Meson`.\n\n> But I'm not even sure whether the fix would be needed in case we don't\n> build with Visual Studio, as the MinGW toolchain probably doesn't have\n> the same behaviour with asserts (but please correct me if I'm wrong,\n> Dscho). So maybe we should also cherry-pick 85e1d6819fb (ci: use Visual\n> Studio for win+meson job on GitHub Workflows, 2025-03-31) at the same\n> time.\n\nIndeed. If we keep both bugs unfixed in `maint` (the bug that `win+Meson`\ndoes not build with Visual Studio, plus the bug that `win+Meson` builds in\nDebug mode), the CI will continue to pass... just not for the intended\nreasons.\n\nCiao,\nJohannes\n"},{"id":"517586","messageId":"xmqqmsbn55iy.fsf@gitster.g","threadId":"63350","inReplyTo":"3952ab1e-dc67-c47b-a86b-88b0e8886755@gmx.de","subject":"Re: [PATCH v2] ci(win+Meson): build in Release mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-08T18:19:49Z","receivedAt":"2025-05-08T18:19:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Indeed. If we keep both bugs unfixed in `maint` (the bug that `win+Meson`\n> does not build with Visual Studio, plus the bug that `win+Meson` builds in\n> Debug mode), the CI will continue to pass... just not for the intended\n> reasons.\n\nThat's fun ;-)\n"}]}