{"thread":{"id":"58826","subject":"[PATCH 0/3] fix t1509-root-work-tree failure","startedAt":"2022-11-21T03:00:29Z","lastAt":"2022-12-09T04:59:24Z","messageCount":14,"participants":["Eric Sunshine via GitGitGadget","Eric Sunshine","Ævar Arnfjörð Bjarmason","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"467645","messageId":"pull.1425.git.1668999621.gitgitgadget@gmail.com","threadId":"58826","inReplyTo":null,"subject":"[PATCH 0/3] fix t1509-root-work-tree failure","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-21T03:00:18Z","receivedAt":"2022-11-21T03:00:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"The t1509-root-work-tree script started failing earlier this year but went\nunnoticed because the script is rarely run since it requires setting up a\nchroot environment or a sacrificial virtual machine. This patch series fixes\nthe failure and makes it a bit easier to run the script repeatedly without\nit tripping over itself.\n\nEric Sunshine (3):\n t1509: fix failing \"root work tree\" test due to owner-check\n t1509: make \"setup\" test more robust\n t1509: facilitate repeated script invocations\n\n t/t1509-root-work-tree.sh | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\n\nbase-commit: a0789512c5a4ae7da935cd2e419f253cb3cb4ce7\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1425%2Fsunshineco%2Ft1509fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1425/sunshineco/t1509fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1425\n-- \ngitgitgadget\n"},{"id":"467646","messageId":"0efeec8abdb913786c67775cbd79c8e4285ded10.1668999621.git.gitgitgadget@gmail.com","threadId":"58826","inReplyTo":"pull.1425.git.1668999621.gitgitgadget@gmail.com","subject":"[PATCH 1/3] t1509: fix failing \"root work tree\" test due to owner-check","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-21T03:00:19Z","receivedAt":"2022-11-21T03:00:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nWhen 8959555cee (setup_git_directory(): add an owner check for the\ntop-level directory, 2022-03-02) tightened security surrounding\ndirectory ownership, it neglected to adjust t1509-root-work-tree.sh to\ntake the new restriction into account. As a result, since the root\ndirectory `/` is typically not owned by the user running the test\n(indeed, t1509 refuses to run as `root`), the ownership check added\nby 8959555cee kicks in and causes the test to fail:\n\n fatal: detected dubious ownership in repository at '/'\n To add an exception for this directory, call:\n\n git config --global --add safe.directory /\n\nThis problem went unnoticed for so long because t1509 is rarely run\nsince it requires setting up a `chroot` environment or a sacrificial\nvirtual machine in which `/` can be made writable and polluted by any\nuser.\n\nSigned-off-by: Eric Sunshine \n---\n t/t1509-root-work-tree.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1509-root-work-tree.sh b/t/t1509-root-work-tree.sh\nindex 553a3f601ba..eb57fe7e19f 100755\n--- a/t/t1509-root-work-tree.sh\n+++ b/t/t1509-root-work-tree.sh\n@@ -221,7 +221,8 @@ test_expect_success 'setup' '\n \trm -rf /.git &&\n \techo \"Initialized empty Git repository in /.git/\" > expected &&\n \tgit init > result &&\n-\ttest_cmp expected result\n+\ttest_cmp expected result &&\n+\tgit config --global --add safe.directory /\n '\n \n test_vars 'auto gitdir, root' \".git\" \"/\" \"\"\n-- \ngitgitgadget\n\n"},{"id":"467647","messageId":"617f98dcb40d417fbb48d9c1de8fa9ab650f5370.1668999621.git.gitgitgadget@gmail.com","threadId":"58826","inReplyTo":"pull.1425.git.1668999621.gitgitgadget@gmail.com","subject":"[PATCH 2/3] t1509: make \"setup\" test more robust","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-21T03:00:20Z","receivedAt":"2022-11-21T03:00:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nOne of the t1509 setup tests is very particular about the output it\nexpects from `git init`, and fails if the output differs even slightly\nwhich can happen easily if the script is run multiple times since it\ndoesn't do a good job of cleaning up after itself (i.e. it leaves\ndetritus in the root directory `/`). One bit of cruft in particular\n(`/HEAD`) makes the test fail since its presence causes `git init` to\nalter its output; rather than reporting \"Initialized empty Git\nrepository\", it instead reports \"Reinitialized existing Git repository\"\nwhen `/HEAD` is present. Address this problem by making the test do a\nmore careful job of crafting its intended initial state.\n\nSigned-off-by: Eric Sunshine \n---\n t/t1509-root-work-tree.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1509-root-work-tree.sh b/t/t1509-root-work-tree.sh\nindex eb57fe7e19f..d0417626280 100755\n--- a/t/t1509-root-work-tree.sh\n+++ b/t/t1509-root-work-tree.sh\n@@ -243,7 +243,7 @@ say \"auto bare gitdir\"\n # DESTROYYYYY!!!!!\n test_expect_success 'setup' '\n \trm -rf /refs /objects /info /hooks &&\n-\trm -f /expected /ls.expected /me /result &&\n+\trm -f /HEAD /expected /ls.expected /me /result &&\n \tcd / &&\n \techo \"Initialized empty Git repository in /\" > expected &&\n \tgit init --bare > result &&\n-- \ngitgitgadget\n\n"},{"id":"467648","messageId":"97ada2a1202190776ce3989d3841dd47e2702316.1668999621.git.gitgitgadget@gmail.com","threadId":"58826","inReplyTo":"pull.1425.git.1668999621.gitgitgadget@gmail.com","subject":"[PATCH 3/3] t1509: facilitate repeated script invocations","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-21T03:00:21Z","receivedAt":"2022-11-21T03:00:36Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nt1509-root-work-tree.sh, which tests behavior of a Git repository\nlocated at the root `/` directory, refuses to run if it detects the\npresence of an existing repository at `/`. This safeguard ensures that\nit won't clobber a legitimate repository at that location. However,\nbecause t1509 does a poor job of cleaning up after itself, it runs afoul\nof its own safety check on subsequent runs, which makes it painful to\nrun the script repeatedly since each run requires manual cleanup of\ndetritus from the previous run.\n\nAddress this shortcoming by making t1509 clean up after itself as its\nlast action. This is safe since the script can only make it to this\ncleanup action if it did not find a legitimate repository at `/` in the\nfirst place, so the resources cleaned up here can only have been created\nby the script itself.\n\nSigned-off-by: Eric Sunshine \n---\n t/t1509-root-work-tree.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t1509-root-work-tree.sh b/t/t1509-root-work-tree.sh\nindex d0417626280..c799f5b6aca 100755\n--- a/t/t1509-root-work-tree.sh\n+++ b/t/t1509-root-work-tree.sh\n@@ -256,4 +256,9 @@ test_expect_success 'go to /foo' 'cd /foo'\n \n test_vars 'auto gitdir, root' \"/\" \"\" \"\"\n \n+test_expect_success 'cleanup root' '\n+\trm -rf /.git /refs /objects /info /hooks /branches /foo &&\n+\trm -f /HEAD /config /description /expected /ls.expected /me /result\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"468539","messageId":"CAPig+cT6z5kzM8suwqxJ0wrzHjnj9ChROVBiQO3AR1rJ11pkNw@mail.gmail.com","threadId":"58826","inReplyTo":"pull.1425.git.1668999621.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] fix t1509-root-work-tree failure","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-12-05T18:21:54Z","receivedAt":"2022-12-05T18:22:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Nov 20, 2022 at 10:00 PM Eric Sunshine via GitGitGadget\n wrote:\n> The t1509-root-work-tree script started failing earlier this year but went\n> unnoticed because the script is rarely run since it requires setting up a\n> chroot environment or a sacrificial virtual machine. This patch series fixes\n> the failure and makes it a bit easier to run the script repeatedly without\n> it tripping over itself.\n>\n> Eric Sunshine (3):\n> t1509: fix failing \"root work tree\" test due to owner-check\n> t1509: make \"setup\" test more robust\n> t1509: facilitate repeated script invocations\n\nPing?\n"},{"id":"468600","messageId":"221206.86ilipckms.gmgdl@evledraar.gmail.com","threadId":"58826","inReplyTo":"97ada2a1202190776ce3989d3841dd47e2702316.1668999621.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] t1509: facilitate repeated script invocations","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-06T02:42:28Z","receivedAt":"2022-12-06T02:49:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 21 2022, Eric Sunshine via GitGitGadget wrote:\n\n> From: Eric Sunshine \n>\n> t1509-root-work-tree.sh, which tests behavior of a Git repository\n> located at the root `/` directory, refuses to run if it detects the\n> presence of an existing repository at `/`. This safeguard ensures that\n> it won't clobber a legitimate repository at that location. However,\n> because t1509 does a poor job of cleaning up after itself, it runs afoul\n> of its own safety check on subsequent runs, which makes it painful to\n> run the script repeatedly since each run requires manual cleanup of\n> detritus from the previous run.\n>\n> Address this shortcoming by making t1509 clean up after itself as its\n> last action. This is safe since the script can only make it to this\n> cleanup action if it did not find a legitimate repository at `/` in the\n> first place, so the resources cleaned up here can only have been created\n> by the script itself.\n>\n> Signed-off-by: Eric Sunshine \n> ---\n> t/t1509-root-work-tree.sh | 5 +++++\n> 1 file changed, 5 insertions(+)\n>\n> diff --git a/t/t1509-root-work-tree.sh b/t/t1509-root-work-tree.sh\n> index d0417626280..c799f5b6aca 100755\n> --- a/t/t1509-root-work-tree.sh\n> +++ b/t/t1509-root-work-tree.sh\n> @@ -256,4 +256,9 @@ test_expect_success 'go to /foo' 'cd /foo'\n> \n> test_vars 'auto gitdir, root' \"/\" \"\" \"\"\n> \n> +test_expect_success 'cleanup root' '\n> +\trm -rf /.git /refs /objects /info /hooks /branches /foo &&\n> +\trm -f /HEAD /config /description /expected /ls.expected /me /result\n> +'\n\nPerhaps it would be nice to split this into a function in an earlier\nstep, as this duplicates what you patched in 2/3. E.g.:\n\t\n\tcleanup_root_git_bare() {\n\t\trm -rf /.git\n\t}\n\tcleanup_root_git() {\n\t\trm -f /HEAD /config /description /expected /ls.expected /me /result\n\t}\n\nThen all 3 resulting users could call some combination of those.\n\nThis is an existing wart, but I also wondered why the \"expected\",\n\"result\" etc. was needed. Either we could make the tests creating those\ndo a \"test_when_finished\" removal of it, or better yet just create those\nin the trash directory.\n\nAt this point we've cd'd to /, but there doesn't seem to be a reason we\ncouldn't use our original trash directory for our own state.\n\nThe \"description\" we could then git rid of with \"git init --template=\".\n\nWe could even get rid of the need to maintain \"HEAD\" etc. by init-ing a\nrepo in the trash directory, copying its contents to \"/\", and then we'd\nknow exactly what we needed to remove afterwards. I.e. just a mirror of\nthe structure we copied from our just init-ed repo.\n\nBut all that's a digression for this series, which I think is good\nenough as-is. I just wondered why we had some of these odd looking\npatterns.\n\n\n\n"},{"id":"468601","messageId":"CAPig+cSfvgu8XjvmmAkFWe1G1VDRgrcx5GjUhr4xSDqoJ4cZOA@mail.gmail.com","threadId":"58826","inReplyTo":"221206.86ilipckms.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 3/3] t1509: facilitate repeated script invocations","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-12-06T03:23:13Z","receivedAt":"2022-12-06T03:23:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 5, 2022 at 9:48 PM Ævar Arnfjörð Bjarmason wrote:\n> On Mon, Nov 21 2022, Eric Sunshine via GitGitGadget wrote:\n> > t1509-root-work-tree.sh, which tests behavior of a Git repository\n> > located at the root `/` directory, refuses to run if it detects the\n> > presence of an existing repository at `/`. This safeguard ensures that\n> > it won't clobber a legitimate repository at that location. However,\n> > because t1509 does a poor job of cleaning up after itself, it runs afoul\n> > of its own safety check on subsequent runs, which makes it painful to\n> > run the script repeatedly since each run requires manual cleanup of\n> > detritus from the previous run.\n> >\n> > Address this shortcoming by making t1509 clean up after itself as its\n> > last action. This is safe since the script can only make it to this\n> > cleanup action if it did not find a legitimate repository at `/` in the\n> > first place, so the resources cleaned up here can only have been created\n> > by the script itself.\n> >\n> > Signed-off-by: Eric Sunshine \n> > ---\n> > +test_expect_success 'cleanup root' '\n> > + rm -rf /.git /refs /objects /info /hooks /branches /foo &&\n> > + rm -f /HEAD /config /description /expected /ls.expected /me /result\n> > +'\n>\n> Perhaps it would be nice to split this into a function in an earlier\n> step, as this duplicates what you patched in 2/3. E.g.:\n>\n> cleanup_root_git_bare() {\n> rm -rf /.git\n> }\n> cleanup_root_git() {\n> rm -f /HEAD /config /description /expected /ls.expected /me /result\n> }\n>\n> Then all 3 resulting users could call some combination of those.\n\nI did something like that originally but decided against it in the\nend, and went with the simpler \"just clean up everything we created\"\ndespite the bit of duplicated cleanup code. After all, this is only a\ntiny bit of duplication in a script filled with much worse: for\ninstance, the `test_foobar_root`, `test_foobar_foo`, and\n`test_foobar_foobar` functions are filled with copy/paste code -- not\nto mention having rather poor names. So, considering that the script\nis probably in need of a major overhaul and modernization at some\npoint anyhow[1], and because I simply wanted to get the script back\ninto a working state, I opted for minimal changes.\n\n[1]: That's assuming anyone even cares enough to clean this script up.\nIt's clearly neglected; the breakage addressed by this series has gone\nunnoticed for many months.\n\n> This is an existing wart, but I also wondered why the \"expected\",\n> \"result\" etc. was needed. Either we could make the tests creating those\n> do a \"test_when_finished\" removal of it, or better yet just create those\n> in the trash directory.\n>\n> At this point we've cd'd to /, but there doesn't seem to be a reason we\n> couldn't use our original trash directory for our own state.\n>\n> The \"description\" we could then git rid of with \"git init --template=\".\n>\n> We could even get rid of the need to maintain \"HEAD\" etc. by init-ing a\n> repo in the trash directory, copying its contents to \"/\", and then we'd\n> know exactly what we needed to remove afterwards. I.e. just a mirror of\n> the structure we copied from our just init-ed repo.\n\nFodder for an eventual overhaul, I suppose.\n\n> But all that's a digression for this series, which I think is good\n> enough as-is. I just wondered why we had some of these odd looking\n> patterns.\n\nThanks for reading through the patches.\n"},{"id":"468776","messageId":"n7q85691-5p25-464n-12o7-q4s4opsr2p0o@tzk.qr","threadId":"58826","inReplyTo":"0efeec8abdb913786c67775cbd79c8e4285ded10.1668999621.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] t1509: fix failing \"root work tree\" test due to owner-check","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-12-08T11:49:09Z","receivedAt":"2022-12-08T11:50:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Mon, 21 Nov 2022, Eric Sunshine via GitGitGadget wrote:\n\n> From: Eric Sunshine \n>\n> When 8959555cee (setup_git_directory(): add an owner check for the\n> top-level directory, 2022-03-02) tightened security surrounding\n> directory ownership, it neglected to adjust t1509-root-work-tree.sh to\n> take the new restriction into account. As a result, since the root\n> directory `/` is typically not owned by the user running the test\n> (indeed, t1509 refuses to run as `root`), the ownership check added\n> by 8959555cee kicks in and causes the test to fail:\n>\n> fatal: detected dubious ownership in repository at '/'\n> To add an exception for this directory, call:\n>\n> git config --global --add safe.directory /\n>\n> This problem went unnoticed for so long because t1509 is rarely run\n> since it requires setting up a `chroot` environment or a sacrificial\n> virtual machine in which `/` can be made writable and polluted by any\n> user.\n\nACK, this is the right thing to do.\n\nThanks for working on it,\nJohannes\n\n>\n> Signed-off-by: Eric Sunshine \n> ---\n> t/t1509-root-work-tree.sh | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t1509-root-work-tree.sh b/t/t1509-root-work-tree.sh\n> index 553a3f601ba..eb57fe7e19f 100755\n> --- a/t/t1509-root-work-tree.sh\n> +++ b/t/t1509-root-work-tree.sh\n> @@ -221,7 +221,8 @@ test_expect_success 'setup' '\n> \trm -rf /.git &&\n> \techo \"Initialized empty Git repository in /.git/\" > expected &&\n> \tgit init > result &&\n> -\ttest_cmp expected result\n> +\ttest_cmp expected result &&\n> +\tgit config --global --add safe.directory /\n> '\n>\n> test_vars 'auto gitdir, root' \".git\" \"/\" \"\"\n> --\n> gitgitgadget\n>\n>\n"},{"id":"468777","messageId":"7rs8633n-s68s-4542-o01o-033p86p51p77@tzk.qr","threadId":"58826","inReplyTo":"617f98dcb40d417fbb48d9c1de8fa9ab650f5370.1668999621.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] t1509: make \"setup\" test more robust","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-12-08T11:49:46Z","receivedAt":"2022-12-08T11:51:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Mon, 21 Nov 2022, Eric Sunshine via GitGitGadget wrote:\n\n> From: Eric Sunshine \n>\n> One of the t1509 setup tests is very particular about the output it\n> expects from `git init`, and fails if the output differs even slightly\n> which can happen easily if the script is run multiple times since it\n> doesn't do a good job of cleaning up after itself (i.e. it leaves\n> detritus in the root directory `/`). One bit of cruft in particular\n> (`/HEAD`) makes the test fail since its presence causes `git init` to\n> alter its output; rather than reporting \"Initialized empty Git\n> repository\", it instead reports \"Reinitialized existing Git repository\"\n> when `/HEAD` is present. Address this problem by making the test do a\n> more careful job of crafting its intended initial state.\n\nGood explanation, and the patch is obviously correct.\n\nACK,\nJohannes\n\n>\n> Signed-off-by: Eric Sunshine \n> ---\n> t/t1509-root-work-tree.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t1509-root-work-tree.sh b/t/t1509-root-work-tree.sh\n> index eb57fe7e19f..d0417626280 100755\n> --- a/t/t1509-root-work-tree.sh\n> +++ b/t/t1509-root-work-tree.sh\n> @@ -243,7 +243,7 @@ say \"auto bare gitdir\"\n> # DESTROYYYYY!!!!!\n> test_expect_success 'setup' '\n> \trm -rf /refs /objects /info /hooks &&\n> -\trm -f /expected /ls.expected /me /result &&\n> +\trm -f /HEAD /expected /ls.expected /me /result &&\n> \tcd / &&\n> \techo \"Initialized empty Git repository in /\" > expected &&\n> \tgit init --bare > result &&\n> --\n> gitgitgadget\n>\n>\n"},{"id":"468778","messageId":"n2586428-1r80-9s29-8345-7p2opnor5086@tzk.qr","threadId":"58826","inReplyTo":"CAPig+cSfvgu8XjvmmAkFWe1G1VDRgrcx5GjUhr4xSDqoJ4cZOA@mail.gmail.com","subject":"Re: [PATCH 3/3] t1509: facilitate repeated script invocations","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-12-08T12:04:10Z","receivedAt":"2022-12-08T12:04:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 5 Dec 2022, Eric Sunshine wrote:\n\n> On Mon, Dec 5, 2022 at 9:48 PM Ævar Arnfjörð Bjarmason wrote:\n> > On Mon, Nov 21 2022, Eric Sunshine via GitGitGadget wrote:\n> > > t1509-root-work-tree.sh, which tests behavior of a Git repository\n> > > located at the root `/` directory, refuses to run if it detects the\n> > > presence of an existing repository at `/`. This safeguard ensures that\n> > > it won't clobber a legitimate repository at that location. However,\n> > > because t1509 does a poor job of cleaning up after itself, it runs afoul\n> > > of its own safety check on subsequent runs, which makes it painful to\n> > > run the script repeatedly since each run requires manual cleanup of\n> > > detritus from the previous run.\n> > >\n> > > Address this shortcoming by making t1509 clean up after itself as its\n> > > last action. This is safe since the script can only make it to this\n> > > cleanup action if it did not find a legitimate repository at `/` in the\n> > > first place, so the resources cleaned up here can only have been created\n> > > by the script itself.\n\nMakes sense.\n\n> > This is an existing wart, but I also wondered why the \"expected\",\n> > \"result\" etc. was needed. Either we could make the tests creating those\n> > do a \"test_when_finished\" removal of it, or better yet just create those\n> > in the trash directory.\n\nAn even better suggestion would be to use `test_atexit`, of course.\n\nCiao,\nJohannes\n"},{"id":"468779","messageId":"96178n12-4255-q093-qo51-r37n5o569s6p@tzk.qr","threadId":"58826","inReplyTo":"CAPig+cT6z5kzM8suwqxJ0wrzHjnj9ChROVBiQO3AR1rJ11pkNw@mail.gmail.com","subject":"Re: [PATCH 0/3] fix t1509-root-work-tree failure","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-12-08T12:10:04Z","receivedAt":"2022-12-08T12:10:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Mon, 5 Dec 2022, Eric Sunshine wrote:\n\n> On Sun, Nov 20, 2022 at 10:00 PM Eric Sunshine via GitGitGadget\n> wrote:\n> > The t1509-root-work-tree script started failing earlier this year but went\n> > unnoticed because the script is rarely run since it requires setting up a\n> > chroot environment or a sacrificial virtual machine. This patch series fixes\n> > the failure and makes it a bit easier to run the script repeatedly without\n> > it tripping over itself.\n> >\n> > Eric Sunshine (3):\n> > t1509: fix failing \"root work tree\" test due to owner-check\n> > t1509: make \"setup\" test more robust\n> > t1509: facilitate repeated script invocations\n>\n> Ping?\n\nThank you for the ping. I did not have much time for the Git mailing list\nas of late (too much Git for Windows stuff going on).\n\nThe patch series looks good to me, with or without the `test_atexit`\nchange I suggested.\n\nThank you,\nJohannes\n"},{"id":"468784","messageId":"221208.86fsdq6nci.gmgdl@evledraar.gmail.com","threadId":"58826","inReplyTo":"n2586428-1r80-9s29-8345-7p2opnor5086@tzk.qr","subject":"\"test_atexit\" v.s. \"test_when_finished\" (was: [PATCH 3/3] t1509: facilitate repeated script invocations)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-08T13:14:39Z","receivedAt":"2022-12-08T13:23:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Dec 08 2022, Johannes Schindelin wrote:\n\n> On Mon, 5 Dec 2022, Eric Sunshine wrote:\n>\n>> On Mon, Dec 5, 2022 at 9:48 PM Ævar Arnfjörð Bjarmason wrote:\n>> > On Mon, Nov 21 2022, Eric Sunshine via GitGitGadget wrote:\n>>> [...]\n>> > This is an existing wart, but I also wondered why the \"expected\",\n>> > \"result\" etc. was needed. Either we could make the tests creating those\n>> > do a \"test_when_finished\" removal of it, or better yet just create those\n>> > in the trash directory.\n>\n> An even better suggestion would be to use `test_atexit`, of course.\n\nWhy?\n\nFor assets that are only needed within a given test we prefer cleaning\nthem up with \"test_when_finished\", there's legitimate uses for\n\"test_atexit\", but those are for global state.\n\nIn this case (and again, we're discussing the #leftoverbits if someone\nwants to poke at this again) the tests in question could relatively\neasily be changed to do the creation and cleanup of the files that are\n\"test_cmp\"'d (or similar) within the lifetime of individual tests\n(\"test_when_finished\"), rather than the lifetime of the script\n(\"test_atexit\").\n\nA good reason for why we do it way is that it has a nice interaction\nwith \"--immediate --debug\".\n\nOn failure we'll skip the cleanup for the current test that just failed,\nbut we're not distracted by scratch files from earlier tests, those\nwould have already been cleaned up if they used the same\n\"test_when_finished\" pattern.\n\nIf you use \"test_atexit\" to do that all subsequent tests need to deal\nwith the sum of your scratch files, until they're cleaned up in one big\noperation at the end.\n\nIt not only makes that debugging case harder, but also to write tests,\nas you'll need to contend with more unwanted global state in your test\nplayground the further down the test file you are.\n\nSo I think what you're recommending here is an anti-pattern for the\ncommon case.\n\nThere *are* cases where we really do need the \"global cleanup\",\ne.g. tests that spawn the apache httpd use \"test_atexit\" rather than\n\"test_when_finished\", we don't want to have to start/stop the httpd for each test.\n\nWe should leave \"test_atexit\" for those sorts of cases, not routine\nper-test scratch file creation.\n\nI semi-regularly run into cases where a stale \"httpd\" is left running in\nthe background from such tests (and not after I kill -9'd a test), so I\nsuspect we also have tricky races in that are, that probably aren't\nimproved by \"test_atexit\".\n"},{"id":"468817","messageId":"xmqq5yel18xf.fsf@gitster.g","threadId":"58826","inReplyTo":"221208.86fsdq6nci.gmgdl@evledraar.gmail.com","subject":"Re: \"test_atexit\" v.s. \"test_when_finished\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-09T04:46:04Z","receivedAt":"2022-12-09T04:46:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On failure we'll skip the cleanup for the current test that just failed,\n> but we're not distracted by scratch files from earlier tests, those\n> would have already been cleaned up if they used the same\n> \"test_when_finished\" pattern.\n\nYup.\n\nA big benefit of using test_when_finished is that the knowledge of\nwhat cruft needs to be cleaned is isolated to the exact test piece\nthat would create the cruft. Instead of test_when_finished, We\ncould use the other convention to clear what others may have left\nbehind to give yourself a clean slate, but that requires you to be\naware of what other tests that came before you did, which will\nchange over time and will add to the maintenance burden. And to\nsome degree, the same downside is shared by the approach to use\ntest_atexit.\n\n"},{"id":"468818","messageId":"CAPig+cRDrX_3C6hzy=7NEVpKG8AtutQthge-AhyFm-xj6FKTqw@mail.gmail.com","threadId":"58826","inReplyTo":"96178n12-4255-q093-qo51-r37n5o569s6p@tzk.qr","subject":"Re: [PATCH 0/3] fix t1509-root-work-tree failure","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-12-09T04:59:09Z","receivedAt":"2022-12-09T04:59:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Dec 8, 2022 at 7:10 AM Johannes Schindelin\n wrote:\n> On Mon, 5 Dec 2022, Eric Sunshine wrote:\n> > On Sun, Nov 20, 2022 at 10:00 PM Eric Sunshine via GitGitGadget\n> > wrote:\n> > > The t1509-root-work-tree script started failing earlier this year but went\n> > > unnoticed because the script is rarely run since it requires setting up a\n> > > chroot environment or a sacrificial virtual machine. This patch series fixes\n> > > the failure and makes it a bit easier to run the script repeatedly without\n> > > it tripping over itself.\n> >\n> > Ping?\n>\n> Thank you for the ping. I did not have much time for the Git mailing list\n> as of late (too much Git for Windows stuff going on).\n>\n> The patch series looks good to me, with or without the `test_atexit`\n> change I suggested.\n\nThanks for looking over the series. Taking downstream discussion[1][2]\ninto account regarding test_atexit(), I think I'll let the series\nstand as-is.\n\nIn the long run, t1509 may need an overhaul, but the current series is\nintended to be the minimal possible change (patch [1/3]) to get the\nscript working again, plus very minor \"fixes\" (patches [2/3] and\n[3/3]) to make it a bit friendlier for the next person who has to\ndebug a failure in the script.\n\n[1]: https://lore.kernel.org/git/221208.86fsdq6nci.gmgdl@evledraar.gmail.com/\n[2]: https://lore.kernel.org/git/xmqq5yel18xf.fsf@gitster.g/\n"}]}