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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: git-owner@xxxxxxxxxxxxxxx [mailto:git-owner@xxxxxxxxxxxxxxx] On
> Behalf Of Jeff King
> Sent: den 18 september 2013 20:46
> To: Peter Kjellerstedt
> Cc: Junio C Hamano; Nguyen Thai Ngoc Duy; git@xxxxxxxxxxxxxxx
> 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@xxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

//Peter

��.n��������+%������w��{.n��������n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]