{"thread":{"id":"63375","subject":"[PATCH 0/2] builtin/mv: bail out when trying to move child and its parent","startedAt":"2025-04-30T12:45:06Z","lastAt":"2025-05-02T16:53:06Z","messageCount":10,"participants":["Patrick Steinhardt","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"517018","messageId":"20250430-pks-mv-parent-child-conflict-v1-0-11a87c55ffb9@pks.im","threadId":"63375","inReplyTo":null,"subject":"[PATCH 0/2] builtin/mv: bail out when trying to move child and its parent","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-04-30T12:44:56Z","receivedAt":"2025-04-30T12:45:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis series addresses a bug in git-mv(1) where moving both a child and\nits parent directory may cause an assert to trigger. This discussion\ncame up in the context of [1] as the assert causes problems on our CI\nsystems on Windows.\n\nThanks!\n\nPatrick\n\n[1]: <pull.1908.git.1745593515875.gitgitgadget@gmail.com>\n\n---\nPatrick Steinhardt (2):\n      builtin/mv: bail out when trying to move child and its parent\n      builtin/mv: convert assert(3p) into `BUG()`\n\n builtin/mv.c  | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---\n t/t7001-mv.sh | 24 ++++++++++++++++++----\n 2 files changed, 81 insertions(+), 7 deletions(-)\n\n\n---\nbase-commit: 6c0bd1fc70efaf053abe4e57c976afdc72d15377\nchange-id: 20250430-pks-mv-parent-child-conflict-1c15f775e638\n\n"},{"id":"517019","messageId":"20250430-pks-mv-parent-child-conflict-v1-2-11a87c55ffb9@pks.im","threadId":"63375","inReplyTo":"20250430-pks-mv-parent-child-conflict-v1-0-11a87c55ffb9@pks.im","subject":"[PATCH 2/2] builtin/mv: convert assert(3p) into `BUG()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-04-30T12:44:58Z","receivedAt":"2025-04-30T12:45:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The use of asserts is discouraged in our codebase because they lead to\ndifferent behaviour depending on how Git is built. When being unsure\nenough whether a condition always holds so that one adds the assert,\nthen the assert should probably trigger regardless of how Git is being\nbuilt.\n\nDrop the call to assert(3p) in git-mv(1) and instead use `BUG()`.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/mv.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex edb854677d9..07548fe96ae 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -562,7 +562,8 @@ int cmd_mv(int argc,\n \t\t\tcontinue;\n \n \t\tpos = index_name_pos(the_repository->index, src, strlen(src));\n-\t\tassert(pos >= 0);\n+\t\tif (pos < 0)\n+\t\t\tBUG(\"could not find source in index: '%s'\", src);\n \t\tif (!(mode & SPARSE) && !lstat(src, &st))\n \t\t\tsparse_and_dirty = ie_modified(the_repository->index,\n \t\t\t\t\t\t       the_repository->index->cache[pos],\n\n-- \n2.49.0.987.g0cc8ee98dc.dirty\n\n"},{"id":"517020","messageId":"20250430-pks-mv-parent-child-conflict-v1-1-11a87c55ffb9@pks.im","threadId":"63375","inReplyTo":"20250430-pks-mv-parent-child-conflict-v1-0-11a87c55ffb9@pks.im","subject":"[PATCH 1/2] builtin/mv: bail out when trying to move child and its parent","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-04-30T12:44:57Z","receivedAt":"2025-04-30T12:45:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We have a known issue in git-mv(1) where moving both a child and any of\nits parents causes an assert to trigger because the child cannot be\nfound anymore in the index. We have added a test for this in commit\n0fcd473fdd3 (t7001: add failure test which triggers assertion,\n2024-10-22) without addressing the issue, which is why the test itself\nis marked as `test_expect_failure`.\n\nThe behaviour of that test relies on a call to assert(3p) though, which\nmay or may not be compiled into the resulting binary depending on\nwhether or not we pass `-DNDEBUG`. When these asserts are compiled into\nGit this may cause our CI to hang on Windows though, because asserts may\ncause a modal window to be shown.\n\nWhile we could work around the issue by converting this into a call to\n`BUG()`, let's rather address the root cause of the issue by bailing out\nin case we see that both a child and any of its parents are being moved\nin the same command.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/mv.c  | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n t/t7001-mv.sh | 24 +++++++++++++++++++----\n 2 files changed, 79 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 54b323fff72..edb854677d9 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -39,6 +39,13 @@ enum update_mode {\n \tINDEX = (1 << 2),\n \tSPARSE = (1 << 3),\n \tSKIP_WORKTREE_DIR = (1 << 4),\n+\t/*\n+\t * A file gets moved implicitly via a move of one of its parent\n+\t * directories. This flag causes us to skip the check that we don't try\n+\t * to move a file and any of its parent directories at the same point\n+\t * in time.\n+\t */\n+\tMOVE_VIA_PARENT_DIR = (1 << 5),\n };\n \n #define DUP_BASENAME 1\n@@ -183,6 +190,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 +235,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,6 +355,7 @@ 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@@ -348,6 +373,11 @@ int cmd_mv(int argc,\n \t\t\t\tgoto act_on_entry;\n \t\t\t}\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\t/* last - first >= 1 */\n \t\t\tmodes[i] |= WORKING_DIRECTORY;\n \n@@ -368,8 +398,7 @@ int cmd_mv(int argc,\n \t\t\t\tstrvec_push(&sources, path);\n \t\t\t\tstrvec_push(&destinations, prefixed_path);\n \n-\t\t\t\tmemset(modes + argc + j, 0, sizeof(enum update_mode));\n-\t\t\t\tmodes[argc + j] |= ce_skip_worktree(ce) ? SPARSE : INDEX;\n+\t\t\t\tmodes[argc + j] = MOVE_VIA_PARENT_DIR | (ce_skip_worktree(ce) ? SPARSE : INDEX);\n \t\t\t\tsubmodule_gitfiles[argc + j] = NULL;\n \n \t\t\t\tfree(prefixed_path);\n@@ -465,6 +494,32 @@ 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\tif (modes[i] & MOVE_VIA_PARENT_DIR)\n+\t\t\tcontinue;\n+\n+\t\tstrbuf_reset(&pathbuf);\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\tdie(_(\"cannot move both '%s' and its parent directory '%s'\"),\n+\t\t\t\t    sources.v[i], pathbuf.buf);\n+\n+\t\t\tslash_pos = strrchr(pathbuf.buf, '/');\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 +644,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_and_free(&moved_dirs, struct pathmap_entry, ent);\n+\tstrbuf_release(&pathbuf);\n \tfree(submodule_gitfiles);\n \tfree(modes);\n \treturn ret;\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex 25334b50622..920479e9256 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -550,16 +550,32 @@ test_expect_success 'moving nested submodules' '\n \tgit status\n '\n \n-test_expect_failure 'nonsense mv triggers assertion failure and partially updated index' '\n+test_expect_success 'moving file and its parent directory at the same time fails' '\n \ttest_when_finished git reset --hard HEAD &&\n \tgit reset --hard HEAD &&\n \tmkdir -p a &&\n \tmkdir -p b &&\n \t>a/a.txt &&\n \tgit add a/a.txt &&\n-\ttest_must_fail git mv a/a.txt a b &&\n-\tgit status --porcelain >actual &&\n-\tgrep \"^A[ ]*a/a.txt$\" actual\n+\tcat >expect <<-EOF &&\n+\tfatal: cannot move both ${SQ}a/a.txt${SQ} and its parent directory ${SQ}a${SQ}\n+\tEOF\n+\ttest_must_fail git mv a/a.txt a b 2>err &&\n+\ttest_cmp expect err\n+'\n+\n+test_expect_success 'moving nested directory and its parent directory at the same time fails' '\n+\ttest_when_finished git reset --hard HEAD &&\n+\tgit reset --hard HEAD &&\n+\tmkdir -p a/b/c &&\n+\t>a/b/c/file.txt &&\n+\tgit add a &&\n+\tmkdir target &&\n+\tcat >expect <<-EOF &&\n+\tfatal: cannot move both ${SQ}a/b/c${SQ} and its parent directory ${SQ}a${SQ}\n+\tEOF\n+\ttest_must_fail git mv a/b/c a target 2>err &&\n+\ttest_cmp expect err\n '\n \n test_done\n\n-- \n2.49.0.987.g0cc8ee98dc.dirty\n\n"},{"id":"517029","messageId":"xmqqr0191oaz.fsf@gitster.g","threadId":"63375","inReplyTo":"20250430-pks-mv-parent-child-conflict-v1-2-11a87c55ffb9@pks.im","subject":"Re: [PATCH 2/2] builtin/mv: convert assert(3p) into `BUG()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-30T18:45:08Z","receivedAt":"2025-04-30T18:45:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> The use of asserts is discouraged in our codebase because they lead to\n> different behaviour depending on how Git is built. When being unsure\n> enough whether a condition always holds so that one adds the assert,\n> then the assert should probably trigger regardless of how Git is being\n> built.\n\nNicely put.  Yes, this is another reason why we frown on the use of\nassert(), in addition to the reason why why Elijah's series that\nends with 5633aa3a (treewide: replace assert() with ASSERT() in\nspecial cases, 2025-03-19) was written.\n\n> Drop the call to assert(3p) in git-mv(1) and instead use `BUG()`.\n\nBeing explicit about what we are unsure about is always good.  It\nwould hopefully entice those who want to get their hands dirty to\nsee if they can \"prove\" that BUG() would never happen, which would\nbe a great outcome ;-).\n\nThanks.\n\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  builtin/mv.c | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/mv.c b/builtin/mv.c\n> index edb854677d9..07548fe96ae 100644\n> --- a/builtin/mv.c\n> +++ b/builtin/mv.c\n> @@ -562,7 +562,8 @@ int cmd_mv(int argc,\n>  \t\t\tcontinue;\n>  \n>  \t\tpos = index_name_pos(the_repository->index, src, strlen(src));\n> -\t\tassert(pos >= 0);\n> +\t\tif (pos < 0)\n> +\t\t\tBUG(\"could not find source in index: '%s'\", src);\n>  \t\tif (!(mode & SPARSE) && !lstat(src, &st))\n>  \t\t\tsparse_and_dirty = ie_modified(the_repository->index,\n>  \t\t\t\t\t\t       the_repository->index->cache[pos],\n"},{"id":"517037","messageId":"xmqqmsbxz3wp.fsf@gitster.g","threadId":"63375","inReplyTo":"20250430-pks-mv-parent-child-conflict-v1-1-11a87c55ffb9@pks.im","subject":"Re: [PATCH 1/2] builtin/mv: bail out when trying to move child and its parent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-30T22:21:42Z","receivedAt":"2025-04-30T22:21:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> @@ -368,8 +398,7 @@ int cmd_mv(int argc,\n>  \t\t\t\tstrvec_push(&sources, path);\n>  \t\t\t\tstrvec_push(&destinations, prefixed_path);\n>  \n> -\t\t\t\tmemset(modes + argc + j, 0, sizeof(enum update_mode));\n> -\t\t\t\tmodes[argc + j] |= ce_skip_worktree(ce) ? SPARSE : INDEX;\n> +\t\t\t\tmodes[argc + j] = MOVE_VIA_PARENT_DIR | (ce_skip_worktree(ce) ? SPARSE : INDEX);\n>  \t\t\t\tsubmodule_gitfiles[argc + j] = NULL;\n\nOK, this is the part we both missed during the earlier round.\n\n> +\t\t\tif (hashmap_get_entry(&moved_dirs, &needle, ent, NULL))\n> +\t\t\t\tdie(_(\"cannot move both '%s' and its parent directory '%s'\"),\n> +\t\t\t\t    sources.v[i], pathbuf.buf);\n\nOK.\n\n> -test_expect_failure 'nonsense mv triggers assertion failure and partially updated index' '\n> +test_expect_success 'moving file and its parent directory at the same time fails' '\n\nThe new title is much more descriptive.\n\n> -\ttest_must_fail git mv a/a.txt a b &&\n> -\tgit status --porcelain >actual &&\n> -\tgrep \"^A[ ]*a/a.txt$\" actual\n> +\tcat >expect <<-EOF &&\n> +\tfatal: cannot move both ${SQ}a/a.txt${SQ} and its parent directory ${SQ}a${SQ}\n> +\tEOF\n> +\ttest_must_fail git mv a/a.txt a b 2>err &&\n> +\ttest_cmp expect err\n> +'\n\nShouldn't we make sure that after failing \"git mv\" the paths and the\nindex entries stay as expected?\n\n> +test_expect_success 'moving nested directory and its parent directory at the same time fails' '\n> +\ttest_when_finished git reset --hard HEAD &&\n> +\tgit reset --hard HEAD &&\n> +\tmkdir -p a/b/c &&\n> +\t>a/b/c/file.txt &&\n> +\tgit add a &&\n> +\tmkdir target &&\n> +\tcat >expect <<-EOF &&\n> +\tfatal: cannot move both ${SQ}a/b/c${SQ} and its parent directory ${SQ}a${SQ}\n> +\tEOF\n> +\ttest_must_fail git mv a/b/c a target 2>err &&\n> +\ttest_cmp expect err\n>  '\n\nDitto.\n\nBy the way I think \"git mv a a/b\" in the same scenario already\nnotices a problematic request, so it probably won't hit this\ncodepath but we shoudl already be covered.\n\nThanks.\n"},{"id":"517038","messageId":"xmqqecx9z1n6.fsf@gitster.g","threadId":"63375","inReplyTo":"xmqqr0191oaz.fsf@gitster.g","subject":"Re: [PATCH 2/2] builtin/mv: convert assert(3p) into `BUG()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-30T23:10:37Z","receivedAt":"2025-04-30T23:10:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Patrick Steinhardt <ps@pks.im> writes:\n>\n>> The use of asserts is discouraged in our codebase because they lead to\n>> different behaviour depending on how Git is built. When being unsure\n>> enough whether a condition always holds so that one adds the assert,\n>> then the assert should probably trigger regardless of how Git is being\n>> built.\n>\n> Nicely put.  Yes, this is another reason why we frown on the use of\n> assert(), in addition to the reason why why Elijah's series that\n> ends with 5633aa3a (treewide: replace assert() with ASSERT() in\n> special cases, 2025-03-19) was written.\n>\n>> Drop the call to assert(3p) in git-mv(1) and instead use `BUG()`.\n>\n> Being explicit about what we are unsure about is always good.  It\n> would hopefully entice those who want to get their hands dirty to\n> see if they can \"prove\" that BUG() would never happen, which would\n> be a great outcome ;-).\n\nBy the way, with this in place, and without Dscho's \"assert() makes\nWin+Meson test job get stuck, so let's make assert() a no-op\" patch,\nthe CI seems to be fine.\n\n    https://github.com/git/git/actions/runs/14765572702\n\nTriggering assert() and BUG() are something we would always want to\nnotice.  They should never trigger in production and it is an event\nto call for fixing the underlying cause that made the condition\ntrigger if it is shown to end-users.  Dscho's patch protects us from\naddition of a new test that triggers an assert().  We won't see such\na test get stuck forever on Windows, but by turning such an assert()\ninto a no-op, we would waste electricity for running CI only to miss\nthe triggering assert(), which does not sound like a good use of our\nresources.\n\nSo I am inclined to drop Dscho's \"build in release mode\" patch when\nwe merge this series down to 'next'.  Being able to notice a\nbreakage (which triggers a real assert(), whether it is due to\nbroken code, or due to a broken test that documents a broken code\npath---which should be rewritten to use \"if (condition) BUG()\"),\neven if it needs to be done by noticing a test that gets stuck,\nwould be much better than missing such a breakage at all, and that\nis the primary reasoning behind my suggesting to do so.  I would not\nbe surprised if I am missing a good reason or two to make build\ntested in CI ignore asserts, so let's hear from others.\n\nOpinions?\n\nThanks.\n"},{"id":"517086","messageId":"aBR86Ct8mMUN_tzk@pks.im","threadId":"63375","inReplyTo":"xmqqecx9z1n6.fsf@gitster.g","subject":"Re: [PATCH 2/2] builtin/mv: convert assert(3p) into `BUG()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-02T08:06:00Z","receivedAt":"2025-05-02T08:06:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 30, 2025 at 04:10:37PM -0700, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Patrick Steinhardt <ps@pks.im> writes:\n> >\n> >> The use of asserts is discouraged in our codebase because they lead to\n> >> different behaviour depending on how Git is built. When being unsure\n> >> enough whether a condition always holds so that one adds the assert,\n> >> then the assert should probably trigger regardless of how Git is being\n> >> built.\n> >\n> > Nicely put.  Yes, this is another reason why we frown on the use of\n> > assert(), in addition to the reason why why Elijah's series that\n> > ends with 5633aa3a (treewide: replace assert() with ASSERT() in\n> > special cases, 2025-03-19) was written.\n> >\n> >> Drop the call to assert(3p) in git-mv(1) and instead use `BUG()`.\n> >\n> > Being explicit about what we are unsure about is always good.  It\n> > would hopefully entice those who want to get their hands dirty to\n> > see if they can \"prove\" that BUG() would never happen, which would\n> > be a great outcome ;-).\n> \n> By the way, with this in place, and without Dscho's \"assert() makes\n> Win+Meson test job get stuck, so let's make assert() a no-op\" patch,\n> the CI seems to be fine.\n> \n>     https://github.com/git/git/actions/runs/14765572702\n> \n> Triggering assert() and BUG() are something we would always want to\n> notice.  They should never trigger in production and it is an event\n> to call for fixing the underlying cause that made the condition\n> trigger if it is shown to end-users.  Dscho's patch protects us from\n> addition of a new test that triggers an assert().  We won't see such\n> a test get stuck forever on Windows, but by turning such an assert()\n> into a no-op, we would waste electricity for running CI only to miss\n> the triggering assert(), which does not sound like a good use of our\n> resources.\n\nIt makes me wonder whether we should forbid `assert()` altogether and\nuse `BUG()` everywhere, similar to the recent discussion with Elijah. We\ndo have >600 callsites of `assert()` though, so we would have to\nintroduce a macro that doesn't require us to provide a reasoning for\nnow. E.g.\n\n    #define BUG_UNLESS(condition) if (!(condition)) BUG(##condition)\n\nor something like this.\n\nAnd once we've done such a conversion we could add `assert()` to our\ndeny list of functions (wherever it was, I forgot).\n\n> So I am inclined to drop Dscho's \"build in release mode\" patch when\n> we merge this series down to 'next'.  Being able to notice a\n> breakage (which triggers a real assert(), whether it is due to\n> broken code, or due to a broken test that documents a broken code\n> path---which should be rewritten to use \"if (condition) BUG()\"),\n> even if it needs to be done by noticing a test that gets stuck,\n> would be much better than missing such a breakage at all, and that\n> is the primary reasoning behind my suggesting to do so.  I would not\n> be surprised if I am missing a good reason or two to make build\n> tested in CI ignore asserts, so let's hear from others.\n> \n> Opinions?\n\nAs far as I understand there is no need for this patch anymore.\n\nPatrick\n"},{"id":"517088","messageId":"aBR9Slq7_F9PRq0W@pks.im","threadId":"63375","inReplyTo":"xmqqmsbxz3wp.fsf@gitster.g","subject":"Re: [PATCH 1/2] builtin/mv: bail out when trying to move child and its parent","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-02T08:07:38Z","receivedAt":"2025-05-02T08:07:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 30, 2025 at 03:21:42PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> > -\ttest_must_fail git mv a/a.txt a b &&\n> > -\tgit status --porcelain >actual &&\n> > -\tgrep \"^A[ ]*a/a.txt$\" actual\n> > +\tcat >expect <<-EOF &&\n> > +\tfatal: cannot move both ${SQ}a/a.txt${SQ} and its parent directory ${SQ}a${SQ}\n> > +\tEOF\n> > +\ttest_must_fail git mv a/a.txt a b 2>err &&\n> > +\ttest_cmp expect err\n> > +'\n> \n> Shouldn't we make sure that after failing \"git mv\" the paths and the\n> index entries stay as expected?\n> \n> > +test_expect_success 'moving nested directory and its parent directory at the same time fails' '\n> > +\ttest_when_finished git reset --hard HEAD &&\n> > +\tgit reset --hard HEAD &&\n> > +\tmkdir -p a/b/c &&\n> > +\t>a/b/c/file.txt &&\n> > +\tgit add a &&\n> > +\tmkdir target &&\n> > +\tcat >expect <<-EOF &&\n> > +\tfatal: cannot move both ${SQ}a/b/c${SQ} and its parent directory ${SQ}a${SQ}\n> > +\tEOF\n> > +\ttest_must_fail git mv a/b/c a target 2>err &&\n> > +\ttest_cmp expect err\n> >  '\n> \n> Ditto.\n\nThis might've been a good idea, but I see that the series has already\nbeen merged to `next`. So I'll refrain from improving the tests.\n\nThanks!\n\nPatrick\n"},{"id":"517103","messageId":"38cc3249-3de2-5ed0-7ca8-bad6d4a5bea8@gmx.de","threadId":"63375","inReplyTo":"aBR86Ct8mMUN_tzk@pks.im","subject":"Re: [PATCH 2/2] builtin/mv: convert assert(3p) into `BUG()`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-05-02T09:44:36Z","receivedAt":"2025-05-02T09:44:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick & Junio,\n\nOn Fri, 2 May 2025, Patrick Steinhardt wrote:\n\n> On Wed, Apr 30, 2025 at 04:10:37PM -0700, Junio C Hamano wrote:\n> > Junio C Hamano <gitster@pobox.com> writes:\n> > \n> > > Patrick Steinhardt <ps@pks.im> writes:\n> > >\n> > >> The use of asserts is discouraged in our codebase because they lead to\n> > >> different behaviour depending on how Git is built. When being unsure\n> > >> enough whether a condition always holds so that one adds the assert,\n> > >> then the assert should probably trigger regardless of how Git is being\n> > >> built.\n> > >\n> > > Nicely put.  Yes, this is another reason why we frown on the use of\n> > > assert(), in addition to the reason why why Elijah's series that\n> > > ends with 5633aa3a (treewide: replace assert() with ASSERT() in\n> > > special cases, 2025-03-19) was written.\n> > >\n> > >> Drop the call to assert(3p) in git-mv(1) and instead use `BUG()`.\n> > >\n> > > Being explicit about what we are unsure about is always good.  It\n> > > would hopefully entice those who want to get their hands dirty to\n> > > see if they can \"prove\" that BUG() would never happen, which would\n> > > be a great outcome ;-).\n> > \n> > By the way, with this in place, and without Dscho's \"assert() makes\n> > Win+Meson test job get stuck, so let's make assert() a no-op\" patch,\n> > the CI seems to be fine.\n> > \n> >     https://github.com/git/git/actions/runs/14765572702\n> > \n> > Triggering assert() and BUG() are something we would always want to\n> > notice.  They should never trigger in production and it is an event\n> > to call for fixing the underlying cause that made the condition\n> > trigger if it is shown to end-users.  Dscho's patch protects us from\n> > addition of a new test that triggers an assert().  We won't see such\n> > a test get stuck forever on Windows, but by turning such an assert()\n> > into a no-op, we would waste electricity for running CI only to miss\n> > the triggering assert(), which does not sound like a good use of our\n> > resources.\n> \n> It makes me wonder whether we should forbid `assert()` altogether and\n> use `BUG()` everywhere, similar to the recent discussion with Elijah. We\n> do have >600 callsites of `assert()` though, so we would have to\n> introduce a macro that doesn't require us to provide a reasoning for\n> now. E.g.\n> \n>     #define BUG_UNLESS(condition) if (!(condition)) BUG(##condition)\n> \n> or something like this.\n> \n> And once we've done such a conversion we could add `assert()` to our\n> deny list of functions (wherever it was, I forgot).\n> \n> > So I am inclined to drop Dscho's \"build in release mode\" patch when\n> > we merge this series down to 'next'.  Being able to notice a\n> > breakage (which triggers a real assert(), whether it is due to\n> > broken code, or due to a broken test that documents a broken code\n> > path---which should be rewritten to use \"if (condition) BUG()\"),\n> > even if it needs to be done by noticing a test that gets stuck,\n> > would be much better than missing such a breakage at all, and that\n> > is the primary reasoning behind my suggesting to do so.  I would not\n> > be surprised if I am missing a good reason or two to make build\n> > tested in CI ignore asserts, so let's hear from others.\n> > \n> > Opinions?\n> \n> As far as I understand there is no need for this patch anymore.\n\nI see a need for this patch, still, and it is not a mere \"opinion\".\n\nThe fact of the matter is that the Visual C-built CI (first the Azure\nPipeline, then GitHub Actions) have built the artifacts in release mode\nsince forever. And the Meson addition simply made a mistake by _not\nspecifying_ release mode (and hence defaulting to debug mode).\n\nThis makes a difference because in `compat/mingw.c`, there is this\n(https://github.com/git/git/blob/v2.49.0/compat/mingw.c#L3247-L3255):\n\n\t#ifdef _MSC_VER\n\t#ifdef _DEBUG\n\t\t_CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_DEBUG);\n\t#endif\n\t[...]\n\t#endif\n\nThis means that each and every `assert()`, whether that be in Git's source\ncode, or in any dependency of Git (which the Git project does not\ncontrol!), is handled with this modal dialog.\n\nIt was an oversight in the win+Meson patches not to use release mode, and\nthe patch I proposed fixes this bug.\n\nIn the alternative, you could also just drop the entire win+Meson stuff,\nof which I would be actually quite in favor: No Visual Studio user will be\nhappy with Meson, therefore it would be the kind thing to drop all\npretense, officially, that Git cares about Visual Studio users.\n\nThe easier and quicker solution, though, (which would maintain a modicum\nof Visual Studio support) would be to un-drop the fix proposed in\nhttps://lore.kernel.org/git/pull.1908.git.1745593515875.gitgitgadget@gmail.com/\n\nCiao,\nJohannes\n"},{"id":"517124","messageId":"xmqqjz6zuf80.fsf@gitster.g","threadId":"63375","inReplyTo":"38cc3249-3de2-5ed0-7ca8-bad6d4a5bea8@gmx.de","subject":"Re: [PATCH 2/2] builtin/mv: convert assert(3p) into `BUG()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-02T16:53:03Z","receivedAt":"2025-05-02T16:53:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> The fact of the matter is that the Visual C-built CI (first the Azure\n> Pipeline, then GitHub Actions) have built the artifacts in release mode\n> since forever. And the Meson addition simply made a mistake by _not\n> specifying_ release mode (and hence defaulting to debug mode).\n\nOK, so this is needed for _different_ reason; it is no longer about\nworking around a stuck CI due to misguided test.  The artifacts\nshould be built in a particular way to be consistent with the other\nbuild pipelines (presumably they are used in production later, or\nsomething?).\n\nIf explained that way in the updated log message, I would be very\nhappy to take the patch and merge it down, even fast-tracking down\nto 'maint'.\n\nThanks.\n"}]}