Re: [PATCH] Fix a signal handler
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 22, 2010, 18:31 UTC
- Message-ID
- <7v1vgdgm02.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <4B82744B.4060805@web.de>
Markus Elfring <Markus.Elfring@web.de> writes:
Show 10 quoted lines
>> 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.
Show 24 quoted lines
> 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.
Show 11 quoted lines
> 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.