{"thread":{"id":"56673","subject":"[PATCH] test-lib.sh: try to re-chmod & retry on failed trash removal","startedAt":"2021-10-09T13:31:15Z","lastAt":"2021-10-12T23:03:24Z","messageCount":6,"participants":["Ævar Arnfjörð Bjarmason","SZEDER Gábor","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"438404","messageId":"patch-1.1-d7e88a77fef-20211009T133043Z-avarab@gmail.com","threadId":"56673","inReplyTo":null,"subject":"[PATCH] test-lib.sh: try to re-chmod & retry on failed trash removal","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-09T13:31:02Z","receivedAt":"2021-10-09T13:31:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This fixes a regression in [1] where t0004-unwritable.sh was made to\nuse \"test_when_finished\" for cleanup, we wouldn't re-chmod the\n\".git/objects\" on failure, leading to a persistent error when running\nthe test.\n\nThis can be demonstrated as e.g. (output snipped for less verbosity):\n\n    $ ./t0004-unwritable.sh --run=3 --immediate\n    ok 1 # skip setup (--run)\n    ok 2 # skip write-tree should notice unwritable repository (--run)\n    not ok 3 - commit should notice unwritable repository\n    [...]\n    $ ./t0004-unwritable.sh --run=3 --immediate\n    rm: cannot remove '[...]/trash directory.t0004-unwritable/.git/objects/info': Permission denied\n    FATAL: Cannot prepare test area\n    [...]\n\nI.e. if any of its tests failed, and the tests were being run under\n\"--debug\"[2] or \"--immediate\"[3] (which was introduced after [1]) we\nwouldn't re-chmod the object directory. We'd then fail on the next run\nsince the test setup couldn't remove the trash files.\n\nInstead of some version of reverting [1] let's make the test-lib.sh\nresilient to this edge-case, it will happen due to [1], but also\ne.g. if the relevant \"test-lib.sh\" process is kill -9'd during the\ntest run. We should try harder to recover in this case. If we fail to\nremove the test directory let's retry after (re-)chmod-ing it.\n\nThis doesn't need to be guarded by something that's equivalent to\n\"POSIXPERM\" since if we don't support \"chmod\" we were about to fail\nanyway. Let's also discard any error output from (a possibly\nnonexisting) \"chmod\", we'll fail on the subsequent \"rm -rf\"\nanyway.\n\nThe lack of &&-chaining between the \"chmod\" and \"rm -rf\" is\nintentional, if we fail the first \"rm -rf\", can't chmod, but then\nsucceed the second time around that's what we were hoping for. We just\nwant to nuke the directory, not carry forward every possible error\ncode or error message.\n\n1. dbda967684d (t0004 (unwritable files): simplify error handling,\n   2010-09-06)\n2. 5a4a088add3 (test-lib: do not remove trash_directory if called with\n   --debug, 2008-08-21)\n3. b586744a864 (test: skip clean-up when running under --immediate\n   mode, 2011-06-27)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/test-lib.sh | 18 +++++++++++++++---\n 1 file changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex aa1ad8180ed..706ce0cdeb9 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1210,10 +1210,10 @@ test_done () {\n \t\t\terror \"Tests passed but trash directory already removed before test cleanup; aborting\"\n \n \t\t\tcd \"$TRASH_DIRECTORY/..\" &&\n-\t\t\trm -fr \"$TRASH_DIRECTORY\" || {\n+\t\t\tremove_trash_directory \"$TRASH_DIRECTORY\" || {\n \t\t\t\t# try again in a bit\n \t\t\t\tsleep 5;\n-\t\t\t\trm -fr \"$TRASH_DIRECTORY\"\n+\t\t\t\tremove_trash_directory \"$TRASH_DIRECTORY\"\n \t\t\t} ||\n \t\t\terror \"Tests passed but test cleanup failed; aborting\"\n \t\tfi\n@@ -1370,6 +1370,18 @@ then\n \texit 1\n fi\n \n+# Try really hard to clean up our mess\n+remove_trash_directory() {\n+\tdir=\"$1\"\n+\tif ! rm -rf \"$dir\"\n+\tthen\n+\t\tsay_color info >&3 \"Failed to remove trash directory, trying to re-chmod it first...\"\n+\t\tchmod -R u+w \"$dir\" 2>/dev/null\n+\t\trm -rf \"$dir\"\n+\tfi\n+\t! test -d \"$dir\"\n+}\n+\n # Are we running this test at all?\n remove_trash=\n this_test=${0##*/}\n@@ -1388,7 +1400,7 @@ GNUPGHOME=\"$HOME/gnupg-home-not-used\"\n export HOME GNUPGHOME USER_HOME\n \n # Test repository\n-rm -fr \"$TRASH_DIRECTORY\" || {\n+remove_trash_directory \"$TRASH_DIRECTORY\" || {\n \tGIT_EXIT_OK=t\n \techo >&5 \"FATAL: Cannot prepare test area\"\n \texit 1\n-- \n2.33.0.1492.gcc3b81fc59a\n\n"},{"id":"438431","messageId":"20211010213643.GD571180@szeder.dev","threadId":"56673","inReplyTo":"patch-1.1-d7e88a77fef-20211009T133043Z-avarab@gmail.com","subject":"Re: [PATCH] test-lib.sh: try to re-chmod & retry on failed trash removal","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-10-10T21:36:43Z","receivedAt":"2021-10-10T21:36:48Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sat, Oct 09, 2021 at 03:31:02PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> This fixes a regression in [1] where t0004-unwritable.sh was made to\n> use \"test_when_finished\" for cleanup, we wouldn't re-chmod the\n> \".git/objects\" on failure, leading to a persistent error when running\n> the test.\n> \n> This can be demonstrated as e.g. (output snipped for less verbosity):\n> \n>     $ ./t0004-unwritable.sh --run=3 --immediate\n>     ok 1 # skip setup (--run)\n>     ok 2 # skip write-tree should notice unwritable repository (--run)\n>     not ok 3 - commit should notice unwritable repository\n>     [...]\n>     $ ./t0004-unwritable.sh --run=3 --immediate\n>     rm: cannot remove '[...]/trash directory.t0004-unwritable/.git/objects/info': Permission denied\n>     FATAL: Cannot prepare test area\n>     [...]\n> \n> I.e. if any of its tests failed, and the tests were being run under\n> \"--debug\"[2] or \"--immediate\"[3] (which was introduced after [1]) we\n\n'--debug' runs the tests as usual, including any 'test_when_finished'\ncommands, so I don't see how it would be affected.  And indeed running\n'./t0004-unwritable.sh --run=3 --debug' repeatedly doesn't have any\nissues with removing the trash directory of the previous run.\n\n> wouldn't re-chmod the object directory. We'd then fail on the next run\n> since the test setup couldn't remove the trash files.\n> \n> Instead of some version of reverting [1] let's make the test-lib.sh\n> resilient to this edge-case, it will happen due to [1], but also\n> e.g. if the relevant \"test-lib.sh\" process is kill -9'd during the\n> test run. We should try harder to recover in this case. If we fail to\n> remove the test directory let's retry after (re-)chmod-ing it.\n> \n> This doesn't need to be guarded by something that's equivalent to\n> \"POSIXPERM\" since if we don't support \"chmod\" we were about to fail\n> anyway. Let's also discard any error output from (a possibly\n> nonexisting) \"chmod\", we'll fail on the subsequent \"rm -rf\"\n> anyway.\n> \n> The lack of &&-chaining between the \"chmod\" and \"rm -rf\" is\n> intentional, if we fail the first \"rm -rf\", can't chmod, but then\n> succeed the second time around that's what we were hoping for. We just\n> want to nuke the directory, not carry forward every possible error\n> code or error message.\n> \n> 1. dbda967684d (t0004 (unwritable files): simplify error handling,\n>    2010-09-06)\n> 2. 5a4a088add3 (test-lib: do not remove trash_directory if called with\n>    --debug, 2008-08-21)\n> 3. b586744a864 (test: skip clean-up when running under --immediate\n>    mode, 2011-06-27)\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  t/test-lib.sh | 18 +++++++++++++++---\n>  1 file changed, 15 insertions(+), 3 deletions(-)\n> \n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index aa1ad8180ed..706ce0cdeb9 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1210,10 +1210,10 @@ test_done () {\n>  \t\t\terror \"Tests passed but trash directory already removed before test cleanup; aborting\"\n>  \n>  \t\t\tcd \"$TRASH_DIRECTORY/..\" &&\n> -\t\t\trm -fr \"$TRASH_DIRECTORY\" || {\n> +\t\t\tremove_trash_directory \"$TRASH_DIRECTORY\" || {\n>  \t\t\t\t# try again in a bit\n>  \t\t\t\tsleep 5;\n> -\t\t\t\trm -fr \"$TRASH_DIRECTORY\"\n> +\t\t\t\tremove_trash_directory \"$TRASH_DIRECTORY\"\n>  \t\t\t} ||\n>  \t\t\terror \"Tests passed but test cleanup failed; aborting\"\n>  \t\tfi\n> @@ -1370,6 +1370,18 @@ then\n>  \texit 1\n>  fi\n>  \n> +# Try really hard to clean up our mess\n> +remove_trash_directory() {\n> +\tdir=\"$1\"\n> +\tif ! rm -rf \"$dir\"\n> +\tthen\n> +\t\tsay_color info >&3 \"Failed to remove trash directory, trying to re-chmod it first...\"\n\nIs this message really necessary?  I don't think it is, and there was\nno similar message in the previous hunk around that 'sleep 5' either.\n\n> +\t\tchmod -R u+w \"$dir\" 2>/dev/null\n> +\t\trm -rf \"$dir\"\n> +\tfi\n> +\t! test -d \"$dir\"\n> +}\n> +\n>  # Are we running this test at all?\n>  remove_trash=\n>  this_test=${0##*/}\n> @@ -1388,7 +1400,7 @@ GNUPGHOME=\"$HOME/gnupg-home-not-used\"\n>  export HOME GNUPGHOME USER_HOME\n>  \n>  # Test repository\n> -rm -fr \"$TRASH_DIRECTORY\" || {\n> +remove_trash_directory \"$TRASH_DIRECTORY\" || {\n>  \tGIT_EXIT_OK=t\n>  \techo >&5 \"FATAL: Cannot prepare test area\"\n>  \texit 1\n> -- \n> 2.33.0.1492.gcc3b81fc59a\n> \n"},{"id":"438434","messageId":"xmqqee8seded.fsf@gitster.g","threadId":"56673","inReplyTo":"patch-1.1-d7e88a77fef-20211009T133043Z-avarab@gmail.com","subject":"Re: [PATCH] test-lib.sh: try to re-chmod & retry on failed trash removal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-10T22:14:02Z","receivedAt":"2021-10-10T22:14:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> This can be demonstrated as e.g. (output snipped for less verbosity):\n>\n>     $ ./t0004-unwritable.sh --run=3 --immediate\n>     ok 1 # skip setup (--run)\n>     ok 2 # skip write-tree should notice unwritable repository (--run)\n>     not ok 3 - commit should notice unwritable repository\n>     [...]\n>     $ ./t0004-unwritable.sh --run=3 --immediate\n>     rm: cannot remove '[...]/trash directory.t0004-unwritable/.git/objects/info': Permission denied\n>     FATAL: Cannot prepare test area\n>     [...]\n>\n> I.e. if any of its tests failed, and the tests were being run under\n> \"--debug\"[2] or \"--immediate\"[3] (which was introduced after [1]) we\n> wouldn't re-chmod the object directory. We'd then fail on the next run\n> since the test setup couldn't remove the trash files.\n\nOK, the first thing these scripts do is to create $TRASH_DIRECTORY\nfor their own use, and that is what fails (hence \"Cannot prepare\").\n\nAnd making sure we try harder to prepare would be a good thing.\nGiven that understanding of the motivation... \n\n> @@ -1210,10 +1210,10 @@ test_done () {\n\n...it is puzzling why we are touching the code that happens _after_\na test is run.  If our philosophy is to leave the mess after a test\nfails so that the debugging person can take a look without\ncontaminating the forensics data, but blindly and forcibly clean up\nbefore starting a new round, reusing the \"do more than usual to\nremove\" logic meant to be used for the latter here feels counter\nproductive and also a bit counter intuitive.\n\n>  \t\t\terror \"Tests passed but trash directory already removed before test cleanup; aborting\"\n>  \n>  \t\t\tcd \"$TRASH_DIRECTORY/..\" &&\n> -\t\t\trm -fr \"$TRASH_DIRECTORY\" || {\n> +\t\t\tremove_trash_directory \"$TRASH_DIRECTORY\" || {\n>  \t\t\t\t# try again in a bit\n>  \t\t\t\tsleep 5;\n> -\t\t\t\trm -fr \"$TRASH_DIRECTORY\"\n> +\t\t\t\tremove_trash_directory \"$TRASH_DIRECTORY\"\n>  \t\t\t} ||\n>  \t\t\terror \"Tests passed but test cleanup failed; aborting\"\n>  \t\tfi\n\n\n\n> +# Try really hard to clean up our mess\n> +remove_trash_directory() {\n> +\tdir=\"$1\"\n> +\tif ! rm -rf \"$dir\"\n> +\tthen\n> +\t\tsay_color info >&3 \"Failed to remove trash directory, trying to re-chmod it first...\"\n> +\t\tchmod -R u+w \"$dir\" 2>/dev/null\n\nIf a test lost searchable bit from directories, \"u+wx\" may be\nnecessary to clean fully, no?\n\n> +\t\trm -rf \"$dir\"\n> +\tfi\n> +\t! test -d \"$dir\"\n> +}\n> +\n\nOn the other hand...\n\n>  # Are we running this test at all?\n>  remove_trash=\n>  this_test=${0##*/}\n> @@ -1388,7 +1400,7 @@ GNUPGHOME=\"$HOME/gnupg-home-not-used\"\n>  export HOME GNUPGHOME USER_HOME\n>  \n>  # Test repository\n> -rm -fr \"$TRASH_DIRECTORY\" || {\n> +remove_trash_directory \"$TRASH_DIRECTORY\" || {\n\n... this one does make sense and matches what the proposed log\nmessage sold us the change as.\n\n>  \tGIT_EXIT_OK=t\n>  \techo >&5 \"FATAL: Cannot prepare test area\"\n>  \texit 1\n\nThanks.\n"},{"id":"438435","messageId":"patch-v2-1.1-9cfc0621067-20211011T014059Z-avarab@gmail.com","threadId":"56673","inReplyTo":"patch-1.1-d7e88a77fef-20211009T133043Z-avarab@gmail.com","subject":"[PATCH v2] test-lib.sh: try to re-chmod & retry on failed trash removal","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-11T01:41:59Z","receivedAt":"2021-10-11T01:42:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Try to re-chmod the trash directory on startup if we fail to \"rm -rf\"\nit. This fixes problems where the test leaves the trash directory\nbehind in a bad permission state for whatever reason.\n\nThis fixes an interaction between [1] where t0004-unwritable.sh was\nmade to use \"test_when_finished\" for cleanup, and [2] which added the\n\"--immediate\" mode. If a test in this file failed when running with\n\"--immediate\" we wouldn't run the \"test_when_finished\" block, which\nre-chmods the \".git/objects\" directory (see [1]).\n\nThis can be demonstrated as e.g. (output snipped for less verbosity):\n\n    $ ./t0004-unwritable.sh --run=3 --immediate\n    ok 1 # skip setup (--run)\n    ok 2 # skip write-tree should notice unwritable repository (--run)\n    not ok 3 - commit should notice unwritable repository\n    [...]\n    $ ./t0004-unwritable.sh --run=3 --immediate\n    rm: cannot remove '[...]/trash directory.t0004-unwritable/.git/objects/info': Permission denied\n    FATAL: Cannot prepare test area\n    [...]\n\nInstead of some version of reverting [1] let's make the test-lib.sh\nresilient to this edge-case, it will happen due to [1], but also\ne.g. if the relevant \"test-lib.sh\" process is kill -9'd during the\ntest run. We should try harder to recover in this case. If we fail to\nremove the test directory let's retry after (re-)chmod-ing it.\n\nThis doesn't need to be guarded by something that's equivalent to\n\"POSIXPERM\" since if we don't support \"chmod\" we were about to fail\nanyway.\n\nLet's also discard any error output from (a possibly nonexisting)\n\"chmod\", we'll fail on the subsequent \"rm -rf\" anyway, likewise for\nthe first \"rm -rf\" invocation, we don't want to get the \"cannot\nremove\" output if we can get around it with the \"chmod\", but we do\nwant any error output from the second \"rm -rf\", in case that doesn't\nfix the issue.\n\nThe lack of &&-chaining between the \"chmod\" and \"rm -rf\" is\nintentional, if we fail the first \"rm -rf\", can't chmod, but then\nsucceed the second time around that's what we were hoping for. We just\nwant to nuke the directory, not carry forward every possible error\ncode or error message.\n\n1. dbda967684d (t0004 (unwritable files): simplify error handling,\n   2010-09-06)\n2. b586744a864 (test: skip clean-up when running under --immediate\n   mode, 2011-06-27)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\nRange-diff against v1:\n1:  d7e88a77fef ! 1:  9cfc0621067 test-lib.sh: try to re-chmod & retry on failed trash removal\n    @@ Metadata\n      ## Commit message ##\n         test-lib.sh: try to re-chmod & retry on failed trash removal\n     \n    -    This fixes a regression in [1] where t0004-unwritable.sh was made to\n    -    use \"test_when_finished\" for cleanup, we wouldn't re-chmod the\n    -    \".git/objects\" on failure, leading to a persistent error when running\n    -    the test.\n    +    Try to re-chmod the trash directory on startup if we fail to \"rm -rf\"\n    +    it. This fixes problems where the test leaves the trash directory\n    +    behind in a bad permission state for whatever reason.\n    +\n    +    This fixes an interaction between [1] where t0004-unwritable.sh was\n    +    made to use \"test_when_finished\" for cleanup, and [2] which added the\n    +    \"--immediate\" mode. If a test in this file failed when running with\n    +    \"--immediate\" we wouldn't run the \"test_when_finished\" block, which\n    +    re-chmods the \".git/objects\" directory (see [1]).\n     \n         This can be demonstrated as e.g. (output snipped for less verbosity):\n     \n    @@ Commit message\n             FATAL: Cannot prepare test area\n             [...]\n     \n    -    I.e. if any of its tests failed, and the tests were being run under\n    -    \"--debug\"[2] or \"--immediate\"[3] (which was introduced after [1]) we\n    -    wouldn't re-chmod the object directory. We'd then fail on the next run\n    -    since the test setup couldn't remove the trash files.\n    -\n         Instead of some version of reverting [1] let's make the test-lib.sh\n         resilient to this edge-case, it will happen due to [1], but also\n         e.g. if the relevant \"test-lib.sh\" process is kill -9'd during the\n    @@ Commit message\n     \n         This doesn't need to be guarded by something that's equivalent to\n         \"POSIXPERM\" since if we don't support \"chmod\" we were about to fail\n    -    anyway. Let's also discard any error output from (a possibly\n    -    nonexisting) \"chmod\", we'll fail on the subsequent \"rm -rf\"\n         anyway.\n     \n    +    Let's also discard any error output from (a possibly nonexisting)\n    +    \"chmod\", we'll fail on the subsequent \"rm -rf\" anyway, likewise for\n    +    the first \"rm -rf\" invocation, we don't want to get the \"cannot\n    +    remove\" output if we can get around it with the \"chmod\", but we do\n    +    want any error output from the second \"rm -rf\", in case that doesn't\n    +    fix the issue.\n    +\n         The lack of &&-chaining between the \"chmod\" and \"rm -rf\" is\n         intentional, if we fail the first \"rm -rf\", can't chmod, but then\n         succeed the second time around that's what we were hoping for. We just\n    @@ Commit message\n     \n         1. dbda967684d (t0004 (unwritable files): simplify error handling,\n            2010-09-06)\n    -    2. 5a4a088add3 (test-lib: do not remove trash_directory if called with\n    -       --debug, 2008-08-21)\n    -    3. b586744a864 (test: skip clean-up when running under --immediate\n    +    2. b586744a864 (test: skip clean-up when running under --immediate\n            mode, 2011-06-27)\n     \n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## t/test-lib.sh ##\n    -@@ t/test-lib.sh: test_done () {\n    - \t\t\terror \"Tests passed but trash directory already removed before test cleanup; aborting\"\n    - \n    - \t\t\tcd \"$TRASH_DIRECTORY/..\" &&\n    --\t\t\trm -fr \"$TRASH_DIRECTORY\" || {\n    -+\t\t\tremove_trash_directory \"$TRASH_DIRECTORY\" || {\n    - \t\t\t\t# try again in a bit\n    - \t\t\t\tsleep 5;\n    --\t\t\t\trm -fr \"$TRASH_DIRECTORY\"\n    -+\t\t\t\tremove_trash_directory \"$TRASH_DIRECTORY\"\n    - \t\t\t} ||\n    - \t\t\terror \"Tests passed but test cleanup failed; aborting\"\n    - \t\tfi\n    -@@ t/test-lib.sh: then\n    - \texit 1\n    - fi\n    +@@ t/test-lib.sh: HOME=\"$TRASH_DIRECTORY\"\n    + GNUPGHOME=\"$HOME/gnupg-home-not-used\"\n    + export HOME GNUPGHOME USER_HOME\n      \n    -+# Try really hard to clean up our mess\n    ++# \"rm -rf\" existing trash directory, even if a previous run left it\n    ++# with bad permissions.\n     +remove_trash_directory() {\n     +\tdir=\"$1\"\n    -+\tif ! rm -rf \"$dir\"\n    ++\tif ! rm -rf \"$dir\" 2>/dev/null\n     +\tthen\n    -+\t\tsay_color info >&3 \"Failed to remove trash directory, trying to re-chmod it first...\"\n    -+\t\tchmod -R u+w \"$dir\" 2>/dev/null\n    ++\t\tchmod -R u+wx \"$dir\"\n     +\t\trm -rf \"$dir\"\n     +\tfi\n     +\t! test -d \"$dir\"\n     +}\n     +\n    - # Are we running this test at all?\n    - remove_trash=\n    - this_test=${0##*/}\n    -@@ t/test-lib.sh: GNUPGHOME=\"$HOME/gnupg-home-not-used\"\n    - export HOME GNUPGHOME USER_HOME\n    - \n      # Test repository\n     -rm -fr \"$TRASH_DIRECTORY\" || {\n     +remove_trash_directory \"$TRASH_DIRECTORY\" || {\n\n t/test-lib.sh | 14 +++++++++++++-\n 1 file changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex aa1ad8180ed..ff6d3c79362 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1387,8 +1387,20 @@ HOME=\"$TRASH_DIRECTORY\"\n GNUPGHOME=\"$HOME/gnupg-home-not-used\"\n export HOME GNUPGHOME USER_HOME\n \n+# \"rm -rf\" existing trash directory, even if a previous run left it\n+# with bad permissions.\n+remove_trash_directory() {\n+\tdir=\"$1\"\n+\tif ! rm -rf \"$dir\" 2>/dev/null\n+\tthen\n+\t\tchmod -R u+wx \"$dir\"\n+\t\trm -rf \"$dir\"\n+\tfi\n+\t! test -d \"$dir\"\n+}\n+\n # Test repository\n-rm -fr \"$TRASH_DIRECTORY\" || {\n+remove_trash_directory \"$TRASH_DIRECTORY\" || {\n \tGIT_EXIT_OK=t\n \techo >&5 \"FATAL: Cannot prepare test area\"\n \texit 1\n-- \n2.33.0.1505.g38ce83107ad\n\n"},{"id":"438442","messageId":"xmqqa6jgdvrk.fsf@gitster.g","threadId":"56673","inReplyTo":"xmqqee8seded.fsf@gitster.g","subject":"Re: [PATCH] test-lib.sh: try to re-chmod & retry on failed trash removal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-11T04:34:55Z","receivedAt":"2021-10-11T04:35:10Z","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>> +\t\tchmod -R u+w \"$dir\" 2>/dev/null\n>\n> If a test lost searchable bit from directories, \"u+wx\" may be\n> necessary to clean fully, no?\n\nSorry, but I was stupid.\n\nIf we were to worry about losing the searchable bit, then I should\nconsidered the possibility that we may also lose the readable bit,\ntoo.\n\n"},{"id":"438617","messageId":"xmqq5yu13kxz.fsf@gitster.g","threadId":"56673","inReplyTo":"patch-v2-1.1-9cfc0621067-20211011T014059Z-avarab@gmail.com","subject":"Re: [PATCH v2] test-lib.sh: try to re-chmod & retry on failed trash removal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-12T23:03:20Z","receivedAt":"2021-10-12T23:03:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mostly looking good, but I applied the following on top to be\nsquashed in.\n\nThanks.\n\n-- >8 --\nStyle fix plus adding more paranoia to cope with the lost 'r' bit.\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 138a26c19b..d820910154 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1386,11 +1386,11 @@ export HOME GNUPGHOME\n \n # \"rm -rf\" existing trash directory, even if a previous run left it\n # with bad permissions.\n-remove_trash_directory() {\n+remove_trash_directory () {\n \tdir=\"$1\"\n \tif ! rm -rf \"$dir\" 2>/dev/null\n \tthen\n-\t\tchmod -R u+wx \"$dir\"\n+\t\tchmod -R u+rwx \"$dir\"\n \t\trm -rf \"$dir\"\n \tfi\n \t! test -d \"$dir\"\n"}]}