Re: [PATCH] Fix signal handler

[Jeff King <peff@peff.net>]

On Wed, Feb 10, 2010 at 06:08:43PM +0100, Markus Elfring wrote:

> 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 involved software design details were also mentioned on the
> mailing list.

Keep in mind commit messages will be read much later through "git log"
and the like.  Mentioning the mailing list is usually not very helpful
there. It is usually a good idea instead to summarize what was said on
the list for later readers of the commit (though in this case, I think
your first paragraph really says everything that needs to be said).

> --- a/builtin-log.c
> +++ b/builtin-log.c
> @@ -123,7 +123,7 @@ static void show_early_header(struct rev_info *rev, const char *stage, int nr)
>  
>  static struct itimerval early_output_timer;
>  
> -static void log_show_early(struct rev_info *revs, struct commit_list *list)
> +extern void log_show_early(struct rev_info *revs, struct commit_list *list)

Why does this need to become extern? It looks like we are still just
assigning the function pointer from within this file.

> -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;

Good. I was worried from the above s/static/extern/ that you were going
to make log_show_early the only possible early output function, but the
way you did it is definitely the right way.

Overall, this change looks sane to me. You still haven't provided any
evidence that this is a problem in practice, but these changes are not
particularly cumbersome, so it is probably better to be on the safe
side.

-Peff