git clone silently aborts if stdout gets a broken pipe

git clone silently aborts if stdout gets a broken pipe

[Peter Kjellerstedt]

[-- Attachment #1: Type: text/plain, Size: 1028 bytes --]

One of our Perl scripts that does a git clone suddenly 
started to fail when I upgraded to git 1.8.4 from 1.8.3.1.

The failing Perl code used a construct like this:

	Git::command_oneline('clone', $url, $path);

There is no error raised, but the directory specified by 
$path is not created. If I look at the process using strace 
I can see the clone taking place, but then it seems to get 
a broken pipe since the code above only cares about the 
first line from stdout (and with the addition of "Checking 
connectivity..." git clone now outputs two lines to stdout).

If I change the code to:

	my @foo = Git::command('clone', $url, $path);

it works as expected.

I have attached a simple Perl script that shows the problem.
Run it as "clone_test.pl <git url>". With git 1.8.4 it will 
fail for the first two test cases, whereas with older git 
versions it succeeds for all four test cases.

I hope this is enough information for someone to look into 
this regression.

Best regards,
//Peter


[-- Attachment #2: clone_test.pl --]
[-- Type: application/octet-stream, Size: 796 bytes --]

#!/bin/perl

use Git;

my $url = $ARGV[0] || die("You need to specify a Git URL!\n");
my $path = 'clone_test';

system("rm -rf ${path}1");
Git::command_oneline('clone', "$url", "${path}1");
warn("Failed to clone to ${path}1!\n") unless (-d "${path}1");

system("rm -rf ${path}2");
my $foo = Git::command_oneline('clone', $url, "${path}2");
warn("Failed to clone to ${path}2!\n") unless (-d "${path}2");

system("rm -rf ${path}3");
my $foo = Git::command('clone', $url, "${path}3");
warn("Failed to clone to ${path}3!\n") unless (-d "${path}3");

system("rm -rf ${path}4");
my @foo = Git::command('clone', $url, "${path}4");
warn("Failed to clone to ${path}4!\n") unless (-d "${path}4");

system("rm -rf ${path}1");
system("rm -rf ${path}2");
system("rm -rf ${path}3");
system("rm -rf ${path}4");

Re: git clone silently aborts if stdout gets a broken pipe

[Jeff King]

On Wed, Sep 18, 2013 at 06:52:13PM +0200, Peter Kjellerstedt wrote:

> The failing Perl code used a construct like this:
> 
> 	Git::command_oneline('clone', $url, $path);
> 
> There is no error raised, but the directory specified by 
> $path is not created. If I look at the process using strace 
> I can see the clone taking place, but then it seems to get 
> a broken pipe since the code above only cares about the 
> first line from stdout (and with the addition of "Checking 
> connectivity..." git clone now outputs two lines to stdout).

I think your perl script is somewhat questionable, as it is making
assumptions about the output of git-clone, and you would do better to
accept arbitrary-sized output (or better yet, leave stdout pointing to
the user, so they can see the output, which is meant for them).

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/clone.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index ca3eb68..3c91844 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -551,12 +551,12 @@ static void update_remote_refs(const struct ref *refs,
 
 	if (check_connectivity) {
 		if (0 <= option_verbosity)
-			printf(_("Checking connectivity... "));
+			fprintf(stderr, _("Checking connectivity... "));
 		if (check_everything_connected_with_transport(iterate_ref_map,
 							      0, &rm, transport))
 			die(_("remote did not send all necessary objects"));
 		if (0 <= option_verbosity)
-			printf(_("done\n"));
+			fprintf(stderr, _("done\n"));
 	}
 
 	if (refs) {
-- 
1.8.4.rc4.16.g228394f

Re: git clone silently aborts if stdout gets a broken pipe

[Jeff King]

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 <peff@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

Re: git clone silently aborts if stdout gets a broken pipe

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

Hmm, where in t5701?  Ah, you meant t5702 and possibly t5601.

> 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.

I actually think "it is long and not meant to be seen sequentially"
is a bad classifier; these new messages are also progress report in
that it reports "we are now in this phase".  So if I were to vote, I
would say we should apply the same progress-silencing criteria,
preferrably by not checking isatty() again, but by recording the
decision we have already made when squelching the progress during
the transfer in order to make sure they stay consistent.

> 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.

Good thinking.  Please make it so ;-)

Re: git clone silently aborts if stdout gets a broken pipe

[Jeff King]

On Wed, Sep 18, 2013 at 12:31:23PM -0700, Junio C Hamano wrote:

> > Hrm, this actually breaks t5701, which expects "clone 2>err" to print
> > nothing to stderr.
> 
> Hmm, where in t5701?  Ah, you meant t5702 and possibly t5601.

Yes, sorry, I meant t5702.

> I actually think "it is long and not meant to be seen sequentially"
> is a bad classifier; these new messages are also progress report in
> that it reports "we are now in this phase".  So if I were to vote, I
> would say we should apply the same progress-silencing criteria,
> preferrably by not checking isatty() again, but by recording the
> decision we have already made when squelching the progress during
> the transfer in order to make sure they stay consistent.

Unfortunately that decision is made in the transport code, not by clone
itself. We can cheat and peek at "transport->progress" after
initializing the transport. That would require some refactoring, though;
we print "Cloning into" before setting up the transport. And we do not
even tell the transport about our progress options if we are doing a
local clone.

If we wanted to _just_ suppress "Checking connectivity" (and not
"Cloning into..."), that's a bit easier. And I could see an argument
that the former is the only one that falls into the "progress report"
category.

> > 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.
> 
> Good thinking.  Please make it so ;-)

OK. I've squashed the "use stderr" patches into one, and added a patch
on top to correctly check the progress flag.

  [1/2]: clone: send diagnostic messages to stderr
  [2/2]: clone: treat "checking connectivity" like other progress

-Peff

[PATCH 1/2] clone: send diagnostic messages to stderr

[Jeff King]

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).

This should not regress any scripts that try to parse the
current output, as the output is already internationalized
and therefore unstable.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/clone.c          | 10 +++++-----
 t/t5601-clone.sh         |  2 +-
 t/t5702-clone-options.sh |  9 +++++----
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index ca3eb68..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;
@@ -551,12 +551,12 @@ static void update_remote_refs(const struct ref *refs,
 
 	if (check_connectivity) {
 		if (0 <= option_verbosity)
-			printf(_("Checking connectivity... "));
+			fprintf(stderr, _("Checking connectivity... "));
 		if (check_everything_connected_with_transport(iterate_ref_map,
 							      0, &rm, transport))
 			die(_("remote did not send all necessary objects"));
 		if (0 <= option_verbosity)
-			printf(_("done\n"));
+			fprintf(stderr, _("done\n"));
 	}
 
 	if (refs) {
@@ -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..d3dbdfe 100755
--- a/t/t5702-clone-options.sh
+++ b/t/t5702-clone-options.sh
@@ -19,17 +19,18 @@ test_expect_success 'redirected clone -v' '
 
 '
 
-test_expect_success 'redirected clone' '
+test_expect_success 'redirected clone does not show progress' '
 
 	git clone "file://$(pwd)/parent" clone-redirected >out 2>err &&
-	test_must_be_empty err
+	! grep % err
 
 '
-test_expect_success 'redirected clone -v' '
+
+test_expect_success 'redirected clone -v does show progress' '
 
 	git clone --progress "file://$(pwd)/parent" clone-redirected-progress \
 		>out 2>err &&
-	test -s err
+	grep % err
 
 '
 
-- 
1.8.4.rc4.16.g228394f

[PATCH 2/2] clone: treat "checking connectivity" like other progress

[Jeff King]

When stderr does not point to a tty, we typically suppress
"we are now in this phase" progress reporting (e.g., we ask
the server not to send us "counting objects" and the like).

The new "checking connectivity" message is in the same vein,
and should be suppressed. Since clone relies on the
transport code to make the decision, we can simply sneak a
peek at the "progress" field of the transport struct. That
properly takes into account both the verbosity and progress
options we were given, as well as the result of isatty().

Note that we do not set up that progress flag for a local
clone, as we do not fetch using the transport at all. That's
acceptable here, though, because we also do not perform a
connectivity check in that case.

Signed-off-by: Jeff King <peff@peff.net>
---
Though the last paragraph explains why this is OK, it feels a bit
fragile. I wonder if we should hoist the call to transport_set_verbosity
outside the "!is_local" conditional. I do not think it would hurt
anything.

 builtin/clone.c          | 4 ++--
 t/t5702-clone-options.sh | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 8723a3a..7c62298 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -550,12 +550,12 @@ static void update_remote_refs(const struct ref *refs,
 	const struct ref *rm = mapped_refs;
 
 	if (check_connectivity) {
-		if (0 <= option_verbosity)
+		if (transport->progress)
 			fprintf(stderr, _("Checking connectivity... "));
 		if (check_everything_connected_with_transport(iterate_ref_map,
 							      0, &rm, transport))
 			die(_("remote did not send all necessary objects"));
-		if (0 <= option_verbosity)
+		if (transport->progress)
 			fprintf(stderr, _("done\n"));
 	}
 
diff --git a/t/t5702-clone-options.sh b/t/t5702-clone-options.sh
index d3dbdfe..9e24ec8 100755
--- a/t/t5702-clone-options.sh
+++ b/t/t5702-clone-options.sh
@@ -22,7 +22,8 @@ test_expect_success 'redirected clone does not show progress' '
 test_expect_success 'redirected clone does not show progress' '
 
 	git clone "file://$(pwd)/parent" clone-redirected >out 2>err &&
-	! grep % err
+	! grep % err &&
+	test_i18ngrep ! "Checking connectivity" err
 
 '
 
-- 
1.8.4.rc4.16.g228394f

[PATCH 3/2] clone: always set transport options

[Jeff King]

On Wed, Sep 18, 2013 at 04:06:50PM -0400, Jeff King wrote:

> Note that we do not set up that progress flag for a local
> clone, as we do not fetch using the transport at all. That's
> acceptable here, though, because we also do not perform a
> connectivity check in that case.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Though the last paragraph explains why this is OK, it feels a bit
> fragile. I wonder if we should hoist the call to transport_set_verbosity
> outside the "!is_local" conditional. I do not think it would hurt
> anything.

Actually, I think the option-setting in clone is a little bit broken.
Mostly it is just making fragile assumptions that happen to be true
(e.g., that fetching the ref list will never care about the progress
flag), but there are some options that should be respected in both
cases.

I think we should do this on top.

-- >8 --
Subject: [PATCH] clone: always set transport options

A clone will always create a transport struct, whether we
are cloning locally or using an actual protocol. In the
local case, we only use the transport to get the list of
refs, and then transfer the objects out-of-band.

However, there are many options that we do not bother
setting up in the local case. For the most part, these are
noops, because they only affect the object-fetching stage
(e.g., the --depth option).  However, some options do have a
visible impact. For example, giving the path to upload-pack
via "-u" does not currently work for a local clone, even
though we need upload-pack to get the ref list.

We can just drop the conditional entirely and set these
options for both local and non-local clones. Rather than
keep track of which options impact the object versus the ref
fetching stage, we can simply let the noops be noops (and
the cost of setting the options in the first place is not
high).

The one exception is that we also check that the transport
provides both a "get_refs_list" and a "fetch" method. We
will now be checking the former for both cases (which is
good, since a transport that cannot fetch refs would not
work for a local clone), and we tweak the conditional to
check for a "fetch" only when we are non-local.

Signed-off-by: Jeff King <peff@peff.net>
---
The diff is rather unreadable, but using "show -b" reveals the actual
changes.

 builtin/clone.c        | 30 ++++++++++++++----------------
 t/t5701-clone-local.sh |  4 ++++
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 7c62298..7ac677d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -884,27 +884,25 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	remote = remote_get(option_origin);
 	transport = transport_get(remote, remote->url[0]);
 
-	if (!is_local) {
-		if (!transport->get_refs_list || !transport->fetch)
-			die(_("Don't know how to clone %s"), transport->url);
+	if (!transport->get_refs_list || (!is_local && !transport->fetch))
+		die(_("Don't know how to clone %s"), transport->url);
 
-		transport_set_option(transport, TRANS_OPT_KEEP, "yes");
+	transport_set_option(transport, TRANS_OPT_KEEP, "yes");
 
-		if (option_depth)
-			transport_set_option(transport, TRANS_OPT_DEPTH,
-					     option_depth);
-		if (option_single_branch)
-			transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
+	if (option_depth)
+		transport_set_option(transport, TRANS_OPT_DEPTH,
+				     option_depth);
+	if (option_single_branch)
+		transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
 
-		transport_set_verbosity(transport, option_verbosity, option_progress);
+	transport_set_verbosity(transport, option_verbosity, option_progress);
 
-		if (option_upload_pack)
-			transport_set_option(transport, TRANS_OPT_UPLOADPACK,
-					     option_upload_pack);
+	if (option_upload_pack)
+		transport_set_option(transport, TRANS_OPT_UPLOADPACK,
+				     option_upload_pack);
 
-		if (transport->smart_options && !option_depth)
-			transport->smart_options->check_self_contained_and_connected = 1;
-	}
+	if (transport->smart_options && !option_depth)
+		transport->smart_options->check_self_contained_and_connected = 1;
 
 	refs = transport_get_remote_refs(transport);
 
diff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh
index 7ff6e0e..c490368 100755
--- a/t/t5701-clone-local.sh
+++ b/t/t5701-clone-local.sh
@@ -134,4 +134,8 @@ test_expect_success 'cloning a local path with --no-local does not hardlink' '
 	! repo_is_hardlinked force-nonlocal
 '
 
+test_expect_success 'cloning locally respects "-u" for fetching refs' '
+	test_must_fail git clone --bare -u false a should_not_work.git
+'
+
 test_done
-- 
1.8.4.rc4.16.g228394f

RE: git clone silently aborts if stdout gets a broken pipe

[Peter Kjellerstedt]

> -----Original Message-----
> From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On
> Behalf Of Jeff King
> Sent: den 18 september 2013 20:46
> To: Peter Kjellerstedt
> Cc: Junio C Hamano; Nguyen Thai Ngoc Duy; git@vger.kernel.org
> Subject: Re: git clone silently aborts if stdout gets a broken pipe
> 
> On Wed, Sep 18, 2013 at 06:52:13PM +0200, Peter Kjellerstedt wrote:
> 
> > The failing Perl code used a construct like this:
> >
> > 	Git::command_oneline('clone', $url, $path);
> >
> > There is no error raised, but the directory specified by
> > $path is not created. If I look at the process using strace
> > I can see the clone taking place, but then it seems to get
> > a broken pipe since the code above only cares about the
> > first line from stdout (and with the addition of "Checking
> > connectivity..." git clone now outputs two lines to stdout).
> 
> I think your perl script is somewhat questionable, as it is making
> assumptions about the output of git-clone, and you would do better to
> accept arbitrary-sized output 

Well, the whole idea of using Git::command_oneline() is that we 
are only interested in the first line of output, similar to using 
"| head -1". If we had wanted all of the output we would have used 
Git::command() instead. Since the Git Perl module is released as a 
part of Git, I would expect it to work as documented regardless of 
which Git command is used with Git::command_oneline().

In the case of git clone the output to stdout is pretty small so 
retrieving all of it would of course not be much overhead, but for 
some other commands retrieving all output when only the first line 
is wanted (or maybe not even that one) seems unnecessary.

However, what surprised me most was that git clone failed silently 
when it got a broken pipe. I cannot really see the reason for 
aborting due to stdout getting a broken pipe in the first place. 
But if it is, I would at least have expected an error which our 
script would have caught and aborted with an appropriate error 
message. Now it instead failed later when it actually tried to 
access the files in the repository it thought it had cloned...

> (or better yet, leave stdout pointing to
> the user, so they can see the output, which is meant for them).

Well, in this specific case it is a script being run as a cron job 
so anything sent to stdout would cause an unnecessary mail.

> That being said, the new messages should almost certainly go to stderr.

I can but agree.

> -- >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.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/clone.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin/clone.c b/builtin/clone.c
> index ca3eb68..3c91844 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -551,12 +551,12 @@ static void update_remote_refs(const struct ref *refs,
> 
>  	if (check_connectivity) {
>  		if (0 <= option_verbosity)
> -			printf(_("Checking connectivity... "));
> +			fprintf(stderr, _("Checking connectivity... "));
>  		if (check_everything_connected_with_transport(iterate_ref_map,
>  							      0, &rm, transport))
>  			die(_("remote did not send all necessary objects"));
>  		if (0 <= option_verbosity)
> -			printf(_("done\n"));
> +			fprintf(stderr, _("done\n"));
>  	}
> 
>  	if (refs) {
> --
> 1.8.4.rc4.16.g228394f
> 
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thanks for taking the time to provide a solution to the problem.

//Peter

Re: git clone silently aborts if stdout gets a broken pipe

[Jeff King]

On Thu, Sep 19, 2013 at 09:54:38AM +0200, Peter Kjellerstedt wrote:

> > I think your perl script is somewhat questionable, as it is making
> > assumptions about the output of git-clone, and you would do better to
> > accept arbitrary-sized output 
> 
> Well, the whole idea of using Git::command_oneline() is that we 
> are only interested in the first line of output, similar to using 
> "| head -1". If we had wanted all of the output we would have used 
> Git::command() instead. Since the Git Perl module is released as a 
> part of Git, I would expect it to work as documented regardless of 
> which Git command is used with Git::command_oneline().

I think command_oneline is exactly like "| head -1" in this case. Doing
"git clone | head -1" would also fail, and should not be used. In
general, you do not want to put a limiting pipe on a command with side
effects beyond output. The design of unix pipes and SIGPIPE is such that
you can do "generate_output | head", and "generate_output" will get
SIGPIPE and die after realizing that its writer no longer cares about
the output. But if your command is doing something besides output, that
assumption doesn't hold.

Arguably, "git clone" should be taking the initiative to ignore SIGPIPE
itself.  Its primary function is not output, but doing the clone. If
output fails, we would want to continue the clone, not die.

By the way, did you actually want to capture the stdout of git-clone, or
were you just trying to suppress it? Because the eventual patch I posted
sends it to stderr, under the assumption that what used to go to stdout
should not be captured and parsed (because it is localized and subject
to change).

> However, what surprised me most was that git clone failed silently 
> when it got a broken pipe.

It's not "git clone" that is doing this, I think, but rather the design
of command_oneline. If I do:

  (sleep 1; git clone ...; echo >&2 exit=$?) | false

then I see:

  exit=141

That is, clone dies from SIGPIPE trying to write "Cloning into...". But
command_oneline is specifically designed to ignore SIGPIPE death,
because you would want something like:

  command_oneline("git", "rev-list", "$A..$B");

to give you the first line, and then you do not care if the rest of the
rev-list dies due to SIGPIPE (it is a good thing, because by closing the
pipe you are telling it that its output is not needed). It may be that
the documentation for command_oneline can be improved to mention this
subtlety.

-Peff

RE: git clone silently aborts if stdout gets a broken pipe

[Peter Kjellerstedt]

> -----Original Message-----
> From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On
> Behalf Of Jeff King
> Sent: den 19 september 2013 10:36
> To: Peter Kjellerstedt
> Cc: Junio C Hamano; Nguyen Thai Ngoc Duy; git@vger.kernel.org
> Subject: Re: git clone silently aborts if stdout gets a broken pipe
> 
> On Thu, Sep 19, 2013 at 09:54:38AM +0200, Peter Kjellerstedt wrote:
> 
> > > I think your perl script is somewhat questionable, as it is making
> > > assumptions about the output of git-clone, and you would do better
> to
> > > accept arbitrary-sized output
> >
> > Well, the whole idea of using Git::command_oneline() is that we
> > are only interested in the first line of output, similar to using
> > "| head -1". If we had wanted all of the output we would have used
> > Git::command() instead. Since the Git Perl module is released as a
> > part of Git, I would expect it to work as documented regardless of
> > which Git command is used with Git::command_oneline().
> 
> I think command_oneline is exactly like "| head -1" in this case. Doing
> "git clone | head -1" would also fail, and should not be used. In
> general, you do not want to put a limiting pipe on a command with side
> effects beyond output. The design of unix pipes and SIGPIPE is such that
> you can do "generate_output | head", and "generate_output" will get
> SIGPIPE and die after realizing that its writer no longer cares about
> the output. But if your command is doing something besides output, that
> assumption doesn't hold.

A very valid point.

> Arguably, "git clone" should be taking the initiative to ignore SIGPIPE
> itself.  Its primary function is not output, but doing the clone. If
> output fails, we would want to continue the clone, not die.
> 
> By the way, did you actually want to capture the stdout of git-clone, or
> were you just trying to suppress it? Because the eventual patch I posted
> sends it to stderr, under the assumption that what used to go to stdout
> should not be captured and parsed (because it is localized and subject
> to change).

No, we were not really interested in the output to stdout (which is 
why the return value from Git::command_oneline() was ignored).

> > However, what surprised me most was that git clone failed silently
> > when it got a broken pipe.
> 
> It's not "git clone" that is doing this, I think, but rather the design
> of command_oneline. If I do:
> 
>   (sleep 1; git clone ...; echo >&2 exit=$?) | false
> 
> then I see:
> 
>   exit=141
> 
> That is, clone dies from SIGPIPE trying to write "Cloning into...". But
> command_oneline is specifically designed to ignore SIGPIPE death,
> because you would want something like:
> 
>   command_oneline("git", "rev-list", "$A..$B");
> 
> to give you the first line, and then you do not care if the rest of the
> rev-list dies due to SIGPIPE (it is a good thing, because by closing the
> pipe you are telling it that its output is not needed). It may be that
> the documentation for command_oneline can be improved to mention this
> subtlety.

Ok, all of it makes sense now. Thank you for the explanation.
I have corrected our script so it now works correctly with 
git 1.8.4 as well.

> -Peff
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

//Peter