[PATCH 0/2] builtin/mv: bail out when trying to move child and its parent

[PATCH 0/2] builtin/mv: bail out when trying to move child and its parent

[Patrick Steinhardt]

Hi,

this series addresses a bug in git-mv(1) where moving both a child and
its parent directory may cause an assert to trigger. This discussion
came up in the context of [1] as the assert causes problems on our CI
systems on Windows.

Thanks!

Patrick

[1]: <pull.1908.git.1745593515875.gitgitgadget@gmail.com>

---
Patrick Steinhardt (2):
      builtin/mv: bail out when trying to move child and its parent
      builtin/mv: convert assert(3p) into `BUG()`

 builtin/mv.c  | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 t/t7001-mv.sh | 24 ++++++++++++++++++----
 2 files changed, 81 insertions(+), 7 deletions(-)


---
base-commit: 6c0bd1fc70efaf053abe4e57c976afdc72d15377
change-id: 20250430-pks-mv-parent-child-conflict-1c15f775e638

[PATCH 1/2] builtin/mv: bail out when trying to move child and its parent

[Patrick Steinhardt]

We have a known issue in git-mv(1) where moving both a child and any of
its parents causes an assert to trigger because the child cannot be
found anymore in the index. We have added a test for this in commit
0fcd473fdd3 (t7001: add failure test which triggers assertion,
2024-10-22) without addressing the issue, which is why the test itself
is marked as `test_expect_failure`.

The behaviour of that test relies on a call to assert(3p) though, which
may or may not be compiled into the resulting binary depending on
whether or not we pass `-DNDEBUG`. When these asserts are compiled into
Git this may cause our CI to hang on Windows though, because asserts may
cause a modal window to be shown.

While we could work around the issue by converting this into a call to
`BUG()`, let's rather address the root cause of the issue by bailing out
in case we see that both a child and any of its parents are being moved
in the same command.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/mv.c  | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 t/t7001-mv.sh | 24 +++++++++++++++++++----
 2 files changed, 79 insertions(+), 6 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 54b323fff72..edb854677d9 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -39,6 +39,13 @@ enum update_mode {
 	INDEX = (1 << 2),
 	SPARSE = (1 << 3),
 	SKIP_WORKTREE_DIR = (1 << 4),
+	/*
+	 * A file gets moved implicitly via a move of one of its parent
+	 * directories. This flag causes us to skip the check that we don't try
+	 * to move a file and any of its parent directories at the same point
+	 * in time.
+	 */
+	MOVE_VIA_PARENT_DIR = (1 << 5),
 };
 
 #define DUP_BASENAME 1
@@ -183,6 +190,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 +235,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,6 +355,7 @@ 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;
@@ -348,6 +373,11 @@ int cmd_mv(int argc,
 				goto act_on_entry;
 			}
 
+			entry = xmalloc(sizeof(*entry));
+			entry->path = src;
+			hashmap_entry_init(&entry->ent, fspathhash(src));
+			hashmap_add(&moved_dirs, &entry->ent);
+
 			/* last - first >= 1 */
 			modes[i] |= WORKING_DIRECTORY;
 
@@ -368,8 +398,7 @@ int cmd_mv(int argc,
 				strvec_push(&sources, path);
 				strvec_push(&destinations, prefixed_path);
 
-				memset(modes + argc + j, 0, sizeof(enum update_mode));
-				modes[argc + j] |= ce_skip_worktree(ce) ? SPARSE : INDEX;
+				modes[argc + j] = MOVE_VIA_PARENT_DIR | (ce_skip_worktree(ce) ? SPARSE : INDEX);
 				submodule_gitfiles[argc + j] = NULL;
 
 				free(prefixed_path);
@@ -465,6 +494,32 @@ int cmd_mv(int argc,
 		}
 	}
 
+	for (i = 0; i < argc; i++) {
+		const char *slash_pos;
+
+		if (modes[i] & MOVE_VIA_PARENT_DIR)
+			continue;
+
+		strbuf_reset(&pathbuf);
+		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))
+				die(_("cannot move both '%s' and its parent directory '%s'"),
+				    sources.v[i], pathbuf.buf);
+
+			slash_pos = strrchr(pathbuf.buf, '/');
+		}
+	}
+
 	if (only_match_skip_worktree.nr) {
 		advise_on_updating_sparse_paths(&only_match_skip_worktree);
 		if (!ignore_errors) {
@@ -589,6 +644,8 @@ int cmd_mv(int argc,
 	strvec_clear(&dest_paths);
 	strvec_clear(&destinations);
 	strvec_clear(&submodule_gitfiles_to_free);
+	hashmap_clear_and_free(&moved_dirs, struct pathmap_entry, ent);
+	strbuf_release(&pathbuf);
 	free(submodule_gitfiles);
 	free(modes);
 	return ret;
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 25334b50622..920479e9256 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -550,16 +550,32 @@ test_expect_success 'moving nested submodules' '
 	git status
 '
 
-test_expect_failure 'nonsense mv triggers assertion failure and partially updated index' '
+test_expect_success 'moving file and its parent directory at the same time fails' '
 	test_when_finished git reset --hard HEAD &&
 	git reset --hard HEAD &&
 	mkdir -p a &&
 	mkdir -p b &&
 	>a/a.txt &&
 	git add a/a.txt &&
-	test_must_fail git mv a/a.txt a b &&
-	git status --porcelain >actual &&
-	grep "^A[ ]*a/a.txt$" actual
+	cat >expect <<-EOF &&
+	fatal: cannot move both ${SQ}a/a.txt${SQ} and its parent directory ${SQ}a${SQ}
+	EOF
+	test_must_fail git mv a/a.txt a b 2>err &&
+	test_cmp expect err
+'
+
+test_expect_success 'moving nested directory and its parent directory at the same time fails' '
+	test_when_finished git reset --hard HEAD &&
+	git reset --hard HEAD &&
+	mkdir -p a/b/c &&
+	>a/b/c/file.txt &&
+	git add a &&
+	mkdir target &&
+	cat >expect <<-EOF &&
+	fatal: cannot move both ${SQ}a/b/c${SQ} and its parent directory ${SQ}a${SQ}
+	EOF
+	test_must_fail git mv a/b/c a target 2>err &&
+	test_cmp expect err
 '
 
 test_done

-- 
2.49.0.987.g0cc8ee98dc.dirty

Re: [PATCH 1/2] builtin/mv: bail out when trying to move child and its parent

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> @@ -368,8 +398,7 @@ int cmd_mv(int argc,
>  				strvec_push(&sources, path);
>  				strvec_push(&destinations, prefixed_path);
>  
> -				memset(modes + argc + j, 0, sizeof(enum update_mode));
> -				modes[argc + j] |= ce_skip_worktree(ce) ? SPARSE : INDEX;
> +				modes[argc + j] = MOVE_VIA_PARENT_DIR | (ce_skip_worktree(ce) ? SPARSE : INDEX);
>  				submodule_gitfiles[argc + j] = NULL;

OK, this is the part we both missed during the earlier round.

> +			if (hashmap_get_entry(&moved_dirs, &needle, ent, NULL))
> +				die(_("cannot move both '%s' and its parent directory '%s'"),
> +				    sources.v[i], pathbuf.buf);

OK.

> -test_expect_failure 'nonsense mv triggers assertion failure and partially updated index' '
> +test_expect_success 'moving file and its parent directory at the same time fails' '

The new title is much more descriptive.

> -	test_must_fail git mv a/a.txt a b &&
> -	git status --porcelain >actual &&
> -	grep "^A[ ]*a/a.txt$" actual
> +	cat >expect <<-EOF &&
> +	fatal: cannot move both ${SQ}a/a.txt${SQ} and its parent directory ${SQ}a${SQ}
> +	EOF
> +	test_must_fail git mv a/a.txt a b 2>err &&
> +	test_cmp expect err
> +'

Shouldn't we make sure that after failing "git mv" the paths and the
index entries stay as expected?

> +test_expect_success 'moving nested directory and its parent directory at the same time fails' '
> +	test_when_finished git reset --hard HEAD &&
> +	git reset --hard HEAD &&
> +	mkdir -p a/b/c &&
> +	>a/b/c/file.txt &&
> +	git add a &&
> +	mkdir target &&
> +	cat >expect <<-EOF &&
> +	fatal: cannot move both ${SQ}a/b/c${SQ} and its parent directory ${SQ}a${SQ}
> +	EOF
> +	test_must_fail git mv a/b/c a target 2>err &&
> +	test_cmp expect err
>  '

Ditto.

By the way I think "git mv a a/b" in the same scenario already
notices a problematic request, so it probably won't hit this
codepath but we shoudl already be covered.

Thanks.

Re: [PATCH 1/2] builtin/mv: bail out when trying to move child and its parent

[Patrick Steinhardt]

On Wed, Apr 30, 2025 at 03:21:42PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > -	test_must_fail git mv a/a.txt a b &&
> > -	git status --porcelain >actual &&
> > -	grep "^A[ ]*a/a.txt$" actual
> > +	cat >expect <<-EOF &&
> > +	fatal: cannot move both ${SQ}a/a.txt${SQ} and its parent directory ${SQ}a${SQ}
> > +	EOF
> > +	test_must_fail git mv a/a.txt a b 2>err &&
> > +	test_cmp expect err
> > +'
> 
> Shouldn't we make sure that after failing "git mv" the paths and the
> index entries stay as expected?
> 
> > +test_expect_success 'moving nested directory and its parent directory at the same time fails' '
> > +	test_when_finished git reset --hard HEAD &&
> > +	git reset --hard HEAD &&
> > +	mkdir -p a/b/c &&
> > +	>a/b/c/file.txt &&
> > +	git add a &&
> > +	mkdir target &&
> > +	cat >expect <<-EOF &&
> > +	fatal: cannot move both ${SQ}a/b/c${SQ} and its parent directory ${SQ}a${SQ}
> > +	EOF
> > +	test_must_fail git mv a/b/c a target 2>err &&
> > +	test_cmp expect err
> >  '
> 
> Ditto.

This might've been a good idea, but I see that the series has already
been merged to `next`. So I'll refrain from improving the tests.

Thanks!

Patrick

[PATCH 2/2] builtin/mv: convert assert(3p) into `BUG()`

[Patrick Steinhardt]

The use of asserts is discouraged in our codebase because they lead to
different behaviour depending on how Git is built. When being unsure
enough whether a condition always holds so that one adds the assert,
then the assert should probably trigger regardless of how Git is being
built.

Drop the call to assert(3p) in git-mv(1) and instead use `BUG()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/mv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index edb854677d9..07548fe96ae 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -562,7 +562,8 @@ int cmd_mv(int argc,
 			continue;
 
 		pos = index_name_pos(the_repository->index, src, strlen(src));
-		assert(pos >= 0);
+		if (pos < 0)
+			BUG("could not find source in index: '%s'", src);
 		if (!(mode & SPARSE) && !lstat(src, &st))
 			sparse_and_dirty = ie_modified(the_repository->index,
 						       the_repository->index->cache[pos],

-- 
2.49.0.987.g0cc8ee98dc.dirty

Re: [PATCH 2/2] builtin/mv: convert assert(3p) into `BUG()`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> The use of asserts is discouraged in our codebase because they lead to
> different behaviour depending on how Git is built. When being unsure
> enough whether a condition always holds so that one adds the assert,
> then the assert should probably trigger regardless of how Git is being
> built.

Nicely put.  Yes, this is another reason why we frown on the use of
assert(), in addition to the reason why why Elijah's series that
ends with 5633aa3a (treewide: replace assert() with ASSERT() in
special cases, 2025-03-19) was written.

> Drop the call to assert(3p) in git-mv(1) and instead use `BUG()`.

Being explicit about what we are unsure about is always good.  It
would hopefully entice those who want to get their hands dirty to
see if they can "prove" that BUG() would never happen, which would
be a great outcome ;-).

Thanks.

>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/mv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index edb854677d9..07548fe96ae 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -562,7 +562,8 @@ int cmd_mv(int argc,
>  			continue;
>  
>  		pos = index_name_pos(the_repository->index, src, strlen(src));
> -		assert(pos >= 0);
> +		if (pos < 0)
> +			BUG("could not find source in index: '%s'", src);
>  		if (!(mode & SPARSE) && !lstat(src, &st))
>  			sparse_and_dirty = ie_modified(the_repository->index,
>  						       the_repository->index->cache[pos],

Re: [PATCH 2/2] builtin/mv: convert assert(3p) into `BUG()`

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Patrick Steinhardt <ps@pks.im> writes:
>
>> The use of asserts is discouraged in our codebase because they lead to
>> different behaviour depending on how Git is built. When being unsure
>> enough whether a condition always holds so that one adds the assert,
>> then the assert should probably trigger regardless of how Git is being
>> built.
>
> Nicely put.  Yes, this is another reason why we frown on the use of
> assert(), in addition to the reason why why Elijah's series that
> ends with 5633aa3a (treewide: replace assert() with ASSERT() in
> special cases, 2025-03-19) was written.
>
>> Drop the call to assert(3p) in git-mv(1) and instead use `BUG()`.
>
> Being explicit about what we are unsure about is always good.  It
> would hopefully entice those who want to get their hands dirty to
> see if they can "prove" that BUG() would never happen, which would
> be a great outcome ;-).

By the way, with this in place, and without Dscho's "assert() makes
Win+Meson test job get stuck, so let's make assert() a no-op" patch,
the CI seems to be fine.

    https://github.com/git/git/actions/runs/14765572702

Triggering assert() and BUG() are something we would always want to
notice.  They should never trigger in production and it is an event
to call for fixing the underlying cause that made the condition
trigger if it is shown to end-users.  Dscho's patch protects us from
addition of a new test that triggers an assert().  We won't see such
a test get stuck forever on Windows, but by turning such an assert()
into a no-op, we would waste electricity for running CI only to miss
the triggering assert(), which does not sound like a good use of our
resources.

So I am inclined to drop Dscho's "build in release mode" patch when
we merge this series down to 'next'.  Being able to notice a
breakage (which triggers a real assert(), whether it is due to
broken code, or due to a broken test that documents a broken code
path---which should be rewritten to use "if (condition) BUG()"),
even if it needs to be done by noticing a test that gets stuck,
would be much better than missing such a breakage at all, and that
is the primary reasoning behind my suggesting to do so.  I would not
be surprised if I am missing a good reason or two to make build
tested in CI ignore asserts, so let's hear from others.

Opinions?

Thanks.

Re: [PATCH 2/2] builtin/mv: convert assert(3p) into `BUG()`

[Patrick Steinhardt]

On Wed, Apr 30, 2025 at 04:10:37PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> >> The use of asserts is discouraged in our codebase because they lead to
> >> different behaviour depending on how Git is built. When being unsure
> >> enough whether a condition always holds so that one adds the assert,
> >> then the assert should probably trigger regardless of how Git is being
> >> built.
> >
> > Nicely put.  Yes, this is another reason why we frown on the use of
> > assert(), in addition to the reason why why Elijah's series that
> > ends with 5633aa3a (treewide: replace assert() with ASSERT() in
> > special cases, 2025-03-19) was written.
> >
> >> Drop the call to assert(3p) in git-mv(1) and instead use `BUG()`.
> >
> > Being explicit about what we are unsure about is always good.  It
> > would hopefully entice those who want to get their hands dirty to
> > see if they can "prove" that BUG() would never happen, which would
> > be a great outcome ;-).
> 
> By the way, with this in place, and without Dscho's "assert() makes
> Win+Meson test job get stuck, so let's make assert() a no-op" patch,
> the CI seems to be fine.
> 
>     https://github.com/git/git/actions/runs/14765572702
> 
> Triggering assert() and BUG() are something we would always want to
> notice.  They should never trigger in production and it is an event
> to call for fixing the underlying cause that made the condition
> trigger if it is shown to end-users.  Dscho's patch protects us from
> addition of a new test that triggers an assert().  We won't see such
> a test get stuck forever on Windows, but by turning such an assert()
> into a no-op, we would waste electricity for running CI only to miss
> the triggering assert(), which does not sound like a good use of our
> resources.

It makes me wonder whether we should forbid `assert()` altogether and
use `BUG()` everywhere, similar to the recent discussion with Elijah. We
do have >600 callsites of `assert()` though, so we would have to
introduce a macro that doesn't require us to provide a reasoning for
now. E.g.

    #define BUG_UNLESS(condition) if (!(condition)) BUG(##condition)

or something like this.

And once we've done such a conversion we could add `assert()` to our
deny list of functions (wherever it was, I forgot).

> So I am inclined to drop Dscho's "build in release mode" patch when
> we merge this series down to 'next'.  Being able to notice a
> breakage (which triggers a real assert(), whether it is due to
> broken code, or due to a broken test that documents a broken code
> path---which should be rewritten to use "if (condition) BUG()"),
> even if it needs to be done by noticing a test that gets stuck,
> would be much better than missing such a breakage at all, and that
> is the primary reasoning behind my suggesting to do so.  I would not
> be surprised if I am missing a good reason or two to make build
> tested in CI ignore asserts, so let's hear from others.
> 
> Opinions?

As far as I understand there is no need for this patch anymore.

Patrick

Re: [PATCH 2/2] builtin/mv: convert assert(3p) into `BUG()`

[Johannes Schindelin]

Hi Patrick & Junio,

On Fri, 2 May 2025, Patrick Steinhardt wrote:

> On Wed, Apr 30, 2025 at 04:10:37PM -0700, Junio C Hamano wrote:
> > Junio C Hamano <gitster@pobox.com> writes:
> > 
> > > Patrick Steinhardt <ps@pks.im> writes:
> > >
> > >> The use of asserts is discouraged in our codebase because they lead to
> > >> different behaviour depending on how Git is built. When being unsure
> > >> enough whether a condition always holds so that one adds the assert,
> > >> then the assert should probably trigger regardless of how Git is being
> > >> built.
> > >
> > > Nicely put.  Yes, this is another reason why we frown on the use of
> > > assert(), in addition to the reason why why Elijah's series that
> > > ends with 5633aa3a (treewide: replace assert() with ASSERT() in
> > > special cases, 2025-03-19) was written.
> > >
> > >> Drop the call to assert(3p) in git-mv(1) and instead use `BUG()`.
> > >
> > > Being explicit about what we are unsure about is always good.  It
> > > would hopefully entice those who want to get their hands dirty to
> > > see if they can "prove" that BUG() would never happen, which would
> > > be a great outcome ;-).
> > 
> > By the way, with this in place, and without Dscho's "assert() makes
> > Win+Meson test job get stuck, so let's make assert() a no-op" patch,
> > the CI seems to be fine.
> > 
> >     https://github.com/git/git/actions/runs/14765572702
> > 
> > Triggering assert() and BUG() are something we would always want to
> > notice.  They should never trigger in production and it is an event
> > to call for fixing the underlying cause that made the condition
> > trigger if it is shown to end-users.  Dscho's patch protects us from
> > addition of a new test that triggers an assert().  We won't see such
> > a test get stuck forever on Windows, but by turning such an assert()
> > into a no-op, we would waste electricity for running CI only to miss
> > the triggering assert(), which does not sound like a good use of our
> > resources.
> 
> It makes me wonder whether we should forbid `assert()` altogether and
> use `BUG()` everywhere, similar to the recent discussion with Elijah. We
> do have >600 callsites of `assert()` though, so we would have to
> introduce a macro that doesn't require us to provide a reasoning for
> now. E.g.
> 
>     #define BUG_UNLESS(condition) if (!(condition)) BUG(##condition)
> 
> or something like this.
> 
> And once we've done such a conversion we could add `assert()` to our
> deny list of functions (wherever it was, I forgot).
> 
> > So I am inclined to drop Dscho's "build in release mode" patch when
> > we merge this series down to 'next'.  Being able to notice a
> > breakage (which triggers a real assert(), whether it is due to
> > broken code, or due to a broken test that documents a broken code
> > path---which should be rewritten to use "if (condition) BUG()"),
> > even if it needs to be done by noticing a test that gets stuck,
> > would be much better than missing such a breakage at all, and that
> > is the primary reasoning behind my suggesting to do so.  I would not
> > be surprised if I am missing a good reason or two to make build
> > tested in CI ignore asserts, so let's hear from others.
> > 
> > Opinions?
> 
> As far as I understand there is no need for this patch anymore.

I see a need for this patch, still, and it is not a mere "opinion".

The fact of the matter is that the Visual C-built CI (first the Azure
Pipeline, then GitHub Actions) have built the artifacts in release mode
since forever. And the Meson addition simply made a mistake by _not
specifying_ release mode (and hence defaulting to debug mode).

This makes a difference because in `compat/mingw.c`, there is this
(https://github.com/git/git/blob/v2.49.0/compat/mingw.c#L3247-L3255):

	#ifdef _MSC_VER
	#ifdef _DEBUG
		_CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_DEBUG);
	#endif
	[...]
	#endif

This means that each and every `assert()`, whether that be in Git's source
code, or in any dependency of Git (which the Git project does not
control!), is handled with this modal dialog.

It was an oversight in the win+Meson patches not to use release mode, and
the patch I proposed fixes this bug.

In the alternative, you could also just drop the entire win+Meson stuff,
of which I would be actually quite in favor: No Visual Studio user will be
happy with Meson, therefore it would be the kind thing to drop all
pretense, officially, that Git cares about Visual Studio users.

The easier and quicker solution, though, (which would maintain a modicum
of Visual Studio support) would be to un-drop the fix proposed in
https://lore.kernel.org/git/pull.1908.git.1745593515875.gitgitgadget@gmail.com/

Ciao,
Johannes

Re: [PATCH 2/2] builtin/mv: convert assert(3p) into `BUG()`

[Junio C Hamano]

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

> The fact of the matter is that the Visual C-built CI (first the Azure
> Pipeline, then GitHub Actions) have built the artifacts in release mode
> since forever. And the Meson addition simply made a mistake by _not
> specifying_ release mode (and hence defaulting to debug mode).

OK, so this is needed for _different_ reason; it is no longer about
working around a stuck CI due to misguided test.  The artifacts
should be built in a particular way to be consistent with the other
build pipelines (presumably they are used in production later, or
something?).

If explained that way in the updated log message, I would be very
happy to take the patch and merge it down, even fast-tracking down
to 'maint'.

Thanks.