patch, 3 partstest-terminal: give the child an empty stdin TTY

So far, test-terminal.perl did not care at all about the stdin (that is, leave it as-is). This mostly works well, but git-shortlog is a problem:

* It takes decisions based on isatty(0).  (No test checks this, but
  compare 'git shortlog </dev/null' with 'git shortlog' in a
  terminal.)

* It reads all of stdin if !isatty(0) and no arguments were passed.

Because of the latter, t7006.58ff cause unexpected results if you do:

  git rev-list <range> |
  while read sha; do
    git checkout sha
    make test
  done

If t7006 runs during any 'make test' run, the next 'read sha' will fail (git-shortlog ate all of stdin already) and the while loop stops immediately. In particular, this loop will only ever successfully test *one* revision.

To fix this, change test-terminal.perl to open a third PTY for stdin, send an EOF (Ctrl-D) immediately and close it later. Since this may not be portable, we use POSIX::Termios to set it to Ctrl-D.

Noticed-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 t/test-terminal.perl |   22 +++++++++++++++++++---
 1 files changed, 19 insertions(+), 3 deletions(-)

Not setting them to raw mode causes funny things to happen, such as \n -> \r\n translation:

  ./test-terminal.perl echo foo | xxd
  0000000: 666f 6f0d 0a                             foo..

(Notice the added 0d.)

To avoid this, set the (pseudo)terminal to raw mode. Note that the IO::Pty docs recommend doing it on both master and slave.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 t/test-terminal.perl |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

Thomas Rast wrote:

[...]

Good idea, so for what it's worth,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>

Does this change the behavior in <https://rt.cpan.org/Ticket/Display.html?id=65692> (oh please oh please)?

Jonathan Nieder wrote:

I don't think so.  I tested this tweak of the script:

  perl -MIO::Pty -MFile::Copy -e '
    for (my $i = 0;; $i++) {
      my $master = new IO::Pty;
      my $slave = $master->slave;
      $master->set_raw();
      $slave->set_raw();
      if (fork == 0) {
        close $master or die "close: $!";
        open STDOUT, ">&", $slave or die "dup2: $!";
        close $slave or die "close: $!";
        exec("echo", "hi", $i) or die "exec: $!";
      }
      close $slave or die "close: $!";
      copy($master, \*STDOUT) or die "copy: $!";
      close $master or die "close: $!"; wait;
    }
  '

That's over ssh on

  $ uname -a
  Darwin mackeller.inf.ethz.ch 11.1.0 Darwin Kernel Version 11.1.0: Tue Jul 26 16:07:11 PDT 2011; root:xnu-1699.22.81~1/RELEASE_X86_64 x86_64

What's odd is that when I was logged in at university (over Gbit ethernet, but still over ssh), the unmodified version wouldn't even get off the ground. I may just have been dreaming however.

Thomas Rast wrote:

[...]

Thanks. I've passed this information on to Apple (rdar://9046540), though I have no reason to believe anyone reads the reports there. :)

The previous two commits show that getting test-terminal.perl right is not trivial. Let lib-terminal.sh run a simple test that ensures it actually opens TTYs for std{in,out,err} and that it does not let stdin pass through.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 t/lib-terminal.sh |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

Thomas Rast wrote:

This test would get run again for each lib-terminal.sh user. If we want to "test the test machinery" like this, I suppose it would make the most sense to run the check somewhere in the t00* series.

Hi,

Thomas Rast wrote:

I'm not thrilled about this change. The original purpose of test_terminal was to test commands like "git log" that need to check whether stdout is a tty in order to decide whether to use color and to paginate their output. Perhaps whether stdin is a tty _should_ affect those decisions, but it currently doesn't (for example, "echo HEAD | git log --stdin" works) and that would deserve a separate test, I'd think.

The testsuite bug you mentioned sounds like a real one and worth fixing, though. Maybe there would be some way for test_terminal to give the caller some control over which file descriptors to replace with a terminal.

Just musing, Jonathan

Jonathan Nieder wrote:

I'm not sure I understand what you are arguing for or why. That I avoid wasting a Pty, and only replace stdin with /dev/null?

(Because with the current state of the tests, this shouldn't make much of a difference. I just figured I should go all the way and give commands an environment that really looks like they'd been called from the terminal.)

Thomas Rast wrote:

Sorry, I was thinking narrowly about the "git log" tests in t7006-pager.sh. I was saying that there, the fact that lib-terminal.sh creates an environment in which stdin is not guaranteed to be a terminal is a feature, not a bug, since it improves the test coverage (and I tend to find the "stdin not a tty" case more interesting).

That said, a test for the implicit HEAD behavior of "git shortlog" would be a welcome addition to t4201-shortlog.sh, and we might need a helper "test_with_stdin_as_a_terminal" for that.

The tests you mentioned in t7006 are just buggy --- it was just meant to make sure we don't regress in the change introduced by 773b69bf ("shortlog: run setup_git_directory_gently() sooner"), and I doubt I was thinking about the "implicit HEAD when stdin is a tty" behavior at all. So independently of everything else, I believe we should do the following.

-- >8 --
Subject: test: do not let "git shortlog" DWIM based on tty

In the spirit of v1.7.3.3~14^2 (t4203: do not let "git shortlog" DWIM based on tty, 2010-10-19), do not leave out the revision argument to "git shortlog" and rely on the default of HEAD in the usual case that standard input is not connected to a terminal. Otherwise, tests break when you do

  git rev-list <range> |
  while read sha; do
    git checkout sha
    make test
  done

Reported-by: Thomas Rast <trast@student.ethz.ch>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t7006-pager.sh |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

Jonathan Nieder <jrnieder@gmail.com> writes:

I agree with Thomas's objective of giving ttys to all the streams of process being tested by default to emulate the usage better, but I also think being able to test some of the streams redirected to non-tty a good feature to have in test-terminal driver. And I do not think these two have to be either-or proposition.

I do not think "Run 'git log' as if the user is on an interactive terminal, but has plugged /dev/null to its standard input" can be spelled like this:

    test-terminal git log </dev/null

but test-terminal could learn an equivalent feature perhaps from the command line, no? Perhaps like

    test-terminal --stdin=/dev/null git log
    test-terminal --stdout=actual --stderr=error git shortlog

or somesuch?

Thomas Rast <trast@student.ethz.ch> writes:

In the above, lack of dollar-sign in "git checkout $sha" is obvious ;-) but I think it is a bug that you are not running make with its stdin redirected from /dev/null in the first place.

Perhaps "make test" should do that for all tests, not just this terminal related one? Doing it this way we do not have to worry about other tests reading from the standard input by mistake.

-- >8 --
Subject: Do not let the tests read from standard input stream

Consider running a loop like this:

   git rev-list <range> |
   while read commit
   do
	git checkout $commit && make test || break
   done

If any of the test reads from the standard input, we may end up running a test for just one commit (and while running the test for that commit, the remainder of the rev-list output is consumed by the test). A workaround would be to redirect like this:

    git checkout $commit && make test </dev/null || break

but the Makefile should be doing that for us instead.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Junio C Hamano wrote:

[...]

But now you haven't fixed (in t/) 'make prove', 'make valgrind', or ./tXXXX-foo.sh. If anything, this angle of attack should go into test-lib.sh...

On Mon, Dec 12, 2011 at 11:07:21AM -0800, Junio C Hamano wrote:

That was my thought, as well. We want the test environment to be as sterile and predictable as possible, so connecting stdin to /dev/null seems like a sensible thing.

Is this right place to do it?

It doesn't catch "cd t && make". I would expect at the least for it to happen in t/Makefile. But I actually wonder if it should be in test-lib.sh, as it is as much about cleaning up the test script's environment as it is about protecting people running "make test" in a loop. I.e., something like:

Jeff King <peff@peff.net> writes:

That's just an illustration. I know my audiences who will give us real tested patches are intelligent ;-).

And I tend to agree that at the test-lib.sh level might be a bit too low for convenience of debugging, but it is probably a good start. An obvious alternative would be a patch similar to the illustration patch applied to t/Makefile.

As I said in a separate post, I think this is orthogonal to the test-terminal patch under discussion. Being able to give the tested programs an environment that mimics an interactive tty session better is a good thing to do regardless of the "test should not read from make's stdin" issue.

Jeff King wrote:

Independently of the changes to explicitly pass HEAD to "git shortlog" in t7006 (we should) and make test_terminal make stdin into a tty, too (if tests still make sure "git log --stdin" launches a pager with stdin not a tty, then it should be a fine change), this looks good to me.