Re: [PATCH 2/6] t1406: prepare for the refs code to fail with BUG()

[Junio C Hamano]

Johannes Schindelin  writes:

>> As discussed in this thread, tests that use t/helper/ executables
>> that try to trickle BUG() codepath to ensure that these "should
>> never happen" conditions are caught do need to deal with it.  If
>> dumping core is undesirable, tweaking BUG() implementation so that
>> it becomes die("BUG: ...") *ONLY* when the caller knows what it is
>> doing (e.g. running t/helper/ commands) is probably a good idea.
>> Perhaps GIT_TEST_OPTS can gain one feature "--bug-no-abort" and set
>> an environment variable so that implementation of BUG() can notice,
>> or something.
>
> I think we can do even better than that. t/helper/*.c could set a global
> variable that no other code is supposed to set, to trigger an alternative
> to SIGABRT. Something like

Yes, I agree with that solution for t/helper/ part.

But we also need to arrange a way for things outside t/helper/ to
set the BUG_exit_code at runtime, so that those like Duy who causes
BUG() to trigger in some "git cmd" under development when running
tests for himself, where he does not want his BUG() to dump core and
contaminate global core storage.

Re: [PATCH 2/6] t1406: prepare for the refs code to fail with BUG()

[Johannes Schindelin]

Hi,

On Wed, 2 May 2018, Junio C Hamano wrote:

> Duy Nguyen  writes:
> 
> > On Mon, Apr 30, 2018 at 12:17 AM, Johannes Schindelin
> >  wrote:
> >> t1406 specifically verifies that certain code paths fail with a BUG: ...
> >> message.
> >>
> >> In the upcoming commit, we will convert that message to be generated via
> >> BUG() instead of die("BUG: ..."), which implies SIGABRT instead of a
> >> regular exit code.
> >
> > On the other hand, SIGABRT on linux creates core dumps. And on some
> > setup (like mine) core dumps may be redirected to some central place
> > via /proc/sys/kernel/core_pattern. I think systemd does it too but I
> > didn't check.
> >
> > This moving to SIGABRT when we know it _will_ happen when running the
> > test suite will accumulate core dumps over time and not cleaned up by
> > the test suite. Maybe keeping die("BUG: here is a good compromise.
> 
> I do not think it is.  At regular runtime, we _do_ want Git to dump
> core if it triggers BUG() condition, whose point is to mark
> conditions that should never happen.

Indeed.

> As discussed in this thread, tests that use t/helper/ executables
> that try to trickle BUG() codepath to ensure that these "should
> never happen" conditions are caught do need to deal with it.  If
> dumping core is undesirable, tweaking BUG() implementation so that
> it becomes die("BUG: ...") *ONLY* when the caller knows what it is
> doing (e.g. running t/helper/ commands) is probably a good idea.
> Perhaps GIT_TEST_OPTS can gain one feature "--bug-no-abort" and set
> an environment variable so that implementation of BUG() can notice,
> or something.

I think we can do even better than that. t/helper/*.c could set a global
variable that no other code is supposed to set, to trigger an alternative
to SIGABRT. Something like

-- snip --
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 87066ced62a..5176f9f20ae 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -47,7 +47,9 @@ static struct test_cmd cmds[] = {
 int cmd_main(int argc, const char **argv)
 {
int i;
+   extern int BUG_exit_code;
 
+   BUG_exit_code = 99;
if (argc < 2)
die("I need a test name!");
 
diff --git a/usage.c b/usage.c
index cdd534c9dfc..9c84dccfa97 100644
--- a/usage.c
+++ b/usage.c
@@ -210,6 +210,9 @@ void warning(const char *warn, ...)
va_end(params);
 }
 
+/* Only set this, ever, from t/helper/, when verifying that bugs are
caught. */
+int BUG_exit_code;
+
 static NORETURN void BUG_vfl(const char *file, int line, const char *fmt,
va_list params)
 {
char prefix[256];
@@ -221,6 +224,8 @@ static NORETURN void BUG_vfl(const char *file, int
line, const char *fmt, va_lis
snprintf(prefix, sizeof(prefix), "BUG: ");
 
vreportf(prefix, fmt, params);
+   if (BUG_exit_code)
+   exit(BUG_exit_code);
abort();
 }
 
-- snap --

I'll try to find some time to play with this.

Ciao,
Dscho
> 
> When we are testing normal parts of Git outside t/helper/, we never
> want to hit BUG().  Aborting and dumping core when that happens is
> an desirable outcome.  From that point of view, the idea in 1/6 of
> this patch series to annotate test_must_fail and say "we know this
> one is going to hit BUG()" is a sound one.  The implementation in
> 1/6 to treat SIGABRT as an acceptable failure needs to be replaced
> to instead use the above mechanism you would use to tell BUG() not
> to abort but die with message to arrange that to happen before
> running the git command (most likely something from t/helper/) under
> test_must_fail ok=sigabrt; and then those who regularly break their
> Git being tested (read: us devs) and hit BUG() could instead set the
> environment variable (i.e. internal implementation detail) manually
> in their environment to turn these BUG()s into die("BUG:...)s while
> testing their early mistakes if they do not want core (of course,
> you could just do "ulimit -c", and that may be simpler solution of
> your "testing Git contaminates central core depot" issue).
> 
> 
> 
> 
> 
> 
> 

Re: [PATCH 2/6] t1406: prepare for the refs code to fail with BUG()

[Junio C Hamano]

Duy Nguyen  writes:

> On Mon, Apr 30, 2018 at 12:17 AM, Johannes Schindelin
>  wrote:
>> t1406 specifically verifies that certain code paths fail with a BUG: ...
>> message.
>>
>> In the upcoming commit, we will convert that message to be generated via
>> BUG() instead of die("BUG: ..."), which implies SIGABRT instead of a
>> regular exit code.
>
> On the other hand, SIGABRT on linux creates core dumps. And on some
> setup (like mine) core dumps may be redirected to some central place
> via /proc/sys/kernel/core_pattern. I think systemd does it too but I
> didn't check.
>
> This moving to SIGABRT when we know it _will_ happen when running the
> test suite will accumulate core dumps over time and not cleaned up by
> the test suite. Maybe keeping die("BUG: here is a good compromise.

I do not think it is.  At regular runtime, we _do_ want Git to dump
core if it triggers BUG() condition, whose point is to mark
conditions that should never happen.

As discussed in this thread, tests that use t/helper/ executables
that try to trickle BUG() codepath to ensure that these "should
never happen" conditions are caught do need to deal with it.  If
dumping core is undesirable, tweaking BUG() implementation so that
it becomes die("BUG: ...") *ONLY* when the caller knows what it is
doing (e.g. running t/helper/ commands) is probably a good idea.
Perhaps GIT_TEST_OPTS can gain one feature "--bug-no-abort" and set
an environment variable so that implementation of BUG() can notice,
or something.

When we are testing normal parts of Git outside t/helper/, we never
want to hit BUG().  Aborting and dumping core when that happens is
an desirable outcome.  From that point of view, the idea in 1/6 of
this patch series to annotate test_must_fail and say "we know this
one is going to hit BUG()" is a sound one.  The implementation in
1/6 to treat SIGABRT as an acceptable failure needs to be replaced
to instead use the above mechanism you would use to tell BUG() not
to abort but die with message to arrange that to happen before
running the git command (most likely something from t/helper/) under
test_must_fail ok=sigabrt; and then those who regularly break their
Git being tested (read: us devs) and hit BUG() could instead set the
environment variable (i.e. internal implementation detail) manually
in their environment to turn these BUG()s into die("BUG:...)s while
testing their early mistakes if they do not want core (of course,
you could just do "ulimit -c", and that may be simpler solution of
your "testing Git contaminates central core depot" issue).

Re: [PATCH 2/6] t1406: prepare for the refs code to fail with BUG()

[Duy Nguyen]

On Mon, Apr 30, 2018 at 12:17 AM, Johannes Schindelin
 wrote:
> t1406 specifically verifies that certain code paths fail with a BUG: ...
> message.
>
> In the upcoming commit, we will convert that message to be generated via
> BUG() instead of die("BUG: ..."), which implies SIGABRT instead of a
> regular exit code.

On the other hand, SIGABRT on linux creates core dumps. And on some
setup (like mine) core dumps may be redirected to some central place
via /proc/sys/kernel/core_pattern. I think systemd does it too but I
didn't check.

This moving to SIGABRT when we know it _will_ happen when running the
test suite will accumulate core dumps over time and not cleaned up by
the test suite. Maybe keeping die("BUG: here is a good compromise.
-- 
Duy

Re: [PATCH 2/6] t1406: prepare for the refs code to fail with BUG()

[Duy Nguyen]

On Tue, May 1, 2018 at 1:04 PM, Johannes Schindelin
 wrote:
>> If SIGABRT occurs as a result of BUG(), and we know that this happens for
>> certain cases, it means we have an unfixed bug.
>
> Not in this case: The code in question is in
> https://github.com/git/git/blob/v2.17.0/t/helper/test-ref-store.c#L190-L201
> and it is called in a way that fails to have the required flags for the
> operation.

To elaborate, in this particular case, developers are not supposed to
call calling create_reflog() on a submodule ref store because the
store's implementation does not support it (yet). So it's definitely
BUG() to catch devs from doing so. But due to the multiple layer
abstractions, we also need to verify that ref code will bug out in
this case :P

> This would normally indicate a bug, but in this case, that is
> exactly what the regression test tries to trigger: we *want* such a bug to
> cause a failure.
-- 
Duy

Re: [PATCH 2/6] t1406: prepare for the refs code to fail with BUG()

[Johannes Schindelin]

Hi Hannes,

On Mon, 30 Apr 2018, Johannes Sixt wrote:

> Am 30.04.2018 um 00:17 schrieb Johannes Schindelin:
> > t1406 specifically verifies that certain code paths fail with a BUG: ...
> > message.
> > 
> > In the upcoming commit, we will convert that message to be generated via
> > BUG() instead of die("BUG: ..."), which implies SIGABRT instead of a
> > regular exit code.
> >
> > [...]
> >
> >   test_expect_success 'create-reflog() not allowed' '
> > -   test_must_fail $RUN create-reflog HEAD 1
> > +   test_must_fail ok=sigabrt $RUN create-reflog HEAD 1
> >   '
> 
> I can't quite follow the rationale for this change. A 'BUG' error exit must
> never be reached, otherwise it is a bug in the program by definition. It
> cannot be OK that SIGABRT is a valid result from Git.

I thought so at first, too. However, what these test cases run is not Git
itself. Instead, they run a t/helper/ command *specifically* designed to
hit the BUG code path, probably to ensure that bugs in future code will
actually not be silently ignored, but do exit with an error.

> If SIGABRT occurs as a result of BUG(), and we know that this happens for
> certain cases, it means we have an unfixed bug.

Not in this case: The code in question is in
https://github.com/git/git/blob/v2.17.0/t/helper/test-ref-store.c#L190-L201
and it is called in a way that fails to have the required flags for the
operation. This would normally indicate a bug, but in this case, that is
exactly what the regression test tries to trigger: we *want* such a bug to
cause a failure.

I'll do my best to clarify that in the commit message.

Ciao,
Dscho

Re: [PATCH 2/6] t1406: prepare for the refs code to fail with BUG()

[Johannes Sixt]

Am 30.04.2018 um 00:17 schrieb Johannes Schindelin:

t1406 specifically verifies that certain code paths fail with a BUG: ...
message.

In the upcoming commit, we will convert that message to be generated via
BUG() instead of die("BUG: ..."), which implies SIGABRT instead of a
regular exit code.

Signed-off-by: Johannes Schindelin 
---
  t/t1406-submodule-ref-store.sh | 15 +--
  1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh
index e093782cc37..0ea3457cae3 100755
--- a/t/t1406-submodule-ref-store.sh
+++ b/t/t1406-submodule-ref-store.sh
@@ -16,7 +16,7 @@ test_expect_success 'setup' '
  '
  
  test_expect_success 'pack_refs() not allowed' '

-   test_must_fail $RUN pack-refs 3
+   test_must_fail ok=sigabrt $RUN pack-refs 3
  '
  
  test_expect_success 'peel_ref(new-tag)' '

@@ -27,15 +27,18 @@ test_expect_success 'peel_ref(new-tag)' '
  '
  
  test_expect_success 'create_symref() not allowed' '

-   test_must_fail $RUN create-symref FOO refs/heads/master nothing
+   test_must_fail ok=sigabrt \
+   $RUN create-symref FOO refs/heads/master nothing
  '
  
  test_expect_success 'delete_refs() not allowed' '

-   test_must_fail $RUN delete-refs 0 nothing FOO refs/tags/new-tag
+   test_must_fail ok=sigabrt \
+   $RUN delete-refs 0 nothing FOO refs/tags/new-tag
  '
  
  test_expect_success 'rename_refs() not allowed' '

-   test_must_fail $RUN rename-ref refs/heads/master refs/heads/new-master
+   test_must_fail ok=sigabrt \
+   $RUN rename-ref refs/heads/master refs/heads/new-master
  '
  
  test_expect_success 'for_each_ref(refs/heads/)' '

@@ -91,11 +94,11 @@ test_expect_success 'reflog_exists(HEAD)' '
  '
  
  test_expect_success 'delete_reflog() not allowed' '

-   test_must_fail $RUN delete-reflog HEAD
+   test_must_fail ok=sigabrt $RUN delete-reflog HEAD
  '
  
  test_expect_success 'create-reflog() not allowed' '

-   test_must_fail $RUN create-reflog HEAD 1
+   test_must_fail ok=sigabrt $RUN create-reflog HEAD 1
  '


I can't quite follow the rationale for this change. A 'BUG' error exit 
must never be reached, otherwise it is a bug in the program by 
definition. It cannot be OK that SIGABRT is a valid result from Git.


If SIGABRT occurs as a result of BUG(), and we know that this happens 
for certain cases, it means we have an unfixed bug. Should then not run 
these cases under test_expect_failure instead of test_expect_success to 
identify them as known bugs?


Confused.

-- Hannes

[PATCH 2/6] t1406: prepare for the refs code to fail with BUG()

[Johannes Schindelin]

t1406 specifically verifies that certain code paths fail with a BUG: ...
message.

In the upcoming commit, we will convert that message to be generated via
BUG() instead of die("BUG: ..."), which implies SIGABRT instead of a
regular exit code.

Signed-off-by: Johannes Schindelin 
---
 t/t1406-submodule-ref-store.sh | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh
index e093782cc37..0ea3457cae3 100755
--- a/t/t1406-submodule-ref-store.sh
+++ b/t/t1406-submodule-ref-store.sh
@@ -16,7 +16,7 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'pack_refs() not allowed' '
-   test_must_fail $RUN pack-refs 3
+   test_must_fail ok=sigabrt $RUN pack-refs 3
 '
 
 test_expect_success 'peel_ref(new-tag)' '
@@ -27,15 +27,18 @@ test_expect_success 'peel_ref(new-tag)' '
 '
 
 test_expect_success 'create_symref() not allowed' '
-   test_must_fail $RUN create-symref FOO refs/heads/master nothing
+   test_must_fail ok=sigabrt \
+   $RUN create-symref FOO refs/heads/master nothing
 '
 
 test_expect_success 'delete_refs() not allowed' '
-   test_must_fail $RUN delete-refs 0 nothing FOO refs/tags/new-tag
+   test_must_fail ok=sigabrt \
+   $RUN delete-refs 0 nothing FOO refs/tags/new-tag
 '
 
 test_expect_success 'rename_refs() not allowed' '
-   test_must_fail $RUN rename-ref refs/heads/master refs/heads/new-master
+   test_must_fail ok=sigabrt \
+   $RUN rename-ref refs/heads/master refs/heads/new-master
 '
 
 test_expect_success 'for_each_ref(refs/heads/)' '
@@ -91,11 +94,11 @@ test_expect_success 'reflog_exists(HEAD)' '
 '
 
 test_expect_success 'delete_reflog() not allowed' '
-   test_must_fail $RUN delete-reflog HEAD
+   test_must_fail ok=sigabrt $RUN delete-reflog HEAD
 '
 
 test_expect_success 'create-reflog() not allowed' '
-   test_must_fail $RUN create-reflog HEAD 1
+   test_must_fail ok=sigabrt $RUN create-reflog HEAD 1
 '
 
 test_done
-- 
2.17.0.windows.1.36.gdf4ca5fb72a