{"thread":{"id":"29140","subject":"[PATCH 1/3] test-terminal: give the child an empty stdin TTY","startedAt":"2011-12-12T18:09:13Z","lastAt":"2011-12-13T05:09:35Z","messageCount":16,"participants":["Thomas Rast","Jonathan Nieder","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"180941","messageId":"b97d14da67bd6097e5b04f6ae3057c0f1d536a0b.1323713230.git.trast@student.ethz.ch","threadId":"29140","inReplyTo":null,"subject":"[PATCH 1/3] test-terminal: give the child an empty stdin TTY","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-12T18:09:13Z","receivedAt":"2011-12-12T18:09:13Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"So far, test-terminal.perl did not care at all about the stdin (that\nis, leave it as-is).  This mostly works well, but git-shortlog is a\nproblem:\n\n* It takes decisions based on isatty(0).  (No test checks this, but\n  compare 'git shortlog </dev/null' with 'git shortlog' in a\n  terminal.)\n\n* It reads all of stdin if !isatty(0) and no arguments were passed.\n\nBecause of the latter, t7006.58ff cause unexpected results if you do:\n\n  git rev-list <range> |\n  while read sha; do\n    git checkout sha\n    make test\n  done\n\nIf t7006 runs during any 'make test' run, the next 'read sha' will\nfail (git-shortlog ate all of stdin already) and the while loop stops\nimmediately.  In particular, this loop will only ever successfully\ntest *one* revision.\n\nTo fix this, change test-terminal.perl to open a third PTY for stdin,\nsend an EOF (Ctrl-D) immediately and close it later.  Since this may\nnot be portable, we use POSIX::Termios to set it to Ctrl-D.\n\nNoticed-by: Michael Haggerty <mhagger@alum.mit.edu>\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n t/test-terminal.perl |   22 +++++++++++++++++++---\n 1 files changed, 19 insertions(+), 3 deletions(-)\n\ndiff --git a/t/test-terminal.perl b/t/test-terminal.perl\nindex ee01eb9..87b5a8c 100755\n--- a/t/test-terminal.perl\n+++ b/t/test-terminal.perl\n@@ -4,14 +4,16 @@\n use warnings;\n use IO::Pty;\n use File::Copy;\n+use POSIX ();\n \n-# Run @$argv in the background with stdio redirected to $out and $err.\n+# Run @$argv in the background with stdio redirected from $in and to $out and $err.\n sub start_child {\n-\tmy ($argv, $out, $err) = @_;\n+\tmy ($argv, $in, $out, $err) = @_;\n \tmy $pid = fork;\n \tif (not defined $pid) {\n \t\tdie \"fork failed: $!\"\n \t} elsif ($pid == 0) {\n+\t\topen STDIN, \"<&\", $in;\n \t\topen STDOUT, \">&\", $out;\n \t\topen STDERR, \">&\", $err;\n \t\tclose $out;\n@@ -64,13 +66,27 @@ sub copy_stdio {\n \t\tor exit 1;\n }\n \n+sub set_default_eof_char {\n+\tmy $fd = fileno shift;\n+\tmy $termios = POSIX::Termios->new;\n+\t$termios->getattr($fd);\n+\t$termios->setcc(&POSIX::VEOF, 4);\n+\t$termios->setattr($fd, &POSIX::TCSANOW)\n+\t\tor die \"Termios::setattr failed: $!\";\n+}\n+\n if ($#ARGV < 1) {\n \tdie \"usage: test-terminal program args\";\n }\n+my $master_in = new IO::Pty;\n my $master_out = new IO::Pty;\n my $master_err = new IO::Pty;\n-my $pid = start_child(\\@ARGV, $master_out->slave, $master_err->slave);\n+set_default_eof_char($master_in->slave);\n+my $pid = start_child(\\@ARGV, $master_in->slave, $master_out->slave, $master_err->slave);\n+close $master_in->slave;\n close $master_out->slave;\n close $master_err->slave;\n+print $master_in \"\\cD\";\n copy_stdio($master_out, $master_err);\n+close $master_in;\n exit(finish_child($pid));\n-- \n1.7.8.431.g2abf2\n"},{"id":"180942","messageId":"04a77f48dd5d5afebbe625ed25ebecd57b6a8840.1323713230.git.trast@student.ethz.ch","threadId":"29140","inReplyTo":"b97d14da67bd6097e5b04f6ae3057c0f1d536a0b.1323713230.git.trast@student.ethz.ch","subject":"[PATCH 2/3] test-terminal: set output terminals to raw mode","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-12T18:09:14Z","receivedAt":"2011-12-12T18:09:14Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Not setting them to raw mode causes funny things to happen, such as\n\\n -> \\r\\n translation:\n\n  ./test-terminal.perl echo foo | xxd\n  0000000: 666f 6f0d 0a                             foo..\n\n(Notice the added 0d.)\n\nTo avoid this, set the (pseudo)terminal to raw mode.  Note that the\nIO::Pty docs recommend doing it on both master and slave.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n t/test-terminal.perl |    4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/t/test-terminal.perl b/t/test-terminal.perl\nindex 87b5a8c..a2bfbe5 100755\n--- a/t/test-terminal.perl\n+++ b/t/test-terminal.perl\n@@ -81,6 +81,10 @@ sub set_default_eof_char {\n my $master_in = new IO::Pty;\n my $master_out = new IO::Pty;\n my $master_err = new IO::Pty;\n+$master_out->set_raw();\n+$master_err->set_raw();\n+$master_out->slave->set_raw();\n+$master_err->slave->set_raw();\n set_default_eof_char($master_in->slave);\n my $pid = start_child(\\@ARGV, $master_in->slave, $master_out->slave, $master_err->slave);\n close $master_in->slave;\n-- \n1.7.8.431.g2abf2\n"},{"id":"180943","messageId":"44602402ecfd0e49c202f03e87540934aa23bce0.1323713230.git.trast@student.ethz.ch","threadId":"29140","inReplyTo":"b97d14da67bd6097e5b04f6ae3057c0f1d536a0b.1323713230.git.trast@student.ethz.ch","subject":"[PATCH 3/3] t/lib-terminal: test test-terminal's sanity","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-12T18:09:15Z","receivedAt":"2011-12-12T18:09:15Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The previous two commits show that getting test-terminal.perl right is\nnot trivial.  Let lib-terminal.sh run a simple test that ensures it\nactually opens TTYs for std{in,out,err} and that it does not let stdin\npass through.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n t/lib-terminal.sh |   19 +++++++++++++++++++\n 1 files changed, 19 insertions(+), 0 deletions(-)\n\ndiff --git a/t/lib-terminal.sh b/t/lib-terminal.sh\nindex 58d911d..9e4ff9c 100644\n--- a/t/lib-terminal.sh\n+++ b/t/lib-terminal.sh\n@@ -33,3 +33,22 @@ test_expect_success PERL 'set up terminal for tests' '\n \t\t}\n \tfi\n '\n+\n+cat >expect1 <<EOF\n+stdin: 1\n+stdout: 1\n+stderr: 1\n+EOF\n+: >expect2\n+\n+test_expect_success TTY 'test-terminal.perl is sane' '\n+\ttest_terminal perl -e \"\n+\t\tuse POSIX qw(isatty);\n+\t\tprint \\\"stdin: \\\", isatty(STDIN), \\\"\\\\n\\\";\n+\t\tprint \\\"stdout: \\\", isatty(STDOUT), \\\"\\\\n\\\";\n+\t\tprint \\\"stderr: \\\", isatty(STDERR), \\\"\\\\n\\\";\n+\t\" >actual1 &&\n+\ttest_cmp expect1 actual1 &&\n+\techo foo | test_terminal cat - >actual2 &&\n+\ttest_cmp expect2 actual2\n+'\n-- \n1.7.8.431.g2abf2\n"},{"id":"180944","messageId":"20111212181915.GD31793@elie.hsd1.il.comcast.net","threadId":"29140","inReplyTo":"b97d14da67bd6097e5b04f6ae3057c0f1d536a0b.1323713230.git.trast@student.ethz.ch","subject":"Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-12T18:19:15Z","receivedAt":"2011-12-12T18:19:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nThomas Rast wrote:\n\n> --- a/t/test-terminal.perl\n> +++ b/t/test-terminal.perl\n> @@ -4,14 +4,16 @@\n>  use warnings;\n>  use IO::Pty;\n>  use File::Copy;\n> +use POSIX ();\n>\n> -# Run @$argv in the background with stdio redirected to $out and $err.\n> +# Run @$argv in the background with stdio redirected from $in and to $out and $err.\n\nI'm not thrilled about this change.  The original purpose of\ntest_terminal was to test commands like \"git log\" that need to check\nwhether stdout is a tty in order to decide whether to use color and to\npaginate their output.  Perhaps whether stdin is a tty _should_ affect\nthose decisions, but it currently doesn't (for example, \"echo HEAD |\ngit log --stdin\" works) and that would deserve a separate test, I'd\nthink.\n\nThe testsuite bug you mentioned sounds like a real one and worth\nfixing, though.  Maybe there would be some way for test_terminal to\ngive the caller some control over which file descriptors to replace\nwith a terminal.\n\nJust musing,\nJonathan\n"},{"id":"180945","messageId":"20111212182318.GE31793@elie.hsd1.il.comcast.net","threadId":"29140","inReplyTo":"04a77f48dd5d5afebbe625ed25ebecd57b6a8840.1323713230.git.trast@student.ethz.ch","subject":"Re: [PATCH 2/3] test-terminal: set output terminals to raw mode","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-12T18:23:18Z","receivedAt":"2011-12-12T18:23:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Thomas Rast wrote:\n\n> Not setting them to raw mode causes funny things to happen, such as\n> \\n -> \\r\\n translation:\n[...]\n> To avoid this, set the (pseudo)terminal to raw mode.  Note that the\n> IO::Pty docs recommend doing it on both master and slave.\n\nGood idea, so for what it's worth,\nAcked-by: Jonathan Nieder <jrnieder@gmail.com>\n\nDoes this change the behavior in\n<https://rt.cpan.org/Ticket/Display.html?id=65692> (oh please oh\nplease)?\n"},{"id":"180946","messageId":"20111212182530.GF31793@elie.hsd1.il.comcast.net","threadId":"29140","inReplyTo":"44602402ecfd0e49c202f03e87540934aa23bce0.1323713230.git.trast@student.ethz.ch","subject":"Re: [PATCH 3/3] t/lib-terminal: test test-terminal's sanity","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-12T18:25:30Z","receivedAt":"2011-12-12T18:25:30Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Thomas Rast wrote:\n\n> The previous two commits show that getting test-terminal.perl right is\n> not trivial.  Let lib-terminal.sh run a simple test that ensures it\n> actually opens TTYs for std{in,out,err} and that it does not let stdin\n> pass through.\n\nThis test would get run again for each lib-terminal.sh user.  If we\nwant to \"test the test machinery\" like this, I suppose it would make\nthe most sense to run the check somewhere in the t00* series.\n"},{"id":"180947","messageId":"201112121934.40953.trast@student.ethz.ch","threadId":"29140","inReplyTo":"20111212181915.GD31793@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-12T18:34:40Z","receivedAt":"2011-12-12T18:34:40Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jonathan Nieder wrote:\n> Hi,\n> \n> Thomas Rast wrote:\n> > -# Run @$argv in the background with stdio redirected to $out and $err.\n> > +# Run @$argv in the background with stdio redirected from $in and to $out and $err.\n> \n> I'm not thrilled about this change.  The original purpose of\n> test_terminal was to test commands like \"git log\" that need to check\n> whether stdout is a tty in order to decide whether to use color and to\n> paginate their output.  Perhaps whether stdin is a tty _should_ affect\n> those decisions, but it currently doesn't (for example, \"echo HEAD |\n> git log --stdin\" works) and that would deserve a separate test, I'd\n> think.\n> \n> The testsuite bug you mentioned sounds like a real one and worth\n> fixing, though.  Maybe there would be some way for test_terminal to\n> give the caller some control over which file descriptors to replace\n> with a terminal.\n\nI'm not sure I understand what you are arguing for or why.  That I\navoid wasting a Pty, and only replace stdin with /dev/null?\n\n(Because with the current state of the tests, this shouldn't make much\nof a difference.  I just figured I should go all the way and give\ncommands an environment that really looks like they'd been called from\nthe terminal.)\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"180949","messageId":"201112122001.36303.trast@student.ethz.ch","threadId":"29140","inReplyTo":"20111212182318.GE31793@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 2/3] test-terminal: set output terminals to raw mode","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-12T19:01:35Z","receivedAt":"2011-12-12T19:01:35Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jonathan Nieder wrote:\n> Thomas Rast wrote:\n> \n> > Not setting them to raw mode causes funny things to happen, such as\n> > \\n -> \\r\\n translation:\n> [...]\n> > To avoid this, set the (pseudo)terminal to raw mode.  Note that the\n> > IO::Pty docs recommend doing it on both master and slave.\n> \n> Good idea, so for what it's worth,\n> Acked-by: Jonathan Nieder <jrnieder@gmail.com>\n> \n> Does this change the behavior in\n> <https://rt.cpan.org/Ticket/Display.html?id=65692> (oh please oh\n> please)?\n\nI don't think so.  I tested this tweak of the script:\n\n  perl -MIO::Pty -MFile::Copy -e '\n    for (my $i = 0;; $i++) {\n      my $master = new IO::Pty;\n      my $slave = $master->slave;\n      $master->set_raw();\n      $slave->set_raw();\n      if (fork == 0) {\n        close $master or die \"close: $!\";\n        open STDOUT, \">&\", $slave or die \"dup2: $!\";\n        close $slave or die \"close: $!\";\n        exec(\"echo\", \"hi\", $i) or die \"exec: $!\";\n      }\n      close $slave or die \"close: $!\";\n      copy($master, \\*STDOUT) or die \"copy: $!\";\n      close $master or die \"close: $!\"; wait;\n    }\n  '\n\nThat's over ssh on\n\n  $ uname -a\n  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\n\nWhat's odd is that when I was logged in at university (over Gbit\nethernet, but still over ssh), the unmodified version wouldn't even\nget off the ground.  I may just have been dreaming however.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"180950","messageId":"20111212190553.GJ31793@elie.hsd1.il.comcast.net","threadId":"29140","inReplyTo":"201112121934.40953.trast@student.ethz.ch","subject":"Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-12T19:05:53Z","receivedAt":"2011-12-12T19:05:53Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Thomas Rast wrote:\n\n> I'm not sure I understand what you are arguing for or why.  That I\n> avoid wasting a Pty, and only replace stdin with /dev/null?\n\nSorry, I was thinking narrowly about the \"git log\" tests in\nt7006-pager.sh.  I was saying that there, the fact that\nlib-terminal.sh creates an environment in which stdin is not\nguaranteed to be a terminal is a feature, not a bug, since it improves\nthe test coverage (and I tend to find the \"stdin not a tty\" case more\ninteresting).\n\nThat said, a test for the implicit HEAD behavior of \"git shortlog\"\nwould be a welcome addition to t4201-shortlog.sh, and we might need a\nhelper \"test_with_stdin_as_a_terminal\" for that.\n\nThe tests you mentioned in t7006 are just buggy --- it was just meant\nto make sure we don't regress in the change introduced by 773b69bf\n(\"shortlog: run setup_git_directory_gently() sooner\"), and I doubt I\nwas thinking about the \"implicit HEAD when stdin is a tty\" behavior at\nall.  So independently of everything else, I believe we should do the\nfollowing.\n\n-- >8 --\nSubject: test: do not let \"git shortlog\" DWIM based on tty\n\nIn the spirit of v1.7.3.3~14^2 (t4203: do not let \"git shortlog\" DWIM\nbased on tty, 2010-10-19), do not leave out the revision argument to\n\"git shortlog\" and rely on the default of HEAD in the usual case that\nstandard input is not connected to a terminal.  Otherwise, tests break\nwhen you do\n\n  git rev-list <range> |\n  while read sha; do\n    git checkout sha\n    make test\n  done\n\nReported-by: Thomas Rast <trast@student.ethz.ch>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n t/t7006-pager.sh |   10 +++++-----\n 1 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 320e1d1d..6f05b11a 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -409,14 +409,14 @@ test_GIT_PAGER_overrides  expect_success test_must_fail 'git -p'\n \n test_doesnt_paginate      expect_failure test_must_fail 'git -p nonsense'\n \n-test_pager_choices                       'git shortlog'\n+test_pager_choices                       'git shortlog HEAD'\n test_expect_success 'setup: configure shortlog not to paginate' '\n \tgit config pager.shortlog false\n '\n-test_doesnt_paginate      expect_success 'git shortlog'\n-test_no_local_config_subdir expect_success 'git shortlog'\n-test_default_pager        expect_success 'git -p shortlog'\n-test_core_pager_subdir    expect_success 'git -p shortlog'\n+test_doesnt_paginate      expect_success 'git shortlog HEAD'\n+test_no_local_config_subdir expect_success 'git shortlog HEAD'\n+test_default_pager        expect_success 'git -p shortlog HEAD'\n+test_core_pager_subdir    expect_success 'git -p shortlog HEAD'\n \n test_core_pager_subdir    expect_success test_must_fail \\\n \t\t\t\t\t 'git -p apply </dev/null'\n-- \n1.7.8\n"},{"id":"180951","messageId":"7vfwgp4niu.fsf@alter.siamese.dyndns.org","threadId":"29140","inReplyTo":"b97d14da67bd6097e5b04f6ae3057c0f1d536a0b.1323713230.git.trast@student.ethz.ch","subject":"Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-12T19:07:21Z","receivedAt":"2011-12-12T19:07:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> So far, test-terminal.perl did not care at all about the stdin (that\n> is, leave it as-is).  This mostly works well, but git-shortlog is a\n> problem:\n>\n> * It takes decisions based on isatty(0).  (No test checks this, but\n>   compare 'git shortlog </dev/null' with 'git shortlog' in a\n>   terminal.)\n>\n> * It reads all of stdin if !isatty(0) and no arguments were passed.\n>\n> Because of the latter, t7006.58ff cause unexpected results if you do:\n>\n>   git rev-list <range> |\n>   while read sha; do\n>     git checkout sha\n>     make test\n>   done\n\nIn the above, lack of dollar-sign in \"git checkout $sha\" is obvious ;-)\nbut I think it is a bug that you are not running make with its stdin\nredirected from /dev/null in the first place.\n\nPerhaps \"make test\" should do that for all tests, not just this terminal\nrelated one? Doing it this way we do not have to worry about other tests\nreading from the standard input by mistake.\n\n-- >8 --\nSubject: Do not let the tests read from standard input stream\n\nConsider running a loop like this:\n\n   git rev-list <range> |\n   while read commit\n   do\n\tgit checkout $commit && make test || break\n   done\n\nIf any of the test reads from the standard input, we may end up running a\ntest for just one commit (and while running the test for that commit, the\nremainder of the rev-list output is consumed by the test). A workaround\nwould be to redirect like this:\n\n    git checkout $commit && make test </dev/null || break\n\nbut the Makefile should be doing that for us instead.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Makefile |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex ed82320..7a85237 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2239,7 +2239,7 @@ export NO_SVN_TESTS\n ### Testing rules\n \n test: all\n-\t$(MAKE) -C t/ all\n+\t$(MAKE) -C t/ all </dev/null\n \n test-ctype$X: ctype.o\n \n"},{"id":"180953","messageId":"201112122014.32029.trast@student.ethz.ch","threadId":"29140","inReplyTo":"7vfwgp4niu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-12T19:14:31Z","receivedAt":"2011-12-12T19:14:31Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano wrote:\n> \n> Perhaps \"make test\" should do that for all tests, not just this terminal\n> related one? Doing it this way we do not have to worry about other tests\n> reading from the standard input by mistake.\n[...]\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2239,7 +2239,7 @@ export NO_SVN_TESTS\n>  ### Testing rules\n>  \n>  test: all\n> -\t$(MAKE) -C t/ all\n> +\t$(MAKE) -C t/ all </dev/null\n\nBut now you haven't fixed (in t/) 'make prove', 'make valgrind', or\n./tXXXX-foo.sh.  If anything, this angle of attack should go into\ntest-lib.sh...\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"180954","messageId":"20111212191602.GA14061@sigill.intra.peff.net","threadId":"29140","inReplyTo":"7vfwgp4niu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-12T19:16:02Z","receivedAt":"2011-12-12T19:16:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 12, 2011 at 11:07:21AM -0800, Junio C Hamano wrote:\n\n> > Because of the latter, t7006.58ff cause unexpected results if you do:\n> >\n> >   git rev-list <range> |\n> >   while read sha; do\n> >     git checkout sha\n> >     make test\n> >   done\n> \n> In the above, lack of dollar-sign in \"git checkout $sha\" is obvious ;-)\n> but I think it is a bug that you are not running make with its stdin\n> redirected from /dev/null in the first place.\n> \n> Perhaps \"make test\" should do that for all tests, not just this terminal\n> related one? Doing it this way we do not have to worry about other tests\n> reading from the standard input by mistake.\n\nThat was my thought, as well. We want the test environment to be as\nsterile and predictable as possible, so connecting stdin to /dev/null\nseems like a sensible thing.\n\n> diff --git a/Makefile b/Makefile\n> index ed82320..7a85237 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2239,7 +2239,7 @@ export NO_SVN_TESTS\n>  ### Testing rules\n>  \n>  test: all\n> -\t$(MAKE) -C t/ all\n> +\t$(MAKE) -C t/ all </dev/null\n\nIs this right place to do it?\n\nIt doesn't catch \"cd t && make\".  I would expect at the least for it to\nhappen in t/Makefile. But I actually wonder if it should be in\ntest-lib.sh, as it is as much about cleaning up the test script's\nenvironment as it is about protecting people running \"make test\" in a\nloop. I.e., something like:\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex bdd9513..5a38505 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -198,6 +198,9 @@ else\n \texec 4>/dev/null 3>/dev/null\n fi\n \n+exec 6<&0\n+exec 0</dev/null\n+\n test_failure=0\n test_count=0\n test_fixed=0\n\nOne downside of that approach is that it makes it harder to insert\nquestionable debugging statements into test scripts. E.g., sometimes I\nwill temporarily throw a \"gdb\" or even a \"bash\" invocation into a test\nscript to investigate a failure. But that would still be possible by\nredirecting from \"<6\".\n\n-Peff\n"},{"id":"180962","messageId":"7vfwgp3683.fsf@alter.siamese.dyndns.org","threadId":"29140","inReplyTo":"20111212190553.GJ31793@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-12T20:06:20Z","receivedAt":"2011-12-12T20:06:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Sorry, I was thinking narrowly about the \"git log\" tests in\n> t7006-pager.sh.  I was saying that there, the fact that\n> lib-terminal.sh creates an environment in which stdin is not\n> guaranteed to be a terminal is a feature, not a bug, since it improves\n> the test coverage (and I tend to find the \"stdin not a tty\" case more\n> interesting).\n\nI agree with Thomas's objective of giving ttys to all the streams of\nprocess being tested by default to emulate the usage better, but I also\nthink being able to test some of the streams redirected to non-tty a good\nfeature to have in test-terminal driver. And I do not think these two have\nto be either-or proposition.\n\nI do not think \"Run 'git log' as if the user is on an interactive\nterminal, but has plugged /dev/null to its standard input\" can be spelled\nlike this:\n\n    test-terminal git log </dev/null\n\nbut test-terminal could learn an equivalent feature perhaps from the\ncommand line, no?  Perhaps like\n\n    test-terminal --stdin=/dev/null git log\n    test-terminal --stdout=actual --stderr=error git shortlog\n\nor somesuch?\n"},{"id":"180963","messageId":"7v7h2134qn.fsf@alter.siamese.dyndns.org","threadId":"29140","inReplyTo":"20111212191602.GA14061@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-12T20:38:24Z","receivedAt":"2011-12-12T20:38:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Is this right place to do it?\n>\n> It doesn't catch \"cd t && make\"....\n\nThat's just an illustration. I know my audiences who will give us real\ntested patches are intelligent ;-).\n\nAnd I tend to agree that at the test-lib.sh level might be a bit too low\nfor convenience of debugging, but it is probably a good start. An obvious\nalternative would be a patch similar to the illustration patch applied to\nt/Makefile.\n\nAs I said in a separate post, I think this is orthogonal to the\ntest-terminal patch under discussion. Being able to give the tested\nprograms an environment that mimics an interactive tty session better is a\ngood thing to do regardless of the \"test should not read from make's stdin\"\nissue.\n\n> happen in t/Makefile. But I actually wonder if it should be in\n> test-lib.sh, as it is as much about cleaning up the test script's\n> environment as it is about protecting people running \"make test\" in a\n> loop. I.e., something like:\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index bdd9513..5a38505 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -198,6 +198,9 @@ else\n>  \texec 4>/dev/null 3>/dev/null\n>  fi\n>  \n> +exec 6<&0\n> +exec 0</dev/null\n> +\n>  test_failure=0\n>  test_count=0\n>  test_fixed=0\n>\n> One downside of that approach is that it makes it harder to insert\n> questionable debugging statements into test scripts. E.g., sometimes I\n> will temporarily throw a \"gdb\" or even a \"bash\" invocation into a test\n> script to investigate a failure. But that would still be possible by\n> redirecting from \"<6\".\n>\n> -Peff\n"},{"id":"180989","messageId":"20111212232503.GA12366@elie.hsd1.il.comcast.net","threadId":"29140","inReplyTo":"20111212191602.GA14061@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-12T23:25:03Z","receivedAt":"2011-12-12T23:25:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -198,6 +198,9 @@ else\n>  \texec 4>/dev/null 3>/dev/null\n>  fi\n>\n> +exec 6<&0\n> +exec 0</dev/null\n> +\n>  test_failure=0\n>  test_count=0\n>  test_fixed=0\n\nIndependently of the changes to explicitly pass HEAD to \"git shortlog\"\nin t7006 (we should) and make test_terminal make stdin into a tty, too\n(if tests still make sure \"git log --stdin\" launches a pager with\nstdin not a tty, then it should be a fine change), this looks good to\nme.\n"},{"id":"181020","messageId":"20111213050935.GA32215@elie.hsd1.il.comcast.net","threadId":"29140","inReplyTo":"201112122001.36303.trast@student.ethz.ch","subject":"Re: [PATCH 2/3] test-terminal: set output terminals to raw mode","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-13T05:09:35Z","receivedAt":"2011-12-13T05:09:35Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Thomas Rast wrote:\n\n> I tested this tweak of the script:\n[...]\n> That's over ssh on\n>\n>   $ uname -a\n>   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\n\nThanks.  I've passed this information on to Apple (rdar://9046540),\nthough I have no reason to believe anyone reads the reports there. :)\n"}]}