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

From: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
Date: Sep 19, 2013, 07:54 UTC
Message-ID: <A612847CFE53224C91B23E3A5B48BAC798CD91DBA7@xmail3.se.axis.com>
In-Reply-To: <20130918184551.GC18821@sigill.intra.peff.net>

Show 24 quoted lines

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

Show 46 quoted lines

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