Commit Graph

10 Commits (master)

Author SHA1 Message Date
Jeff King af22a63c39 sideband: diagnose more sideband anomalies
In demultiplex_sideband(), there are two oddities when we check an
incoming packet:

  - if it has zero length, then we assume it's a flush packet. This
    means we fail to notice the difference between a real flush and a
    true zero-length packet that's missing its sideband designator. It's
    not a huge problem in practice because we'd never send a zero-length
    data packet (even our keepalives are otherwise-empty sideband-1

    But it would be nice to detect and report the error, since it's
    likely to cause other confusion (we think the other side flushed,
    but they do not).

  - we try to detect packets missing their designator by checking for
    "if (len < 1)". But this will never trigger for "len == 0"; we've
    already detected that and left the function before then.

    It _could_ detect a negative "len" parameter. But in that case, the
    error message is wrong. The issue is not "no sideband" but rather
    "eof while reading the packet". However, this can't actually be
    triggered in practice, because neither of the two callers uses
    pkt_read's GENTLE_ON_EOF flag. Which means they'd die with "the
    remote end hung up unexpectedly" before we even get here.

    So this truly is dead code.

We can improve these cases by passing in a pkt-line status to the
demultiplexer, and by having recv_sideband() use GENTLE_ON_EOF. This
gives us two improvements:

  - we can now reliably detect flush packets, and will report a normal
    packet missing its sideband designator as an error

  - we'll report an eof with a more detailed "protocol error: eof while
    reading sideband packet", rather than the generic "the remote end
    hung up unexpectedly"

  - when we see an eof, we'll flush the sideband scratch buffer, which
    may provide some hints from the remote about why they hung up
    (though note we already flush on newlines, so it's likely that most
    such messages already made it through)

In some sense this patch goes against fbd76cd450 (sideband: reverse its
dependency on pkt-line, 2019-01-16), which caused the sideband code not
to depend on the pkt-line code. But that commit was really just trying
to deal with the circular header dependency. The two modules are
conceptually interlinked, and it was just trying to keep things
compiling. And indeed, there's a sticking point in this patch: because
pkt-line.h includes sideband.h, we can't add the reverse include we need
for the sideband code to have an "enum packet_read_status" parameter.
Nor can we forward declare it, because you can't forward declare an enum
in C. However, C does guarantee that enums fit in an int, so we can just
use that type.

One alternative would be for the callers to check themselves that they
got something sane from the pkt-line code. But besides duplicating
logic, this gets quite tricky. Any error condition requires flushing the
sideband #2 scratch buffer, which only demultiplex_sideband() knows how
to do.

Signed-off-by: Jeff King <>
Signed-off-by: Junio C Hamano <>
2020-10-29 09:23:29 -07:00
Jonathan Tan 0bbc0bc574 {fetch,upload}-pack: sideband v2 fetch response
Currently, a response to a fetch request has sideband support only while
the packfile is being sent, meaning that the server cannot send notices
until the start of the packfile.

Extend sideband support in protocol v2 fetch responses to the whole
response. upload-pack will advertise it if the
uploadpack.allowsidebandall configuration variable is set, and
fetch-pack will automatically request it if advertised.

If the sideband is to be used throughout the whole response, upload-pack
will use it to send errors instead of prefixing a PKT-LINE payload with
"ERR ".

This will be tested in a subsequent patch.

Signed-off-by: Jonathan Tan <>
Signed-off-by: Junio C Hamano <>
2019-01-17 11:25:07 -08:00
Jonathan Tan fbd76cd450 sideband: reverse its dependency on pkt-line
A subsequent patch will teach struct packet_reader a new field that, if
set, instructs it to interpret read data as multiplexed. This will
create a dependency from pkt-line to sideband.

To avoid a circular dependency, split recv_sideband() into 2 parts: the
reading loop (left in recv_sideband()) and the processing of the
contents (in demultiplex_sideband()), and move the former into pkt-line.
This reverses the direction of dependency: sideband no longer depends on
pkt-line, and pkt-line now depends on sideband.

Signed-off-by: Jonathan Tan <>
Signed-off-by: Junio C Hamano <>
2019-01-17 11:25:07 -08:00
Lukas Fleischer 4c4b7d1d3b sideband.c: make send_sideband() return void
The send_sideband() function uses write_or_die() for writing data which
immediately terminates the process on errors. If no such error occurred,
send_sideband() always returned the value that was passed as fourth
parameter prior to this commit. This value is already known to the
caller in any case, so let's turn send_sideband() into a void function

Signed-off-by: Lukas Fleischer <>
Signed-off-by: Junio C Hamano <>
2016-06-16 11:40:19 -07:00
Jeff King 047ec60205 pkt-line: move LARGE_PACKET_MAX definition from sideband
Having the packet sizes defined near the packet read/write
functions makes more sense.

Signed-off-by: Jeff King <>
Signed-off-by: Junio C Hamano <>
2013-02-20 13:42:22 -08:00
Johannes Sixt 34df8abaf3 recv_sideband: Bands #2 and #3 always go to stderr
This removes the last parameter of recv_sideband, by which the callers
told which channel bands #2 and #3 should be written to.

Sayeth Shawn Pearce:

   The definition of the streams in the current sideband protocol
   are rather well defined for the one protocol that uses it,

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

Since both callers of the function passed 2 for the parameter, we hereby
remove it and send bands #2 and #3 to stderr explicitly using fprintf.

This has the nice side-effect that these two streams pass through our
ANSI emulation layer on Windows.

Signed-off-by: Johannes Sixt <>
Acked-by: Nicolas Pitre <>
Signed-off-by: Junio C Hamano <>
2009-03-10 23:23:02 -07:00
Nicolas Pitre 9ac13ec941 atomic write for sideband remote messages
It has been a few times that I ended up with such a confusing display:

|remote: Generating pack...
|remote: Done counting 17 objects.
|remote: Result has 9 objects.
|remote: Deltifying 9 objects.
|remote:  100% (9/9) done
|remote: Unpacking 9 objects
|Total 9, written 9 (delta 8), reused 0 (delta 0)
| 100% (9/9) done

The confusion can be avoided in most cases by writing the remote message
in one go to prevent interleacing with local messages.  The buffer
declaration has been moved inside recv_sideband() to avoid extra string

Signed-off-by: Nicolas Pitre <>
Signed-off-by: Junio C Hamano <>
2006-10-11 11:13:01 -07:00
Junio C Hamano d47f3db75c Prepare larger packet buffer for upload-pack protocol.
The original side-band support added to the upload-pack protocol used the
default 1000-byte packet length.  The pkt-line format allows up to 64k, so
prepare the receiver for the maximum size, and have the uploader and
downloader negotiate if larger packet length is allowed.

Signed-off-by: Junio C Hamano <>
2006-09-10 16:27:08 -07:00
Junio C Hamano 958c24b1b8 Move sideband server side support into reusable form.
The server side support; this is just the very low level, and the
caller needs to know which band it wants to send things out.

Signed-off-by: Junio C Hamano <>
(cherry picked from b786552b67878c7780c50def4c069d46dc54efbe commit)
2006-09-10 13:36:50 -07:00
Junio C Hamano 49a52b1d1f Move sideband client side support into reusable form.
This moves the receiver side of the sideband support from
fetch-clone.c to sideband.c and its header file, so that
archiver protocol can use it.

Signed-off-by: Junio C Hamano <>
2006-09-10 13:36:35 -07:00