Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?

From: Jeff King <peff@peff.net>
Date: Nov 9, 2015, 15:53 UTC
Message-ID: <20151109155342.GB27224@sigill.intra.peff.net>
In-Reply-To: <CAM-tV-9CNO_hqnweFpLaRHx4xEA32CPRdq56y6vYMWqURV9kgg@mail.gmail.com>

On Sun, Nov 08, 2015 at 09:58:06PM -0500, Noam Postavsky wrote:

> > I am leaning more towards ignoring SIGHUP (configurably) being the only
> > really sane path forward. Do you want to try your hand at a patch?
> 
> Something like this?

Yes, but with a proper commit message, and an update to Documentation/config.txt. :)

Automated tests would be nice, but I suspect it may be too complicated to be worth it.

Show 10 quoted lines

> diff --git i/credential-cache--daemon.c w/credential-cache--daemon.c
> index eef6fce..e3f2612 100644
> --- i/credential-cache--daemon.c
> +++ w/credential-cache--daemon.c
> @@ -256,6 +256,9 @@ int main(int argc, const char **argv)
>          OPT_END()
>      };
> 
> +    int ignore_sighup = 0;
> +    git_config_get_bool("credential.cache.ignoreSighup", &ignore_sighup);

I don't think we should use the credential.X.* namespace here. That is already reserved for credential setup for URLs matching "X".

Probably "credentialCache.ignoreSIGHUP" would be better. Or maybe "credential-cache". We usually avoid dashes in our config names, but in this case it matches the program name.

Also, we usually spell config names as all-lowercase in the code. The older callback-interface config code needed this (since we just strcmp'd the keys against a normalized case). I think git_config_get_bool() will normalize the key we feed it, but I'd rather stay consistent.

Show 10 quoted lines

> @@ -264,6 +267,12 @@ int main(int argc, const char **argv)
> 
>      check_socket_directory(socket_path);
>      register_tempfile(&socket_file, socket_path);
> +
> +    if (ignore_sighup) {
> +        sigchain_pop(SIGHUP);
> +        signal(SIGHUP, SIG_IGN);
> +    }
> +

I don't think you need to pop the tempfile handler here. You can simply sigchain_push() the SIG_IGN, and since we won't ever pop and propagate that, it doesn't matter what is under it.

-Peff