Re: [PATCH] Fix signal handler

From: Jeff King <peff@peff.net>
Date: Feb 10, 2010, 17:33 UTC
Message-ID: <20100210173348.GA5091@coredump.intra.peff.net>
In-Reply-To: <4B72E81B.3020900@web.de>

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

Show 7 quoted lines

> 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).

Show 8 quoted lines

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