Re: [PATCH] test-lib: abort when can't remove trash directory

[Junio C Hamano]

Jeff King  writes:

> Sort of. It still has the bug that it dies with error() when "--debug"
> is used.

Ah, I forgot about that one.  Let me squash it in without the
remove_trash change then, perhaps, but not today.

Re: [PATCH] test-lib: abort when can't remove trash directory

[Jeff King]

On Mon, Apr 24, 2017 at 11:05:15PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Good point. There's only one caller, but it does care about being in
> > that directory.
> >
> >> Second try that hopefully is much less damaging
> 
> I've been carrying it as a SQUASH??? patch, but I think it is better
> to split it as a separate pach, as removal of $remove_trash is an
> optional thing.  The main thing, i.e. SZEDER's "abort when trash is
> already gone or when we cannot remove" _can_ be (and is) correctly
> done with his pach alone.

Sort of. It still has the bug that it dies with error() when "--debug"
is used. But that is relatively minor, and as long as yours gets applied
on top I do not mind the momentary breakage.

> So here is what I queued on top.
> 
> -- >8 --
> Subject: [PATCH] test-lib: retire $remove_trash variable

Yeah, this looks fine (though it could mention that it is fixing the
"test -d" bug).

-Peff

Re: [PATCH] test-lib: abort when can't remove trash directory

[Junio C Hamano]

Jeff King  writes:

> Good point. There's only one caller, but it does care about being in
> that directory.
>
>> Second try that hopefully is much less damaging

I've been carrying it as a SQUASH??? patch, but I think it is better
to split it as a separate pach, as removal of $remove_trash is an
optional thing.  The main thing, i.e. SZEDER's "abort when trash is
already gone or when we cannot remove" _can_ be (and is) correctly
done with his pach alone.

So here is what I queued on top.

-- >8 --
Subject: [PATCH] test-lib: retire $remove_trash variable

The convention "$remove_trash is set to the trash directory that is
used during the test, so that it will be removed at the end, but
under --debug option we set the varilable to empty string to
preserve the directory" made sense back when it was introduced, as
there was no $TRASH_DIRECTORY variable.  These days, since no tests
looks at the variable, it is obscure and even risks that by mistake
the variable gets used for something else (e.g. remove_trash=yes)
and cause us misbehave.

Rewrite the clean-up sequence in test_done helper to explicitly
check the $debug condition and remove the trash directory using
the $TRASH_DIRECTORY variable.

Note that "go to the directory one level above the trash and then
remove it" is kept and this is deliverate; test_at_end_hook_ will
keep running from the expected location, and also some platforms may
not like a directory that is serving as the $cwd of a still-active
process removed.

We _might_ want to rewrite

cd "$(dirname "$TRASH_DIRECTORY")"

further to

cd "$TRASH_DIRECTORY/.."

to lose one extra process, but let's leave it to a later patch.

Signed-off-by: Junio C Hamano 
---
 t/test-lib.sh | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index cb0766b9ee..976566047d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -760,12 +760,15 @@ test_done () {
say "1..$test_count$skip_all"
fi
 
-   test -d "$remove_trash" ||
-   error "Tests passed but trash directory already removed before 
test cleanup; aborting"
+   if test -z "$debug"
+   then
+   test -d "$TRASH_DIRECTORY" ||
+   error "Tests passed but trash directory already removed 
before test cleanup; aborting"
 
-   cd "$(dirname "$remove_trash")" &&
-   rm -rf "$(basename "$remove_trash")" ||
-   error "Tests passed but test cleanup failed; aborting"
+   cd "$(dirname "$TRASH_DIRECTORY")" &&
+   rm -fr "$TRASH_DIRECTORY" ||
+   error "Tests passed but test cleanup failed; aborting"
+   fi
 
test_at_end_hook_
 
@@ -921,7 +924,6 @@ case "$TRASH_DIRECTORY" in
 /*) ;; # absolute path is good
  *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
 esac
-test ! -z "$debug" || remove_trash=$TRASH_DIRECTORY
 rm -fr "$TRASH_DIRECTORY" || {
GIT_EXIT_OK=t
echo >&5 "FATAL: Cannot prepare test area"
-- 
2.13.0-rc0-308-g931de5db53

Re: [PATCH] test-lib: abort when can't remove trash directory

[Junio C Hamano]

Torsten Bögershausen  writes:

> []
>>>
>>> -   cd "$(dirname "$remove_trash")" &&
>>> -   rm -rf "$(basename "$remove_trash")" ||
>>> -   error "Tests passed but test cleanup failed; aborting"
>>> +   cd "$(dirname "$TRASH_DIRECTORY")" &&
>>> +   rm -fr "$TRASH_DIRECTORY" ||
>>> +   error "Tests passed but test cleanup failed; aborting"
>>> +   fi
>>
>> Yeah, that looks good to me.
>
> Does it ?
> Checking the error code of "rm -f" doesn't work as expected from the script:
> rm -rf DoesNotExist ; echo $?
> 0
>
> I think it should be
>
>>> +   cd "$(dirname "$TRASH_DIRECTORY")" &&
>>> +   rm -r "$TRASH_DIRECTORY" ||
>>> +   error "Tests passed but test cleanup failed; aborting"

What Peff told you in his response is all correct, but besides, you
can see the patch and realize that the original has been using "rm
-rf" for ages.

This change is about not using $remove_trash variable, as it felt
brittle and error prone than detecting $debug case and removing
$TRASH_DIRECTORY.  So in that sense, I shouldn't have even rolled
the removal of basename into it.

Having said that, because people care about the number of subprocess
invocations, I am tempted to suggest doing

cd "$TRASH_DIRECTORY/.." &&
rm -fr "$TRASH_DIRECTORY" ||
error ...

which should reduce another process ;-)

Re: [PATCH] test-lib: abort when can't remove trash directory

[Jeff King]

On Mon, Apr 24, 2017 at 11:39:26AM +0200, Torsten Bögershausen wrote:

> []
> > > 
> > > - cd "$(dirname "$remove_trash")" &&
> > > - rm -rf "$(basename "$remove_trash")" ||
> > > - error "Tests passed but test cleanup failed; aborting"
> > > + cd "$(dirname "$TRASH_DIRECTORY")" &&
> > > + rm -fr "$TRASH_DIRECTORY" ||
> > > + error "Tests passed but test cleanup failed; aborting"
> > > + fi
> > 
> > Yeah, that looks good to me.
> 
> Does it ?
> Checking the error code of "rm -f" doesn't work as expected from the script:
> rm -rf DoesNotExist ; echo $?
> 0

ENOENT is treated specially by "rm -f". We cover that case explicitly
with the "test -d" above (in the bit you didn't quote), and then rely on
"rm" to report other errors.  Like:

  $ rm -rf /etc/passwd ; echo $?
  rm: cannot remove '/etc/passwd': Permission denied
  1

> I think it should be
> 
> > > + cd "$(dirname "$TRASH_DIRECTORY")" &&
> > > + rm -r "$TRASH_DIRECTORY" ||
> > > + error "Tests passed but test cleanup failed; aborting"

I think we need the "-f" to overcome rm's tendency to prompt in some
cases:

  $ echo content >foo
  $ chmod 0 foo
  $ rm foo
  rm: remove write-protected regular file 'foo'? ^C
  $ rm -f foo
  [no output; it just works]

-Peff

Re: [PATCH] test-lib: abort when can't remove trash directory

[Torsten Bögershausen]

[]


-   cd "$(dirname "$remove_trash")" &&
-   rm -rf "$(basename "$remove_trash")" ||
-   error "Tests passed but test cleanup failed; aborting"
+   cd "$(dirname "$TRASH_DIRECTORY")" &&
+   rm -fr "$TRASH_DIRECTORY" ||
+   error "Tests passed but test cleanup failed; aborting"
+   fi


Yeah, that looks good to me.


Does it ?
Checking the error code of "rm -f" doesn't work as expected from the script:
rm -rf DoesNotExist ; echo $?
0

I think it should be


+   cd "$(dirname "$TRASH_DIRECTORY")" &&
+   rm -r "$TRASH_DIRECTORY" ||
+   error "Tests passed but test cleanup failed; aborting"

Re: [PATCH] test-lib: abort when can't remove trash directory

[Jeff King]

On Sun, Apr 23, 2017 at 09:02:41PM -0700, Junio C Hamano wrote:

> >> That looks fine, assuming the answer to the "is the cwd important"
> >> question is "no".
> >
> > And I do think the answer would be "yes", unfortunately.  There are
> > systems that do not even allow a file to be removed while it is
> > open, so...
> 
> In addition to "some platforms may not want cwd removed", this
> directory would be where test_at_end_hook_ will be running in, so
> we'd better be consistent with the current behaviour.

Good point. There's only one caller, but it does care about being in
that directory.

> Second try that hopefully is much less damaging
> 
>  t/test-lib.sh | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index cb0766b9ee..4e8f511870 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -760,12 +760,15 @@ test_done () {
>   say "1..$test_count$skip_all"
>   fi
>  
> - test -d "$remove_trash" ||
> - error "Tests passed but trash directory already removed before 
> test cleanup; aborting"
> + if test -z "$debug"
> + then
> + test -d "$TRASH_DIRECTORY" ||
> + error "Tests passed but trash directory already removed 
> before test cleanup; aborting"
>  
> - cd "$(dirname "$remove_trash")" &&
> - rm -rf "$(basename "$remove_trash")" ||
> - error "Tests passed but test cleanup failed; aborting"
> + cd "$(dirname "$TRASH_DIRECTORY")" &&
> + rm -fr "$TRASH_DIRECTORY" ||
> + error "Tests passed but test cleanup failed; aborting"
> + fi

Yeah, that looks good to me.

-Peff

Re: [PATCH] test-lib: abort when can't remove trash directory

[Junio C Hamano]

Junio C Hamano  writes:

>> That looks fine, assuming the answer to the "is the cwd important"
>> question is "no".
>
> And I do think the answer would be "yes", unfortunately.  There are
> systems that do not even allow a file to be removed while it is
> open, so...

In addition to "some platforms may not want cwd removed", this
directory would be where test_at_end_hook_ will be running in, so
we'd better be consistent with the current behaviour.

Second try that hopefully is much less damaging

 t/test-lib.sh | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index cb0766b9ee..4e8f511870 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -760,12 +760,15 @@ test_done () {
say "1..$test_count$skip_all"
fi
 
-   test -d "$remove_trash" ||
-   error "Tests passed but trash directory already removed before 
test cleanup; aborting"
+   if test -z "$debug"
+   then
+   test -d "$TRASH_DIRECTORY" ||
+   error "Tests passed but trash directory already removed 
before test cleanup; aborting"
 
-   cd "$(dirname "$remove_trash")" &&
-   rm -rf "$(basename "$remove_trash")" ||
-   error "Tests passed but test cleanup failed; aborting"
+   cd "$(dirname "$TRASH_DIRECTORY")" &&
+   rm -fr "$TRASH_DIRECTORY" ||
+   error "Tests passed but test cleanup failed; aborting"
+   fi
 
test_at_end_hook_
 

Re: [PATCH] test-lib: abort when can't remove trash directory

[Junio C Hamano]

Jeff King  writes:

> On Sun, Apr 23, 2017 at 05:14:54PM -0700, Junio C Hamano wrote:
>
>> OK.  I am wondering why we do not do 
>> 
>>  rm -fr "$TRASH_DIRECTORY"
>> 
>> and do this instead:
>> 
>>  cd "$(dirname "$remove_trash")" &&
>>  rm -rf "$(basename "$remove_trash")"
>> 
>> in the original.  It feels somewhat unnatural.
>
> I assumed the "cd" was there because some systems may be unhappy
> removing a directory which is our current working directory. That might
> just be superstition, though.

Ahh, OK, that makes sense.  I forgot about that.

> The use of "basename" in the second does seem superfluous, since the
> trash directory should be the full path (I suspect it wasn't in the
> early days, though).
>
>> So perhaps we can simplify and make it more robust by doing this?
>> [...]
>> +if test -z "$debug"
>> +then
>> +test -d "$TRASH_DIRECTORY" ||
>> +error "Tests passed but trash directory already removed 
>> before test cleanup; aborting"
>> +
>> +rm -fr "$TRASH_DIRECTORY" ||
>> +error "Tests passed but test cleanup failed; aborting"
>> +fi
>
> That looks fine, assuming the answer to the "is the cwd important"
> question is "no".

And I do think the answer would be "yes", unfortunately.  There are
systems that do not even allow a file to be removed while it is
open, so...

Re: [PATCH] test-lib: abort when can't remove trash directory

[Jeff King]

On Sun, Apr 23, 2017 at 05:14:54PM -0700, Junio C Hamano wrote:

> OK.  I am wondering why we do not do 
> 
>   rm -fr "$TRASH_DIRECTORY"
> 
> and do this instead:
> 
>   cd "$(dirname "$remove_trash")" &&
>   rm -rf "$(basename "$remove_trash")"
> 
> in the original.  It feels somewhat unnatural.

I assumed the "cd" was there because some systems may be unhappy
removing a directory which is our current working directory. That might
just be superstition, though.

The use of "basename" in the second does seem superfluous, since the
trash directory should be the full path (I suspect it wasn't in the
early days, though).

> So perhaps we can simplify and make it more robust by doing this?
> [...]
> + if test -z "$debug"
> + then
> + test -d "$TRASH_DIRECTORY" ||
> + error "Tests passed but trash directory already removed 
> before test cleanup; aborting"
> +
> + rm -fr "$TRASH_DIRECTORY" ||
> + error "Tests passed but test cleanup failed; aborting"
> + fi

That looks fine, assuming the answer to the "is the cwd important"
question is "no".

-Peff

Re: [PATCH] test-lib: abort when can't remove trash directory

[Junio C Hamano]

Jeff King  writes:

>> -test -d "$remove_trash" &&
>> +test -d "$remove_trash" ||
>> +error "Tests passed but trash directory already removed before 
>> test cleanup; aborting"
>
> I think I found out why this "test -d" was here in the first place:
>
>   $ ./t-basic.sh --debug
>   [...]
>   # passed all 77 test(s)
>   1..77
>   error: Tests passed but trash directory already removed before test 
> cleanup; aborting
>
> When --debug is in use, we do not set $remove_trash. The original was
> relying on 'test -d ""' to return false.
>
> I think this whole removal block should probably be moved inside a
> conditional like:
>
>   if test -n "$remove_trash"
>   then
>  ...
>   fi

Thanks for digging.  Yes, checking for non-empty string is
definitely better.

> I also wonder if we should come up with a better name than
> $remove_trash. A script which unknowingly overwrites that variable would
> be disastrous.
>
> Perhaps we should drop it entirely and just do:
>
>   if test -z "$debug"
>   then
>  test -d "$TRASH_DIRECTORY" ||
>  error "Tests passed but..."
>   [and so forth...]
>   fi

OK.  I am wondering why we do not do 

rm -fr "$TRASH_DIRECTORY"

and do this instead:

cd "$(dirname "$remove_trash")" &&
rm -rf "$(basename "$remove_trash")"

in the original.  It feels somewhat unnatural.

... goes and looks ...

Of course, back when abc5d372 ("Enable parallel tests", 2008-08-08)
was writen, we didn't even have TRASH_DIRECTORY variable; because
the way the test-lib.sh ensured that the trash directory is prestine
was to first do a 'rm -fr "$test"' before the first test_create_repo,
the above makes sort of matches how that initial removal is done.

So perhaps we can simplify and make it more robust by doing this?

 t/test-lib.sh | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index cde7fc7fcf..f1ab8f33d9 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -760,9 +760,14 @@ test_done () {
say "1..$test_count$skip_all"
fi
 
-   test -d "$remove_trash" &&
-   cd "$(dirname "$remove_trash")" &&
-   rm -rf "$(basename "$remove_trash")"
+   if test -z "$debug"
+   then
+   test -d "$TRASH_DIRECTORY" ||
+   error "Tests passed but trash directory already removed 
before test cleanup; aborting"
+
+   rm -fr "$TRASH_DIRECTORY" ||
+   error "Tests passed but test cleanup failed; aborting"
+   fi
 
test_at_end_hook_
 

Re: [PATCH] test-lib: abort when can't remove trash directory

[Jeff King]

On Thu, Apr 20, 2017 at 06:52:30PM +0200, SZEDER Gábor wrote:

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 13b569682..e9e6f677d 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -761,9 +761,12 @@ test_done () {
>   say "1..$test_count$skip_all"
>   fi
>  
> - test -d "$remove_trash" &&
> + test -d "$remove_trash" ||
> + error "Tests passed but trash directory already removed before 
> test cleanup; aborting"

I think I found out why this "test -d" was here in the first place:

  $ ./t-basic.sh --debug
  [...]
  # passed all 77 test(s)
  1..77
  error: Tests passed but trash directory already removed before test cleanup; 
aborting

When --debug is in use, we do not set $remove_trash. The original was
relying on 'test -d ""' to return false.

I think this whole removal block should probably be moved inside a
conditional like:

  if test -n "$remove_trash"
  then
 ...
  fi

I also wonder if we should come up with a better name than
$remove_trash. A script which unknowingly overwrites that variable would
be disastrous.

Perhaps we should drop it entirely and just do:

  if test -z "$debug"
  then
 test -d "$TRASH_DIRECTORY" ||
 error "Tests passed but..."
  [and so forth...]
  fi

-Peff

Re: [PATCH] test-lib: abort when can't remove trash directory

[SZEDER Gábor]

On Fri, Apr 21, 2017 at 2:48 AM, Junio C Hamano  wrote:
> SZEDER Gábor  writes:
>
>> We had two similar bugs in the tests sporadically triggering error
>> messages during the removal of the trash directory, see commits
>> bb05510e5 (t5510: run auto-gc in the foreground, 2016-05-01) and
>> ef09036cf (t6500: wait for detached auto gc at the end of the test
>> script, 2017-04-13).  The test script succeeded nonetheless, because
>> these errors are ignored during housekeeping in 'test_done'.
>>
>> However, such an error is a sign that something is fishy in the test
>> script.  Print an error message and abort the test script when the
>> trash directory can't be removed successfully or is already removed,
>> because that's unexpected and we woud prefer somebody notice and

s/woud/would/

>> figure out why.


> Note, that the commit message references ef09036cf (t6500: wait for
>> detached auto gc at the end of the test script, 2017-04-13), which
>> is still only in 'pu'.
>
> I think that one is already part of 2.13-rc0 ;-)

Yeah, I should have fetched before submitting.

Re: [PATCH] test-lib: abort when can't remove trash directory

[Junio C Hamano]

SZEDER Gábor  writes:

> We had two similar bugs in the tests sporadically triggering error
> messages during the removal of the trash directory, see commits
> bb05510e5 (t5510: run auto-gc in the foreground, 2016-05-01) and
> ef09036cf (t6500: wait for detached auto gc at the end of the test
> script, 2017-04-13).  The test script succeeded nonetheless, because
> these errors are ignored during housekeeping in 'test_done'.
>
> However, such an error is a sign that something is fishy in the test
> script.  Print an error message and abort the test script when the
> trash directory can't be removed successfully or is already removed,
> because that's unexpected and we woud prefer somebody notice and
> figure out why.

Makes sense to me, too.

>
> Signed-off-by: SZEDER Gábor 
> ---
>
> Note, that the commit message references ef09036cf (t6500: wait for
> detached auto gc at the end of the test script, 2017-04-13), which
> is still only in 'pu'.

I think that one is already part of 2.13-rc0 ;-)

>  t/test-lib.sh | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 13b569682..e9e6f677d 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -761,9 +761,12 @@ test_done () {
>   say "1..$test_count$skip_all"
>   fi
>  
> - test -d "$remove_trash" &&
> + test -d "$remove_trash" ||
> + error "Tests passed but trash directory already removed before 
> test cleanup; aborting"
> +
>   cd "$(dirname "$remove_trash")" &&
> - rm -rf "$(basename "$remove_trash")"
> + rm -rf "$(basename "$remove_trash")" ||
> + error "Tests passed but test cleanup failed; aborting"
>  
>   test_at_end_hook_

Re: [PATCH] test-lib: abort when can't remove trash directory

[Jeff King]

On Thu, Apr 20, 2017 at 06:52:30PM +0200, SZEDER Gábor wrote:

> We had two similar bugs in the tests sporadically triggering error
> messages during the removal of the trash directory, see commits
> bb05510e5 (t5510: run auto-gc in the foreground, 2016-05-01) and
> ef09036cf (t6500: wait for detached auto gc at the end of the test
> script, 2017-04-13).  The test script succeeded nonetheless, because
> these errors are ignored during housekeeping in 'test_done'.
> 
> However, such an error is a sign that something is fishy in the test
> script.  Print an error message and abort the test script when the
> trash directory can't be removed successfully or is already removed,
> because that's unexpected and we woud prefer somebody notice and
> figure out why.
> 
> Signed-off-by: SZEDER Gábor 

I think this is a good idea, and the patch itself looks good. Thanks.

-Peff

[PATCH] test-lib: abort when can't remove trash directory

[SZEDER Gábor]

We had two similar bugs in the tests sporadically triggering error
messages during the removal of the trash directory, see commits
bb05510e5 (t5510: run auto-gc in the foreground, 2016-05-01) and
ef09036cf (t6500: wait for detached auto gc at the end of the test
script, 2017-04-13).  The test script succeeded nonetheless, because
these errors are ignored during housekeeping in 'test_done'.

However, such an error is a sign that something is fishy in the test
script.  Print an error message and abort the test script when the
trash directory can't be removed successfully or is already removed,
because that's unexpected and we woud prefer somebody notice and
figure out why.

Signed-off-by: SZEDER Gábor 
---

Note, that the commit message references ef09036cf (t6500: wait for
detached auto gc at the end of the test script, 2017-04-13), which
is still only in 'pu'.

 t/test-lib.sh | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 13b569682..e9e6f677d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -761,9 +761,12 @@ test_done () {
say "1..$test_count$skip_all"
fi
 
-   test -d "$remove_trash" &&
+   test -d "$remove_trash" ||
+   error "Tests passed but trash directory already removed before 
test cleanup; aborting"
+
cd "$(dirname "$remove_trash")" &&
-   rm -rf "$(basename "$remove_trash")"
+   rm -rf "$(basename "$remove_trash")" ||
+   error "Tests passed but test cleanup failed; aborting"
 
test_at_end_hook_
 
-- 
2.12.2.613.g9c5b79913