On Mon, Nov 09, 2015 at 08:05:25PM -0500, Noam Postavsky wrote:
> > 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.
> From 5fc95b6e2f956403da6845fc3ced83b21bee7bb0 Mon Sep 17 00:00:00 2001
> From: Noam Postavsky <npostavs@xxxxxxxxxxxxxxxxxxxxx>
> 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).
> 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.
> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html