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 <npost...@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). > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html