Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Fri, Dec 04, 2015 at 12:05:52PM -0800, Junio C Hamano wrote: > > This probably _does_ trigger setup_git_env() when it was not otherwise > > called, and it will back to looking at ".git/config" for the repo-level > > config. That may fail to find the file if we are in a bare repository, > > or a subdirectory of the working tree. IOW, I suspect this: > > > > git init --bare foo.git > > cd foo.git > > git config credential.helper cache > > git config credentialcache.ignoreSIGHUP true ;# goes into local config > > git fetch https://example.com/foo.git > > > > may fail to respect the ignoreSIGHUP option. > > > > I guess the solution would be to setup_git_director_gently() in the > > daemon process. > > So I guess I did notice the right breakage ;-) > > At least, this won't be a regression but "a new feature initially > shipped with a broken corner case", so a follow-up fix is welcome, > but not a big deal that I've already merged it to 'master'. Yeah, agreed on all counts. Thanks for noticing. I suspect in practice is a pretty rare corner case, but I will leave it to those who are interested in the feature to do the fixup. -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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
Jeff Kingwrites: > On Fri, Dec 04, 2015 at 10:55:32AM -0800, Junio C Hamano wrote: >> >> I was puzzled that git_config_get_bool() is used here without even >> checking if we are inside any Git repository and wondered if it was >> correct > > This probably _does_ trigger setup_git_env() when it was not otherwise > called, and it will back to looking at ".git/config" for the repo-level > config. That may fail to find the file if we are in a bare repository, > or a subdirectory of the working tree. IOW, I suspect this: > > git init --bare foo.git > cd foo.git > git config credential.helper cache > git config credentialcache.ignoreSIGHUP true ;# goes into local config > git fetch https://example.com/foo.git > > may fail to respect the ignoreSIGHUP option. > > I guess the solution would be to setup_git_director_gently() in the > daemon process. So I guess I did notice the right breakage ;-) At least, this won't be a regression but "a new feature initially shipped with a broken corner case", so a follow-up fix is welcome, but not a big deal that I've already merged it to 'master'. Thanks. -- 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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
Jeff Kingwrites: >> 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", _sighup); >> + > > Style-wise, I think the declaration should go above the options-list. I was about to merge this to 'master', following your last issue of "What's cooking" report. I was puzzled that git_config_get_bool() is used here without even checking if we are inside any Git repository and wondered if it was correct. I'd imagine this is not a problem, as this process is spawned by "credential-cache" that was spawned by somebody (either push or fetch) who has read $GIT_DIR/config for credential.helper to determine that credential-cache needs to be used. >> @@ -264,6 +267,10 @@ int main(int argc, const char **argv) >> >> check_socket_directory(socket_path); >> register_tempfile(_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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Fri, Dec 04, 2015 at 10:55:32AM -0800, Junio C Hamano wrote: > >> + int ignore_sighup = 0; > >> + git_config_get_bool("credentialcache.ignoresighup", _sighup); > >> + > > > > Style-wise, I think the declaration should go above the options-list. > > I was about to merge this to 'master', following your last issue of > "What's cooking" report. > > I was puzzled that git_config_get_bool() is used here without even > checking if we are inside any Git repository and wondered if it was > correct. I'd imagine this is not a problem, as this process is > spawned by "credential-cache" that was spawned by somebody (either > push or fetch) who has read $GIT_DIR/config for credential.helper to > determine that credential-cache needs to be used. That does not have to be the case; I imagine most people would put credential.helper in their ~/.gitconfig. But I'm not sure I understand how that is relevant. The config subsystem should work just fine whether we are in a repository or not (and if not, return results only from system and user-wide config). This probably _does_ trigger setup_git_env() when it was not otherwise called, and it will back to looking at ".git/config" for the repo-level config. That may fail to find the file if we are in a bare repository, or a subdirectory of the working tree. IOW, I suspect this: git init --bare foo.git cd foo.git git config credential.helper cache git config credentialcache.ignoreSIGHUP true ;# goes into local config git fetch https://example.com/foo.git may fail to respect the ignoreSIGHUP option. I guess the solution would be to setup_git_director_gently() in the daemon process. -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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
I think you're right about the automated test not being worth the trouble. On Tue, Nov 10, 2015 at 7:26 AM, Jeff Kingwrote: > To clarify: just telling me it's OK to add your sign-off is fine here. I > can add it (and fix up the style thing) as I apply. Ok, please do that then, thanks. -- 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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Tue, Nov 10, 2015 at 07:25:02AM -0500, Jeff King wrote: > > 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). To clarify: just telling me it's OK to add your sign-off is fine here. I can add it (and fix up the style thing) as I apply. -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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
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> 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", _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(_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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
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. > 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", _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. > @@ -264,6 +267,12 @@ int main(int argc, const char **argv) > > check_socket_directory(socket_path); > register_tempfile(_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 -- 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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Mon, Nov 9, 2015 at 10:53 AM, Jeff Kingwrote: > Yes, but with a proper commit message, and an update to > Documentation/config.txt. :) Right, see attached. > > 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). > > 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. I went with "credentialCache.ignoreSIGHUP". > > 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. Oh, I didn't even realize git config names were case insensitive. > 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. Yup. From 5fc95b6e2f956403da6845fc3ced83b21bee7bb0 Mon Sep 17 00:00:00 2001 From: Noam Postavsky 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. --- Documentation/config.txt | 3 +++ credential-cache--daemon.c | 7 +++ 2 files changed, 10 insertions(+) diff --git a/Documentation/config.txt b/Documentation/config.txt index 4d3cb10..e5b 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1122,6 +1122,9 @@ credential..*:: example.com. See linkgit:gitcredentials[7] for details on how URLs are matched. +credentialCache.ignoreSIGHUP:: + Tell git-credential-cache--daemon to ignore SIGHUP, instead of quitting. + include::diff-config.txt[] difftool..path:: 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", _sighup); + argc = parse_options(argc, argv, NULL, options, usage, 0); socket_path = argv[0]; @@ -264,6 +267,10 @@ int main(int argc, const char **argv) check_socket_directory(socket_path); register_tempfile(_file, socket_path); + + if (ignore_sighup) + signal(SIGHUP, SIG_IGN); + serve_cache(socket_path, debug); delete_tempfile(_file); -- 2.6.1
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Fri, Oct 30, 2015 at 5:08 PM, Jeff Kingwrote: > Right. And not only is that hard to get right (I doubt, for example, you > support the arbitrary "!" shell expressions that git does), but it's > impossible to know for sure that will be needed, because you cannot know > all possible helpers (I might even have a helper that is a shell snippet > that calls credential-cache). Yep, in that case the user would have to override the result of parsing. >> Ah, maybe the missing piece I forgot to mention is that we could make >> our pre/1st-helper be an emacsclient command, which would tell Emacs >> to startup the daemon. So the daemon would be a subprocess of Emacs, >> not "git push", thereby avoiding the SIGHUP. In our current workaround >> we startup the daemon (if it's not running) before git commands that >> we think are going to run credential helpers (i.e. "push", "pull", >> "fetch"), hence my thought that it would be nicer if we only did that >> before git is *actually* going to run the helpers. > > I don't think even git knows it will need a helper until it is actually > ready to call one (e.g., it may depend on getting an HTTP 401 from the > server). Yes, so just call me first. :) > > 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? 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", _sighup); + argc = parse_options(argc, argv, NULL, options, usage, 0); socket_path = argv[0]; @@ -264,6 +267,12 @@ int main(int argc, const char **argv) check_socket_directory(socket_path); register_tempfile(_file, socket_path); + +if (ignore_sighup) { +sigchain_pop(SIGHUP); +signal(SIGHUP, SIG_IGN); +} + serve_cache(socket_path, debug); delete_tempfile(_file); -- 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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Thu, Oct 29, 2015 at 09:20:01PM -0400, Noam Postavsky wrote: > On Thu, Oct 29, 2015 at 8:50 PM, Jeff Kingwrote: > > workaround (the real inelegance is that you are assuming that "foo" > > needs run in the first place). > > Well, we currently check the output from "git config > credential.helpers" to determine what's needed, so the inelegance here > is that we reimplement git's checking of this option. Right. And not only is that hard to get right (I doubt, for example, you support the arbitrary "!" shell expressions that git does), but it's impossible to know for sure that will be needed, because you cannot know all possible helpers (I might even have a helper that is a shell snippet that calls credential-cache). As a workaround, I don't think looking for just "cache" is unreasonable. But it is not a very robust solution. :) > > I'm still not sure how the pre-helper would work. What git command kicks > > off the pre-helper command? Wouldn't it also be subject to the SIGHUP > > problem? > > Ah, maybe the missing piece I forgot to mention is that we could make > our pre/1st-helper be an emacsclient command, which would tell Emacs > to startup the daemon. So the daemon would be a subprocess of Emacs, > not "git push", thereby avoiding the SIGHUP. In our current workaround > we startup the daemon (if it's not running) before git commands that > we think are going to run credential helpers (i.e. "push", "pull", > "fetch"), hence my thought that it would be nicer if we only did that > before git is *actually* going to run the helpers. I don't think even git knows it will need a helper until it is actually ready to call one (e.g., it may depend on getting an HTTP 401 from the server). 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? -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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Thu, Oct 29, 2015 at 8:10 PM, Jeff Kingwrote: > On Tue, Oct 27, 2015 at 11:46:20PM -0400, Noam Postavsky wrote: >> Perhaps we could express the auto-spawn more explicitly, something >> like "git config credential.pre-helper start-cache-daemon". A way to >> run a command before the credential helpers start would be useful to >> our magit workaround for this issue (currently we start the daemon >> before "push", "fetch", and "pull", but it won't work with user >> aliases that run those commands). > > I'm not clear on when the pre-helper would be run. Git runs the helper > when it needs a credential. What git command would start it? I was just thinking in terms of our current workaround, it would have been helpful to be able to run a command just before the helpers are run. Or in other words, as the first helper. (doing "git -c credential.helper=foo" puts foo as the last helper). > I guess the most elegant thing would be to add an "init" command to the > helper interface. So magit would run: > > git credential init Although, we could use something like that too, as we're currently checking the helpers configured and then running git credential-cache--daemon directly. > I dunno. It almost seems like adding a credentialcache.ignoreHUP option > would be less hacky. :) The pre-helper thing is probably the best way to make the current hacks less hacky, but maybe not so great for actually fixing the problem and getting rid of the need for said hacks. :) > Mostly, I think, because there was a lot of support load in teaching > people to set up ssh. But I guess a lot of those people are on > non-Linux platforms. Hmm, the pre-helper thing would also help for the hack I wrote getting ssh-agent to autostart and work from Emacs on Windows (ssh-agent on Windows is a total PITA). -- 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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Thu, Oct 29, 2015 at 08:43:58PM -0400, Noam Postavsky wrote: > > I'm not clear on when the pre-helper would be run. Git runs the helper > > when it needs a credential. What git command would start it? > > I was just thinking in terms of our current workaround, it would have > been helpful to be able to run a command just before the helpers are > run. Or in other words, as the first helper. (doing "git -c > credential.helper=foo" puts foo as the last helper). If you know the helper you want to run, you are free to just run "git credential-foo". So I don't think that helps the inelegance of your workaround (the real inelegance is that you are assuming that "foo" needs run in the first place). I'm still not sure how the pre-helper would work. What git command kicks off the pre-helper command? Wouldn't it also be subject to the SIGHUP problem? -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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Tue, Oct 27, 2015 at 11:46:20PM -0400, Noam Postavsky wrote: > > I dunno. I think the auto-spawn is really what makes it usable; you can > > drop it in with "git config credential.helper config" and forget about > > it. Anything more fancy requires touching your login/startup files. > > Certainly I'm not opposed to people setting it up outside of the > > auto-spawn, but I wouldn't want that feature to go away. > > Perhaps we could express the auto-spawn more explicitly, something > like "git config credential.pre-helper start-cache-daemon". A way to > run a command before the credential helpers start would be useful to > our magit workaround for this issue (currently we start the daemon > before "push", "fetch", and "pull", but it won't work with user > aliases that run those commands). I'm not clear on when the pre-helper would be run. Git runs the helper when it needs a credential. What git command would start it? Or are you proposing a new credential interface for programs (like magit) to call along the lines of "do any prep work you need; I might be asking for a credential soon", without having to know the specifics of the user's helper config. You can do that already like: echo url=dummy://a:b@c/ | git credential approve though I guess for some helpers, that may actually store the bogus value (for the cache, it inserts a dummy cache entry, but that's not a problem; something with permanent storage would be more annoying). I guess the most elegant thing would be to add an "init" command to the helper interface. So magit would run: git credential init which would then see that we have "credential.helper" defined as "foo", and would run: git credential-foo init which could be a noop, start a daemon, or whatever, depending on how the helper operates. I dunno. It almost seems like adding a credentialcache.ignoreHUP option would be less hacky. :) > I'm not sure it's that widely used. Perhaps most people use ssh-agent? > That's what I do, though I've been experimenting with credential-cache > since it was brought up by a magit user. I don't know. Certainly up until a few years ago, anybody sane was using ssh-agent, because the http password stuff was so awful (no storage, and we didn't even respect 401s for a long time). But these days sites like GitHub push people into using https as the protocol of choice. Mostly, I think, because there was a lot of support load in teaching people to set up ssh. But I guess a lot of those people are on non-Linux platforms. -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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Thu, Oct 29, 2015 at 8:50 PM, Jeff Kingwrote: > workaround (the real inelegance is that you are assuming that "foo" > needs run in the first place). Well, we currently check the output from "git config credential.helpers" to determine what's needed, so the inelegance here is that we reimplement git's checking of this option. > I'm still not sure how the pre-helper would work. What git command kicks > off the pre-helper command? Wouldn't it also be subject to the SIGHUP > problem? Ah, maybe the missing piece I forgot to mention is that we could make our pre/1st-helper be an emacsclient command, which would tell Emacs to startup the daemon. So the daemon would be a subprocess of Emacs, not "git push", thereby avoiding the SIGHUP. In our current workaround we startup the daemon (if it's not running) before git commands that we think are going to run credential helpers (i.e. "push", "pull", "fetch"), hence my thought that it would be nicer if we only did that before git is *actually* going to run the helpers. -- 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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
Jeff Kingwrites: > But these days, people often have several simultaneous sessions open. > They may have multiple ssh sessions to a single machine, or they may > have a bunch of terminal windows open, each of which has a login shell > and will send HUP to its children when it exits. In that case, you have > a meta-session surrounding those individual terminal sessions, and you > probably do want to keep the cache going as long as the meta session[1]. > ... > [1] Of course we have no idea when that meta-session is closed. But if > you have a script that runs on X logout, for instance, you could put > "git credential-cache exit" in it. Yes. Probably the right way forward is to make it a non-issue by teaching users how to control the lifetime of the "daemon" process, and wean them off relying on "it is auto-spawned if you forgot to start", as that convenience of auto-spawning is associated with "...but how it is auto-shutdown really depends on many things in your specific environment", which is the issue. -- 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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Mon, Oct 26, 2015 at 08:50:10PM -0400, Noam Postavsky wrote: > On Mon, Oct 26, 2015 at 5:50 PM, Jeff King wrote: > > But these days, people often have several simultaneous sessions open. > > They may have multiple ssh sessions to a single machine, or they may > > have a bunch of terminal windows open, each of which has a login shell > > and will send HUP to its children when it exits. > > Yes, except that as far as I can tell the shell never sends HUP. Ah, OK, I thought your original problem was too many HUPs. But reading more, it is really "too many HUPs from Emacs". I certainly do not get SIGHUPs when I close a terminal window. But I would not be surprised if that behavior is dependent on shell version, shell options, and terminal version. > > I don't know what shell Noam is using, but I wonder if tweaking > > that option (or a similar one if not bash) might be helpful to signal > > "let this stuff keep running even after I exit". > > My normal login shell is zsh, but I've been testing with bash and I > see the same behaviour with both. But the original reason for this > whole thread is that when running from Emacs (not via shell), the > daemon *always* get a SIGHUP as soon as "git push" finishes, which > makes the caching thing not so useful. We do have a workaround for > this by now though (start the daemon independently before calling "git > push"). That makes sense. According to the emacs docs[1], "killing a buffer sends a SIGHUP signal to all its associated processes". I don't know if that is configurable or not, but even if it were, it might not do the right thing (you probably _do_ want to send a signal to most processes, just not the credential daemon). Certainly the daemon could do more to "daemonize" itself. Besides ignoring SIGHUP, it could detach from the controlling tty, close more descriptors, etc. But along the lines that Junio mentioned, I'm not sure if that's what people want. In general, it _is_ kind of associated with your terminal session and should not live on past it. I wonder if it would work in your case to simply nohup the credential helper. I.e., put this in your git config: [credential] helper = "!nohup git credential-cache" There are probably some weird things with how nohup works, though (e.g., it redirects stderr to stdout, which is not what you want). Ignored signals are inherited by children, so you could probably just do: [credential] helper = "!trap '' HUP; git credential-cache" That _almost_ works. Unfortunately, credential-cache installs its own SIGHUP handler to clean up its socket. So it cleans up the socket, and then chains to the original handler, which was to ignore the signal. Meaning we keep running, but nobody can contact us. Whoops. So I dunno. I think it would be reasonable to provide a config option to tell the cache-daemon to just ignore SIGHUP (or alternatively, a general option to try harder to dissociate itself from the caller). But I'm not sure I'd agree with making it the default. I'd want to know if anybody is actually _using_ the SIGHUP-exits-daemon feature. Clearly neither of us is, but I wouldn't be surprised if other setups are. -Peff [1] http://www.gnu.org/s/emacs/manual/html_node/elisp/Signals-to-Processes.html -- 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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
Jeff Kingwrites: > So I dunno. I think it would be reasonable to provide a config option to > tell the cache-daemon to just ignore SIGHUP (or alternatively, a general > option to try harder to dissociate itself from the caller). But I'm not > sure I'd agree with making it the default. I'd want to know if anybody > is actually _using_ the SIGHUP-exits-daemon feature. Clearly neither of > us is, but I wouldn't be surprised if other setups are. That also is a very good summary of what I think. I agree it is a good thing to have an option between "keep the die-on-HUP for those who have a working set-up that rely on it" and "daemonize fully and require the user to explicitly kill it with 'credential-cache exit'". Thanks. -- 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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Tue, Oct 27, 2015 at 2:47 PM, Jeff Kingwrote: > On Tue, Oct 27, 2015 at 10:52:52AM -0700, Junio C Hamano wrote: >> Yes. Probably the right way forward is to make it a non-issue by >> teaching users how to control the lifetime of the "daemon" process, >> and wean them off relying on "it is auto-spawned if you forgot to >> start", as that convenience of auto-spawning is associated with >> "...but how it is auto-shutdown really depends on many things in >> your specific environment", which is the issue. > > I dunno. I think the auto-spawn is really what makes it usable; you can > drop it in with "git config credential.helper config" and forget about > it. Anything more fancy requires touching your login/startup files. > Certainly I'm not opposed to people setting it up outside of the > auto-spawn, but I wouldn't want that feature to go away. Perhaps we could express the auto-spawn more explicitly, something like "git config credential.pre-helper start-cache-daemon". A way to run a command before the credential helpers start would be useful to our magit workaround for this issue (currently we start the daemon before "push", "fetch", and "pull", but it won't work with user aliases that run those commands). > > I am a little surprised that credential-cache gets wide use. I would > think most people would prefer to use a system-specific secure-storage > helper. I don't know what the state of the art is for that on Linux[1], but > we seem to have only gnome-keyring in contrib/. I'm not sure it's that widely used. Perhaps most people use ssh-agent? That's what I do, though I've been experimenting with credential-cache since it was brought up by a magit user. -- 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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Sun, Oct 25, 2015 at 09:58:56AM -0700, Junio C Hamano wrote: > >>> I cannot speak for the person who was primarily responsible for > >>> designing this behaviour, but I happen to agree with the current > >>> behaviour in the situation where it was designed to be used. Upon > >>> the first use in your session, the "daemon" is auto-spawned, you can > >>> keep talking with that same instance during your session, and you do > >>> not have to do anything special to shut it down when you log out. > >>> Isn't that what happens here? > >> > >> After looking at this some more, I've discovered this is NOT what > >> actually happens here. If I "git push" from a shell and then log out > >> and log in again, another "git push" does NOT ask me for a password. > >> In other words, the daemon is NOT shut down automatically when I log > >> out. Given that, does it make sense to change the daemon to ignore > >> SIGHUP, or is there some way to change it so that it does exit on > >> logout? > > I have a feeling that it would be moving in a wrong direction to > change the code to ignore HUP, as I do think "logout to shutdown" > would be the desired behaviour. If you are not seeing that happen, > perhaps the first thing to do is to figure out why and fix the code > so that it happens? > > I dunno. I'll cc Peff so that he can take a look when he comes > back. I could see it going both ways. If SIGHUP means "I am logging out, my session is over", then I agree it makes sense to drop any credentials. And that is what SIGHUP meant when people logged in through hard-wired terminals. But these days, people often have several simultaneous sessions open. They may have multiple ssh sessions to a single machine, or they may have a bunch of terminal windows open, each of which has a login shell and will send HUP to its children when it exits. In that case, you have a meta-session surrounding those individual terminal sessions, and you probably do want to keep the cache going as long as the meta session[1]. This is all further complicated by bash's huponexit option, which I think is off by default. So I, for example, have never noticed this behavior even with multiple xterms, because my cache never actually gets SIGHUP. I don't know what shell Noam is using, but I wonder if tweaking that option (or a similar one if not bash) might be helpful to signal "let this stuff keep running even after I exit". But I am also not opposed to making it configurable somehow in git, if there really are two cases that cannot otherwise be distinguished. -Peff [1] Of course we have no idea when that meta-session is closed. But if you have a script that runs on X logout, for instance, you could put "git credential-cache exit" in it. -- 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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Mon, Oct 26, 2015 at 5:50 PM, Jeff King wrote: > But these days, people often have several simultaneous sessions open. > They may have multiple ssh sessions to a single machine, or they may > have a bunch of terminal windows open, each of which has a login shell > and will send HUP to its children when it exits. Yes, except that as far as I can tell the shell never sends HUP. > This is all further complicated by bash's huponexit option, which I > think is off by default. So I, for example, have never noticed this > behavior even with multiple xterms, because my cache never actually gets > SIGHUP. Interesting, I was not aware of this option. I tried enabling it, but I see no difference, the daemon still receives no SIGHUP. Could it be a bug in my version of bash? GNU bash, version 4.3.42(1)-release (x86_64-pc-linux-gnu) Ah, it seems I'm not the only one: (Raphael Ahrens at http://unix.stackexchange.com/a/85296/47926) Bash seems to send the SIGHUP only if it self received a SIGHUP[...] So if you type exit or press Ctrl+D all background process will remain, since this does not send a hang up signal to the Bash. [...] There is an option to send the SIGHUP on exit, but it does not work on my Bash 4.2.25. Maybe it works for you > I don't know what shell Noam is using, but I wonder if tweaking > that option (or a similar one if not bash) might be helpful to signal > "let this stuff keep running even after I exit". My normal login shell is zsh, but I've been testing with bash and I see the same behaviour with both. But the original reason for this whole thread is that when running from Emacs (not via shell), the daemon *always* get a SIGHUP as soon as "git push" finishes, which makes the caching thing not so useful. We do have a workaround for this by now though (start the daemon independently before calling "git push"). -- 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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
Noam Postavskywrites: > On Tue, Oct 20, 2015 at 10:35 PM, Noam Postavsky > wrote: >> On Sun, Oct 18, 2015 at 1:58 PM, Junio C Hamano wrote: >>> I cannot speak for the person who was primarily responsible for >>> designing this behaviour, but I happen to agree with the current >>> behaviour in the situation where it was designed to be used. Upon >>> the first use in your session, the "daemon" is auto-spawned, you can >>> keep talking with that same instance during your session, and you do >>> not have to do anything special to shut it down when you log out. >>> Isn't that what happens here? >> >> After looking at this some more, I've discovered this is NOT what >> actually happens here. If I "git push" from a shell and then log out >> and log in again, another "git push" does NOT ask me for a password. >> In other words, the daemon is NOT shut down automatically when I log >> out. Given that, does it make sense to change the daemon to ignore >> SIGHUP, or is there some way to change it so that it does exit on >> logout? I have a feeling that it would be moving in a wrong direction to change the code to ignore HUP, as I do think "logout to shutdown" would be the desired behaviour. If you are not seeing that happen, perhaps the first thing to do is to figure out why and fix the code so that it happens? I dunno. I'll cc Peff so that he can take a look when he comes back. -- 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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Tue, Oct 20, 2015 at 10:35 PM, Noam Postavskywrote: > On Sun, Oct 18, 2015 at 1:58 PM, Junio C Hamano wrote: >> I cannot speak for the person who was primarily responsible for >> designing this behaviour, but I happen to agree with the current >> behaviour in the situation where it was designed to be used. Upon >> the first use in your session, the "daemon" is auto-spawned, you can >> keep talking with that same instance during your session, and you do >> not have to do anything special to shut it down when you log out. >> Isn't that what happens here? > > After looking at this some more, I've discovered this is NOT what > actually happens here. If I "git push" from a shell and then log out > and log in again, another "git push" does NOT ask me for a password. > In other words, the daemon is NOT shut down automatically when I log > out. Given that, does it make sense to change the daemon to ignore > SIGHUP, or is there some way to change it so that it does exit on > logout? ping? -- 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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Sun, Oct 18, 2015 at 1:58 PM, Junio C Hamanowrote: > I cannot speak for the person who was primarily responsible for > designing this behaviour, but I happen to agree with the current > behaviour in the situation where it was designed to be used. Upon > the first use in your session, the "daemon" is auto-spawned, you can > keep talking with that same instance during your session, and you do > not have to do anything special to shut it down when you log out. > Isn't that what happens here? After looking at this some more, I've discovered this is NOT what actually happens here. If I "git push" from a shell and then log out and log in again, another "git push" does NOT ask me for a password. In other words, the daemon is NOT shut down automatically when I log out. Given that, does it make sense to change the daemon to ignore SIGHUP, or is there some way to change it so that it does exit on logout? - I don't understand why this happens, but the attached self-contained pair of programs demonstrate the behaviour: If I do make call-note-sighup note-sighup urxvt -e ./call-note-sighup ; cat note-sighup sighup.log DOES contain "got sighup", but if I instead do make call-note-sighup note-sighup ./call-note-sighup ; exit afterwards sighup.log does NOT contain "got sighup" (and I must do "pkill note-sighup" to get rid of the lingering note-sighup process). #include #include int main(void) { remove("sighup.log"); int pid = fork(); if (pid < 0) { perror("fork"); return 1; } else if (pid == 0) { execl("./note-sighup", "./note-sighup", NULL); perror("execl(./note-sighup)"); return 1; } else { fprintf(stderr, "waiting for sigup.log to appear\n"); while (!access("sighup.log", F_OK)) { } fprintf(stderr, "okay, note-sighup.log has arrived, exiting in 5 seconds...\n"); sleep(5); } return 0; } #include #include #include #include static volatile int got_sighup = 0; void sighup_handler(int sig) { got_sighup = 1; } int main(void) { struct sigaction act; memset(, 0, sizeof act); act.sa_handler = sighup_handler; sigaction(SIGHUP, , NULL); FILE* out = fopen("sighup.log", "w"); setbuf(out, NULL); fprintf(out, "starting note-sighup\n"); for (;;) { sleep(1000); if (got_sighup) { got_sighup = 0; fprintf(out, "got sighup\n"); } } return 0; }
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Sun, Oct 18, 2015 at 1:58 PM, Junio C Hamanowrote: > I cannot speak for the person who was primarily responsible for > designing this behaviour, but I happen to agree with the current > behaviour in the situation where it was designed to be used. Upon > the first use in your session, the "daemon" is auto-spawned, you can > keep talking with that same instance during your session, and you do > not have to do anything special to shut it down when you log out. Oh, hmm this does makes sense. Now that I think of the log out case, I think ignoring the SIGHUP would be the wrong thing for us too. > Isn't that what happens here? I think our problem is that when Emacs creates the "git push" process with a pty[1], it somehow puts that process in its own new session, so when "git push" finishes it takes the daemon down with it. But seeing it like this, it seems clear that the problem is from the Emacs side. [1]: and using pipes instead of a pty doesn't allow entering the password: https://github.com/magit/magit/issues/2309#issuecomment-147101903 -- 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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
On Sat, Oct 10, 2015 at 12:45 PM, Noam Postavskywrote: > I noticed that git-credential-cache--daemon quits on SIGHUP. This > seems like surprising behaviour for a daemon. Would it be acceptable > to change it to ignore SIGHUP? ping? -- 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
Re: git-credential-cache--daemon quits on SIGHUP, can we change it to ignore instead?
Noam Postavskywrites: > On Sat, Oct 10, 2015 at 12:45 PM, Noam Postavsky > wrote: >> I noticed that git-credential-cache--daemon quits on SIGHUP. This >> seems like surprising behaviour for a daemon. Would it be acceptable >> to change it to ignore SIGHUP? > > ping? Thanks for pinging. I guess this either fell in the cracks while people were busy discussing other topics, or nobody agreed with the reasoning behind the change, or perhaps a bit of both. In any case, it is a prodent thing to ping on the thread after a week or so, which is what you did. Very much appreciated. I cannot speak for the person who was primarily responsible for designing this behaviour, but I happen to agree with the current behaviour in the situation where it was designed to be used. Upon the first use in your session, the "daemon" is auto-spawned, you can keep talking with that same instance during your session, and you do not have to do anything special to shut it down when you log out. Isn't that what happens here? If this were "when you start the system you start this free-standing daemon once, and it will stay around until it gets shut down. If you are staying around in logged-in state is immaterial" kind of daemon, I'd expect it, upon being killed with HUP, to do something useful, like re-reading its configuration file, and continue, instead of dying. Perhaps you can tweak the system to get both, by making it continue upon HUP by default, but teaching it an option not to (i.e. the current behaviour). Pass that option when spawn_daemon() in credential-cache.c starts the daemon. When using the daemon as a free-standing one (against the way its documentation expects you to---see "git help credential-cache--daemon"), you do not pass that option and your "daemon" will ignore HUP. Hmm? -- 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