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

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
    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(-)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 83ca8e4182b..275240be5dc 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -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
-- 
gitgitgadget

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

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.

> 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?

> 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).

OK.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>     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(-)
>
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index 83ca8e4182b..275240be5dc 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -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

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

[Patrick Steinhardt]

On Fri, Apr 25, 2025 at 08:18:02AM -0700, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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.

Indeed, thanks for fixing the issue! I haven't noticed these failing
jobs yet on my end, probably because on GitLab we only execute the
Win+Meson changes manually. Which is not great given that it makes me
miss issues with Meson like this.

I'll send a patch to run these tests by default so that I can see the
pain myself and address any issues that come up before others are forced
to.

> > 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.

> > diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> > index 83ca8e4182b..275240be5dc 100644
> > --- a/.github/workflows/main.yml
> > +++ b/.github/workflows/main.yml
> > @@ -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

So I'm fine with this trivial change as a band aid for now. The diff
looks obviously good to me.

Patrick

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

[Junio C Hamano]

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/

---- snip snap ----
Thanks for reporting, Kristoffer.

Any takers?

$ git shortlog --since=3.years -s -n -e --no-merges v2.43.0 builtin/mv.c
    15	Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
    10	Elijah Newren <newren@gmail.com>
     5	Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     2	Junio C Hamano <gitster@pobox.com>
     1	Andrzej Hunt <ajrhunt@google.com>
     1	Calvin Wan <calvinwan@google.com>
     1	Derrick Stolee <stolee@gmail.com>
     1	Sebastian Thiel <sebastian.thiel@icloud.com>
     1	Torsten Bögershausen <tboegi@web.de>

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

[Patrick Steinhardt]

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;

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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

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

> +	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?

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

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

> +		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.

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

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

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

Otherwise we are in trouble.

> +			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.

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

[Patrick Steinhardt]

On Tue, Apr 29, 2025 at 01:48:18PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > @@ -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);
> > +
> 
> OK, this collects in moved_dirs the directories that will get moved.
> And then a separate loop, ...
> 
> > +	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.

> > +		slash_pos = strrchr(pathbuf.buf, '/');
> 
> And start from the deepest directory, going one level up per
> iteration, ...
> 
> > +		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.

> > +			if (!hashmap_get_entry(&moved_dirs, &needle, ent, NULL))
> > +				continue;
> 
> If there is no overlap, we need to do anything special.
> 
> > +			if (!ignore_errors)
> > +				die(_("cannot move both parent directory '%s' and its child '%s'"),
> > +				    pathbuf.buf, sources.v[i]);
> 
> Otherwise we are in trouble.
> 
> > +			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.

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.

> 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.

> 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

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

[Patrick Steinhardt]

On Wed, Apr 30, 2025 at 10:58:24AM +0200, Patrick Steinhardt wrote:
> On Tue, Apr 29, 2025 at 01:48:18PM -0700, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> > 
> > > @@ -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);
> > > +
> > 
> > OK, this collects in moved_dirs the directories that will get moved.
> > And then a separate loop, ...
> > 
> > > +	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.
> 
> > > +		slash_pos = strrchr(pathbuf.buf, '/');
> > 
> > And start from the deepest directory, going one level up per
> > iteration, ...
> > 
> > > +		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.
> 
> > > +			if (!hashmap_get_entry(&moved_dirs, &needle, ent, NULL))
> > > +				continue;
> > 
> > If there is no overlap, we need to do anything special.
> > 
> > > +			if (!ignore_errors)
> > > +				die(_("cannot move both parent directory '%s' and its child '%s'"),
> > > +				    pathbuf.buf, sources.v[i]);
> > 
> > Otherwise we are in trouble.
> > 
> > > +			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.
> 
> 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.
> 
> > 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.
> 
> > 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
> 

I have polished this patch a bit now and sent it via [1]. Thanks!

Patrick

[1]: <20250430-pks-mv-parent-child-conflict-v1-0-11a87c55ffb9@pks.im>

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

[Kristoffer Haugsbakk]

On Fri, Apr 25, 2025, at 17:05, Johannes Schindelin via GitGitGadget wrote:
> 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.

Sorry for the trouble.

[PATCH v2] ci(win+Meson): build in Release mode

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When the `win+Meson` job was added to Git's CI, modeled after the
`win+vs` job, it overlooked that the latter built the Git artifacts in
release mode.

The reason for this is that there is code in `compat/mingw.c` that turns
on the modal assertion dialogs in debug mode, which are very useful when
debugging interactively (as they offer to attach Visual Studio's
debugger), but they are scarcely useful in CI builds (where that modal
dialog would sit around, waiting for a human being to see and deal with
it, which obviously won't ever happen).

This problem was not realized immediately because of a separate bug: the
`win+Meson` job erroneously built using the `gcc` that is in the `PATH`
by default on hosted GitHub Actions runners. Since that bug was fixed by
switching to `--vsenv`, though, the t7001-mv test consistently timed out
after six hours in the CI builds on GitHub, quite often, and wasting
build minutes without any benefit in return.

The reason for this timeout was a symptom of aforementioned debug mode
problem, where the test case 'nonsense mv triggers assertion failure and
partially updated index' in t7001-mv triggered an assertion.

I originally proposed this here patch to address the timeouts in CI
builds. The Git project decided to address this timeout differently,
though: by fixing the bug that the t7001-mv test case demonstrated. This
does not address the debug mode problem, though, as an `assert()` call
could be triggered in other ways in CI, and it should still not cause
the CI build to hang but should cause Git to error out instead. To avoid
having to accept this here patch, it was then proposed to replace all
`assert()` calls in Git's code base by `BUG()` calls. This might be
reasonable for independent reasons, but it obviously still does not
address the debug mode problem, as `assert()` calls could be easily
re-introduced by mistake, and besides, Git has a couple of dependencies
that all may have their own `assert()` calls (which are then safely
outside the control of the Git project to remove), therefore this here
patch is still needed.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    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.
    
    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.

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

Range-diff vs v1:

 1:  18d1d18c48a ! 1:  c04b6c97b25 ci(win+Meson): build in Release mode, avoiding t7001-mv hangs
     @@ Metadata
      Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
      
       ## Commit message ##
     -    ci(win+Meson): build in Release mode, avoiding t7001-mv hangs
     -
     -    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).
     +    ci(win+Meson): build in Release mode
     +
     +    When the `win+Meson` job was added to Git's CI, modeled after the
     +    `win+vs` job, it overlooked that the latter built the Git artifacts in
     +    release mode.
     +
     +    The reason for this is that there is code in `compat/mingw.c` that turns
     +    on the modal assertion dialogs in debug mode, which are very useful when
     +    debugging interactively (as they offer to attach Visual Studio's
     +    debugger), but they are scarcely useful in CI builds (where that modal
     +    dialog would sit around, waiting for a human being to see and deal with
     +    it, which obviously won't ever happen).
     +
     +    This problem was not realized immediately because of a separate bug: the
     +    `win+Meson` job erroneously built using the `gcc` that is in the `PATH`
     +    by default on hosted GitHub Actions runners. Since that bug was fixed by
     +    switching to `--vsenv`, though, the t7001-mv test consistently timed out
     +    after six hours in the CI builds on GitHub, quite often, and wasting
     +    build minutes without any benefit in return.
     +
     +    The reason for this timeout was a symptom of aforementioned debug mode
     +    problem, where the test case 'nonsense mv triggers assertion failure and
     +    partially updated index' in t7001-mv triggered an assertion.
     +
     +    I originally proposed this here patch to address the timeouts in CI
     +    builds. The Git project decided to address this timeout differently,
     +    though: by fixing the bug that the t7001-mv test case demonstrated. This
     +    does not address the debug mode problem, though, as an `assert()` call
     +    could be triggered in other ways in CI, and it should still not cause
     +    the CI build to hang but should cause Git to error out instead. To avoid
     +    having to accept this here patch, it was then proposed to replace all
     +    `assert()` calls in Git's code base by `BUG()` calls. This might be
     +    reasonable for independent reasons, but it obviously still does not
     +    address the debug mode problem, as `assert()` calls could be easily
     +    re-introduced by mistake, and besides, Git has a couple of dependencies
     +    that all may have their own `assert()` calls (which are then safely
     +    outside the control of the Git project to remove), therefore this here
     +    patch is still needed.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      


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

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 83ca8e4182b..275240be5dc 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -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
-- 
gitgitgadget

Re: [PATCH v2] ci(win+Meson): build in Release mode

[Patrick Steinhardt]

On Sat, May 03, 2025 at 02:25:46PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>     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.

Thanks!

Patrick

Re: [PATCH v2] ci(win+Meson): build in Release mode

[Johannes Schindelin]

Hi Patrick,

On Mon, 5 May 2025, Patrick Steinhardt wrote:

> On Sat, May 03, 2025 at 02:25:46PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >     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.

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.

Ciao,
Johannes

Re: [PATCH v2] ci(win+Meson): build in Release mode

[Patrick Steinhardt]

On Mon, May 05, 2025 at 09:27:10AM +0200, Johannes Schindelin wrote:
> Hi Patrick,
> 
> On Mon, 5 May 2025, Patrick Steinhardt wrote:
> 
> > On Sat, May 03, 2025 at 02:25:46PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > >     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.
> 
> 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.

> 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.

Yup.

Patrick

Re: [PATCH v2] ci(win+Meson): build in Release mode

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> 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 <johannes.schindelin@gmx.de>
    +    Acked-by: Patrick Steinhardt <ps@pks.im>
    +    [jc: rebased on 'maint' to enable fast-tracking the change down]
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## .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

Re: [PATCH v2] ci(win+Meson): build in Release mode

[Patrick Steinhardt]

On Mon, May 05, 2025 at 08:54:23AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> 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 <johannes.schindelin@gmx.de>
>     +    Acked-by: Patrick Steinhardt <ps@pks.im>
>     +    [jc: rebased on 'maint' to enable fast-tracking the change down]
>          Signed-off-by: Junio C Hamano <gitster@pobox.com>
>      
>       ## .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.

Patrick

Re: [PATCH v2] ci(win+Meson): build in Release mode

[Johannes Schindelin]

Hi Patrick,

On Tue, 6 May 2025, Patrick Steinhardt wrote:

> On Mon, May 05, 2025 at 08:54:23AM -0700, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> > 
> > >> 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 <johannes.schindelin@gmx.de>
> >     +    Acked-by: Patrick Steinhardt <ps@pks.im>
> >     +    [jc: rebased on 'maint' to enable fast-tracking the change down]
> >          Signed-off-by: Junio C Hamano <gitster@pobox.com>
> >      
> >       ## .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?

That's similar to my reading as well, but...

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

Correct.

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

... I think this is the reason. It would probably make more sense to
backport the `--vsenv` patch first, seeing as having `win+Meson` without
that flag defeats the purpose of `win+Meson`.

> 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.

Ciao,
Johannes

Re: [PATCH v2] ci(win+Meson): build in Release mode

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> 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.

That's fun ;-)