Re: [PATCH 2/3] git-daemon: make the signal handler almost a no-op

[Junio C Hamano <gitster@pobox.com>]

"Stephen R. van den Berg" <srb@cuci.nl> writes:

>>> @@ -1036,10 +1034,7 @@ int main(int argc, char **argv)
>>>  	gid_t gid = 0;
>>>  	int i;
>
>>> -	/* Without this we cannot rely on waitpid() to tell
>>> -	 * what happened to our children.
>>> -	 */
>>> -	signal(SIGCHLD, SIG_DFL);
>>> +	child_handler(0);
>
>>Why?
>
> child_handler() now does barely more than setup the signal handler,
> which is exactly what we want to do here.
>
>>With your change, the first part happens to be almost no-op, but I do not
>>think it justifies this hunk.
>
>>After all, we might even want to do something like:
>
>>	static void child_handler(int signo)
>>        {
>>        	if (USE_SYSV_SIGNAL_SEMANTICS)
>>                	signal(SIGCHLD, child_handler);
>
>>and have the compiler optimize out the signal rearming with
>
>>	cc CFLAGS=-DUSE_SYSV_SIGNAL_SEMANTICS=0
>
> In return I ask: why?

Please read the part you omitted from your quote again.

I agree it would be very meaningless change as an optimization, but I am
concerned more about robustness and what makes sense.

Do you agree that "child_handler()" is a signal handler to handle SIGCHLD,
and such a signal handler conceptually consists of two parts? i.e.

	static void child_handler()
        {
        	reap_dead_children();
                rearm_signal_as_needed();
	}

Your argument is it is Ok to call this function when you are arming the
signal for the first time, because reap_dead_children() happens to be
empty, and your rearm_signal_as_needed() happens to be the same as
arm_signal_always().

Yes, it happens to be _Ok_ now.  But is it an improvement in the longer
term?  I do not think so.

I do not see why you think it is better to rely on these two assumptions
than being explicit and say "we set up the signal for the first time
always on any platform", especially when the latter is much more direct
way to say what your intention is.  Or are you gaining something by not
explicitly calling signal() for the first time?  I may be missing some
benefit for doing so.

It is a trade-off between that some benefit I am not seeing, and downside
that your version can be broken more easily by future changes to
child_handler(), because you are assuming more about what it happens to do
currently.

That's the kind of thing maintainers worry more about.