On Wed, Sep 18, 2013 at 02:45:51PM -0400, Jeff King wrote:
> That being said, the new messages should almost certainly go to stderr.
>
> -- >8 --
> Subject: [PATCH] clone: write "checking connectivity" to stderr
>
> In commit 0781aa4 (clone: let the user know when
> check_everything_connected is run, 2013-05-03), we started
> giving the user a progress report during clone. However,
> since the actual work happens in a sub-process, we do not
> use the usual progress code that counts the objects, but
> rather just print a message ourselves.
>
> This message goes to stdout via printf, which is unlike
> other progress messages (both the eye candy within clone,
> and the "checking connectivity" progress in other commands).
> Let's send it to stderr for consistency.
Hrm, this actually breaks t5701, which expects "clone 2>err" to print
nothing to stderr.
What should happen here? The message is emulating the usual progress
messages, which are silent when stderr is redirected. So we could
actually use isatty() in the usual way to suppress them. On the other
hand, the point of that suppression is that the regular progress code
produces long output that is not meant to be seen sequentially (i.e., it
is overwritten in the terminal with "\r"). But this message does not do
so. So we can just tweak t5701 to be more careful about what it is
looking for.
Also, we should arguably give the "Cloning into..." message the same
treatment. We have printed that to stdout for a very long time, so there
is a slim chance that somebody actually tries to parse it. But I think
they are wrong to do so; we already changed it once (in 28ba96a), and
these days it is internationalized, anyway.
In May of 2012 I posted this patch, but it got overlooked, and I forgot
about it but carried it in my tree since then. Maybe we should apply it
now (it fixes t5701; the "checking connectivity" patch can come on top,
or even just be squashed in).
-- >8 --
Subject: [PATCH] clone: send diagnostic messages to stderr
Putting messages like "Cloning into.." and "done" on stdout
is un-Unix and uselessly clutters the stdout channel. Send
them to stderr.
We have to tweak two tests to accommodate this:
1. t5601 checks for doubled output due to forking, and
doesn't actually care where the output goes; adjust it
to check stderr.
2. t5702 is trying to test whether progress output was
sent to stderr, but naively does so by checking
whether stderr produced any output. Instead, have it
look for "%", a token found in progress output but not
elsewhere (and which lets us avoid hard-coding the
progress text in the test).
Signed-off-by: Jeff King <p...@peff.net>
---
builtin/clone.c | 6 +++---
t/t5601-clone.sh | 2 +-
t/t5702-clone-options.sh | 4 ++--
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 3c91844..8723a3a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -379,7 +379,7 @@ static void clone_local(const char *src_repo, const char
*dest_repo)
}
if (0 <= option_verbosity)
- printf(_("done.\n"));
+ fprintf(stderr, _("done.\n"));
}
static const char *junk_work_tree;
@@ -849,9 +849,9 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
if (0 <= option_verbosity) {
if (option_bare)
- printf(_("Cloning into bare repository '%s'...\n"),
dir);
+ fprintf(stderr, _("Cloning into bare repository
'%s'...\n"), dir);
else
- printf(_("Cloning into '%s'...\n"), dir);
+ fprintf(stderr, _("Cloning into '%s'...\n"), dir);
}
init_db(option_template, INIT_DB_QUIET);
write_config(&option_config);
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 0629149..b3b11e6 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -36,7 +36,7 @@ test_expect_success C_LOCALE_OUTPUT 'output from clone' '
test_expect_success C_LOCALE_OUTPUT 'output from clone' '
rm -fr dst &&
- git clone -n "file://$(pwd)/src" dst >output &&
+ git clone -n "file://$(pwd)/src" dst >output 2>&1 &&
test $(grep Clon output | wc -l) = 1
'
diff --git a/t/t5702-clone-options.sh b/t/t5702-clone-options.sh
index 85cadfa..67e170e 100755
--- a/t/t5702-clone-options.sh
+++ b/t/t5702-clone-options.sh
@@ -22,14 +22,14 @@ test_expect_success 'redirected clone -v' '
test_expect_success 'redirected clone' '
git clone "file://$(pwd)/parent" clone-redirected >out 2>err &&
- test_must_be_empty err
+ ! grep % err
'
test_expect_success 'redirected clone -v' '
git clone --progress "file://$(pwd)/parent" clone-redirected-progress \
>out 2>err &&
- test -s err
+ grep % err
'
--
1.8.4.rc4.16.g228394f
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
