Re: [PATCH] Resolve testsuite interruption in SVN 1.8

On 7 August 2016 at 23:14, Stefan <luke1410_at_posteo.de> wrote:
> On 8/7/2016 21:16, Ivan Zhakov wrote:
>> On 7 August 2016 at 21:27, Stefan Sperling <stsp_at_elego.de> wrote:
>>> On Sun, Aug 07, 2016 at 08:23:58PM +0200, Stefan Sperling wrote:
>>>> On Sun, Aug 07, 2016 at 02:08:35PM +0200, Stefan wrote:
>>>>> Hi,
>>>>>
>>>>> the attached patch is the outcome of talking with jcorval on IRC about a
>>>>> test suite issue on Windows (release builds) in SVN 1.8. It resolves the
>>>>> fact that the tests will be interrupted on Windows by a Windows popup
>>>>> upon an abort()-call.
>>>>>
>>>>> Atm this is triggered for the 1.8 test suite for the move-test (no 8)
>>>>> which is marked as XFail and triggers an SVN_ERR_ASSERT() and therefore
>>>>> breaks fully automated tests.
>>>> I'm not a windows person but this looks reasonable to me. +1
>>> Forgot to mention: If this is a 1.8-only fix, I'd suggest you create
>>> a branch of 1.8.x and commit your patch there, and then nominate your
>>> branch in the 1.8.x/STATUS file (you can add my +1 to the nomination).
>> I'm not sure that we should add handling of new environment variable
>> in patch release.
>>
>>
> I can understand the concern. Please bare in mind however that the
> default behavior (aka: if the environment variable is not set) would
> stay unchanged and the effect of the environment variable is quite
> limited (it will only disable the Watson crash dumps in release builds -
> obviously only applies on Windows builds). It's by default only
> activated for the test suites and therefore the risks involved is IMO
> close to non existent.
>
Could you please first clarify: does this problem exist in trunk? I'm
asking because subject is somewhat confusing because it only mention
"SVN 1.8". If yes, we should discuss solution for trunk first. And
then discuss backporting committed solution to active stable branches
separately.

Regarding the patch itself (assuming it for trunk): why you have
decided to check for SVN_CMDLINE_DISABLE_WATSON_ON_ABORT environment
variable only if SVN_CMDLINE_USE_DIALOG_FOR_ABORT is not set? It
creates very hard to understand dependency on other environment
variables that controls crashhandler/abort() behavior. Because
currently suggested behavior would be:
if (!getenv("SVN_CMDLINE_DISABLE_CRASH_HANDLER"))
{
      if (!getenv("SVN_CMDLINE_USE_DIALOG_FOR_ABORT"))
      {
          if (getenv("SVN_CMDLINE_DISABLE_WATSON_ON_ABORT"))
          {
             _set_abort_behavior(0, _CALL_REPORTFAULT);
          }
      }
}

Btw what are the problems with approach to disable watson reports on
abort(), except backporting? I mean we already override Windows Error
Reporting by installing our own SEH exception handler, so it looks
natural to disable abort() reporting also.

Also your patch is missing documentation changes in notes/knobs.

-- 
Ivan Zhakov