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

From: Jeff King <peff@peff.net>
Date: Nov 10, 2015, 12:25 UTC
Message-ID: <20151110122501.GA14418@sigill.intra.peff.net>
In-Reply-To: <CAM-tV--hBSdCJckCnMtKgkQB2f_3eN8sXHdFWwg2hzb6s7ufxw@mail.gmail.com>

On Mon, Nov 09, 2015 at 08:05:25PM -0500, Noam Postavsky wrote:

Show 20 quoted lines

> > Automated tests would be nice, but I suspect it may be too complicated
> > to be worth it.
> 
> I attempted
> 
> test_ignore_sighup ()
> {
>     mkdir "$HOME/.git-credential-cache" &&
>     chmod 0700 "$HOME/.git-credential-cache" &&
>     git -c credentialCache.ignoreSIGHUP=true credential-cache--daemon
> "$HOME/.git-credential-cache/socket" &
>     kill -SIGHUP $! &&
>     ps $!
> }
> 
> test_expect_success 'credentialCache.ignoreSIGHUP works' 'test_ignore_sighup'
> 
> but it does't pass (testing manually by running
> ./git-credential-cache--daemon $HOME/.git-credential-cache/test-socket
> & and then kill -HUP does work).

Your test looks racy. After the "&" in spawning the daemon, we don't have any guarantee that the daemon actually reached the signal() call before we issued our kill.

The daemon issues an "ok\n" to stdout to report that it's ready to serve (this is how the auto-spawn avoids races). So you could use that with a fifo to fix this. It's a little complicated; see lib-git-daemon.sh for an example.

I'm also not sure the use of "ps" here is portable (i.e., does it always report a useful error code on all systems?).

So I dunno. Given the complexity of the test, and that it is such a simple feature that is unlikely to be broken, I'm not sure it is worth it. A test failure seems like it would more likely be a problem in the test, not the code.

Show 11 quoted lines

> From 5fc95b6e2f956403da6845fc3ced83b21bee7bb0 Mon Sep 17 00:00:00 2001
> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Mon, 9 Nov 2015 19:26:29 -0500
> Subject: [PATCH] credential-cache: new option to ignore sighup
> 
> Introduce new option "credentialCache.ignoreSIGHUP" which stops
> git-credential-cache--daemon from quitting on SIGHUP.  This is useful
> when "git push" is started from Emacs, because all child
> processes (including the daemon) will receive a SIGHUP when "git push"
> exits.
> ---

Can you add a signed-off-by here (see the "sign-off" section in Documentation/SubmittingPatches).

Show 11 quoted lines

> diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
> index eef6fce..6cda9c0 100644
> --- a/credential-cache--daemon.c
> +++ b/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("credentialcache.ignoresighup", &ignore_sighup);
> +

Style-wise, I think the declaration should go above the options-list.

Show 8 quoted lines

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

This part looks obviously correct. :)

I don't think there's any need to use sigchain_push here (we have no intention of ever popping it).

-Peff