Regression: Ctrl-c from the pager in an alias exits it

Regression: Ctrl-c from the pager in an alias exits it

[Trygve Aaberge]

I'm experiencing an issue when using aliases for commands that open the pager.
When I press Ctrl-c from the pager, it exits. This does not happen when I
don't use an alias and did not happen before. It causes problems because
Ctrl-c is also used for other things, such as canceling a search that hasn't
completed.

To reproduce, create e.g. the alias `l = log` and run `git l`. Then press
Ctrl-c. The expected behavior is that nothing happens. The actual behavior is
that the pager exits.

I bisected the repo, and found that the commit 86d26f240 [0] introduced the
issue.

[0]: 86d26f240 (setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE
                when .. - 2015-12-20)

-- 
Trygve Aaberge

Re: Regression: Ctrl-c from the pager in an alias exits it

[Jeff King]

On Thu, Jan 05, 2017 at 03:25:29PM +0100, Trygve Aaberge wrote:

> I'm experiencing an issue when using aliases for commands that open the pager.
> When I press Ctrl-c from the pager, it exits. This does not happen when I
> don't use an alias and did not happen before. It causes problems because
> Ctrl-c is also used for other things, such as canceling a search that hasn't
> completed.
> 
> To reproduce, create e.g. the alias `l = log` and run `git l`. Then press
> Ctrl-c. The expected behavior is that nothing happens. The actual behavior is
> that the pager exits.

That's weird. I can't reproduce at all here. But I also can't think of a
thing that Git could do that would impact the behavior. For example:

  1. Git should never kill() the pager; in fact it waits for it to
     finish.

  2. Git can impact the pager environment and command line options, but
     those haven't changed anytime recently (and besides, I'm not sure
     there even is an option to convince less to die).

  3. Git can impact the set of blocked signals that the pager sees, but
     I think less would set up its own handler for SIGINT anyway.

I suppose it's possible that your shell or another program (tmux,
maybe?) catches the SIGINT and does a process-group kill. But then I
don't know why it would matter that you're using an alias. The process
tree without an alias:

  |-xterm,21861,peff
  |   `-bash,21863
  |       `-git,22376 log
  |           `-less,22377

and with:

  |-xterm,21861,peff
  |   `-bash,21863
  |       `-git,22391 l
  |           `-git-log,22393
  |               `-less,22394

are pretty similar.

Puzzling.

-Peff

Re: Regression: Ctrl-c from the pager in an alias exits it

[Jeff King]

On Fri, Jan 06, 2017 at 01:40:32AM -0500, Jeff King wrote:

> On Thu, Jan 05, 2017 at 03:25:29PM +0100, Trygve Aaberge wrote:
> 
> > I'm experiencing an issue when using aliases for commands that open the pager.
> > When I press Ctrl-c from the pager, it exits. This does not happen when I
> > don't use an alias and did not happen before. It causes problems because
> > Ctrl-c is also used for other things, such as canceling a search that hasn't
> > completed.
> > 
> > To reproduce, create e.g. the alias `l = log` and run `git l`. Then press
> > Ctrl-c. The expected behavior is that nothing happens. The actual behavior is
> > that the pager exits.
> 
> That's weird. I can't reproduce at all here. But I also can't think of a
> thing that Git could do that would impact the behavior. For example:

I take it back. I could not reproduce under my normal config, which sets
the pager to "diff-highlight | less". But if I drop that config, I can
reproduce easily. And bisect it to that same commit, 86d26f240f
(setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..,
2015-12-20).

-Peff

Re: Regression: Ctrl-c from the pager in an alias exits it

[Jeff King]

On Fri, Jan 06, 2017 at 01:47:52AM -0500, Jeff King wrote:

> > > To reproduce, create e.g. the alias `l = log` and run `git l`. Then press
> > > Ctrl-c. The expected behavior is that nothing happens. The actual behavior is
> > > that the pager exits.
> > 
> > That's weird. I can't reproduce at all here. But I also can't think of a
> > thing that Git could do that would impact the behavior. For example:
> 
> I take it back. I could not reproduce under my normal config, which sets
> the pager to "diff-highlight | less". But if I drop that config, I can
> reproduce easily. And bisect it to that same commit, 86d26f240f
> (setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..,
> 2015-12-20).

I made a little progress with strace. Prior to 86d26f240f, we would run
the "log" builtin directly inside the same executable. After it, we exec
a separate process, and the process tree looks like:

  |       `-bash,10123
  |           `-git,10125 l
  |               `-git-log,10127
  |                   `-less,10128

Now if I strace all of those, I see (I've reordered and snipped a bit
for clarity):

  $ strace -p 10125 -p 10127 -p 10128
  strace: Process 10125 attached
  strace: Process 10127 attached
  strace: Process 10128 attached
  [pid 10127] write(1, "\n\33[33mcommit bae73e80d48ace1faa3"..., 1481 <unfinished ...>
  [pid 10128] read(5,  <unfinished ...>
  [pid 10125] wait4(10127,  <unfinished ...>

The main git process is waiting for the child to finish, the child is
blocked writing to the pager, and the pager is waiting for input from
the terminal (fd 5).

So I hit ^C:

  [pid 10128] <... read resumed> 0x7ffd39153b57, 1) = ? ERESTARTSYS (To be restarted if SA_RESTART is set)
  [pid 10127] <... write resumed> )       = ? ERESTARTSYS (To be restarted if SA_RESTART is set)
  [pid 10125] <... wait4 resumed> 0x7ffe88d0a560, 0, NULL) = ? ERESTARTSYS (To be restarted if SA_RESTART is set)
  [pid 10128] --- SIGINT {si_signo=SIGINT, si_code=SI_KERNEL} ---
  [pid 10127] --- SIGINT {si_signo=SIGINT, si_code=SI_KERNEL} ---
  [pid 10125] --- SIGINT {si_signo=SIGINT, si_code=SI_KERNEL} ---

Everybody gets the signal...

  [pid 10127] write(1, "\n\33[33mcommit bae73e80d48ace1faa3"..., 1481 <unfinished ...>

The blocked writer will resume its write() until it returns. This is the
same as it would get under the older code, too (after write() returns it
will handle the signal and die).

  [pid 10125] kill(10127, SIGINT <unfinished ...>
  [pid 10125] <... kill resumed> )        = 0

The parent git process tries to propagate the signal to the child.
Unnecessary in this instance, but helpful when the signal is only
delivered to the parent. This is due to 

  [pid 10128] rt_sigaction(SIGINT, {sa_handler=0x558dd1af0300, sa_mask=[INT], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7f03fc8c2040},  <unfinished ...>
  [pid 10128] <... rt_sigaction resumed> {sa_handler=0x558dd1af0300, sa_mask=[INT], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7f03fc8c2040}, 8) = 0
  [pid 10128] rt_sigprocmask(SIG_SETMASK, [],  <unfinished ...>
  [pid 10128] <... rt_sigprocmask resumed> NULL, 8) = 0
  [pid 10128] write(1, "\7\r\33[K:\33[K", 9 <unfinished ...>
  [pid 10128] <... write resumed> )       = 9
  [pid 10128] read(5,  <unfinished ...>

And here's the pager handling the signal, and going back to waiting for
terminal input.

  [pid 10125] rt_sigaction(SIGINT, {sa_handler=SIG_DFL, sa_mask=[INT], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7fbe2b4c1040},  <unfinished ...>
  [pid 10125] <... rt_sigaction resumed> {sa_handler=0x55aec373a6a0, sa_mask=[INT], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7fbe2b4c1040}, 8) = 0
  [pid 10125] rt_sigprocmask(SIG_BLOCK, ~[RTMIN RT_1], [INT], 8) = 0
  [pid 10125] getpid( <unfinished ...>
  [pid 10125] <... getpid resumed> )      = 10125
  [pid 10125] gettid()                    = 10125
  [pid 10125] tgkill(10125, 10125, SIGINT) = 0
  [pid 10125] rt_sigprocmask(SIG_SETMASK, [INT], NULL, 8) = 0
  [pid 10125] rt_sigreturn({mask=[]})     = 61
  [pid 10125] --- SIGINT {si_signo=SIGINT, si_code=SI_TKILL, si_pid=10125, si_uid=1000} ---
  [pid 10125] +++ killed by SIGINT +++

The parent process pops the signal handler and propagates to itself,
dying.

At this point things pause, and nothing happens. But as soon as I hit a
key, the pager dies:

  [pid 10128] <... read resumed> "\r", 1) = 1
  [pid 10128] write(1, "\r\33[K", 4)      = 4
  [pid 10128] write(1, "     \"Another round of MIPS fixe"..., 50) = 50
  [pid 10128] read(5, 0x7ffd39153b57, 1)  = -1 EIO (Input/output error)
  [pid 10128] write(1, "\r\33[K\33[?1l\33>", 11) = 11
  [pid 10128] fsync(5)                    = -1 EINVAL (Invalid argument)
  [pid 10128] ioctl(5, TCGETS, {B38400 opost isig -icanon -echo ...}) = 0
  [pid 10128] ioctl(5, SNDCTL_TMR_STOP or TCSETSW, {B38400 opost isig icanon echo ...}) = -1 EIO (Input/output error)
  [pid 10128] exit_group(1)               = ?
  [pid 10128] +++ exited with 1 +++

The key thing is the EIO we get reading from the terminal. I think
because the head of the process group died, we lost our controlling
terminal.

And then naturally the git-log process gets SIGPIPE and dies:

  <... write resumed> )                   = -1 EPIPE (Broken pipe)
  --- SIGPIPE {si_signo=SIGPIPE, si_code=SI_USER, si_pid=10127, si_uid=1000} ---
  --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=10128, si_uid=1000, si_status=1, si_utime=0, si_stime=0} ---
  write(1, "\n\33[33mcommit bae73e80d48ace1faa3"..., 1481) = -1 EPIPE (Broken pipe)
  close(1)                                = 0
  close(2)                                = 0
  wait4(10128, [{WIFEXITED(s) && WEXITSTATUS(s) == 1}], 0, NULL) = 10128
  rt_sigaction(SIGPIPE, {sa_handler=SIG_DFL, sa_mask=[PIPE], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7fa2d51e2040}, {sa_handler=0x564583689d30, sa_mask=[PIPE], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7fa2d51e2040}, 8) = 0
  rt_sigprocmask(SIG_BLOCK, ~[RTMIN RT_1], [INT PIPE], 8) = 0
  getpid()                                = 10127
  gettid()                                = 10127
  tgkill(10127, 10127, SIGPIPE)           = 0
  rt_sigprocmask(SIG_SETMASK, [INT PIPE], NULL, 8) = 0
  rt_sigreturn({mask=[INT]})              = -1 EPIPE (Broken pipe)
  --- SIGPIPE {si_signo=SIGPIPE, si_code=SI_USER, si_pid=10127, si_uid=1000} ---
  +++ killed by SIGPIPE +++

You'll notice that it actually calls wait() on the pager. That's due to
a3da882120 (pager: do wait_for_pager on signal death, 2009-01-22), which
IIRC was addressing a very similar problem. We want to stop feeding the
pager when we get a signal, but we don't want the main process to
actually exit, or the pager loses the controlling terminal.

In our new scenario we have an extra process, though. The git-log child
will wait on the pager, but the parent process can't. It doesn't know
about it. I think that it in turn needs to wait on the child when it
dies, and then the whole chain will stand still until the pager exits.

-Peff

Re: Regression: Ctrl-c from the pager in an alias exits it

[Jeff King]

On Fri, Jan 06, 2017 at 02:26:02AM -0500, Jeff King wrote:

> You'll notice that it actually calls wait() on the pager. That's due to
> a3da882120 (pager: do wait_for_pager on signal death, 2009-01-22), which
> IIRC was addressing a very similar problem. We want to stop feeding the
> pager when we get a signal, but we don't want the main process to
> actually exit, or the pager loses the controlling terminal.
> 
> In our new scenario we have an extra process, though. The git-log child
> will wait on the pager, but the parent process can't. It doesn't know
> about it. I think that it in turn needs to wait on the child when it
> dies, and then the whole chain will stand still until the pager exits.

And here's a patch to do that. It seems to work.

I'll sleep on it and then write up a commit message tomorrow if it still
makes sense.

diff --git a/run-command.c b/run-command.c
index ca905a9e80..db47c429b7 100644
--- a/run-command.c
+++ b/run-command.c
@@ -29,6 +29,8 @@ static int installed_child_cleanup_handler;
 
 static void cleanup_children(int sig, int in_signal)
 {
+	struct child_to_clean *children_to_wait_for = NULL;
+
 	while (children_to_clean) {
 		struct child_to_clean *p = children_to_clean;
 		children_to_clean = p->next;
@@ -45,6 +47,17 @@ static void cleanup_children(int sig, int in_signal)
 		}
 
 		kill(p->pid, sig);
+		p->next = children_to_wait_for;
+		children_to_wait_for = p;
+	}
+
+	while (children_to_wait_for) {
+		struct child_to_clean *p = children_to_wait_for;
+		children_to_wait_for = p->next;
+
+		while (waitpid(p->pid, NULL, 0) < 0 && errno == EINTR)
+			; /* spin waiting for process exit or error */
+
 		if (!in_signal)
 			free(p);
 	}

Re: Regression: Ctrl-c from the pager in an alias exits it

[Trygve Aaberge]

On Fri, Jan 06, 2017 at 02:32:25 -0500, Jeff King wrote:
> And here's a patch to do that. It seems to work.

Nice, thanks! This works very well for me too.

-- 
Trygve Aaberge

Re: Regression: Ctrl-c from the pager in an alias exits it

[Johannes Sixt]

Am 06.01.2017 um 08:32 schrieb Jeff King:
> On Fri, Jan 06, 2017 at 02:26:02AM -0500, Jeff King wrote:
>
>> You'll notice that it actually calls wait() on the pager. That's due to
>> a3da882120 (pager: do wait_for_pager on signal death, 2009-01-22), which
>> IIRC was addressing a very similar problem. We want to stop feeding the
>> pager when we get a signal, but we don't want the main process to
>> actually exit, or the pager loses the controlling terminal.
>>
>> In our new scenario we have an extra process, though. The git-log child
>> will wait on the pager, but the parent process can't. It doesn't know
>> about it. I think that it in turn needs to wait on the child when it
>> dies, and then the whole chain will stand still until the pager exits.
>
> And here's a patch to do that. It seems to work.
>
> I'll sleep on it and then write up a commit message tomorrow if it still
> makes sense.
>
> diff --git a/run-command.c b/run-command.c
> index ca905a9e80..db47c429b7 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -29,6 +29,8 @@ static int installed_child_cleanup_handler;
>
>  static void cleanup_children(int sig, int in_signal)
>  {
> +	struct child_to_clean *children_to_wait_for = NULL;
> +
>  	while (children_to_clean) {
>  		struct child_to_clean *p = children_to_clean;
>  		children_to_clean = p->next;
> @@ -45,6 +47,17 @@ static void cleanup_children(int sig, int in_signal)
>  		}
>
>  		kill(p->pid, sig);
> +		p->next = children_to_wait_for;
> +		children_to_wait_for = p;
> +	}
> +
> +	while (children_to_wait_for) {
> +		struct child_to_clean *p = children_to_wait_for;
> +		children_to_wait_for = p->next;
> +
> +		while (waitpid(p->pid, NULL, 0) < 0 && errno == EINTR)
> +			; /* spin waiting for process exit or error */
> +
>  		if (!in_signal)
>  			free(p);
>  	}
>

This looks like the minimal change necessary. I wonder, though, whether 
the new local variable is really required. Wouldn't it be sufficient to 
walk the children_to_clean chain twice?

-- Hannes

Re: Regression: Ctrl-c from the pager in an alias exits it

[Jeff King]

On Fri, Jan 06, 2017 at 03:39:59PM +0100, Johannes Sixt wrote:

> > diff --git a/run-command.c b/run-command.c
> > index ca905a9e80..db47c429b7 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -29,6 +29,8 @@ static int installed_child_cleanup_handler;
> > 
> >  static void cleanup_children(int sig, int in_signal)
> >  {
> > +	struct child_to_clean *children_to_wait_for = NULL;
> > +
> >  	while (children_to_clean) {
> >  		struct child_to_clean *p = children_to_clean;
> >  		children_to_clean = p->next;
> > @@ -45,6 +47,17 @@ static void cleanup_children(int sig, int in_signal)
> >  		}
> > 
> >  		kill(p->pid, sig);
> > +		p->next = children_to_wait_for;
> > +		children_to_wait_for = p;
> > +	}
> > +
> > +	while (children_to_wait_for) {
> > +		struct child_to_clean *p = children_to_wait_for;
> > +		children_to_wait_for = p->next;
> > +
> > +		while (waitpid(p->pid, NULL, 0) < 0 && errno == EINTR)
> > +			; /* spin waiting for process exit or error */
> > +
> >  		if (!in_signal)
> >  			free(p);
> >  	}
> > 
> 
> This looks like the minimal change necessary. I wonder, though, whether the
> new local variable is really required. Wouldn't it be sufficient to walk the
> children_to_clean chain twice?

Yeah, I considered that. The fact that we disassemble the list in the
first loop has two side effects:

  1. It lets us free the list as we go (for the !in_signal case).

  2. If we were to get another signal, it makes us sort-of reentrant. We
     will only kill and wait for each pid once.

Obviously (1) moves down to the lower loop, but I was trying to preserve
(2). I'm not sure if it is worth bothering, though. The way we pull
items off of the list is certainly not atomic (it does shorten the race
to a few instructions, though, versus potentially waiting on waitpid()
to return).

My bigger concern with the whole thing is whether we could hit some sort
of deadlock if the child doesn't die when we send it a signal. E.g.,
imagine we have a pipe open to the child and somebody sends SIGTERM to
us. We propagate SIGTERM to the child, and then waitpid() for it. The
child decides to ignore our SIGTERM for some reason and keep reading
until EOF on the pipe. It won't ever get it, and the two processes will
hang forever.

You can argue perhaps that the child is broken in that case. And I doubt
this could trigger when running a git sub-command. But we may add more
children in the future. Right now we use it for the new multi-file
clean/smudge filters. They use the hook feature to close the
descriptors, but note that that won't run in the in_signal case.

So I dunno. Maybe this waiting should be restricted only to certain
cases like executing git sub-commands.

-Peff

Re: Regression: Ctrl-c from the pager in an alias exits it

[Johannes Sixt]

Am 06.01.2017 um 20:41 schrieb Jeff King:
> On Fri, Jan 06, 2017 at 03:39:59PM +0100, Johannes Sixt wrote:
>
>>> diff --git a/run-command.c b/run-command.c
>>> index ca905a9e80..db47c429b7 100644
>>> --- a/run-command.c
>>> +++ b/run-command.c
>>> @@ -29,6 +29,8 @@ static int installed_child_cleanup_handler;
>>>
>>>  static void cleanup_children(int sig, int in_signal)
>>>  {
>>> +	struct child_to_clean *children_to_wait_for = NULL;
>>> +
>>>  	while (children_to_clean) {
>>>  		struct child_to_clean *p = children_to_clean;
>>>  		children_to_clean = p->next;
>>> @@ -45,6 +47,17 @@ static void cleanup_children(int sig, int in_signal)
>>>  		}
>>>
>>>  		kill(p->pid, sig);
>>> +		p->next = children_to_wait_for;
>>> +		children_to_wait_for = p;
>>> +	}
>>> +
>>> +	while (children_to_wait_for) {
>>> +		struct child_to_clean *p = children_to_wait_for;
>>> +		children_to_wait_for = p->next;
>>> +
>>> +		while (waitpid(p->pid, NULL, 0) < 0 && errno == EINTR)
>>> +			; /* spin waiting for process exit or error */
>>> +
>>>  		if (!in_signal)
>>>  			free(p);
>>>  	}
>>>
>>
>> This looks like the minimal change necessary. I wonder, though, whether the
>> new local variable is really required. Wouldn't it be sufficient to walk the
>> children_to_clean chain twice?
>
> Yeah, I considered that. The fact that we disassemble the list in the
> first loop has two side effects:
>
>   1. It lets us free the list as we go (for the !in_signal case).
>
>   2. If we were to get another signal, it makes us sort-of reentrant. We
>      will only kill and wait for each pid once.
>
> Obviously (1) moves down to the lower loop, but I was trying to preserve
> (2). I'm not sure if it is worth bothering, though.

Makes sense.

> The way we pull
> items off of the list is certainly not atomic (it does shorten the race
> to a few instructions, though, versus potentially waiting on waitpid()
> to return).
>
> My bigger concern with the whole thing is whether we could hit some sort
> of deadlock if the child doesn't die when we send it a signal. E.g.,
> imagine we have a pipe open to the child and somebody sends SIGTERM to
> us. We propagate SIGTERM to the child, and then waitpid() for it. The
> child decides to ignore our SIGTERM for some reason and keep reading
> until EOF on the pipe. It won't ever get it, and the two processes will
> hang forever.
>
> You can argue perhaps that the child is broken in that case. And I doubt
> this could trigger when running a git sub-command. But we may add more
> children in the future. Right now we use it for the new multi-file
> clean/smudge filters. They use the hook feature to close the
> descriptors, but note that that won't run in the in_signal case.
>
> So I dunno. Maybe this waiting should be restricted only to certain
> cases like executing git sub-commands.

If given it some thought.

In general, I think it is wrong to wait for child processes when a 
signal was received. After all, it is the purpose of a (deadly) signal 
to have the process go away. There may be programs that know it better, 
like less, but git should not attempt to know better in general.

We do apply some special behavior for certain cases like we do for the 
pager. And now the case with aliases is another special situation. The 
parent git process only delegates to the child, and as such it is 
reasonable that it binds its life time to the first child, which 
executes the expanded alias.

-- Hannes

Re: Regression: Ctrl-c from the pager in an alias exits it

[Jeff King]

On Fri, Jan 06, 2017 at 11:42:25PM +0100, Johannes Sixt wrote:

> > So I dunno. Maybe this waiting should be restricted only to certain
> > cases like executing git sub-commands.
> 
> If given it some thought.
> 
> In general, I think it is wrong to wait for child processes when a signal
> was received. After all, it is the purpose of a (deadly) signal to have the
> process go away. There may be programs that know it better, like less, but
> git should not attempt to know better in general.
> 
> We do apply some special behavior for certain cases like we do for the
> pager. And now the case with aliases is another special situation. The
> parent git process only delegates to the child, and as such it is reasonable
> that it binds its life time to the first child, which executes the expanded
> alias.

Yeah, I think I agree. That binding is something you want in many cases,
but not necessarily all. The original purpose of clean_on_exit was to
create a binding like that, but of course it can be (and with the
smudge-filter stuff, arguably has been) used for other cases, too.

I'll work up a patch that makes it a separate option, which should be
pretty easy.

-Peff

[PATCH 0/3] fix ^C killing pager when running alias

[Jeff King]

On Fri, Jan 06, 2017 at 06:20:42PM -0500, Jeff King wrote:

> > In general, I think it is wrong to wait for child processes when a signal
> > was received. After all, it is the purpose of a (deadly) signal to have the
> > process go away. There may be programs that know it better, like less, but
> > git should not attempt to know better in general.
> > 
> > We do apply some special behavior for certain cases like we do for the
> > pager. And now the case with aliases is another special situation. The
> > parent git process only delegates to the child, and as such it is reasonable
> > that it binds its life time to the first child, which executes the expanded
> > alias.
> 
> Yeah, I think I agree. That binding is something you want in many cases,
> but not necessarily all. The original purpose of clean_on_exit was to
> create a binding like that, but of course it can be (and with the
> smudge-filter stuff, arguably has been) used for other cases, too.
> 
> I'll work up a patch that makes it a separate option, which should be
> pretty easy.

Yeah, this did turn out to be really easy. I spent most of the time
trying to explain the issue in the commit message in a sane way.
Hopefully it didn't end up _too_ long. :)

The interesting bit is in the third one. The first is a necessary
preparatory step, and the second is a cleanup I noticed in the
neighborhood.

  [1/3]: execv_dashed_external: use child_process struct
  [2/3]: execv_dashed_external: stop exiting with negative code
  [3/3]: execv_dashed_external: wait for child on signal death

 git.c         | 36 +++++++++++++++---------------------
 run-command.c | 19 +++++++++++++++++++
 run-command.h |  1 +
 3 files changed, 35 insertions(+), 21 deletions(-)

-Peff

[PATCH 1/3] execv_dashed_external: use child_process struct

[Jeff King]

When we run a dashed external, we use the one-liner
run_command_v_opt() to do so. Let's switch to using a
child_process struct, which has two advantages:

  1. We can drop all of the allocation and cleanup code for
     building our custom argv array, and just rely on the
     builtin argv_array (at the minor cost of doing a few
     extra mallocs).

  2. We have access to the complete range of child_process
     options, not just the ones that the "_opt()" form can
     forward.

Signed-off-by: Jeff King <peff@peff.net>
---

It's possible people may disagree with reason (2), and we should add a
new RUN_WAIT_AFTER_CLEAN flag in the final patch. IMHO the whole
run_command_v_opt() thing is a good sign that you should be switching to
a child_process struct. :)

 git.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/git.c b/git.c
index dce529fcbf..d0e04d5c97 100644
--- a/git.c
+++ b/git.c
@@ -575,8 +575,7 @@ static void handle_builtin(int argc, const char **argv)
 
 static void execv_dashed_external(const char **argv)
 {
-	struct strbuf cmd = STRBUF_INIT;
-	const char *tmp;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	int status;
 
 	if (get_super_prefix())
@@ -586,30 +585,20 @@ static void execv_dashed_external(const char **argv)
 		use_pager = check_pager_config(argv[0]);
 	commit_pager_choice();
 
-	strbuf_addf(&cmd, "git-%s", argv[0]);
+	argv_array_pushf(&cmd.args, "git-%s", argv[0]);
+	argv_array_pushv(&cmd.args, argv + 1);
+	cmd.clean_on_exit = 1;
+	cmd.silent_exec_failure = 1;
 
-	/*
-	 * argv[0] must be the git command, but the argv array
-	 * belongs to the caller, and may be reused in
-	 * subsequent loop iterations. Save argv[0] and
-	 * restore it on error.
-	 */
-	tmp = argv[0];
-	argv[0] = cmd.buf;
-
-	trace_argv_printf(argv, "trace: exec:");
+	trace_argv_printf(cmd.args.argv, "trace: exec:");
 
 	/*
 	 * if we fail because the command is not found, it is
 	 * OK to return. Otherwise, we just pass along the status code.
 	 */
-	status = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE | RUN_CLEAN_ON_EXIT);
+	status = run_command(&cmd);
 	if (status >= 0 || errno != ENOENT)
 		exit(status);
-
-	argv[0] = tmp;
-
-	strbuf_release(&cmd);
 }
 
 static int run_argv(int *argcp, const char ***argv)
-- 
2.11.0.527.gfef230ca76

[PATCH 2/3] execv_dashed_external: stop exiting with negative code

[Jeff King]

When we try to exec a git sub-command, we pass along the
status code from run_command(). But that may return -1 if we
ran into an error with pipe() or execve(). This tends to
work (and end up as 255 due to twos-complement wraparound
and truncation), but in general it's probably a good idea to
avoid negative exit codes for portability.

We can easily translate to the normal generic "128" code we
get when syscalls cause us to die.

Signed-off-by: Jeff King <peff@peff.net>
---
I know that negative exit codes were a problem once upon a time on
Windows, but I think that is fine since 47e3de0e79 (MinGW: truncate
exit()'s argument to lowest 8 bits, 2009-07-05). Still, I think it's a
weird portability thing that we are better off avoiding (and certainly I
wouldn't be surprised if some callers assume everything >128 is a
signal).

 git.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/git.c b/git.c
index d0e04d5c97..bc2f2a7ec9 100644
--- a/git.c
+++ b/git.c
@@ -593,12 +593,16 @@ static void execv_dashed_external(const char **argv)
 	trace_argv_printf(cmd.args.argv, "trace: exec:");
 
 	/*
-	 * if we fail because the command is not found, it is
-	 * OK to return. Otherwise, we just pass along the status code.
+	 * If we fail because the command is not found, it is
+	 * OK to return. Otherwise, we just pass along the status code,
+	 * or our usual generic code if we were not even able to exec
+	 * the program.
 	 */
 	status = run_command(&cmd);
-	if (status >= 0 || errno != ENOENT)
+	if (status >= 0)
 		exit(status);
+	else if (errno != ENOENT)
+		exit(128);
 }
 
 static int run_argv(int *argcp, const char ***argv)
-- 
2.11.0.527.gfef230ca76

[PATCH 3/3] execv_dashed_external: wait for child on signal death

[Jeff King]

When you hit ^C to interrupt a git command going to a pager,
this usually leaves the pager running. But when a dashed
external is in use, the pager ends up in a funny state and
quits (but only after eating one more character from the
terminal!). This fixes it.

Explaining the reason will require a little background.

When git runs a pager, it's important for the git process to
hang around and wait for the pager to finish, even though it
has no more data to feed it. This is because git spawns the
pager as a child, and thus the git process is the session
leader on the terminal. After it dies, the pager will finish
its current read from the terminal (eating the one
character), and then get EIO trying to read again.

When you hit ^C, that sends SIGINT to git and to the pager,
and it's a similar situation.  The pager ignores it, but the
git process needs to hang around until the pager is done. We
addressed that long ago in a3da882120 (pager: do
wait_for_pager on signal death, 2009-01-22).

But when you have a dashed external (or an alias pointing to
a builtin, which will re-exec git for the builtin), there's
an extra process in the mix. For instance, running:

  git -c alias.l=log log

will end up with a process tree like:

  git (parent)
    \
     git-log (child)
      \
       less (pager)

If you hit ^C, SIGINT goes to all of them. The pager ignores
it, and the child git process will end up in wait_for_pager().
But the parent git process will die, and the usual EIO
trouble happens.

So we really want the parent git process to wait_for_pager(),
but of course it doesn't know anything about the pager at
all, since it was started by the child.  However, we can
have it wait on the git-log child, which in turn is waiting
on the pager. And that's what this patch does.

There are a few design decisions here worth explaining:

  1. The new feature is attached to run-command's
     clean_on_exit feature. Partly this is convenience,
     since that feature already has a signal handler that
     deals with child cleanup.

     But it's also a meaningful connection. The main reason
     that dashed externals use clean_on_exit is to bind the
     two processes together. If somebody kills the parent
     with a signal, we propagate that to the child (in this
     instance with SIGINT, we do propagate but it doesn't
     matter because the original signal went to the whole
     process group). Likewise, we do not want the parent
     to go away until the child has done so.

     In a traditional Unix world, we'd probably accomplish
     this binding by just having the parent execve() the
     child directly. But since that doesn't work on Windows,
     everything goes through run_command's more spawn-like
     interface.

  2. We do _not_ automatically waitpid() on any
     clean_on_exit children. For dashed externals this makes
     sense; we know that the parent is doing nothing but
     waiting for the child to exit anyway. But with other
     children, it's possible that the child, after getting
     the signal, could be waiting on the parent to do
     something (like closing a descriptor). If we were to
     wait on such a child, we'd end up in a deadlock. So
     this errs on the side of caution, and lets callers
     enable the feature explicitly.

  3. When we send children the cleanup signal, we send all
     the signals first, before waiting on any children. This
     is to avoid the case where one child might be waiting
     on another one to exit, causing a deadlock. We inform
     all of them that it's time to die before reaping any.

     In practice, there is only ever one dashed external run
     from a given process, so this doesn't matter much now.
     But it future-proofs us if other callers start using
     the wait_after_clean mechanism.

There's no automated test here, because it would end up racy
and unportable. But it's easy to reproduce the situation by
running the log command given above and hitting ^C.

Signed-off-by: Jeff King <peff@peff.net>
---
 git.c         |  1 +
 run-command.c | 19 +++++++++++++++++++
 run-command.h |  1 +
 3 files changed, 21 insertions(+)

diff --git a/git.c b/git.c
index bc2f2a7ec9..c8fe6637df 100644
--- a/git.c
+++ b/git.c
@@ -588,6 +588,7 @@ static void execv_dashed_external(const char **argv)
 	argv_array_pushf(&cmd.args, "git-%s", argv[0]);
 	argv_array_pushv(&cmd.args, argv + 1);
 	cmd.clean_on_exit = 1;
+	cmd.wait_after_clean = 1;
 	cmd.silent_exec_failure = 1;
 
 	trace_argv_printf(cmd.args.argv, "trace: exec:");
diff --git a/run-command.c b/run-command.c
index ca905a9e80..73bfba7ef9 100644
--- a/run-command.c
+++ b/run-command.c
@@ -29,6 +29,8 @@ static int installed_child_cleanup_handler;
 
 static void cleanup_children(int sig, int in_signal)
 {
+	struct child_to_clean *children_to_wait_for = NULL;
+
 	while (children_to_clean) {
 		struct child_to_clean *p = children_to_clean;
 		children_to_clean = p->next;
@@ -45,6 +47,23 @@ static void cleanup_children(int sig, int in_signal)
 		}
 
 		kill(p->pid, sig);
+
+		if (p->process->wait_after_clean) {
+			p->next = children_to_wait_for;
+			children_to_wait_for = p;
+		} else {
+			if (!in_signal)
+				free(p);
+		}
+	}
+
+	while (children_to_wait_for) {
+		struct child_to_clean *p = children_to_wait_for;
+		children_to_wait_for = p->next;
+
+		while (waitpid(p->pid, NULL, 0) < 0 && errno == EINTR)
+			; /* spin waiting for process exit or error */
+
 		if (!in_signal)
 			free(p);
 	}
diff --git a/run-command.h b/run-command.h
index dd1c78c28d..4fa8f65adb 100644
--- a/run-command.h
+++ b/run-command.h
@@ -43,6 +43,7 @@ struct child_process {
 	unsigned stdout_to_stderr:1;
 	unsigned use_shell:1;
 	unsigned clean_on_exit:1;
+	unsigned wait_after_clean:1;
 	void (*clean_on_exit_handler)(struct child_process *process);
 	void *clean_on_exit_handler_cbdata;
 };
-- 
2.11.0.527.gfef230ca76

Re: [PATCH 3/3] execv_dashed_external: wait for child on signal death

[Johannes Sixt]

Am 07.01.2017 um 02:22 schrieb Jeff King:
> When you hit ^C to interrupt a git command going to a pager,
> this usually leaves the pager running. But when a dashed
> external is in use, the pager ends up in a funny state and
> quits (but only after eating one more character from the
> terminal!). This fixes it.
>
> Explaining the reason will require a little background.
>
> When git runs a pager, it's important for the git process to
> hang around and wait for the pager to finish, even though it
> has no more data to feed it. This is because git spawns the
> pager as a child, and thus the git process is the session
> leader on the terminal. After it dies, the pager will finish
> its current read from the terminal (eating the one
> character), and then get EIO trying to read again.
>
> When you hit ^C, that sends SIGINT to git and to the pager,
> and it's a similar situation.  The pager ignores it, but the
> git process needs to hang around until the pager is done. We
> addressed that long ago in a3da882120 (pager: do
> wait_for_pager on signal death, 2009-01-22).
>
> But when you have a dashed external (or an alias pointing to
> a builtin, which will re-exec git for the builtin), there's
> an extra process in the mix. For instance, running:
>
>   git -c alias.l=log log

This should be

   git -c alias.l=log l

>
> will end up with a process tree like:
>
>   git (parent)
>     \
>      git-log (child)
>       \
>        less (pager)
>
> If you hit ^C, SIGINT goes to all of them. The pager ignores
> it, and the child git process will end up in wait_for_pager().
> But the parent git process will die, and the usual EIO
> trouble happens.
>
> So we really want the parent git process to wait_for_pager(),
> but of course it doesn't know anything about the pager at
> all, since it was started by the child.  However, we can
> have it wait on the git-log child, which in turn is waiting
> on the pager. And that's what this patch does.
>
> There are a few design decisions here worth explaining:
>
>   1. The new feature is attached to run-command's
>      clean_on_exit feature. Partly this is convenience,
>      since that feature already has a signal handler that
>      deals with child cleanup.
>
>      But it's also a meaningful connection. The main reason
>      that dashed externals use clean_on_exit is to bind the
>      two processes together. If somebody kills the parent
>      with a signal, we propagate that to the child (in this
>      instance with SIGINT, we do propagate but it doesn't
>      matter because the original signal went to the whole
>      process group). Likewise, we do not want the parent
>      to go away until the child has done so.
>
>      In a traditional Unix world, we'd probably accomplish
>      this binding by just having the parent execve() the
>      child directly. But since that doesn't work on Windows,
>      everything goes through run_command's more spawn-like
>      interface.
>
>   2. We do _not_ automatically waitpid() on any
>      clean_on_exit children. For dashed externals this makes
>      sense; we know that the parent is doing nothing but
>      waiting for the child to exit anyway. But with other
>      children, it's possible that the child, after getting
>      the signal, could be waiting on the parent to do
>      something (like closing a descriptor). If we were to
>      wait on such a child, we'd end up in a deadlock. So
>      this errs on the side of caution, and lets callers
>      enable the feature explicitly.
>
>   3. When we send children the cleanup signal, we send all
>      the signals first, before waiting on any children. This
>      is to avoid the case where one child might be waiting
>      on another one to exit, causing a deadlock. We inform
>      all of them that it's time to die before reaping any.
>
>      In practice, there is only ever one dashed external run
>      from a given process, so this doesn't matter much now.
>      But it future-proofs us if other callers start using
>      the wait_after_clean mechanism.
>
> There's no automated test here, because it would end up racy
> and unportable. But it's easy to reproduce the situation by
> running the log command given above and hitting ^C.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  git.c         |  1 +
>  run-command.c | 19 +++++++++++++++++++
>  run-command.h |  1 +
>  3 files changed, 21 insertions(+)
>
> diff --git a/git.c b/git.c
> index bc2f2a7ec9..c8fe6637df 100644
> --- a/git.c
> +++ b/git.c
> @@ -588,6 +588,7 @@ static void execv_dashed_external(const char **argv)
>  	argv_array_pushf(&cmd.args, "git-%s", argv[0]);
>  	argv_array_pushv(&cmd.args, argv + 1);
>  	cmd.clean_on_exit = 1;
> +	cmd.wait_after_clean = 1;
>  	cmd.silent_exec_failure = 1;
>
>  	trace_argv_printf(cmd.args.argv, "trace: exec:");
> diff --git a/run-command.c b/run-command.c
> index ca905a9e80..73bfba7ef9 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -29,6 +29,8 @@ static int installed_child_cleanup_handler;
>
>  static void cleanup_children(int sig, int in_signal)
>  {
> +	struct child_to_clean *children_to_wait_for = NULL;
> +
>  	while (children_to_clean) {
>  		struct child_to_clean *p = children_to_clean;
>  		children_to_clean = p->next;
> @@ -45,6 +47,23 @@ static void cleanup_children(int sig, int in_signal)
>  		}
>
>  		kill(p->pid, sig);
> +
> +		if (p->process->wait_after_clean) {
> +			p->next = children_to_wait_for;
> +			children_to_wait_for = p;
> +		} else {
> +			if (!in_signal)
> +				free(p);
> +		}
> +	}
> +
> +	while (children_to_wait_for) {
> +		struct child_to_clean *p = children_to_wait_for;
> +		children_to_wait_for = p->next;
> +
> +		while (waitpid(p->pid, NULL, 0) < 0 && errno == EINTR)
> +			; /* spin waiting for process exit or error */
> +
>  		if (!in_signal)
>  			free(p);
>  	}
> diff --git a/run-command.h b/run-command.h
> index dd1c78c28d..4fa8f65adb 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -43,6 +43,7 @@ struct child_process {
>  	unsigned stdout_to_stderr:1;
>  	unsigned use_shell:1;
>  	unsigned clean_on_exit:1;
> +	unsigned wait_after_clean:1;
>  	void (*clean_on_exit_handler)(struct child_process *process);
>  	void *clean_on_exit_handler_cbdata;
>  };
>

Very nice write-up and an "obviously correct patch" (FLW).

For the complete series:

Acked-by: Johannes Sixt <j6t@kdbg.org>

What should we add to Documentation/technical/api-run-command.txt about 
the new flag? "Do not use?" "Understand the commit message of <this 
commit> before setting the flag to true?"

-- Hannes

Re: [PATCH 3/3] execv_dashed_external: wait for child on signal death

[Jeff King]

On Sat, Jan 07, 2017 at 08:28:22AM +0100, Johannes Sixt wrote:

> > But when you have a dashed external (or an alias pointing to
> > a builtin, which will re-exec git for the builtin), there's
> > an extra process in the mix. For instance, running:
> > 
> >   git -c alias.l=log log
> 
> This should be
> 
>   git -c alias.l=log l

Yeah, it should be.

> For the complete series:
> 
> Acked-by: Johannes Sixt <j6t@kdbg.org>

Thanks.

> What should we add to Documentation/technical/api-run-command.txt about the
> new flag? "Do not use?" "Understand the commit message of <this commit>
> before setting the flag to true?"

I think it can be explained pretty easily as "after killing all children
marked for clean_on_exit, do a blocking waitpid() on any child that is
also marked wait_after_clean". But I notice we do not actually document
clean_on_exit, either, nor most of the options in "struct
child_process".

IMHO it would be an improvement to merge the contents of the
technical/api-run-command.txt documentation into the header file, and
document each option with a comment above its definition. That makes it
a lot easier to have the will to document any newly-added options,
because if you don't they look bad next to the others. :)

Mechanics aside, I am not sure if we need a "do not use" or "here are
the caveats". I think if we explain what it does, we can rely on list
discussion and review to catch any obviously-stupid uses (both of it and
of clean_on_exit).

-Peff

Re: [PATCH 3/3] execv_dashed_external: wait for child on signal death

[Duy Nguyen]

On Sat, Jan 7, 2017 at 8:22 AM, Jeff King <peff@peff.net> wrote:
> When you hit ^C to interrupt a git command going to a pager,
> this usually leaves the pager running. But when a dashed
> external is in use, the pager ends up in a funny state and
> quits (but only after eating one more character from the
> terminal!). This fixes it.
>
> Explaining the reason will require a little background.
>
> ...

I see I have exposed a bug and I'm glad you are around to fix it. Even
reading your explanation is scary enough, let alone finding it.
Thanks!
-- 
Duy

Re: [PATCH 0/3] fix ^C killing pager when running alias

[Jacob Keller]

On Fri, Jan 6, 2017 at 5:14 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Jan 06, 2017 at 06:20:42PM -0500, Jeff King wrote:
>
>> > In general, I think it is wrong to wait for child processes when a signal
>> > was received. After all, it is the purpose of a (deadly) signal to have the
>> > process go away. There may be programs that know it better, like less, but
>> > git should not attempt to know better in general.
>> >
>> > We do apply some special behavior for certain cases like we do for the
>> > pager. And now the case with aliases is another special situation. The
>> > parent git process only delegates to the child, and as such it is reasonable
>> > that it binds its life time to the first child, which executes the expanded
>> > alias.
>>
>> Yeah, I think I agree. That binding is something you want in many cases,
>> but not necessarily all. The original purpose of clean_on_exit was to
>> create a binding like that, but of course it can be (and with the
>> smudge-filter stuff, arguably has been) used for other cases, too.
>>
>> I'll work up a patch that makes it a separate option, which should be
>> pretty easy.
>
> Yeah, this did turn out to be really easy. I spent most of the time
> trying to explain the issue in the commit message in a sane way.
> Hopefully it didn't end up _too_ long. :)
>
> The interesting bit is in the third one. The first is a necessary
> preparatory step, and the second is a cleanup I noticed in the
> neighborhood.
>
>   [1/3]: execv_dashed_external: use child_process struct
>   [2/3]: execv_dashed_external: stop exiting with negative code
>   [3/3]: execv_dashed_external: wait for child on signal death
>
>  git.c         | 36 +++++++++++++++---------------------
>  run-command.c | 19 +++++++++++++++++++
>  run-command.h |  1 +
>  3 files changed, 35 insertions(+), 21 deletions(-)
>
> -Peff

I don't see the rest of the patches on the list..?

Thanks,
Jake

Re: [PATCH 0/3] fix ^C killing pager when running alias

[Jacob Keller]

On Sat, Jan 7, 2017 at 3:26 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Fri, Jan 6, 2017 at 5:14 PM, Jeff King <peff@peff.net> wrote:
>> On Fri, Jan 06, 2017 at 06:20:42PM -0500, Jeff King wrote:
>>
>>> > In general, I think it is wrong to wait for child processes when a signal
>>> > was received. After all, it is the purpose of a (deadly) signal to have the
>>> > process go away. There may be programs that know it better, like less, but
>>> > git should not attempt to know better in general.
>>> >
>>> > We do apply some special behavior for certain cases like we do for the
>>> > pager. And now the case with aliases is another special situation. The
>>> > parent git process only delegates to the child, and as such it is reasonable
>>> > that it binds its life time to the first child, which executes the expanded
>>> > alias.
>>>
>>> Yeah, I think I agree. That binding is something you want in many cases,
>>> but not necessarily all. The original purpose of clean_on_exit was to
>>> create a binding like that, but of course it can be (and with the
>>> smudge-filter stuff, arguably has been) used for other cases, too.
>>>
>>> I'll work up a patch that makes it a separate option, which should be
>>> pretty easy.
>>
>> Yeah, this did turn out to be really easy. I spent most of the time
>> trying to explain the issue in the commit message in a sane way.
>> Hopefully it didn't end up _too_ long. :)
>>
>> The interesting bit is in the third one. The first is a necessary
>> preparatory step, and the second is a cleanup I noticed in the
>> neighborhood.
>>
>>   [1/3]: execv_dashed_external: use child_process struct
>>   [2/3]: execv_dashed_external: stop exiting with negative code
>>   [3/3]: execv_dashed_external: wait for child on signal death
>>
>>  git.c         | 36 +++++++++++++++---------------------
>>  run-command.c | 19 +++++++++++++++++++
>>  run-command.h |  1 +
>>  3 files changed, 35 insertions(+), 21 deletions(-)
>>
>> -Peff
>
> I don't see the rest of the patches on the list..?
>
> Thanks,
> Jake

They showed up on public inbox so I assume it must be some spam filter
on my end..

Hmm,
Jake