Re: [PATCH] Fix a signal handler

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

Markus Elfring <Markus.Elfring@web.de> writes:

>> Of are you asking me if I'd apply your patch if you send a polished update,
>> and asking me to decide it before seeing the patch?
>
> Would you like to pick this source code adjustment up?
>
> Regards,
> Markus
>
> ---
>>From e138904a08ceaf469fa2f4d0ec87b5891be14760 Mon Sep 17 00:00:00 2001

It would be preferred to remove everything above this line when you send
patches.

> From: Markus Elfring <Markus.Elfring@web.de>
> Date: Mon, 22 Feb 2010 11:53:35 +0100
> Subject: [PATCH] Fix a signal handler

Please be a bit more careful when coming up with what Subject: says; it
will be the only piece of information to tell the reader of output from
"git shortlog" what the commit made from this patch is about.  E.g.

    Subject: [PATCH] log --early-output: signal handler pedantic fix

> A global flag can only be set by a signal handler in a portable way
> if it has got the data type "sig_atomic_t". The previously used assignment
> of a function pointer in the function "early_output" was moved to another
> variable in the function "setup_early_output".

The first sentence gives the basis of your argument (so that people can
decide to agree or disagree with you).  Then you describe why you think
the code you are changing is wrong --- oops, that part is missing --- and
what you did based on the above two observations --- oops, that part is
missing, too --- and then any additional info.

I'd phrase the above like this:

    The behavior is undefined if the signal handler refers to any object
    other than errno with static storage duration other than by assigning
    a value to a static storage duration variable of type "volatile
    sig_atomic_t", but the existing code updates a variable that holds a
    pointer to a function (i.e. not a sigatomic_t variable).

    Change it to only set a flag, and adjust the callsite that calls the
    early-output function to check it.

and that would be sufficiently clear without saying anything else.

> diff --git a/builtin-log.c b/builtin-log.c
> index e0d5caa..beccf7f 100644
> --- a/builtin-log.c
> +++ b/builtin-log.c
> @@ -170,20 +170,14 @@ static void log_show_early(struct rev_info *revs, struct commit_list *list)
>  
>  static void early_output(int signal)
>  {
> -	show_early_output = log_show_early;
> +	show_early_output = 1;
>  }
>  
>  static void setup_early_output(struct rev_info *rev)
>  {
>  	struct sigaction sa;
>  
> -	/*
> -	 * Set up the signal handler, minimally intrusively:
> -	 * we only set a single volatile integer word (not
> -	 * using sigatomic_t - trying to avoid unnecessary
> -	 * system dependencies and headers), and using
> -	 * SA_RESTART.
> -	 */
> +	early_output_function = &log_show_early;

Your proposed log message also needs to make a good counter-argument why
the above "we purposely avoid using sigatomic_t --- it is not worth the
hassle of having to deal with systems that lack this type in practice" is
worried too much, and it now is sensible to assume that everybody has
sigatomic_t these days to allow us do "the right thing".  It can be just
as simple as 'Output from "git grep sigatomic_t" indicates that we are
already using it.' but you need to say something, as this comment you are
removing makes it clear that it was not a bug by mistake or ignorance, but
instead was a deliberate choice.

> diff --git a/revision.c b/revision.c
> index 3ba6d99..62402fb 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -13,7 +13,8 @@
>  #include "decorate.h"
>  #include "log-tree.h"
>  
> -volatile show_early_output_fn_t show_early_output;
> +sig_atomic_t show_early_output = 0;
> +show_early_output_fn_t early_output_function = NULL;

According to POSIX, "s-e-o" has to be "volatile sig_atomic_t".  Also we do
not explicitly initialize bss variables to zero or NULL.

Thanks.