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

["Shawn O. Pearce" <spearce@spearce.org>]

Johannes Sixt <j.sixt@viscovery.net> wrote:
> 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.
 
> 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.