Discussion:
Push not writing to standard error
(too old to reply)
Chase Brammer
2010-10-12 19:04:02 UTC
Permalink
=46irst time on the mailing list, but I enjoy the IRC channel. =A0Excus=
e
me if this is a logged bug, or if there is a known workaround.

When using git outside of bash, or saving the standard error from bash
to a file during a push doesn't seem to be working. =A0I am only able t=
o
get standard output, which doesn't give the progress of the push
(counting, delta, compressing, and writing status). =A0This does howeve=
r
work just fine with git fetch. For example:

git fetch origin master --progress > /fetch_error_ouput.txt 2>&1

Works just fine and writes a long file with the progress data.
However, the following push doesn't write any data (even when pushing
large data sets to verify progress output happens)

git push origin master --progress > ~/push_error_output.txt 2>&1

As far as I can tell this is a bug with push. =A0I am a bit biased
because I really need this feature, but it seems to me that this is a
fairly large bug because pushing is such a pillar to all things git.

Idea's on work arounds or upcoming patches to fix this?

Thanks
Chase Brammer
Jonathan Nieder
2010-10-12 19:21:17 UTC
Permalink
saving the standard error from bas=
h
to a file during a push doesn't seem to be working. =C2=A0I am only a=
ble to
get standard output, which doesn't give the progress of the push
(counting, delta, compressing, and writing status).
[...]
git push origin master --progress > ~/push_error_output.txt 2>&1
[...]
Idea's on work arounds or upcoming patches to fix this?
None from me. But some hints for a patch:

- As the man page says,

--progress

Progress status is reported on the standard error stream
by default when it is attached to a terminal, unless -q is
specified. This flag forces progress status even if the
standard error stream is not directed to a terminal.

It looks like this facility is not working.

- Terminals are distinguished from nonterminals with isatty()

- The "Counting objects..." output comes from pack-objects.
Running with GIT_TRACE=3D1 reveals that the --progress option is
not being passed to pack-objects as it should be.

- Is this a regression? If so, narrowing the regression window
with a few rounds of "git bisect" could be helpful.

Thanks for the report.
Jeff King
2010-10-12 19:32:04 UTC
Permalink
Post by Jonathan Nieder
=20
saving the standard error from b=
ash
Post by Jonathan Nieder
to a file during a push doesn't seem to be working. =C2=A0I am only=
able to
Post by Jonathan Nieder
get standard output, which doesn't give the progress of the push
(counting, delta, compressing, and writing status).
[...]
git push origin master --progress > ~/push_error_output.txt 2>&1
[...]
Idea's on work arounds or upcoming patches to fix this?
=20
=20
- As the man page says,
=20
--progress
=20
Progress status is reported on the standard error stream
by default when it is attached to a terminal, unless -q is
specified. This flag forces progress status even if the
standard error stream is not directed to a terminal.
=20
It looks like this facility is not working.
=20
- Terminals are distinguished from nonterminals with isatty()
=20
- The "Counting objects..." output comes from pack-objects.
Running with GIT_TRACE=3D1 reveals that the --progress option is
not being passed to pack-objects as it should be.
=20
- Is this a regression? If so, narrowing the regression window
with a few rounds of "git bisect" could be helpful.
It looks like transport_set_verbosity gets called correctly, and then
sets the "progress" flag for the transport. But for the push side, I
don't see any transports actually looking at that flag. I think there
needs to be code in git_transport_push to handle the progress flag, and
it just isn't there.

-Peff
Jeff King
2010-10-12 19:38:30 UTC
Permalink
Post by Jeff King
It looks like transport_set_verbosity gets called correctly, and then
sets the "progress" flag for the transport. But for the push side, I
don't see any transports actually looking at that flag. I think there
needs to be code in git_transport_push to handle the progress flag, and
it just isn't there.
Here's a quick 5-minute patch. It works on my test case:

rm -rf parent child
git init parent &&
git clone parent child &&
cd child &&
echo content >file && git add file && git commit -m one &&
git push --progress origin master:foo >foo.out 2>&1 &&
cat foo.out

but I didn't even run the test suite. Maybe somebody more clueful in the
area can pick it up?

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 481602d..efd9be6 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -48,6 +48,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
NULL,
NULL,
NULL,
+ NULL,
};
struct child_process po;
int i;
@@ -59,6 +60,8 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
argv[i++] = "--delta-base-offset";
if (args->quiet)
argv[i++] = "-q";
+ if (args->progress)
+ argv[i++] = "--progress";
memset(&po, 0, sizeof(po));
po.argv = argv;
po.in = -1;
diff --git a/send-pack.h b/send-pack.h
index 60b4ba6..fcf4707 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -4,6 +4,7 @@
struct send_pack_args {
unsigned verbose:1,
quiet:1,
+ progress:1,
porcelain:1,
send_mirror:1,
force_update:1,
diff --git a/transport.c b/transport.c
index 4dba6f8..0078660 100644
--- a/transport.c
+++ b/transport.c
@@ -789,6 +789,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
args.use_thin_pack = data->options.thin;
args.verbose = (transport->verbose > 0);
args.quiet = (transport->verbose < 0);
+ args.progress = transport->progress;
args.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);
args.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN);
Chase Brammer
2010-10-12 20:37:50 UTC
Permalink
Wow, I am amazed at how quick you churned that out. I haven't
participated in the git patch and release cycle, so forgive my
ignorance. Do you think that this will be released in the next
release (1.7.3.2) ? If so, any expectations on release date?

Chase
It looks like transport_set_verbosity gets called correctly, and th=
en
sets the "progress" flag for the transport. But for the push side, =
I
don't see any transports actually looking at that flag. I think the=
re
needs to be code in git_transport_push to handle the progress flag,=
and
it just isn't there.
=A0rm -rf parent child
=A0git init parent &&
=A0git clone parent child &&
=A0cd child &&
=A0echo content >file && git add file && git commit -m one &&
=A0git push --progress origin master:foo >foo.out 2>&1 &&
=A0cat foo.out
but I didn't even run the test suite. Maybe somebody more clueful in =
the
area can pick it up?
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 481602d..efd9be6 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -48,6 +48,7 @@ static int pack_objects(int fd, struct ref *refs, s=
truct extra_have_objects *ext
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0NULL,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0NULL,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0NULL,
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 NULL,
=A0 =A0 =A0 =A0};
=A0 =A0 =A0 =A0struct child_process po;
=A0 =A0 =A0 =A0int i;
@@ -59,6 +60,8 @@ static int pack_objects(int fd, struct ref *refs, s=
truct extra_have_objects *ext
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0argv[i++] =3D "--delta-base-offset";
=A0 =A0 =A0 =A0if (args->quiet)
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0argv[i++] =3D "-q";
+ =A0 =A0 =A0 if (args->progress)
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 argv[i++] =3D "--progress";
=A0 =A0 =A0 =A0memset(&po, 0, sizeof(po));
=A0 =A0 =A0 =A0po.argv =3D argv;
=A0 =A0 =A0 =A0po.in =3D -1;
diff --git a/send-pack.h b/send-pack.h
index 60b4ba6..fcf4707 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -4,6 +4,7 @@
=A0struct send_pack_args {
=A0 =A0 =A0 =A0unsigned verbose:1,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0quiet:1,
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 progress:1,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0porcelain:1,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0send_mirror:1,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0force_update:1,
diff --git a/transport.c b/transport.c
index 4dba6f8..0078660 100644
--- a/transport.c
+++ b/transport.c
@@ -789,6 +789,7 @@ static int git_transport_push(struct transport *t=
ransport, struct ref *remote_re
=A0 =A0 =A0 =A0args.use_thin_pack =3D data->options.thin;
=A0 =A0 =A0 =A0args.verbose =3D (transport->verbose > 0);
=A0 =A0 =A0 =A0args.quiet =3D (transport->verbose < 0);
+ =A0 =A0 =A0 args.progress =3D transport->progress;
=A0 =A0 =A0 =A0args.dry_run =3D !!(flags & TRANSPORT_PUSH_DRY_RUN);
=A0 =A0 =A0 =A0args.porcelain =3D !!(flags & TRANSPORT_PUSH_PORCELAIN=
);
Jeff King
2010-10-12 20:48:46 UTC
Permalink
Post by Chase Brammer
Wow, I am amazed at how quick you churned that out. I haven't
participated in the git patch and release cycle, so forgive my
ignorance. Do you think that this will be released in the next
release (1.7.3.2) ? If so, any expectations on release date?
Well, at 5 minutes it was really only 1 line of code per minute. ;)

I'm hoping that somebody else on the list who has worked in the
transport code recently can comment on whether this is the right fix.
Did you test it? Does it fix your issue?

If it seems OK, then somebody needs to submit a cleaned-up version with
commit message to Junio, who will probably cook it in "next" for at
least a few weeks, and then hopefully it would be in v1.7.3.2. He does
maintenance releases as-needed, which seems to generally be every few
weeks.

-Peff
Chase Brammer
2010-10-12 22:18:58 UTC
Permalink
Peff

Thanks for all the help. It worked fantastic. I hope you don't mind
me packing this into a commit and submitting it to Junio. It is
something I really need in the next release. I don't know much about
protocol here, and I don't want to step on toes.

Chase
Post by Jeff King
Wow, I am amazed at how quick you churned that out. =A0I haven't
participated in the git patch and release cycle, so forgive my
ignorance. =A0Do you think that this will be released in the next
release (1.7.3.2) ? If so, any expectations on release date?
Well, at 5 minutes it was really only 1 line of code per minute. ;)
I'm hoping that somebody else on the list who has worked in the
transport code recently can comment on whether this is the right fix.
Did you test it? Does it fix your issue?
If it seems OK, then somebody needs to submit a cleaned-up version wi=
th
Post by Jeff King
commit message to Junio, who will probably cook it in "next" for at
least a few weeks, and then hopefully it would be in v1.7.3.2. He doe=
s
Post by Jeff King
maintenance releases as-needed, which seems to generally be every few
weeks.
-Peff
Junio C Hamano
2010-10-13 17:33:22 UTC
Permalink
Post by Jeff King
Post by Jeff King
It looks like transport_set_verbosity gets called correctly, and then
sets the "progress" flag for the transport. But for the push side, I
don't see any transports actually looking at that flag. I think there
needs to be code in git_transport_push to handle the progress flag, and
it just isn't there.
rm -rf parent child
git init parent &&
git clone parent child &&
cd child &&
echo content >file && git add file && git commit -m one &&
git push --progress origin master:foo >foo.out 2>&1 &&
cat foo.out
Does it still work with "git push" without --progress? I didn't apply nor
test, but just wondering as the manpage description suggests progress is
implicitly set when standard error is terminal even when there is no
command line --progress is given, and also interaction with -q option, but
the patch does not seem to show such subtleties...
Post by Jeff King
but I didn't even run the test suite. Maybe somebody more clueful in the
area can pick it up?
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 481602d..efd9be6 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -48,6 +48,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
NULL,
NULL,
NULL,
+ NULL,
};
struct child_process po;
int i;
@@ -59,6 +60,8 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
argv[i++] = "--delta-base-offset";
if (args->quiet)
argv[i++] = "-q";
+ if (args->progress)
+ argv[i++] = "--progress";
memset(&po, 0, sizeof(po));
po.argv = argv;
po.in = -1;
diff --git a/send-pack.h b/send-pack.h
index 60b4ba6..fcf4707 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -4,6 +4,7 @@
struct send_pack_args {
unsigned verbose:1,
quiet:1,
+ progress:1,
porcelain:1,
send_mirror:1,
force_update:1,
diff --git a/transport.c b/transport.c
index 4dba6f8..0078660 100644
--- a/transport.c
+++ b/transport.c
@@ -789,6 +789,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
args.use_thin_pack = data->options.thin;
args.verbose = (transport->verbose > 0);
args.quiet = (transport->verbose < 0);
+ args.progress = transport->progress;
args.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);
args.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN);
Jeff King
2010-10-13 17:45:44 UTC
Permalink
Post by Junio C Hamano
Post by Jeff King
rm -rf parent child
git init parent &&
git clone parent child &&
cd child &&
echo content >file && git add file && git commit -m one &&
git push --progress origin master:foo >foo.out 2>&1 &&
cat foo.out
Does it still work with "git push" without --progress? I didn't apply nor
test, but just wondering as the manpage description suggests progress is
implicitly set when standard error is terminal even when there is no
command line --progress is given, and also interaction with -q option, but
the patch does not seem to show such subtleties...
Yes, it works in both of those cases. The transport code already does
the right thing to set transport->progress (see the code at the end of
transport_set_verbosity). And we even pass that value on to remote
helpers, which presumably make use of it. But the internal
git_transport_push simply ignored it (probably because it predates the
rest of the transport code, but I didn't check).

What concerns me a bit is that "git push --no-progress" does not do what
I expected (turn off progress, but keep the status table which would
otherwise be suppressed by "-q"). Instead, --no-progress is silently
ignored. We should at least set it to NONEG to generate an error, but
ideally we would handle it properly.

However, that bug exists with or without my patch. The transport code
seems to only ever consider "force progress" or "default progress", but
never "no progress".

-Peff
Tay Ray Chuan
2010-10-13 19:31:48 UTC
Permalink
*** BLURB HERE ***

Contents:
[PATCH 1/3] t5523-push-upstream: add function to ensure fresh upstream repo
[PATCH 2/3] t5523-push-upstream: test progress messages
[PATCH 3/3] push: pass --progress down to git-pack-objects

builtin/send-pack.c | 3 +++
send-pack.h | 1 +
t/t5523-push-upstream.sh | 24 +++++++++++++++++++++++-
transport.c | 1 +
4 files changed, 28 insertions(+), 1 deletions(-)

--
1.7.2.2.513.ge1ef3
Tay Ray Chuan
2010-10-13 19:31:49 UTC
Permalink
Signed-off-by: Tay Ray Chuan <***@gmail.com>
---
t/t5523-push-upstream.sh | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
index 00da707..337a69e 100755
--- a/t/t5523-push-upstream.sh
+++ b/t/t5523-push-upstream.sh
@@ -3,8 +3,14 @@
test_description='push with --set-upstream'
. ./test-lib.sh

+ensure_fresh_upstream() {
+ test -d parent &&
+ rm -rf parent
+ git init --bare parent
+}
+
test_expect_success 'setup bare parent' '
- git init --bare parent &&
+ ensure_fresh_upstream &&
git remote add upstream parent
'
--
1.7.2.2.513.ge1ef3
Tay Ray Chuan
2010-10-13 19:31:51 UTC
Permalink
From: Jeff King <***@peff.net>

When pushing via builtin transports (like file://, git://), the
underlying transport helper (in this case, git-pack-objects) did not get
the --progress option, even if it was passed to git push.

Fix this, and update the tests to reflect this.

Note that according to the git-pack-objects documentation, we can safely
apply the usual --progress semantics for the transport commands like
clone and fetch (and for pushing over other smart transports).

Reported-by: Chase Brammer <***@gmail.com>
Helped-by: Jonathan Nieder <***@gmail.com>
Signed-off-by: Tay Ray Chuan <***@gmail.com>
---
builtin/send-pack.c | 3 +++
send-pack.h | 1 +
t/t5523-push-upstream.sh | 4 ++--
transport.c | 1 +
4 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 481602d..efd9be6 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -48,6 +48,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
NULL,
NULL,
NULL,
+ NULL,
};
struct child_process po;
int i;
@@ -59,6 +60,8 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
argv[i++] = "--delta-base-offset";
if (args->quiet)
argv[i++] = "-q";
+ if (args->progress)
+ argv[i++] = "--progress";
memset(&po, 0, sizeof(po));
po.argv = argv;
po.in = -1;
diff --git a/send-pack.h b/send-pack.h
index 60b4ba6..05d7ab1 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -5,6 +5,7 @@ struct send_pack_args {
unsigned verbose:1,
quiet:1,
porcelain:1,
+ progress:1,
send_mirror:1,
force_update:1,
use_thin_pack:1,
diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
index 554f55e..113626b 100755
--- a/t/t5523-push-upstream.sh
+++ b/t/t5523-push-upstream.sh
@@ -72,7 +72,7 @@ test_expect_success 'push -u HEAD' '
check_config headbranch upstream refs/heads/headbranch
'

-test_expect_failure 'progress messages to non-tty' '
+test_expect_success 'progress messages to non-tty' '
ensure_fresh_upstream &&

# skip progress messages, since stderr is non-tty
@@ -80,7 +80,7 @@ test_expect_failure 'progress messages to non-tty' '
! grep "Writing objects" err
'

-test_expect_failure 'progress messages to non-tty (forced)' '
+test_expect_success 'progress messages to non-tty (forced)' '
ensure_fresh_upstream &&

# force progress messages to stderr, even though it is non-tty
diff --git a/transport.c b/transport.c
index 4dba6f8..0078660 100644
--- a/transport.c
+++ b/transport.c
@@ -789,6 +789,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
args.use_thin_pack = data->options.thin;
args.verbose = (transport->verbose > 0);
args.quiet = (transport->verbose < 0);
+ args.progress = transport->progress;
args.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);
args.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN);
--
1.7.2.2.513.ge1ef3
Tay Ray Chuan
2010-10-14 00:59:41 UTC
Permalink
Hi,
[snip]
Chase, I switched the authorship to Jeff - after all, he was the one
who wrote the patch. I hope you're fine with that.

Jeff, if this patch is ok, since you're the author, perhaps you might
want to add your SOB?
--
Cheers,
Ray Chuan
Jeff King
2010-10-14 01:24:15 UTC
Permalink
Post by Tay Ray Chuan
[snip]
Chase, I switched the authorship to Jeff - after all, he was the one
who wrote the patch. I hope you're fine with that.
Jeff, if this patch is ok, since you're the author, perhaps you might
want to add your SOB?
Yeah, definitely:

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

-Peff
Tay Ray Chuan
2010-10-13 19:31:50 UTC
Permalink
Reported-by: Chase Brammer <***@gmail.com>
Signed-off-by: Tay Ray Chuan <***@gmail.com>
---
t/t5523-push-upstream.sh | 16 ++++++++++++++++
1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
index 337a69e..554f55e 100755
--- a/t/t5523-push-upstream.sh
+++ b/t/t5523-push-upstream.sh
@@ -72,4 +72,20 @@ test_expect_success 'push -u HEAD' '
check_config headbranch upstream refs/heads/headbranch
'

+test_expect_failure 'progress messages to non-tty' '
+ ensure_fresh_upstream &&
+
+ # skip progress messages, since stderr is non-tty
+ git push -u upstream master >out 2>err &&
+ ! grep "Writing objects" err
+'
+
+test_expect_failure 'progress messages to non-tty (forced)' '
+ ensure_fresh_upstream &&
+
+ # force progress messages to stderr, even though it is non-tty
+ git push -u --progress upstream master >out 2>err &&
+ grep "Writing objects" err
+'
+
test_done
--
1.7.2.2.513.ge1ef3
Jonathan Nieder
2010-10-13 19:30:39 UTC
Permalink
Post by Tay Ray Chuan
--- a/t/t5523-push-upstream.sh
+++ b/t/t5523-push-upstream.sh
@@ -3,8 +3,14 @@
test_description='push with --set-upstream'
. ./test-lib.sh
+ensure_fresh_upstream() {
+ test -d parent &&
+ rm -rf parent
+ git init --bare parent
+}
Wouldn't

rm -rf parent &&
git init --bare parent

do it?
Tay Ray Chuan
2010-10-13 19:35:40 UTC
Permalink
Post by Tay Ray Chuan
*** BLURB HERE ***
Whoops. Let me try again:

This patch series addresses the issue of git push not displaying
progress messages to non-tty stderr, even if --progress is used. As
suggested by the subject, this issue afflicts the "builtin smart
transports" - file://, git://, ssh://. (All of them use
git_transport_push() and thus git-pack-objects.)
--
Cheers,
Ray Chuan
Tay Ray Chuan
2010-10-16 18:36:55 UTC
Permalink
This patch series addresses the issue of git push not displaying
progress messages to non-tty stderr, even if --progress is used. As
suggested by the subject, this issue afflicts the "builtin smart
transports" - file://, git://, ssh://. (All of them use
git_transport_push() and thus git-pack-objects.)

The last patch contains the actual fix; most of the other patches
improve tests that depend on a tty.

Contents:
[PATCH v2 0/8] fix push --progress over file://, git://, etc.
[PATCH v2 1/8] tests: factor out terminal handling from t7006
[PATCH v2 2/8] tests: test terminal output to both stdout and stderr
[PATCH v2 3/8] test-lib: allow test code to check the list of declared prerequisites
[PATCH v2 4/8] test_terminal: catch use without TTY prerequisite
[PATCH v2 5/8] test_terminal: give priority to test-terminal.perl usage
[PATCH v2 6/8] t5523-push-upstream: add function to ensure fresh upstream repo
[PATCH v2 7/8] t5523-push-upstream: test progress messages
[PATCH v2 8/8] push: pass --progress down to git-pack-objects

Jeff King (3):
tests: factor out terminal handling from t7006
tests: test terminal output to both stdout and stderr
push: pass --progress down to git-pack-objects

Jonathan Nieder (3):
test-lib: allow test code to check the list of declared prerequisites
test_terminal: catch use without TTY prerequisite
test_terminal: give priority to test-terminal.perl usage

Tay Ray Chuan (2):
t5523-push-upstream: add function to ensure fresh upstream repo
t5523-push-upstream: test progress messages

builtin/send-pack.c | 3 ++
send-pack.h | 1 +
t/lib-terminal.sh | 39 +++++++++++++++++++++++
t/t5523-push-upstream.sh | 44 +++++++++++++++++++++++++-
t/t7006-pager.sh | 38 +---------------------
t/t7006/test-terminal.perl | 58 ----------------------------------
t/test-lib.sh | 26 +++++++++++----
t/test-terminal.perl | 75 ++++++++++++++++++++++++++++++++++++++++++++
transport.c | 1 +
9 files changed, 183 insertions(+), 102 deletions(-)
create mode 100644 t/lib-terminal.sh
delete mode 100755 t/t7006/test-terminal.perl
create mode 100755 t/test-terminal.perl
--
1.7.2.2.513.ge1ef3
Tay Ray Chuan
2010-10-16 18:36:56 UTC
Permalink
From: Jeff King <***@peff.net>

Other tests besides the pager ones may want to check how we handle
output to a terminal. This patch makes the code reusable.

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

No change.

t/lib-terminal.sh | 28 +++++++++++++++++++++
t/t7006-pager.sh | 31 +----------------------
t/t7006/test-terminal.perl | 58 --------------------------------------------
t/test-terminal.perl | 58 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 87 insertions(+), 88 deletions(-)
create mode 100644 t/lib-terminal.sh
delete mode 100755 t/t7006/test-terminal.perl
create mode 100755 t/test-terminal.perl

diff --git a/t/lib-terminal.sh b/t/lib-terminal.sh
new file mode 100644
index 0000000..6fc33db
--- /dev/null
+++ b/t/lib-terminal.sh
@@ -0,0 +1,28 @@
+#!/bin/sh
+
+test_expect_success 'set up terminal for tests' '
+ if test -t 1
+ then
+ >stdout_is_tty
+ elif
+ test_have_prereq PERL &&
+ "$PERL_PATH" "$TEST_DIRECTORY"/test-terminal.perl \
+ sh -c "test -t 1"
+ then
+ >test_terminal_works
+ fi
+'
+
+if test -e stdout_is_tty
+then
+ test_terminal() { "$@"; }
+ test_set_prereq TTY
+elif test -e test_terminal_works
+then
+ test_terminal() {
+ "$PERL_PATH" "$TEST_DIRECTORY"/test-terminal.perl "$@"
+ }
+ test_set_prereq TTY
+else
+ say "# no usable terminal, so skipping some tests"
+fi
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index fb744e3..17e54d3 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -4,42 +4,13 @@ test_description='Test automatic use of a pager.'

. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-pager.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh

cleanup_fail() {
echo >&2 cleanup failed
(exit 1)
}

-test_expect_success 'set up terminal for tests' '
- rm -f stdout_is_tty ||
- cleanup_fail &&
-
- if test -t 1
- then
- >stdout_is_tty
- elif
- test_have_prereq PERL &&
- "$PERL_PATH" "$TEST_DIRECTORY"/t7006/test-terminal.perl \
- sh -c "test -t 1"
- then
- >test_terminal_works
- fi
-'
-
-if test -e stdout_is_tty
-then
- test_terminal() { "$@"; }
- test_set_prereq TTY
-elif test -e test_terminal_works
-then
- test_terminal() {
- "$PERL_PATH" "$TEST_DIRECTORY"/t7006/test-terminal.perl "$@"
- }
- test_set_prereq TTY
-else
- say "# no usable terminal, so skipping some tests"
-fi
-
test_expect_success 'setup' '
unset GIT_PAGER GIT_PAGER_IN_USE;
test_might_fail git config --unset core.pager &&
diff --git a/t/t7006/test-terminal.perl b/t/t7006/test-terminal.perl
deleted file mode 100755
index 73ff809..0000000
--- a/t/t7006/test-terminal.perl
+++ /dev/null
@@ -1,58 +0,0 @@
-#!/usr/bin/perl
-use strict;
-use warnings;
-use IO::Pty;
-use File::Copy;
-
-# Run @$argv in the background with stdout redirected to $out.
-sub start_child {
- my ($argv, $out) = @_;
- my $pid = fork;
- if (not defined $pid) {
- die "fork failed: $!"
- } elsif ($pid == 0) {
- open STDOUT, ">&", $out;
- close $out;
- exec(@$argv) or die "cannot exec '$argv->[0]': $!"
- }
- return $pid;
-}
-
-# Wait for $pid to finish.
-sub finish_child {
- # Simplified from wait_or_whine() in run-command.c.
- my ($pid) = @_;
-
- my $waiting = waitpid($pid, 0);
- if ($waiting < 0) {
- die "waitpid failed: $!";
- } elsif ($? & 127) {
- my $code = $? & 127;
- warn "died of signal $code";
- return $code - 128;
- } else {
- return $? >> 8;
- }
-}
-
-sub xsendfile {
- my ($out, $in) = @_;
-
- # Note: the real sendfile() cannot read from a terminal.
-
- # It is unspecified by POSIX whether reads
- # from a disconnected terminal will return
- # EIO (as in AIX 4.x, IRIX, and Linux) or
- # end-of-file. Either is fine.
- copy($in, $out, 4096) or $!{EIO} or die "cannot copy from child: $!";
-}
-
-if ($#ARGV < 1) {
- die "usage: test-terminal program args";
-}
-my $master = new IO::Pty;
-my $slave = $master->slave;
-my $pid = start_child(\@ARGV, $slave);
-close $slave;
-xsendfile(\*STDOUT, $master);
-exit(finish_child($pid));
diff --git a/t/test-terminal.perl b/t/test-terminal.perl
new file mode 100755
index 0000000..73ff809
--- /dev/null
+++ b/t/test-terminal.perl
@@ -0,0 +1,58 @@
+#!/usr/bin/perl
+use strict;
+use warnings;
+use IO::Pty;
+use File::Copy;
+
+# Run @$argv in the background with stdout redirected to $out.
+sub start_child {
+ my ($argv, $out) = @_;
+ my $pid = fork;
+ if (not defined $pid) {
+ die "fork failed: $!"
+ } elsif ($pid == 0) {
+ open STDOUT, ">&", $out;
+ close $out;
+ exec(@$argv) or die "cannot exec '$argv->[0]': $!"
+ }
+ return $pid;
+}
+
+# Wait for $pid to finish.
+sub finish_child {
+ # Simplified from wait_or_whine() in run-command.c.
+ my ($pid) = @_;
+
+ my $waiting = waitpid($pid, 0);
+ if ($waiting < 0) {
+ die "waitpid failed: $!";
+ } elsif ($? & 127) {
+ my $code = $? & 127;
+ warn "died of signal $code";
+ return $code - 128;
+ } else {
+ return $? >> 8;
+ }
+}
+
+sub xsendfile {
+ my ($out, $in) = @_;
+
+ # Note: the real sendfile() cannot read from a terminal.
+
+ # It is unspecified by POSIX whether reads
+ # from a disconnected terminal will return
+ # EIO (as in AIX 4.x, IRIX, and Linux) or
+ # end-of-file. Either is fine.
+ copy($in, $out, 4096) or $!{EIO} or die "cannot copy from child: $!";
+}
+
+if ($#ARGV < 1) {
+ die "usage: test-terminal program args";
+}
+my $master = new IO::Pty;
+my $slave = $master->slave;
+my $pid = start_child(\@ARGV, $slave);
+close $slave;
+xsendfile(\*STDOUT, $master);
+exit(finish_child($pid));
--
1.7.2.2.513.ge1ef3
Tay Ray Chuan
2010-10-16 18:36:59 UTC
Permalink
From: Jonathan Nieder <***@gmail.com>

It is easy to forget to declare the TTY prerequisite when
writing tests on a system where it would always be satisfied
(because IO::Pty is installed; see v1.7.3-rc0~33^2, 2010-08-16
for example). Automatically detect this problem so there is
no need to remember.

test_terminal: need to declare TTY prerequisite
test_must_fail: command not found: test_terminal echo hi

test_terminal returns status 127 in this case to simulate
not being available.

Also replace the SIMPLEPAGERTTY prerequisite on one test with
"SIMPLEPAGER,TTY", since (1) the latter is supported now and
(2) the prerequisite detection relies on the TTY prereq being
explicitly declared.

Signed-off-by: Jonathan Nieder <***@gmail.com>
---

Rebased on top of Jeff's series, so that lib-terminal's test_terminal is
changed instead.

t/lib-terminal.sh | 13 +++++++++++--
t/t7006-pager.sh | 7 +------
2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/t/lib-terminal.sh b/t/lib-terminal.sh
index 3258b8f..5e7ee9a 100644
--- a/t/lib-terminal.sh
+++ b/t/lib-terminal.sh
@@ -15,14 +15,23 @@ test_expect_success 'set up terminal for tests' '

if test -e have_tty
then
- test_terminal() { "$@"; }
+ test_terminal_() { "$@"; }
test_set_prereq TTY
elif test -e test_terminal_works
then
- test_terminal() {
+ test_terminal_() {
"$PERL_PATH" "$TEST_DIRECTORY"/test-terminal.perl "$@"
}
test_set_prereq TTY
else
say "# no usable terminal, so skipping some tests"
fi
+
+test_terminal () {
+ if ! test_declared_prereq TTY
+ then
+ echo >&2 'test_terminal: need to declare TTY prerequisite'
+ return 127
+ fi
+ test_terminal_ "$@"
+}
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 17e54d3..5641b59 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -184,11 +184,6 @@ test_expect_success 'color when writing to a file intended for a pager' '
colorful colorful.log
'

-if test_have_prereq SIMPLEPAGER && test_have_prereq TTY
-then
- test_set_prereq SIMPLEPAGERTTY
-fi
-
# Use this helper to make it easy for the caller of your
# terminal-using function to specify whether it should fail.
# If you write
@@ -224,7 +219,7 @@ parse_args() {
test_default_pager() {
parse_args "$@"

- $test_expectation SIMPLEPAGERTTY "$cmd - default pager is used by default" "
+ $test_expectation SIMPLEPAGER,TTY "$cmd - default pager is used by default" "
unset PAGER GIT_PAGER;
test_might_fail git config --unset core.pager &&
rm -f default_pager_used ||
--
1.7.2.2.513.ge1ef3
Tay Ray Chuan
2010-10-16 18:37:00 UTC
Permalink
[snip]
2. For terminal tests that capture output/stderr, the TTY prerequisite
warning does not quite work for things like
test_terminal foo >out 2>err
because the warning gets "swallowed" up by the redirection that's
supposed only to be done by the subcommand.
Good catch. Such cases (like Jeff's patch) are not well supported
currently. :(

The outcome depends on whether stdout was already a terminal (in which
case test_terminal is a noop) or not (in which case test_terminal
introduces a pseudo-tty in the middle of the pipeline).

$ test_terminal.perl sh -c 'test -t 1 && echo >&2 YES' >out
YES
$ sh -c 'test -t 1 && echo >&2 YES' >out
$

How about this?

- use the test_terminal script even when running with "-v"
if IO::Pty is available, to allow commands like

test_terminal foo >out 2>err

- add a separate TTYREDIR prerequisite which is only set
when the test_terminal script is usable

- write the "need to declare TTY prerequisite" message to fd 4,
where it will be printed when running tests with -v, rather
than being swallowed up by an unrelated redireciton.

Signed-off-by: Jonathan Nieder <***@gmail.com>
Signed-off-by: Tay Ray Chuan <***@gmail.com>
---

Picked up from a private discussion. I left the discussion in the
patch message to give some background, and it also gives a nice
summary of the changes.

t/lib-terminal.sh | 24 +++++++++++++-----------
1 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/t/lib-terminal.sh b/t/lib-terminal.sh
index 5e7ee9a..71d147f 100644
--- a/t/lib-terminal.sh
+++ b/t/lib-terminal.sh
@@ -1,36 +1,38 @@
#!/bin/sh

test_expect_success 'set up terminal for tests' '
- if test -t 1 && test -t 2
- then
- >have_tty
- elif
+ if
test_have_prereq PERL &&
"$PERL_PATH" "$TEST_DIRECTORY"/test-terminal.perl \
sh -c "test -t 1 && test -t 2"
then
test_terminal_works
+ elif test -t 1 && test -t 2
+ then
+ >have_tty
fi
'

-if test -e have_tty
-then
- test_terminal_() { "$@"; }
- test_set_prereq TTY
-elif test -e test_terminal_works
+if test -e test_terminal_works
then
test_terminal_() {
"$PERL_PATH" "$TEST_DIRECTORY"/test-terminal.perl "$@"
}
test_set_prereq TTY
+ test_set_prereq TTYREDIR
+elif test -e have_tty
+then
+ test_terminal_() { "$@"; }
+ test_set_prereq TTY
+
else
say "# no usable terminal, so skipping some tests"
fi

test_terminal () {
- if ! test_declared_prereq TTY
+ if ! test_declared_prereq TTY && ! test_declared_prereq TTYREDIR
then
- echo >&2 'test_terminal: need to declare TTY prerequisite'
+ echo >&4 'test_terminal: need to declare TTY prerequisite'
return 127
fi
test_terminal_ "$@"
--
1.7.2.2.513.ge1ef3
Tay Ray Chuan
2010-10-16 18:37:01 UTC
Permalink
Signed-off-by: Tay Ray Chuan <***@gmail.com>
---
t/t5523-push-upstream.sh | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
index 00da707..5a18533 100755
--- a/t/t5523-push-upstream.sh
+++ b/t/t5523-push-upstream.sh
@@ -3,8 +3,12 @@
test_description='push with --set-upstream'
. ./test-lib.sh

+ensure_fresh_upstream() {
+ rm -rf parent && git init --bare parent
+}
+
test_expect_success 'setup bare parent' '
- git init --bare parent &&
+ ensure_fresh_upstream &&
git remote add upstream parent
'
--
1.7.2.2.513.ge1ef3
Tay Ray Chuan
2010-10-16 18:37:02 UTC
Permalink
Reported-by: Chase Brammer <***@gmail.com>
Signed-off-by: Tay Ray Chuan <***@gmail.com>
---

Squashed in Jeff's additional tests, as well as added (missing) TTY
pre-requisites pointed out by Johnathan.

t/t5523-push-upstream.sh | 38 ++++++++++++++++++++++++++++++++++++++
1 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
index 5a18533..f43d760 100755
--- a/t/t5523-push-upstream.sh
+++ b/t/t5523-push-upstream.sh
@@ -2,6 +2,7 @@

test_description='push with --set-upstream'
. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh

ensure_fresh_upstream() {
rm -rf parent && git init --bare parent
@@ -70,4 +71,41 @@ test_expect_success 'push -u HEAD' '
check_config headbranch upstream refs/heads/headbranch
'

+test_expect_success TTY 'progress messages go to tty' '
+ ensure_fresh_upstream &&
+
+ test_terminal git push -u upstream master >out 2>err &&
+ grep "Writing objects" err
+'
+
+test_expect_failure 'progress messages do not go to non-tty' '
+ ensure_fresh_upstream &&
+
+ # skip progress messages, since stderr is non-tty
+ git push -u upstream master >out 2>err &&
+ ! grep "Writing objects" err
+'
+
+test_expect_failure 'progress messages go to non-tty (forced)' '
+ ensure_fresh_upstream &&
+
+ # force progress messages to stderr, even though it is non-tty
+ git push -u --progress upstream master >out 2>err &&
+ grep "Writing objects" err
+'
+
+test_expect_success TTY 'push -q suppresses progress' '
+ ensure_fresh_upstream &&
+
+ test_terminal git push -u -q upstream master >out 2>err &&
+ ! grep "Writing objects" err
+'
+
+test_expect_failure TTY 'push --no-progress suppresses progress' '
+ ensure_fresh_upstream &&
+
+ test_terminal git push -u --no-progress upstream master >out 2>err &&
+ ! grep "Writing objects" err
+'
+
test_done
--
1.7.2.2.513.ge1ef3
Tay Ray Chuan
2010-10-16 18:37:03 UTC
Permalink
From: Jeff King <***@peff.net>

When pushing via builtin transports (like file://, git://), the
underlying transport helper (in this case, git-pack-objects) did not get
the --progress option, even if it was passed to git push.

Fix this, and update the tests to reflect this.

Note that according to the git-pack-objects documentation, we can safely
apply the usual --progress semantics for the transport commands like
clone and fetch (and for pushing over other smart transports).

Reported-by: Chase Brammer <***@gmail.com>
Helped-by: Jonathan Nieder <***@gmail.com>
Signed-off-by: Jeff King <***@peff.net>
Signed-off-by: Tay Ray Chuan <***@gmail.com>
---

No significant changes other than those incurred while rebasing on
top of Jeff's patches.

builtin/send-pack.c | 3 +++
send-pack.h | 1 +
t/t5523-push-upstream.sh | 4 ++--
transport.c | 1 +
4 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 481602d..efd9be6 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -48,6 +48,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
NULL,
NULL,
NULL,
+ NULL,
};
struct child_process po;
int i;
@@ -59,6 +60,8 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
argv[i++] = "--delta-base-offset";
if (args->quiet)
argv[i++] = "-q";
+ if (args->progress)
+ argv[i++] = "--progress";
memset(&po, 0, sizeof(po));
po.argv = argv;
po.in = -1;
diff --git a/send-pack.h b/send-pack.h
index 60b4ba6..05d7ab1 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -5,6 +5,7 @@ struct send_pack_args {
unsigned verbose:1,
quiet:1,
porcelain:1,
+ progress:1,
send_mirror:1,
force_update:1,
use_thin_pack:1,
diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
index f43d760..c229fe6 100755
--- a/t/t5523-push-upstream.sh
+++ b/t/t5523-push-upstream.sh
@@ -78,7 +78,7 @@ test_expect_success TTY 'progress messages go to tty' '
grep "Writing objects" err
'

-test_expect_failure 'progress messages do not go to non-tty' '
+test_expect_success 'progress messages do not go to non-tty' '
ensure_fresh_upstream &&

# skip progress messages, since stderr is non-tty
@@ -86,7 +86,7 @@ test_expect_failure 'progress messages do not go to non-tty' '
! grep "Writing objects" err
'

-test_expect_failure 'progress messages go to non-tty (forced)' '
+test_expect_success 'progress messages go to non-tty (forced)' '
ensure_fresh_upstream &&

# force progress messages to stderr, even though it is non-tty
diff --git a/transport.c b/transport.c
index 4dba6f8..0078660 100644
--- a/transport.c
+++ b/transport.c
@@ -789,6 +789,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
args.use_thin_pack = data->options.thin;
args.verbose = (transport->verbose > 0);
args.quiet = (transport->verbose < 0);
+ args.progress = transport->progress;
args.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);
args.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN);
--
1.7.2.2.513.ge1ef3
Jonathan Nieder
2010-10-17 00:46:13 UTC
Permalink
Tay Ray Chuan wrote:

[...]
Post by Tay Ray Chuan
--- a/t/t5523-push-upstream.sh
+++ b/t/t5523-push-upstream.sh
@@ -70,4 +71,41 @@ test_expect_success 'push -u HEAD' '
check_config headbranch upstream refs/heads/headbranch
'
+test_expect_success TTY 'progress messages go to tty' '
+ ensure_fresh_upstream &&
+
+ test_terminal git push -u upstream master >out 2>err &&
+ grep "Writing objects" err
+'
Thanks for testing the usual case. It is easy to forget sometimes.

The tests using the TTY prerequisite would need to use TTYREDIR
unless we simplify the latter out of existence.
Jonathan Nieder
2010-10-17 00:38:07 UTC
Permalink
Post by Tay Ray Chuan
- use the test_terminal script even when running with "-v"
if IO::Pty is available, to allow commands like
test_terminal foo >out 2>err
- add a separate TTYREDIR prerequisite which is only set
when the test_terminal script is usable
- write the "need to declare TTY prerequisite" message to fd 4,
where it will be printed when running tests with -v, rather
than being swallowed up by an unrelated redireciton.
The patches up to this one look good to me. This one behaves
as advertised, but I think the API is lousy --- it is just
begging people to use the TTY prereq where TTYREDIR is needed.

Better to change TTY to mean TTYREDIR and drop support for
test_terminal on systems without IO::Pty:

-- 8< --
Subject: test_terminal: ensure redirections work reliably

For terminal tests that capture output/stderr, the TTY prerequisite
warning does not quite work for commands like

test_terminal foo >out 2>err

because the warning gets "swallowed" up by the redirection that's
supposed only to be done by the subcommand.

Even worse, the outcome depends on whether stdout was already a
terminal (in which case test_terminal is a noop) or not (in which case
test_terminal introduces a pseudo-tty in the middle of the pipeline).

$ test_terminal.perl sh -c 'test -t 1 && echo >&2 YES' >out
YES
$ sh -c 'test -t 1 && echo >&2 YES' >out
$

So:

- use the test_terminal script even when running with "-v".

- skip tests that require a terminal when the test_terminal
script is unusable because IO::Pty is not installed.

- write the "need to declare TTY prerequisite" message to fd 4,
where it will be printed when running tests with -v, rather
than being swallowed up by an unrelated redireciton.

Noticed-by: Tay Ray Chuan <***@gmail.com>
Signed-off-by: Jonathan Nieder <***@gmail.com>
---
The only other sane alternative I can think of is to introduce
TTYNOREDIR, since at least people wouldn't be tempted to use
that. Distinguishing between

test_expect_success 'foo' '
test_terminal bar >out 2>err
'

and

test_expect_success 'foo' '
test_terminal bar
'

from a script run as

sh t1234-some-script.sh >log 2>err.log

does not seem to be easy without OS-specific hacks like
"readlink /dev/fd/1".

t/lib-terminal.sh | 38 ++++++++++----------------------------
1 files changed, 10 insertions(+), 28 deletions(-)

diff --git a/t/lib-terminal.sh b/t/lib-terminal.sh
index 5e7ee9a..c383b57 100644
--- a/t/lib-terminal.sh
+++ b/t/lib-terminal.sh
@@ -1,37 +1,19 @@
#!/bin/sh

test_expect_success 'set up terminal for tests' '
- if test -t 1 && test -t 2
- then
- >have_tty
- elif
+ if
test_have_prereq PERL &&
"$PERL_PATH" "$TEST_DIRECTORY"/test-terminal.perl \
sh -c "test -t 1 && test -t 2"
then
- >test_terminal_works
+ test_set_prereq TTY &&
+ test_terminal () {
+ if ! test_declared_prereq TTY
+ then
+ echo >&4 "test_terminal: need to declare TTY prerequisite"
+ return 127
+ fi
+ "$PERL_PATH" "$TEST_DIRECTORY"/test-terminal.perl "$@"
+ }
fi
'
-
-if test -e have_tty
-then
- test_terminal_() { "$@"; }
- test_set_prereq TTY
-elif test -e test_terminal_works
-then
- test_terminal_() {
- "$PERL_PATH" "$TEST_DIRECTORY"/test-terminal.perl "$@"
- }
- test_set_prereq TTY
-else
- say "# no usable terminal, so skipping some tests"
-fi
-
-test_terminal () {
- if ! test_declared_prereq TTY
- then
- echo >&2 'test_terminal: need to declare TTY prerequisite'
- return 127
- fi
- test_terminal_ "$@"
-}
--
1.7.2.3
Jeff King
2010-10-22 19:42:52 UTC
Permalink
Post by Tay Ray Chuan
The outcome depends on whether stdout was already a terminal (in which
case test_terminal is a noop) or not (in which case test_terminal
introduces a pseudo-tty in the middle of the pipeline).
$ test_terminal.perl sh -c 'test -t 1 && echo >&2 YES' >out
YES
$ sh -c 'test -t 1 && echo >&2 YES' >out
$
How about this?
- use the test_terminal script even when running with "-v"
if IO::Pty is available, to allow commands like
test_terminal foo >out 2>err
- add a separate TTYREDIR prerequisite which is only set
when the test_terminal script is usable
Is it even worth keeping the direct-to-tty code at all? Yes, it means
that people without IO::Pty can use _some_ terminal tests with "-v". But
it creates a headache for test writers in understanding the subtle
difference between TTY and TTYREDIR.

-Peff

Tay Ray Chuan
2010-10-16 18:36:58 UTC
Permalink
From: Jonathan Nieder <***@gmail.com>

This is plumbing to prepare helpers like test_terminal to notice buggy
test scripts that do not declare all of the necessary prerequisites.

Signed-off-by: Jonathan Nieder <***@gmail.com>
---

No change.

t/test-lib.sh | 26 +++++++++++++++++++-------
1 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index dff5e25..94bff17 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -362,6 +362,15 @@ test_have_prereq () {
test $total_prereq = $ok_prereq
}

+test_declared_prereq () {
+ case ",$test_prereq," in
+ *,$1,*)
+ return 0
+ ;;
+ esac
+ return 1
+}
+
# You are not expected to call test_ok_ and test_failure_ directly, use
# the text_expect_* functions instead.

@@ -414,17 +423,17 @@ test_skip () {
break
esac
done
- if test -z "$to_skip" && test -n "$prereq" &&
- ! test_have_prereq "$prereq"
+ if test -z "$to_skip" && test -n "$test_prereq" &&
+ ! test_have_prereq "$test_prereq"
then
to_skip=t
fi
case "$to_skip" in
t)
of_prereq=
- if test "$missing_prereq" != "$prereq"
+ if test "$missing_prereq" != "$test_prereq"
then
- of_prereq=" of $prereq"
+ of_prereq=" of $test_prereq"
fi

say_color skip >&3 "skipping test: $@"
@@ -438,9 +447,10 @@ test_skip () {
}

test_expect_failure () {
- test "$#" = 3 && { prereq=$1; shift; } || prereq=
+ test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
test "$#" = 2 ||
error "bug in the test script: not 2 or 3 parameters to test-expect-failure"
+ export test_prereq
if ! test_skip "$@"
then
say >&3 "checking known breakage: $2"
@@ -456,9 +466,10 @@ test_expect_failure () {
}

test_expect_success () {
- test "$#" = 3 && { prereq=$1; shift; } || prereq=
+ test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
test "$#" = 2 ||
error "bug in the test script: not 2 or 3 parameters to test-expect-success"
+ export test_prereq
if ! test_skip "$@"
then
say >&3 "expecting success: $2"
@@ -500,11 +511,12 @@ test_expect_code () {
# Usage: test_external description command arguments...
# Example: test_external 'Perl API' perl ../path/to/test.pl
test_external () {
- test "$#" = 4 && { prereq=$1; shift; } || prereq=
+ test "$#" = 4 && { test_prereq=$1; shift; } || test_prereq=
test "$#" = 3 ||
error >&5 "bug in the test script: not 3 or 4 parameters to test_external"
descr="$1"
shift
+ export test_prereq
if ! test_skip "$descr" "$@"
then
# Announce the script to reduce confusion about the
--
1.7.2.2.513.ge1ef3
Tay Ray Chuan
2010-10-16 18:36:57 UTC
Permalink
From: Jeff King <***@peff.net>

Some outputs (like the pager) care whether stdout is a
terminal. Others (like progress meters) care about stderr.

This patch sets up both. Technically speaking, we could go
further and set up just one (because either the other goes
to a terminal, or because our tests are only interested in
one). This patch does both to keep the interface to
lib-terminal simple.

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

No change.

t/lib-terminal.sh | 8 ++++----
t/test-terminal.perl | 31 ++++++++++++++++++++++++-------
2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/t/lib-terminal.sh b/t/lib-terminal.sh
index 6fc33db..3258b8f 100644
--- a/t/lib-terminal.sh
+++ b/t/lib-terminal.sh
@@ -1,19 +1,19 @@
#!/bin/sh

test_expect_success 'set up terminal for tests' '
- if test -t 1
+ if test -t 1 && test -t 2
then
- >stdout_is_tty
+ >have_tty
elif
test_have_prereq PERL &&
"$PERL_PATH" "$TEST_DIRECTORY"/test-terminal.perl \
- sh -c "test -t 1"
+ sh -c "test -t 1 && test -t 2"
then
Post by Jeff King
test_terminal_works
fi
'

-if test -e stdout_is_tty
+if test -e have_tty
then
test_terminal() { "$@"; }
test_set_prereq TTY
diff --git a/t/test-terminal.perl b/t/test-terminal.perl
index 73ff809..c2e9dac 100755
--- a/t/test-terminal.perl
+++ b/t/test-terminal.perl
@@ -4,14 +4,15 @@ use warnings;
use IO::Pty;
use File::Copy;

-# Run @$argv in the background with stdout redirected to $out.
+# Run @$argv in the background with stdio redirected to $out and $err.
sub start_child {
- my ($argv, $out) = @_;
+ my ($argv, $out, $err) = @_;
my $pid = fork;
if (not defined $pid) {
die "fork failed: $!"
} elsif ($pid == 0) {
open STDOUT, ">&", $out;
+ open STDERR, ">&", $err;
close $out;
exec(@$argv) or die "cannot exec '$argv->[0]': $!"
}
@@ -47,12 +48,28 @@ sub xsendfile {
copy($in, $out, 4096) or $!{EIO} or die "cannot copy from child: $!";
}

+sub copy_stdio {
+ my ($out, $err) = @_;
+ my $pid = fork;
+ defined $pid or die "fork failed: $!";
+ if (!$pid) {
+ close($out);
+ xsendfile(\*STDERR, $err);
+ exit 0;
+ }
+ close($err);
+ xsendfile(\*STDOUT, $out);
+ finish_child($pid) == 0
+ or exit 1;
+}
+
if ($#ARGV < 1) {
die "usage: test-terminal program args";
}
-my $master = new IO::Pty;
-my $slave = $master->slave;
-my $pid = start_child(\@ARGV, $slave);
-close $slave;
-xsendfile(\*STDOUT, $master);
+my $master_out = new IO::Pty;
+my $master_err = new IO::Pty;
+my $pid = start_child(\@ARGV, $master_out->slave, $master_err->slave);
+close $master_out->slave;
+close $master_err->slave;
+copy_stdio($master_out, $master_err);
exit(finish_child($pid));
--
1.7.2.2.513.ge1ef3
Jonathan Nieder
2010-10-17 00:51:07 UTC
Permalink
Post by Jeff King
tests: factor out terminal handling from t7006
tests: test terminal output to both stdout and stderr
push: pass --progress down to git-pack-objects
test-lib: allow test code to check the list of declared prerequisites
test_terminal: catch use without TTY prerequisite
test_terminal: give priority to test-terminal.perl usage
t5523-push-upstream: add function to ensure fresh upstream repo
t5523-push-upstream: test progress messages
I've sent some comments on patches 5 (give priority..) and 7 (test
progress messages). Except as mentioned,

Reviewed-by: Jonathan Nieder <***@gmail.com>

Thanks for cleaning up the test_terminal mess.
Jeff King
2010-10-14 03:02:20 UTC
Permalink
Post by Tay Ray Chuan
[PATCH 1/3] t5523-push-upstream: add function to ensure fresh upstream repo
[PATCH 2/3] t5523-push-upstream: test progress messages
[PATCH 3/3] push: pass --progress down to git-pack-objects
I had hoped to have a fix for --no-progress, but munging the tests took
so long that now I am sleepy. :) So here are some extra tests on top of
your series. The first two are refactoring, and the third has the new
tests. It checks regular stderr-is-tty progress and that "push -q"
suppresses progress, as Junio asked elsewhere. And it reveals the bug in
--no-progress.

It might make more sense to actually re-roll your series with the
refactoring at the front, and my 3/3 squashed into your 2/3.

Also, these tests feel a bit out of place in t5523, but I don't see a
better place for them to go. Perhaps they should go in their own test
script. I don't feel strongly, though.

[1/3]: tests: factor out terminal handling from t7006
[2/3]: tests: test terminal output to both stdout and stderr
[3/3]: t5523: test push progress output to tty

-Peff
Jeff King
2010-10-14 03:04:04 UTC
Permalink
Other tests besides the pager ones may want to check how we handle
output to a terminal. This patch makes the code reusable.

Signed-off-by: Jeff King <***@peff.net>
---
t/lib-terminal.sh | 28 ++++++++++++++++++++++++++++
t/t7006-pager.sh | 31 +------------------------------
t/{t7006 => }/test-terminal.perl | 0
3 files changed, 29 insertions(+), 30 deletions(-)
create mode 100644 t/lib-terminal.sh
rename t/{t7006 => }/test-terminal.perl (100%)

diff --git a/t/lib-terminal.sh b/t/lib-terminal.sh
new file mode 100644
index 0000000..6fc33db
--- /dev/null
+++ b/t/lib-terminal.sh
@@ -0,0 +1,28 @@
+#!/bin/sh
+
+test_expect_success 'set up terminal for tests' '
+ if test -t 1
+ then
+ >stdout_is_tty
+ elif
+ test_have_prereq PERL &&
+ "$PERL_PATH" "$TEST_DIRECTORY"/test-terminal.perl \
+ sh -c "test -t 1"
+ then
+ >test_terminal_works
+ fi
+'
+
+if test -e stdout_is_tty
+then
+ test_terminal() { "$@"; }
+ test_set_prereq TTY
+elif test -e test_terminal_works
+then
+ test_terminal() {
+ "$PERL_PATH" "$TEST_DIRECTORY"/test-terminal.perl "$@"
+ }
+ test_set_prereq TTY
+else
+ say "# no usable terminal, so skipping some tests"
+fi
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index fb744e3..17e54d3 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -4,42 +4,13 @@ test_description='Test automatic use of a pager.'

. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-pager.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh

cleanup_fail() {
echo >&2 cleanup failed
(exit 1)
}

-test_expect_success 'set up terminal for tests' '
- rm -f stdout_is_tty ||
- cleanup_fail &&
-
- if test -t 1
- then
- >stdout_is_tty
- elif
- test_have_prereq PERL &&
- "$PERL_PATH" "$TEST_DIRECTORY"/t7006/test-terminal.perl \
- sh -c "test -t 1"
- then
- >test_terminal_works
- fi
-'
-
-if test -e stdout_is_tty
-then
- test_terminal() { "$@"; }
- test_set_prereq TTY
-elif test -e test_terminal_works
-then
- test_terminal() {
- "$PERL_PATH" "$TEST_DIRECTORY"/t7006/test-terminal.perl "$@"
- }
- test_set_prereq TTY
-else
- say "# no usable terminal, so skipping some tests"
-fi
-
test_expect_success 'setup' '
unset GIT_PAGER GIT_PAGER_IN_USE;
test_might_fail git config --unset core.pager &&
diff --git a/t/t7006/test-terminal.perl b/t/test-terminal.perl
similarity index 100%
rename from t/t7006/test-terminal.perl
rename to t/test-terminal.perl
--
1.7.3.1.204.g337d6.dirty
Jonathan Nieder
2010-10-14 03:10:14 UTC
Permalink
Post by Jeff King
Other tests besides the pager ones may want to check how we handle
output to a terminal. This patch makes the code reusable.
Thanks! Looks good to me.
Jeff King
2010-10-14 03:04:44 UTC
Permalink
Some outputs (like the pager) care whether stdout is a
terminal. Others (like progress meters) care about stderr.

This patch sets up both. Technically speaking, we could go
further and set up just one (because either the other goes
to a terminal, or because our tests are only interested in
one). This patch does both to keep the interface to
lib-terminal simple.

Signed-off-by: Jeff King <***@peff.net>
---
t/lib-terminal.sh | 8 ++++----
t/test-terminal.perl | 31 ++++++++++++++++++++++++-------
2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/t/lib-terminal.sh b/t/lib-terminal.sh
index 6fc33db..3258b8f 100644
--- a/t/lib-terminal.sh
+++ b/t/lib-terminal.sh
@@ -1,19 +1,19 @@
#!/bin/sh

test_expect_success 'set up terminal for tests' '
- if test -t 1
+ if test -t 1 && test -t 2
then
- >stdout_is_tty
+ >have_tty
elif
test_have_prereq PERL &&
"$PERL_PATH" "$TEST_DIRECTORY"/test-terminal.perl \
- sh -c "test -t 1"
+ sh -c "test -t 1 && test -t 2"
then
Post by Jeff King
test_terminal_works
fi
'

-if test -e stdout_is_tty
+if test -e have_tty
then
test_terminal() { "$@"; }
test_set_prereq TTY
diff --git a/t/test-terminal.perl b/t/test-terminal.perl
index 73ff809..c2e9dac 100755
--- a/t/test-terminal.perl
+++ b/t/test-terminal.perl
@@ -4,14 +4,15 @@ use warnings;
use IO::Pty;
use File::Copy;

-# Run @$argv in the background with stdout redirected to $out.
+# Run @$argv in the background with stdio redirected to $out and $err.
sub start_child {
- my ($argv, $out) = @_;
+ my ($argv, $out, $err) = @_;
my $pid = fork;
if (not defined $pid) {
die "fork failed: $!"
} elsif ($pid == 0) {
open STDOUT, ">&", $out;
+ open STDERR, ">&", $err;
close $out;
exec(@$argv) or die "cannot exec '$argv->[0]': $!"
}
@@ -47,12 +48,28 @@ sub xsendfile {
copy($in, $out, 4096) or $!{EIO} or die "cannot copy from child: $!";
}

+sub copy_stdio {
+ my ($out, $err) = @_;
+ my $pid = fork;
+ defined $pid or die "fork failed: $!";
+ if (!$pid) {
+ close($out);
+ xsendfile(\*STDERR, $err);
+ exit 0;
+ }
+ close($err);
+ xsendfile(\*STDOUT, $out);
+ finish_child($pid) == 0
+ or exit 1;
+}
+
if ($#ARGV < 1) {
die "usage: test-terminal program args";
}
-my $master = new IO::Pty;
-my $slave = $master->slave;
-my $pid = start_child(\@ARGV, $slave);
-close $slave;
-xsendfile(\*STDOUT, $master);
+my $master_out = new IO::Pty;
+my $master_err = new IO::Pty;
+my $pid = start_child(\@ARGV, $master_out->slave, $master_err->slave);
+close $master_out->slave;
+close $master_err->slave;
+copy_stdio($master_out, $master_err);
exit(finish_child($pid));
--
1.7.3.1.204.g337d6.dirty
Jonathan Nieder
2010-10-14 03:27:34 UTC
Permalink
Post by Jeff King
Some outputs (like the pager) care whether stdout is a
terminal. Others (like progress meters) care about stderr.
This patch sets up both. Technically speaking, we could go
further and set up just one (because either the other goes
to a terminal, or because our tests are only interested in
one).
This makes test_terminal more realistic, too: the usual case is for
stdout and stderr to go to a terminal (unless explicitly captured or
redirected).

Tests can use 'test_terminal sh -c "foo >/dev/null"' to test that a
command correctly handles being run with stderr a terminal and
stdout not.

And I doubt this would make test_terminal much slower.

So for what it's worth:
Acked-by: Jonathan Nieder <***@gmail.com>

Thanks.
Jeff King
2010-10-14 03:05:05 UTC
Permalink
We already test the non-tty cases, but until recent changes
made lib-terminal.sh available, we couldn't test the case
with a tty. These tests reveal a bug: --no-progress is
silently ignored.

Signed-off-by: Jeff King <***@peff.net>
---
t/t5523-push-upstream.sh | 26 ++++++++++++++++++++++++--
1 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
index 113626b..78c5978 100755
--- a/t/t5523-push-upstream.sh
+++ b/t/t5523-push-upstream.sh
@@ -2,6 +2,7 @@

test_description='push with --set-upstream'
. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh

ensure_fresh_upstream() {
test -d parent &&
@@ -72,7 +73,14 @@ test_expect_success 'push -u HEAD' '
check_config headbranch upstream refs/heads/headbranch
'

-test_expect_success 'progress messages to non-tty' '
+test_expect_success 'progress messages go to tty' '
+ ensure_fresh_upstream &&
+
+ test_terminal git push -u upstream master >out 2>err &&
+ grep "Writing objects" err
+'
+
+test_expect_success 'progress messages do not go to non-tty' '
ensure_fresh_upstream &&

# skip progress messages, since stderr is non-tty
@@ -80,7 +88,7 @@ test_expect_success 'progress messages to non-tty' '
! grep "Writing objects" err
'

-test_expect_success 'progress messages to non-tty (forced)' '
+test_expect_success 'progress messages go to non-tty (forced)' '
ensure_fresh_upstream &&

# force progress messages to stderr, even though it is non-tty
@@ -88,4 +96,18 @@ test_expect_success 'progress messages to non-tty (forced)' '
grep "Writing objects" err
'

+test_expect_success 'push -q suppresses progress' '
+ ensure_fresh_upstream &&
+
+ test_terminal git push -u -q upstream master >out 2>err &&
+ ! grep "Writing objects" err
+'
+
+test_expect_failure 'push --no-progress suppresses progress' '
+ ensure_fresh_upstream &&
+
+ test_terminal git push -u --no-progress upstream master >out 2>err &&
+ ! grep "Writing objects" err
+'
+
test_done
--
1.7.3.1.204.g337d6.dirty
Jonathan Nieder
2010-10-14 03:16:42 UTC
Permalink
Post by Tay Ray Chuan
--- a/t/t5523-push-upstream.sh
+++ b/t/t5523-push-upstream.sh
[...]
Post by Tay Ray Chuan
@@ -72,7 +73,14 @@ test_expect_success 'push -u HEAD' '
check_config headbranch upstream refs/heads/headbranch
'
-test_expect_success 'progress messages to non-tty' '
+test_expect_success 'progress messages go to tty' '
+ ensure_fresh_upstream &&
+
+ test_terminal git push -u upstream master >out 2>err &&
+ grep "Writing objects" err
+'
Missing TTY prerequisite. (Do you think test_terminal should check
$prereq to prevent this?)
Post by Tay Ray Chuan
@@ -88,4 +96,18 @@ test_expect_success 'progress messages to non-tty (forced)' '
grep "Writing objects" err
'
+test_expect_success 'push -q suppresses progress' '
+ ensure_fresh_upstream &&
+
+ test_terminal git push -u -q upstream master >out 2>err &&
+ ! grep "Writing objects" err
+'
Likewise.
Post by Tay Ray Chuan
+
+test_expect_failure 'push --no-progress suppresses progress' '
+ ensure_fresh_upstream &&
+
+ test_terminal git push -u --no-progress upstream master >out 2>err &&
+ ! grep "Writing objects" err
+'
Likewise.

Regards,
Jonathan
Jeff King
2010-10-14 03:34:48 UTC
Permalink
Post by Jonathan Nieder
Post by Tay Ray Chuan
--- a/t/t5523-push-upstream.sh
+++ b/t/t5523-push-upstream.sh
[...]
Post by Tay Ray Chuan
@@ -72,7 +73,14 @@ test_expect_success 'push -u HEAD' '
check_config headbranch upstream refs/heads/headbranch
'
-test_expect_success 'progress messages to non-tty' '
+test_expect_success 'progress messages go to tty' '
+ ensure_fresh_upstream &&
+
+ test_terminal git push -u upstream master >out 2>err &&
+ grep "Writing objects" err
+'
Missing TTY prerequisite. (Do you think test_terminal should check
$prereq to prevent this?)
Oops, good catch. I think we should already catch it, as test_terminal
will not be defined at all in the no-tty case. We could print a nicer
message, but it is not likely to be seen by the user. If they are
using "-v", then stderr probably _is_ a tty. And if not, they will not
see the message. There are ways around it, but they are not likely to be
seen unless the user is really trying (e.g., "./t5523-* -v >not_a_tty").

-Peff
Jonathan Nieder
2010-10-14 20:37:21 UTC
Permalink
Post by Jeff King
Post by Jonathan Nieder
Missing TTY prerequisite. (Do you think test_terminal should check
$prereq to prevent this?)
Oops, good catch. I think we should already catch it, as test_terminal
will not be defined at all in the no-tty case. We could print a nicer
message, but
I rather meant something like this.

Patch 1 exposes the internal $prereq variable from
test_expect_(success|failure). Maybe it should be called
GIT_TEST_something to avoid trampling other programs' namespaces? Not
sure.

Patch 2 introduces some magic autodetection so people that never run
tests without -v can still notice the missing TTY prereqs.

Jonathan Nieder (2):
test-lib: allow test code to check the list of declared prerequisites
test_terminal: catch use without TTY prerequisite

t/t7006-pager.sh | 20 ++++++++++++--------
t/test-lib.sh | 26 +++++++++++++++++++-------
2 files changed, 31 insertions(+), 15 deletions(-)
--
1.7.2.3
Jonathan Nieder
2010-10-14 20:40:01 UTC
Permalink
This is plumbing to prepare helpers like test_terminal to notice buggy
test scripts that do not declare all of the necessary prerequisites.

Signed-off-by: Jonathan Nieder <***@gmail.com>
---
Applies on top of 892e6f7 (test-lib: make test_expect_code a test
command) from the en/and-cascade-tests branch. On master,
text_expect_code would have to be adjusted, too.

t/test-lib.sh | 26 +++++++++++++++++++-------
1 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index d86edcd..cd69ffa 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -362,6 +362,15 @@ test_have_prereq () {
test $total_prereq = $ok_prereq
}

+test_declared_prereq () {
+ case ",$test_prereq," in
+ *,$1,*)
+ return 0
+ ;;
+ esac
+ return 1
+}
+
# You are not expected to call test_ok_ and test_failure_ directly, use
# the text_expect_* functions instead.

@@ -414,17 +423,17 @@ test_skip () {
break
esac
done
- if test -z "$to_skip" && test -n "$prereq" &&
- ! test_have_prereq "$prereq"
+ if test -z "$to_skip" && test -n "$test_prereq" &&
+ ! test_have_prereq "$test_prereq"
then
to_skip=t
fi
case "$to_skip" in
t)
of_prereq=
- if test "$missing_prereq" != "$prereq"
+ if test "$missing_prereq" != "$test_prereq"
then
- of_prereq=" of $prereq"
+ of_prereq=" of $test_prereq"
fi

say_color skip >&3 "skipping test: $@"
@@ -438,9 +447,10 @@ test_skip () {
}

test_expect_failure () {
- test "$#" = 3 && { prereq=$1; shift; } || prereq=
+ test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
test "$#" = 2 ||
error "bug in the test script: not 2 or 3 parameters to test-expect-failure"
+ export test_prereq
if ! test_skip "$@"
then
say >&3 "checking known breakage: $2"
@@ -456,9 +466,10 @@ test_expect_failure () {
}

test_expect_success () {
- test "$#" = 3 && { prereq=$1; shift; } || prereq=
+ test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
test "$#" = 2 ||
error "bug in the test script: not 2 or 3 parameters to test-expect-success"
+ export test_prereq
if ! test_skip "$@"
then
say >&3 "expecting success: $2"
@@ -482,11 +493,12 @@ test_expect_success () {
# Usage: test_external description command arguments...
# Example: test_external 'Perl API' perl ../path/to/test.pl
test_external () {
- test "$#" = 4 && { prereq=$1; shift; } || prereq=
+ test "$#" = 4 && { test_prereq=$1; shift; } || test_prereq=
test "$#" = 3 ||
error >&5 "bug in the test script: not 3 or 4 parameters to test_external"
descr="$1"
shift
+ export test_prereq
if ! test_skip "$descr" "$@"
then
# Announce the script to reduce confusion about the
--
1.7.2.3
Ævar Arnfjörð Bjarmason
2010-10-15 05:18:50 UTC
Permalink
+ =C2=A0 =C2=A0 =C2=A0 case ",$test_prereq," in
+ =C2=A0 =C2=A0 =C2=A0 *,$1,*)
Won't this only work with:

test_expect_success FOO,THINGYOUWANT,BAR '...'

And not:

test_expect_success THINGYOUWANT,FOO,BAR '...'

?
Jonathan Nieder
2010-10-15 05:34:14 UTC
Permalink
Post by Ævar Arnfjörð Bjarmason
+ =C2=A0 =C2=A0 =C2=A0 case ",$test_prereq," in
+ =C2=A0 =C2=A0 =C2=A0 *,$1,*)
=20
test_expect_success FOO,THINGYOUWANT,BAR '...'
=20
=20
test_expect_success THINGYOUWANT,FOO,BAR '...'
=20
?
$ case ,X,FOO,BAR, in
*,X,*)
echo ok
;;
*)
echo not ok
;;
esac
ok
$

Looks safe to me. A * can match any string, including the empty string=
[1].

[1] http://www.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.=
html#tag_18_13_02
Jonathan Nieder
2010-10-14 20:41:41 UTC
Permalink
It is easy to forget to declare the TTY prerequisite when
writing tests on a system where it would always be satisfied
(because IO::Pty is installed; see v1.7.3-rc0~33^2, 2010-08-16
for example). Automatically detect this problem so there is
no need to remember.

test_terminal: need to declare TTY prerequisite
test_must_fail: command not found: test_terminal echo hi

test_terminal returns status 127 in this case to simulate
not being available.

Also replace the SIMPLEPAGERTTY prerequisite on one test with
"SIMPLEPAGER,TTY", since (1) the latter is supported now and
(2) the prerequisite detection relies on the TTY prereq being
explicitly declared.

Signed-off-by: Jonathan Nieder <***@gmail.com>
---
Penance for introducing that bug a few times.

t/t7006-pager.sh | 20 ++++++++++++--------
1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index fb744e3..53868f6 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -28,11 +28,11 @@ test_expect_success 'set up terminal for tests' '

if test -e stdout_is_tty
then
- test_terminal() { "$@"; }
+ test_terminal_() { "$@"; }
test_set_prereq TTY
elif test -e test_terminal_works
then
- test_terminal() {
+ test_terminal_() {
"$PERL_PATH" "$TEST_DIRECTORY"/t7006/test-terminal.perl "$@"
}
test_set_prereq TTY
@@ -40,6 +40,15 @@ else
say "# no usable terminal, so skipping some tests"
fi

+test_terminal () {
+ if ! test_declared_prereq TTY
+ then
+ echo >&2 'test_terminal: need to declare TTY prerequisite'
+ return 127
+ fi
+ test_terminal_ "$@"
+}
+
test_expect_success 'setup' '
unset GIT_PAGER GIT_PAGER_IN_USE;
test_might_fail git config --unset core.pager &&
@@ -213,11 +222,6 @@ test_expect_success 'color when writing to a file intended for a pager' '
colorful colorful.log
'

-if test_have_prereq SIMPLEPAGER && test_have_prereq TTY
-then
- test_set_prereq SIMPLEPAGERTTY
-fi
-
# Use this helper to make it easy for the caller of your
# terminal-using function to specify whether it should fail.
# If you write
@@ -253,7 +257,7 @@ parse_args() {
test_default_pager() {
parse_args "$@"

- $test_expectation SIMPLEPAGERTTY "$cmd - default pager is used by default" "
+ $test_expectation SIMPLEPAGER,TTY "$cmd - default pager is used by default" "
unset PAGER GIT_PAGER;
test_might_fail git config --unset core.pager &&
rm -f default_pager_used ||
--
1.7.2.3
Jeff King
2010-10-15 04:42:53 UTC
Permalink
Post by Jonathan Nieder
Post by Jeff King
Oops, good catch. I think we should already catch it, as test_terminal
will not be defined at all in the no-tty case. We could print a nicer
message, but
I rather meant something like this.
Patch 1 exposes the internal $prereq variable from
test_expect_(success|failure). Maybe it should be called
GIT_TEST_something to avoid trampling other programs' namespaces? Not
sure.
Patch 2 introduces some magic autodetection so people that never run
tests without -v can still notice the missing TTY prereqs.
Yeah, that is better, as it will catch the lack of prerequisite even on
systems where the prerequisite is met.

It seems like a lot of code to catch something small, but on the other
hand, it does seem to be a repeated mistake.

-Peff
Tay Ray Chuan
2010-10-15 11:27:34 UTC
Permalink
Post by Jonathan Nieder
Oops, good catch. I think we should already catch it, as test_term=
inal
Post by Jonathan Nieder
will not be defined at all in the no-tty case. We could print a ni=
cer
Post by Jonathan Nieder
message, but
I rather meant something like this.
Patch 1 exposes the internal $prereq variable from
test_expect_(success|failure). =A0Maybe it should be called
GIT_TEST_something to avoid trampling other programs' namespaces? =A0=
Not
Post by Jonathan Nieder
sure.
Patch 2 introduces some magic autodetection so people that never run
tests without -v can still notice the missing TTY prereqs.
Yeah, that is better, as it will catch the lack of prerequisite even =
on
systems where the prerequisite is met.
It seems like a lot of code to catch something small, but on the othe=
r
hand, it does seem to be a repeated mistake.
I'll probably be re-rolling the push --progress fix series with this an=
d Jeff's.

--=20
Cheers,
Ray Chuan
Scott R. Godin
2010-10-18 16:39:38 UTC
Permalink
git fetch origin master --progress> /fetch_error_ouput.txt 2>&1
Just as a small tip, you can shorthand this in bash using
git fech origin master --progress >& /fetch_error_output.txt

HTH :)
--
(please respond to the list as opposed to my email box directly,
unless you are supplying private information you don't want public
on the list)
Continue reading on narkive:
Loading...