{"thread":{"id":"60688","subject":"[BUG] mv: can trigger assertion failure with three parameters (builtin/mv.c:481)","startedAt":"2024-01-05T17:41:57Z","lastAt":"2024-10-23T20:36:39Z","messageCount":10,"participants":["Kristoffer Haugsbakk","Junio C Hamano","Dragan Simic","kristofferhaugsbakk@fastmail.com","Taylor Blau"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"486350","messageId":"d1f739fe-b28e-451f-9e01-3d2e24a0fe0d@app.fastmail.com","threadId":"60688","inReplyTo":null,"subject":"[BUG] mv: can trigger assertion failure with three parameters (builtin/mv.c:481)","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-01-05T17:41:35Z","receivedAt":"2024-01-05T17:41:57Z","isPatch":false,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"You can trigger an assertion by giving these arguments to `git mv`:\n\n /file \n\n> Thank you for filling out a Git bug report!\n> Please answer the following questions to help us understand your issue.\n>\n> What did you do before the bug happened? (Steps to reproduce your issue)\n\n```\ngit config --global --add safe.directory /tmp\ndir=$(mktemp -d)\ncd $dir\ngit init\nmkdir a\ntouch a/a.txt\ngit add a/a.txt\ngit commit -m 'init'\nmkdir b\n# Assertion triggered\ngit mv a/a.txt a b\n# `.git/index.lock` still lingers after this; commands like `git add\n# ` will fail\n```\n\nThe output:\n\n```\ngit: builtin/mv.c:481: cmd_mv: Assertion `pos >= 0' failed.\nAborted (core dumped)\n```\n\nAlso `.git/index.lock` is still there.\n\n> What did you expect to happen? (Expected behavior)\n\nA normal error message if the command is nonsensical (I don’t know; that’s\nnot the point). Also `.git/index.lock` to be cleaned up.\n\n> What happened instead? (Actual behavior)\n\nAn assertion failed. `.git/index.lock` is not cleaned up.\n\n> What's different between what you expected and what actually happened?\n\nSee above.\n\n> Anything else you want to add:\n\nSame behavior on `master` (a26002b628 (The fifth batch, 2024-01-02)).\n\n```\n./bin-wrappers/git config --global --add safe.directory /tmp\ndir=$(mktemp -d)\n./bin-wrappers/git -C $dir init\nmkdir $dir/a\ntouch $dir/a/a.txt\n./bin-wrappers/git -C $dir add $dir/a/a.txt\n./bin-wrappers/git -C $dir commit -m 'init'\nmkdir $dir/b\n# Assertion triggered\n./bin-wrappers/git -C $dir mv $dir/a/a.txt $dir/a $dir/b\n```\n\n> Please review the rest of the bug report below.\n> You can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.43.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 6.2.0-39-generic #40~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Thu Nov 16 10:53:04 UTC 2 x86_64\ncompiler info: gnuc: 11.4\nlibc info: glibc: 2.35\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\n\n-- \nKristoffer Haugsbakk\n"},{"id":"486351","messageId":"xmqqil47obnw.fsf@gitster.g","threadId":"60688","inReplyTo":"d1f739fe-b28e-451f-9e01-3d2e24a0fe0d@app.fastmail.com","subject":"Re: [BUG] mv: can trigger assertion failure with three parameters (builtin/mv.c:481)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-05T18:52:35Z","receivedAt":"2024-01-05T18:52:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n> You can trigger an assertion by giving these arguments to `git mv`:\n>\n> /file \n> ...\n>> What did you expect to happen? (Expected behavior)\n>\n> A normal error message if the command is nonsensical (I don’t know; that’s\n> not the point). Also `.git/index.lock` to be cleaned up.\n\nGood find.\n\nNot just that, but when the command fails in the middle like this,\nit leaves the working tree in a half-updated state, i.e.\n\n> ./bin-wrappers/git -C $dir mv $dir/a/a.txt $dir/a $dir/b\n\nwill first move a/a.txt to b/a.txt, then try to move a (actually,\nall contents of it, including a/a.txt) to b/a and finds that \"the\ncommand is nonsensical\" and aborts, and by that time, there is no\na/a.txt (i.e. the working tree has been modified). The failure\nshould be made atomic, just like \"git switch\" to another branch may\nstop _without_ touching anything in the working tree when it may\nhave to fail (e.g., due to a file being dirty).\n\nThanks for reporting, Kristoffer.\n\nAny takers?\n\n$ git shortlog --since=3.years -s -n -e --no-merges v2.43.0 builtin/mv.c\n 15\tShaoxuan Yuan \n 10\tElijah Newren \n 5\tÆvar Arnfjörð Bjarmason \n 2\tJunio C Hamano \n 1\tAndrzej Hunt \n 1\tCalvin Wan \n 1\tDerrick Stolee \n 1\tSebastian Thiel \n 1\tTorsten Bögershausen \n"},{"id":"486352","messageId":"7c689d58c63998e6a690207be00d3875@manjaro.org","threadId":"60688","inReplyTo":"xmqqil47obnw.fsf@gitster.g","subject":"Re: [BUG] mv: can trigger assertion failure with three parameters (builtin/mv.c:481)","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-01-05T19:06:46Z","receivedAt":"2024-01-05T19:06:54Z","isPatch":false,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-01-05 19:52, Junio C Hamano wrote:\n> \"Kristoffer Haugsbakk\" writes:\n> \n>> You can trigger an assertion by giving these arguments to `git mv`:\n>> \n>> /file \n>> ...\n>>> What did you expect to happen? (Expected behavior)\n>> \n>> A normal error message if the command is nonsensical (I don’t know; \n>> that’s\n>> not the point). Also `.git/index.lock` to be cleaned up.\n> \n> Good find.\n\nYes, thanks to Kristoffer for reporting this issue.\n\n> Not just that, but when the command fails in the middle like this,\n> it leaves the working tree in a half-updated state, i.e.\n> \n>> ./bin-wrappers/git -C $dir mv $dir/a/a.txt $dir/a $dir/b\n> \n> will first move a/a.txt to b/a.txt, then try to move a (actually,\n> all contents of it, including a/a.txt) to b/a and finds that \"the\n> command is nonsensical\" and aborts, and by that time, there is no\n> a/a.txt (i.e. the working tree has been modified). The failure\n> should be made atomic, just like \"git switch\" to another branch may\n> stop _without_ touching anything in the working tree when it may\n> have to fail (e.g., due to a file being dirty).\n> \n> Thanks for reporting, Kristoffer.\n> \n> Any takers?\n\nThis looks like a rather interesting bugfix to me. :) Though, I've \nunfortunately contracted some _nasty_ flu, so I'm still simply unable to \nwork on pretty much anything in the next 5-6 days or so, at which point \nI hope to be operational again.\n\nThus, unless someone else can get it done faster, I should be able to \nstart working on it in about a week or so. Hopefully, that is.\n\n> $ git shortlog --since=3.years -s -n -e --no-merges v2.43.0 \n> builtin/mv.c\n> 15\tShaoxuan Yuan \n> 10\tElijah Newren \n> 5\tÆvar Arnfjörð Bjarmason \n> 2\tJunio C Hamano \n> 1\tAndrzej Hunt \n> 1\tCalvin Wan \n> 1\tDerrick Stolee \n> 1\tSebastian Thiel \n> 1\tTorsten Bögershausen \n"},{"id":"488873","messageId":"ce1b5ae24ed8356b9beed340fff61e96f7c1d549.1708259215.git.code@khaugsbakk.name","threadId":"60688","inReplyTo":"d1f739fe-b28e-451f-9e01-3d2e24a0fe0d@app.fastmail.com","subject":"Re: [BUG] mv: can trigger assertion failure with three parameters (builtin/mv.c:481)","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-18T12:42:27Z","receivedAt":"2024-02-18T12:42:49Z","isPatch":false,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Here’s a failing test. This fails on top of `master` (3e0d3cd5c7 (Merge branch\n'jx/dirstat-parseopt-help', 2024-02-15)).\n\nSigned-off-by: Kristoffer Haugsbakk \n\n-- >8 --\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex 879a6dce601..4f180903486 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -549,4 +549,16 @@ test_expect_success 'moving nested submodules' '\n \tgit status\n '\n \n+test_expect_success '(TODO title) nonsense move' '\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+'\n+\n test_done\n"},{"id":"505581","messageId":"29d71db280c972c91174bd0a501af66be72643af.1729462326.git.code@khaugsbakk.name","threadId":"60688","inReplyTo":"ce1b5ae24ed8356b9beed340fff61e96f7c1d549.1708259215.git.code@khaugsbakk.name","subject":"[PATCH v2] t7001: add failure test which triggers assertion","fromName":"","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-10-20T22:14:46Z","receivedAt":"2024-10-20T22:15:21Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"From: Kristoffer Haugsbakk \n\ngit-mv(1) gets very confused:\n\n git: builtin/mv.c:506: cmd_mv: Assertion `pos >= 0' failed.\n Aborted (core dumped)\n test_must_fail: died by signal 6: git mv a/a.txt a b\n fatal: Unable to create '.git/index.lock': File exists.\n\n Another git process seems to be running in this repository, e.g.\n an editor opened by 'git commit'. Please make sure all processes\n are terminated then try again. If it still fails, a git process\n may have crashed in this repository earlier:\n remove the file manually to continue.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n\nNotes (series):\n It’s been a good while. Let’s just add this as a known breakage?\n\n t/t7001-mv.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex 86258f9f430..739c25e2551 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -551,4 +551,16 @@ test_expect_success 'moving nested submodules' '\n \tgit status\n '\n \n+test_expect_failure 'nonsense mv triggers assertion failure at builtin/mv.c:502' '\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+'\n+\n test_done\n\nbase-commit: 34b6ce9b30747131b6e781ff718a45328aa887d0\n-- \n2.46.1.641.g54e7913fcb6\n\n"},{"id":"505771","messageId":"ZxbF2xxqh3rTgXvN@nand.local","threadId":"60688","inReplyTo":"29d71db280c972c91174bd0a501af66be72643af.1729462326.git.code@khaugsbakk.name","subject":"Re: [PATCH v2] t7001: add failure test which triggers assertion","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-21T21:21:31Z","receivedAt":"2024-10-21T21:21:33Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 21, 2024 at 12:14:46AM +0200, kristofferhaugsbakk@fastmail.com wrote:\n> From: Kristoffer Haugsbakk \n>\n> git-mv(1) gets very confused:\n>\n> git: builtin/mv.c:506: cmd_mv: Assertion `pos >= 0' failed.\n> Aborted (core dumped)\n> test_must_fail: died by signal 6: git mv a/a.txt a b\n> fatal: Unable to create '.git/index.lock': File exists.\n>\n> Another git process seems to be running in this repository, e.g.\n> an editor opened by 'git commit'. Please make sure all processes\n> are terminated then try again. If it still fails, a git process\n> may have crashed in this repository earlier:\n> remove the file manually to continue.\n\nThere was some good analysis of what the problem was in the earlier\nparts of this thread. I think it is probably worth capturing some of\nthose here, too.\n\n> Signed-off-by: Kristoffer Haugsbakk \n> ---\n>\n> Notes (series):\n> It’s been a good while. Let’s just add this as a known breakage?\n>\n> t/t7001-mv.sh | 12 ++++++++++++\n> 1 file changed, 12 insertions(+)\n>\n> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\n> index 86258f9f430..739c25e2551 100755\n> --- a/t/t7001-mv.sh\n> +++ b/t/t7001-mv.sh\n> @@ -551,4 +551,16 @@ test_expect_success 'moving nested submodules' '\n> \tgit status\n> '\n>\n> +test_expect_failure 'nonsense mv triggers assertion failure at builtin/mv.c:502' '\n\nDo we want to be so specific about the line number that the assertion\nfailure occurs on? The actual coredump triggered by this test will tell\nus that information. But in the meantime this line is likely to go stale\nas builtin/mv.c changes over time.\n\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> +'\n\nThanks,\nTaylor\n"},{"id":"505773","messageId":"1f3b62bb-7c3a-4bac-8995-8df417228ed2@app.fastmail.com","threadId":"60688","inReplyTo":"ZxbF2xxqh3rTgXvN@nand.local","subject":"Re: [PATCH v2] t7001: add failure test which triggers assertion","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-10-21T21:25:26Z","receivedAt":"2024-10-21T21:25:48Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Mon, Oct 21, 2024, at 23:21, Taylor Blau wrote:\n> On Mon, Oct 21, 2024 at 12:14:46AM +0200,\n> kristofferhaugsbakk@fastmail.com wrote:\n>> From: Kristoffer Haugsbakk \n>>\n>> git-mv(1) gets very confused:\n>>\n>> git: builtin/mv.c:506: cmd_mv: Assertion `pos >= 0' failed.\n>> Aborted (core dumped)\n>> test_must_fail: died by signal 6: git mv a/a.txt a b\n>> fatal: Unable to create '.git/index.lock': File exists.\n>>\n>> Another git process seems to be running in this repository, e.g.\n>> an editor opened by 'git commit'. Please make sure all processes\n>> are terminated then try again. If it still fails, a git process\n>> may have crashed in this repository earlier:\n>> remove the file manually to continue.\n>\n> There was some good analysis of what the problem was in the earlier\n> parts of this thread. I think it is probably worth capturing some of\n> those here, too.\n\nI will try to incorporate Junio’s analysis into the commit message\ntomorrow. :)\n\n>> +test_expect_failure 'nonsense mv triggers assertion failure at builtin/mv.c:502' '\n>\n> Do we want to be so specific about the line number that the assertion\n> failure occurs on? The actual coredump triggered by this test will tell\n> us that information. But in the meantime this line is likely to go stale\n> as builtin/mv.c changes over time.\n\nYou’re right, it’s overly specific/volatile.\n\nThanks\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"505774","messageId":"ZxbHp9ndCIWj14xH@nand.local","threadId":"60688","inReplyTo":"1f3b62bb-7c3a-4bac-8995-8df417228ed2@app.fastmail.com","subject":"Re: [PATCH v2] t7001: add failure test which triggers assertion","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-21T21:29:11Z","receivedAt":"2024-10-21T21:29:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 21, 2024 at 11:25:26PM +0200, Kristoffer Haugsbakk wrote:\n> On Mon, Oct 21, 2024, at 23:21, Taylor Blau wrote:\n> > On Mon, Oct 21, 2024 at 12:14:46AM +0200,\n> > kristofferhaugsbakk@fastmail.com wrote:\n> >> From: Kristoffer Haugsbakk \n> >>\n> >> git-mv(1) gets very confused:\n> >>\n> >> git: builtin/mv.c:506: cmd_mv: Assertion `pos >= 0' failed.\n> >> Aborted (core dumped)\n> >> test_must_fail: died by signal 6: git mv a/a.txt a b\n> >> fatal: Unable to create '.git/index.lock': File exists.\n> >>\n> >> Another git process seems to be running in this repository, e.g.\n> >> an editor opened by 'git commit'. Please make sure all processes\n> >> are terminated then try again. If it still fails, a git process\n> >> may have crashed in this repository earlier:\n> >> remove the file manually to continue.\n> >\n> > There was some good analysis of what the problem was in the earlier\n> > parts of this thread. I think it is probably worth capturing some of\n> > those here, too.\n>\n> I will try to incorporate Junio’s analysis into the commit message\n> tomorrow. :)\n\nThanks, I look forward to it!\n\nThanks,\nTaylor\n"},{"id":"505866","messageId":"c4ada0b787736ecd5aee986b1b8a4f90ccb84e21.1729631436.git.code@khaugsbakk.name","threadId":"60688","inReplyTo":"29d71db280c972c91174bd0a501af66be72643af.1729462326.git.code@khaugsbakk.name","subject":"[PATCH v3] t7001: add failure test which triggers assertion","fromName":"","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-10-22T21:14:33Z","receivedAt":"2024-10-22T21:14:43Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"From: Kristoffer Haugsbakk \n\n`git mv a/a.txt a b/` is a nonsense instruction. Instead of failing\ngracefully the command trips over itself,[1] leaving behind unfinished\nwork:\n\n1. first it moves `a/a.txt` to `b/a.txt`; then\n2. tries to move `a/`, including `a/a.txt`; then\n3. figures out that it’s in a bad state (assertion); and finally\n4. aborts.\n\nNow you’re left with a partially-updated index.\n\nThe command should instead fail gracefully and make no changes to the\nindex until it knows that it can complete a sensible action.\n\nFor now just add a failing test since this has been known about for\na while.[2]\n\n† 1: Caused by a `pos >= 0` assertion\n[2]: https://lore.kernel.org/git/d1f739fe-b28e-451f-9e01-3d2e24a0fe0d@app.fastmail.com/\n\nHelped-by: Junio C Hamano \nSigned-off-by: Kristoffer Haugsbakk \n---\n\nNotes (series):\n v3:\n • Rewrite commit message based on Junio’s reply\n • Tweak test description: less volatile. Also mention index state.\n v1/v2:\n • It’s been a good while. Let’s just add this as a known breakage?\n\nNotes (meta-trailers):\n Helped-by: Junio C Hamano \n Comment: Commit message is based on his description\n Link: https://lore.kernel.org/git/xmqqil47obnw.fsf@gitster.g/\n\n t/t7001-mv.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex 86258f9f43..69c9190772 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -551,4 +551,16 @@ 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+\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+'\n+\n test_done\n\nInterdiff against v2:\n diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\n index 739c25e255..69c9190772 100755\n --- a/t/t7001-mv.sh\n +++ b/t/t7001-mv.sh\n @@ -551,7 +551,7 @@ test_expect_success 'moving nested submodules' '\n \tgit status\n '\n \n -test_expect_failure 'nonsense mv triggers assertion failure at builtin/mv.c:502' '\n +test_expect_failure 'nonsense mv triggers assertion failure and partially updated index' '\n \ttest_when_finished git reset --hard HEAD &&\n \tgit reset --hard HEAD &&\n \tmkdir -p a &&\n\nbase-commit: 34b6ce9b30747131b6e781ff718a45328aa887d0\n-- \n2.46.1.641.g54e7913fcb6\n\n"},{"id":"505982","messageId":"ZxleVZ5Jh52Wj3e7@nand.local","threadId":"60688","inReplyTo":"c4ada0b787736ecd5aee986b1b8a4f90ccb84e21.1729631436.git.code@khaugsbakk.name","subject":"Re: [PATCH v3] t7001: add failure test which triggers assertion","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-23T20:36:37Z","receivedAt":"2024-10-23T20:36:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 22, 2024 at 11:14:33PM +0200, kristofferhaugsbakk@fastmail.com wrote:\n> Notes (series):\n> v3:\n> • Rewrite commit message based on Junio’s reply\n> • Tweak test description: less volatile. Also mention index state.\n> v1/v2:\n> • It’s been a good while. Let’s just add this as a known breakage?\n\nThanks, this the new version looks good to me. Let's start merging this\none down.\n\nThanks,\nTaylor\n"}]}