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

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

[Noam Postavsky]

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?

(This came up while investigating a Magit bug[1], we are also
considering ways to avoid sending a SIGHUP in the first place)

[1]: https://github.com/magit/magit/issues/2309

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

[Noam Postavsky]

On Sat, Oct 10, 2015 at 12:45 PM, Noam Postavsky
<npostavs@users.sourceforge.net> 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?

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

[Junio C Hamano]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> On Sat, Oct 10, 2015 at 12:45 PM, Noam Postavsky
> <npostavs@users.sourceforge.net> 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?

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

[Noam Postavsky]

On Sun, Oct 18, 2015 at 1:58 PM, Junio C Hamano <gitster@pobox.com> 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.

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

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

[Noam Postavsky]

[-- Attachment #1: Type: text/plain, Size: 1431 bytes --]

On Sun, Oct 18, 2015 at 1:58 PM, Junio C Hamano <gitster@pobox.com> 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 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).

[-- Attachment #2: call-note-sighup.c --]
[-- Type: text/x-csrc, Size: 569 bytes --]

#include <stdio.h>
#include <unistd.h>

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;
}

[-- Attachment #3: note-sighup.c --]
[-- Type: text/x-csrc, Size: 596 bytes --]

#include <unistd.h>
#include <stdio.h>
#include <string.h>
#include <signal.h>

static volatile int got_sighup = 0;

void sighup_handler(int sig) {
    got_sighup = 1;
}

int main(void) {

    struct sigaction act;
    memset(&act, 0, sizeof act);
    act.sa_handler = sighup_handler;
    sigaction(SIGHUP, &act, 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?

[Noam Postavsky]

On Tue, Oct 20, 2015 at 10:35 PM, Noam Postavsky
<npostavs@users.sourceforge.net> wrote:
> On Sun, Oct 18, 2015 at 1:58 PM, Junio C Hamano <gitster@pobox.com> 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?

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

[Junio C Hamano]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> On Tue, Oct 20, 2015 at 10:35 PM, Noam Postavsky
> <npostavs@users.sourceforge.net> wrote:
>> On Sun, Oct 18, 2015 at 1:58 PM, Junio C Hamano <gitster@pobox.com> 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.

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

[Jeff King]

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.

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

[Noam Postavsky]

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").

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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

[Jeff King]

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

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

[Jeff King]

On Tue, Oct 27, 2015 at 10:52:52AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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.

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.

AFAICT, it works pretty well out of the box for most setups (where
terminals do _not_ send SIGHUP; so we auto-start, and then it holds the
credential until the timer expires).

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/.

-Peff

[1] I use Linux, but I do not use any of the common desktop
    environments. However, I have my own personal read-only key program
    that speaks the helper protocol.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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

[Noam Postavsky]

On Tue, Oct 27, 2015 at 2:47 PM, Jeff King <peff@peff.net> wrote:
> 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.

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

[Jeff King]

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

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

[Noam Postavsky]

On Thu, Oct 29, 2015 at 8:10 PM, Jeff King <peff@peff.net> wrote:
> 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).

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

[Jeff King]

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

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

[Noam Postavsky]

On Thu, Oct 29, 2015 at 8:50 PM, Jeff King <peff@peff.net> wrote:
> 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.

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

[Jeff King]

On Thu, Oct 29, 2015 at 09:20:01PM -0400, Noam Postavsky wrote:

> On Thu, Oct 29, 2015 at 8:50 PM, Jeff King <peff@peff.net> wrote:
> > 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

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

[Noam Postavsky]

On Fri, Oct 30, 2015 at 5:08 PM, Jeff King <peff@peff.net> wrote:
> 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", &ignore_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(&socket_file, socket_path);
+
+    if (ignore_sighup) {
+        sigchain_pop(SIGHUP);
+        signal(SIGHUP, SIG_IGN);
+    }
+
     serve_cache(socket_path, debug);
     delete_tempfile(&socket_file);

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

[Jeff King]

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", &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.

> @@ -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

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

[Noam Postavsky]

[-- Attachment #1: Type: text/plain, Size: 1698 bytes --]

On Mon, Nov 9, 2015 at 10:53 AM, Jeff King <peff@peff.net> wrote:
> 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.

[-- Attachment #2: 0001-credential-cache-new-option-to-ignore-sighup.patch --]
[-- Type: text/x-diff, Size: 1771 bytes --]

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.
---
 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..4444e5b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1122,6 +1122,9 @@ credential.<url>.*::
 	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.<tool>.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", &ignore_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(&socket_file, socket_path);
+
+	if (ignore_sighup)
+		signal(SIGHUP, SIG_IGN);
+
 	serve_cache(socket_path, debug);
 	delete_tempfile(&socket_file);
 
-- 
2.6.1

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

[Jeff King]

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@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

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

[Jeff King]

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

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

[Noam Postavsky]

I think you're right about the automated test not being worth the trouble.

On Tue, Nov 10, 2015 at 7:26 AM, Jeff King <peff@peff.net> wrote:
> 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.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> 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.

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(&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

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

[Jeff King]

On Fri, Dec 04, 2015 at 10:55:32AM -0800, Junio C Hamano wrote:

> >> +	int ignore_sighup = 0;
> >> +	git_config_get_bool("credentialcache.ignoresighup", &ignore_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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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

[Jeff King]

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