Re: [BUG] MSVC: error box when interrupting `gitlog` by quitting less

2014-03-28 Thread Marat Radchenko
Marat Radchenko marat at slonopotamus.org writes:

 
 Jeff King peff at peff.net writes:
 
  
  The write_or_die function will always die on an error,
  including EPIPE. However, it currently treats EPIPE
  specially by suppressing any error message, and by exiting
  with exit code 0.
 
 This causes error box on Windows in MSVC=1 build:

After deeper investigation it turned out that Windows supports
much less signals [1] than POSIX and If the argument is not a valid signal 
as specified above, the invalid parameter handler is invoked.

The question is - what is the proper way to fix this?
Patch mingw_raise in compat/mingw.c to map unsupported signals into
supported ones like SIGPIPE - SIGTERM?

[1]: http://msdn.microsoft.com/en-us/library/dwwzkt4c.aspx

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [BUG] MSVC: error box when interrupting `gitlog` by quitting less

2014-03-28 Thread Jeff King
On Fri, Mar 28, 2014 at 09:14:07AM +, Marat Radchenko wrote:

  Jeff King peff at peff.net writes:
  
   
   The write_or_die function will always die on an error,
   including EPIPE. However, it currently treats EPIPE
   specially by suppressing any error message, and by exiting
   with exit code 0.
  
  This causes error box on Windows in MSVC=1 build:
 
 After deeper investigation it turned out that Windows supports
 much less signals [1] than POSIX and If the argument is not a valid signal 
 as specified above, the invalid parameter handler is invoked.
 
 The question is - what is the proper way to fix this?
 Patch mingw_raise in compat/mingw.c to map unsupported signals into
 supported ones like SIGPIPE - SIGTERM?
 
 [1]: http://msdn.microsoft.com/en-us/library/dwwzkt4c.aspx

I'm not sure what an actual SIGPIPE death looks like on Windows. What
happens if git is still writing data to the pager and the pager exits?
Does it receive a signal of some sort?

The point of the code in check_pipe is to simulate that death. So
whatever happens to git in that case is what we would want to happen
when we call raise(SIGPIPE).

A possibly simpler option would be to just have the MSVC build skip the
raise() call, and do the exit(141) that comes just after. That is
probably close enough simulation of SIGPIPE death.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [BUG] MSVC: error box when interrupting `gitlog` by quitting less

2014-03-28 Thread Marat Radchenko
Jeff King peff at peff.net writes:

 
 I'm not sure what an actual SIGPIPE death looks like on Windows.

There is no SIGPIPE death on Windows due to total absence of SIGPIPE.
raise(unsupported int) just causes ugly git.exe has stopped working
window and possibly ends up as SIGABT (I don't know how to check this).

 What
 happens if git is still writing data to the pager and the pager exits?
 Does it receive a signal of some sort?

I'm not sure what you mean, sorry. check_pipe properly detects pager exit.
The problem is with the way it tries to die.

 The point of the code in check_pipe is to simulate that death. So
 whatever happens to git in that case is what we would want to happen
 when we call raise(SIGPIPE).

That's what I'm talking about. On Windows, you can't raise(SIGPIPE).
You can only raise(Windows_supported_signal) where signal is one of:
SIGABRT, SIGFPE, SIGILL, SIGINT, SIGSEGV, SIGTERM as MSDN tells us.

 A possibly simpler option would be to just have the MSVC build skip the
 raise() call, and do the exit(141) that comes just after. That is
 probably close enough simulation of SIGPIPE death.

Isn't raise(SIGTERM/SIGINT) good enough?

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [BUG] MSVC: error box when interrupting `gitlog` by quitting less

2014-03-28 Thread Jeff King
On Fri, Mar 28, 2014 at 10:07:22AM +, Marat Radchenko wrote:

  What
  happens if git is still writing data to the pager and the pager exits?
  Does it receive a signal of some sort?
 
 I'm not sure what you mean, sorry. check_pipe properly detects pager exit.
 The problem is with the way it tries to die.

Right, but check_pipe shouldn't trigger in most cases on Unix because
the process will be killed by SIGPIPE automatically. It's only there to
catch the case where we have disabled SIGPIPE.

On Windows, what happens to yes if you run:

  yes | (exit 0)

On Unix, yes receives SIGPIPE and dies. Does it run forever on
Windows? If it dies, what does the death look like (does it have a
signal death, or exit with a specific code?).

  The point of the code in check_pipe is to simulate that death. So
  whatever happens to git in that case is what we would want to happen
  when we call raise(SIGPIPE).
 
 That's what I'm talking about. On Windows, you can't raise(SIGPIPE).
 You can only raise(Windows_supported_signal) where signal is one of:
 SIGABRT, SIGFPE, SIGILL, SIGINT, SIGSEGV, SIGTERM as MSDN tells us.

Right, I understand that you don't have SIGPIPE. But we want to emulate
whatever happens in the case I described above.

  A possibly simpler option would be to just have the MSVC build skip the
  raise() call, and do the exit(141) that comes just after. That is
  probably close enough simulation of SIGPIPE death.
 
 Isn't raise(SIGTERM/SIGINT) good enough?

Perhaps. It is a slight lie. We _didn't_ get a SIGTERM, and anybody
looking at our exit code to find out why we died would be misled. But
the most important thing is that we die and that the exit status is
non-zero.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [BUG] MSVC: error box when interrupting `gitlog` by quitting less

2014-03-28 Thread Johannes Sixt
Please do not cull the Cc list.

Am 3/28/2014 11:07, schrieb Marat Radchenko:
 Jeff King peff at peff.net writes:
 

 I'm not sure what an actual SIGPIPE death looks like on Windows.
 
 There is no SIGPIPE death on Windows due to total absence of SIGPIPE.
 raise(unsupported int) just causes ugly git.exe has stopped working
 window and possibly ends up as SIGABT (I don't know how to check this).

This happens only with newer Microsoft C runtime libraries. They do not
return EINVAL (because that usually indicates a bug caused by insufficient
checks before the function call), but crash the program by default in the
way that you observed.

 What
 happens if git is still writing data to the pager and the pager exits?
 Does it receive a signal of some sort?

No; the write attempt returns with EPIPE.

 
 I'm not sure what you mean, sorry. check_pipe properly detects pager exit.
 The problem is with the way it tries to die.
 
 The point of the code in check_pipe is to simulate that death. So
 whatever happens to git in that case is what we would want to happen
 when we call raise(SIGPIPE).
 
 That's what I'm talking about. On Windows, you can't raise(SIGPIPE).
 You can only raise(Windows_supported_signal) where signal is one of:
 SIGABRT, SIGFPE, SIGILL, SIGINT, SIGSEGV, SIGTERM as MSDN tells us.

Correct. All other signal number should return EINVAL. But, as I said,
that does not happen by default.

The correct solution is to link against invalidcontinue.obj in the MSVC
build. This is a compiler-provided object file that changes the default
behavior to the expected kind, i.e., C runtime functions return EINVAL
when appropriate instead of crashing the application.

 A possibly simpler option would be to just have the MSVC build skip the
 raise() call, and do the exit(141) that comes just after. That is
 probably close enough simulation of SIGPIPE death.

Correct. The MinGW build uses an older C runtime library, which does not
have the strange default behavior, and we do use that exit(141). And with
the fix to the MSVC build suggested above, that version would do likewise.

-- Hannes
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html