Re: [PATCH/RFC] recv_sideband: Band #2 always goes to stderr

From: Shawn O. Pearce <spearce@spearce.org>
Date: Mar 10, 2009, 14:38 UTC
Message-ID: <20090310143851.GP11989@spearce.org>
In-Reply-To: <49B6788F.2080609@viscovery.net>

Johannes Sixt <j.sixt@viscovery.net> wrote:

Show 8 quoted lines

> But don't you see that are mixing a high-level concept of "terminal" into
> the low-level function that you want it to be? In its current form,
> recv_sideband() is *not* a low-level utility, it's already at a high level
> that knows about the line-oriented nature of band #2. What you need for
> GitTorrent is a different function that *only* demultiplexes the sideband
> protocol data into different streams without munging them. That's a
> totally different function that *maybe* can share some code with the
> current recv_sideband().

ACK.

The definition of the streams in the current sideband protocol are rather well defined for the one protocol that uses it, fetch-pack/receive-pack:

  stream #1:  pack data
  stream #2:  stderr messages, progress, meant for tty
  stream #3:  oh-sh*t abort message, remote is dead, goodbye!

The stream number is encoded as a byte.  Anyone trying to reuse the
sideband protocol within the fetch-pack/receive-pack protocol to
carry *extra* data should use new channel numbers.  We have another
252 remaining.  I don't think we're lacking on description space.

Show 5 quoted lines

> With reference to Peter's reply, I'm not the only one who gets nervous if
> write() is replaced in a non-trivial way.
> 
> After all, you are sneaking the high-level concept "terminal emulation"
> into the low-level write() function.

Oh god.  Please do not do this.  I usually ignore the Win32 port
stuff.  But I agree with you Hannes, please do not do this.

> I'm absolutely not annoyed. And I am as married to my POV as you are to
> yours. ;) In this case we perhaps need a tie-breaker.

I cast my vote in with you Hannes. Your original RFC patch made sense as a cleanup in the code.

I agree with you that the way the code is currently structured and used, sideband #2 should always dump to the stderr channel of the process. We also assume in a lot of other places that fprintf(stderr, ...) is a good way to report progress to the end-user. This is no different, its just progress data from the remote system rather than the local system.

Huge refactorings would be necessary to split sideband #2 into something other than stderr. You would also want to replace nearly all references to stderr. We're not going down that road.

FWIW, JGit takes the meanings of the streams as I described them above. Stream #1 goes to the pack processing, stream #2 gets parsed and converted into updates calls to our ProgressMonitor API (which in turn goes to GUI progress meters in the host IDE), stream #3, if it ever shows up, turns into the message of an exception being thrown up the stack. That is the definition of this particular protocol. Accept it, and move it. :-)

The sideband protocol is fairly simple. If we ever needed to use it for other data, we could probably refactor some of the header formatting/parsing out a bit and create a more generalized split function, under a different name. But that's all in the future and something we just don't have an immediate need to worry about. So don't worry about it.

-- 
Shawn.