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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 18, 2013, 19:31 UTC
Message-ID: <xmqqmwnaudtg.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20130918190437.GD18821@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 21 quoted lines

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

Show 8 quoted lines

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

Show 5 quoted lines

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