How to handle terminal detection in a daemon calling git?

How to handle terminal detection in a daemon calling git?

[Travis P]

I've got a script that runs in the background without a terminal.
It actually does have STDOUT and STDERR set to a rotating log file.

When it runs:
  /bin/sh -c 'cd /to/my/wc; git pull --ff-only'
the git command fails (rc 32768).

When it runs
 /bin/sh -c 'cd /to/my/wc; git pull --ff-only > /to/a/file 2>&1'

or even
 /bin/sh -c 'cd /to/my/wc; git pull --ff-only | cat'

then all is well.  The command succeeds (rc 0, and I see the expected
results).

Piping through 'cat' is okay, but I'd rather avoid the 'trick'.  Is
there
some way to communicate to git that it should operate just as if output
were redirected?

-Travis

Re: How to handle terminal detection in a daemon calling git?

[Jeff King]

On Wed, May 30, 2012 at 04:16:47PM -0500, Travis P wrote:

> I've got a script that runs in the background without a terminal.
> It actually does have STDOUT and STDERR set to a rotating log file.
> 
> When it runs:
>   /bin/sh -c 'cd /to/my/wc; git pull --ff-only'
> the git command fails (rc 32768).
> 
> When it runs
>  /bin/sh -c 'cd /to/my/wc; git pull --ff-only > /to/a/file 2>&1'
> 
> or even
>  /bin/sh -c 'cd /to/my/wc; git pull --ff-only | cat'
> 
> then all is well.  The command succeeds (rc 0, and I see the expected
> results).

If your stdout and stderr are not a terminal in the first place (you say
they go to a rotating log file), then that should not be making a
difference. Are they connected by a pty or something odd?

Can you describe the failure in the first case more? Does git produce
any output?

> Piping through 'cat' is okay, but I'd rather avoid the 'trick'.  Is
> there some way to communicate to git that it should operate just as if
> output were redirected?

Git cares about terminals in only a few cases:

  1. We check isatty(2) to enable progress reporting by default. You can
     use --no-progress to disable this. However, in your final example
     you only redirect stdout, which makes me think that stderr is not
     relevant.

  2. We check isatty(1) for starting a pager, auto-selecting color, and
     in recent versions of git, for column support. But none of those
     things should be in use by git-pull anyway.

  3. Merge was changed recently to open an editor when we have a
     terminal. That can be changed by setting GIT_MERGE_AUTOEDIT=no in
     the environment. However, since you pass --ff-only, we shouldn't be
     running merge at all.

So I'm confused. Could it be not related to a terminal at all, but that
there is a problem writing to the original stdout? Something that might
give git a SIGPIPE? Can you describe the original stdout destination
more?

-Peff

Re: How to handle terminal detection in a daemon calling git?

[Sitaram Chamarty]

On Thu, May 31, 2012 at 2:46 AM, Travis P <git@castle.fastmail.fm> wrote:
>
> I've got a script that runs in the background without a terminal.
> It actually does have STDOUT and STDERR set to a rotating log file.
>
> When it runs:
>  /bin/sh -c 'cd /to/my/wc; git pull --ff-only'
> the git command fails (rc 32768).
>
> When it runs
>  /bin/sh -c 'cd /to/my/wc; git pull --ff-only > /to/a/file 2>&1'
>
> or even
>  /bin/sh -c 'cd /to/my/wc; git pull --ff-only | cat'
>
> then all is well.  The command succeeds (rc 0, and I see the expected
> results).
>
> Piping through 'cat' is okay, but I'd rather avoid the 'trick'.  Is
> there
> some way to communicate to git that it should operate just as if output
> were redirected?

would "git --no-pager" help?  Honestly I don't know why your first
error occurred at all -- they should all have worked, IMO.

Re: How to handle terminal detection in a daemon calling git?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, May 30, 2012 at 04:16:47PM -0500, Travis P wrote:
>
>> I've got a script that runs in the background without a terminal.
>> It actually does have STDOUT and STDERR set to a rotating log file.
>> 
>> When it runs:
>>   /bin/sh -c 'cd /to/my/wc; git pull --ff-only'
>> the git command fails (rc 32768).
>> 
>> When it runs
>>  /bin/sh -c 'cd /to/my/wc; git pull --ff-only > /to/a/file 2>&1'
>> 
>> or even
>>  /bin/sh -c 'cd /to/my/wc; git pull --ff-only | cat'
>> 
>> then all is well.  The command succeeds (rc 0, and I see the expected
>> results).
>
> If your stdout and stderr are not a terminal in the first place (you say
> they go to a rotating log file), then that should not be making a
> difference. Are they connected by a pty or something odd?

A more likely failure case is when fd 0, 1 and 2 are _closed_.

I vaguely recall we once saw a failure report for that particular
case, and then audited the code several years ago, but I do not
offhand know if we have regressed over time.

Re: How to handle terminal detection in a daemon calling git?

[Travis P]

On Wed, May 30, 2012, at 11:22 PM, Junio C Hamano wrote:
> 
> A more likely failure case is when fd 0, 1 and 2 are _closed_.
> 
> I vaguely recall we once saw a failure report for that particular
> case, and then audited the code several years ago, but I do not
> offhand know if we have regressed over time.

Thanks all for your suggestions about where to look.  I've been doing
some more experiments, and I see that Junio's comment is very relevant.

Here's what I learned this morning: it appears to work when I don't
close STDIN.

    #close $_ for *STDIN, *STDOUT, *STDERR; # What I was doing. Fails.
    close $_ for *STDOUT, *STDERR;  # Tried this, it works.
    *STDOUT = $log_fh;
    *STDERR = $log_fh;

Jeff King wrote:
> Could it be not related to a terminal at all, but that
> there is a problem writing to the original stdout?

Writing to stdout, stderr are both fine.  I've been doing
  print STDOUT "Stdout test.\n";
  print STDERR "Stderr test.\n";
right before the git commands to ensure that STDOUT, STDERR are hooked
up fine to the log file.

But, you gave me a hint that led to an error message.

In a test rig where I've been looking at this just now, I was using
backticks and losing
a message to stderr:
 'cd /local/test_web; git pull --ff-only | cat'   rc=0 out={{}}
 'cd /local/test_web; git pull --ff-only 2>&1 | cat'   rc=0 out={{fatal:
 The remote end hung up unexpectedly
 
Note however:  despite the "fatal", I am seeing rc=0 and I was observing
my working
copy update as expected, so I wasn't looking for error output.

> 1. We check isatty(2) to enable progress reporting by default. You can
> use --no-progress to disable this.

I don't see "--no-progress" making any difference.

>  2. We check isatty(1) for starting a pager, auto-selecting color, and
>    in recent versions of git, for column support. But none of those
>    things should be in use by git-pull anyway.

Ahh, this could be it:  when the pull does receive an output and I'm
running
the command in the shell, I get output with a "+-" where the plus is
green
and the minus red.  So, I think that git may be trying to check whether
color
 (and columns?) is supported to output.  However, it appears that this
 check
is sensitive to stdin being connected (based on test mentioned earlier
here),
which is surprising.

Is the code that calls isatty, calling it on all 3 descriptors, even
when
STDIN is not relevant?

>  3. Merge was changed recently to open an editor when we have a
>     terminal. That can be changed by setting GIT_MERGE_AUTOEDIT=no in
>     the environment. However, since you pass --ff-only, we shouldn't be
>     running merge at all.

Yes, I tried the env switch just to be sure, and this doesn't appear
relevant.

-Travis

Re: How to handle terminal detection in a daemon calling git?

[Travis P]

Oops, I meant to mention that I'm using git 1.7.6 on RedHat.
Someone else did the git installation.

Re: How to handle terminal detection in a daemon calling git?

[Jeff King]

On Thu, May 31, 2012 at 08:39:42AM -0500, Travis P wrote:

> Here's what I learned this morning: it appears to work when I don't
> close STDIN.
> 
>     #close $_ for *STDIN, *STDOUT, *STDERR; # What I was doing. Fails.
>     close $_ for *STDOUT, *STDERR;  # Tried this, it works.
>     *STDOUT = $log_fh;
>     *STDERR = $log_fh;

Yeah, don't do that.  This can cause subtle bugs in subprocesses. For
example:

  1. You don't have a descriptor 0, because it is closed.

  2. Some part of the program opens a new descriptor (e.g., to read a
     file, making a pipe, etc). This becomes descriptor 0, because it is
     the lowest unused descriptor.

  3. The program wants to redirect its stdin (e.g., because it is
     forking and exec'ing a child). So it calls dup2(fd, 0), closing
     what was at stdin previously, which might have been valuable.

The right thing to do is to redirect stdin from /dev/null, not close it
entirely.

> >  2. We check isatty(1) for starting a pager, auto-selecting color,
> >  and in recent versions of git, for column support. But none of
> >  those things should be in use by git-pull anyway.
> 
> Ahh, this could be it:  when the pull does receive an output and I'm
> running the command in the shell, I get output with a "+-" where the
> plus is green and the minus red.  So, I think that git may be trying
> to check whether color (and columns?) is supported to output.
> However, it appears that this check is sensitive to stdin being
> connected (based on test mentioned earlier here), which is surprising.
>
> Is the code that calls isatty, calling it on all 3 descriptors, even
> when STDIN is not relevant?

No, the color code just checks isatty(1). And even if it checked some
random descriptor, the worst case should be that it affects the color
flag. So I think the terminal thing is a red herring, and it is more
likely you are seeing some subtle issue like the one I described above.

-Peff

Re: How to handle terminal detection in a daemon calling git?

[Travis]

On Jun 1, 2012, at 4:53 AM, Jeff King wrote:

> On Thu, May 31, 2012 at 08:39:42AM -0500, Travis P wrote:
>
>> Here's what I learned this morning: it appears to work when I don't
>> close STDIN.
>>
>>    #close $_ for *STDIN, *STDOUT, *STDERR; # What I was doing. Fails.
>>    close $_ for *STDOUT, *STDERR;  # Tried this, it works.
>>    *STDOUT = $log_fh;
>>    *STDERR = $log_fh;
>
> Yeah, don't do that.  This can cause subtle bugs in subprocesses. For
> example:
>
>  1. You don't have a descriptor 0, because it is closed.
>
>  2. Some part of the program opens a new descriptor (e.g., to read a
>     file, making a pipe, etc). This becomes descriptor 0, because it  
> is
>     the lowest unused descriptor.
>
>  3. The program wants to redirect its stdin (e.g., because it is
>     forking and exec'ing a child). So it calls dup2(fd, 0), closing
>     what was at stdin previously, which might have been valuable.
>
> The right thing to do is to redirect stdin from /dev/null, not close  
> it
> entirely.

Ah, yes, that's a normal thing I would think about in C.  In Perl,
I imagined that those details were handled by Perl.

With that in mind, I'm still seeing strange behavior when I do this,
where it looks to me like I'm closing and then immediately assigning
STDIN:

  my $null_in_fh;
  open($null_in_fh, '<', '/dev/null') or die;
  close *STDIN;  # this appears to mess things up, even with the  
following assignment
  *STDIN = $null_in_fh;

But if I don't do the close STDIN (with or without the glob), then  
things work.
Maybe that assignment doesn't work for some Perl-ish reason.

Because, if I just do this
   open(STDIN, '<', '/dev/null') or die;
even after the close and/or assignment, then all appears okay.

I'll stick with this last technique and just chalk it up to something
to something about Perl I don't understand.  (I had the idea that
maybe I wanted to redirect /dev/null to STDIN at some point anyway.)

Thanks for your comments.

-Travis

Re: How to handle terminal detection in a daemon calling git?

[Jeff King]

On Fri, Jun 01, 2012 at 08:52:04AM -0500, Travis wrote:

> With that in mind, I'm still seeing strange behavior when I do this,
> where it looks to me like I'm closing and then immediately assigning
> STDIN:
> 
>  my $null_in_fh;
>  open($null_in_fh, '<', '/dev/null') or die;
>  close *STDIN;  # this appears to mess things up, even with the
> following assignment
>  *STDIN = $null_in_fh;

Keep in mind that STDIN is a perl filehandle, not a file descriptor.
When you close it, you close the filehandle and its underlying
descriptor. Then you assign another filehandle to STDIN, which has its
own descriptor.

Try this:

  strace perl -e '
    open($null, "</dev/null");
    close *STDIN;
    *STDIN = $null;
    <STDIN>;
  '

you'll see that it is doing something like:

  open("/dev/null", O_RDONLY)             = 3
  close(0)                                = 0
  read(3, "", 8192)                       = 0

The final read comes from the new descriptor, and we never re-opened
descriptor 0. You are basically just pointing the name STDIN to a new
handle, not doing anything with the underlying descriptor.

> Because, if I just do this
>   open(STDIN, '<', '/dev/null') or die;
> even after the close and/or assignment, then all appears okay.

Yeah, this is the right way to re-open a descriptor in perl, because
open() does the magic to dup2 the newly opened descriptor onto STDIN's
descriptor.

> Thanks for your comments.

No problem. Glad it is working now.

-Peff