Re: [PATCH v3] Refactor recv_sideband()

From: Nicolas Pitre <nico@fluxnic.net>
Date: Jun 23, 2016, 18:59 UTC
Message-ID: <alpine.LFD.2.20.1606231453400.2550@knanqh.ubzr>
In-Reply-To: <146670333753.555.4852543207010532908@typhoon.lan>

On Thu, 23 Jun 2016, Lukas Fleischer wrote:

Show 21 quoted lines

> On Thu, 23 Jun 2016 at 00:47:39, Nicolas Pitre wrote:
> > On Wed, 22 Jun 2016, Nicolas Pitre wrote:
> > [...]
> > >                 if (*b) {
> > >                         xwrite(STDERR_FILENO, outbuf.buf, outbuf.len);
> > >                         /* Incomplete line, skip the next prefix. */
> > >                         strbuf_reset(&outbuf);
> > >                 }
> > >                 continue;
> > > 
> > > You are probably missing a strbuf_addf() before the last xwrite().
> > 
> > In fact, you could simply append the partial line to the strbuf and make 
> > it the prefix for the next packet rather than writing a partial line.  
> > You'd only have to write a partial line before leaving the function if 
> > the strbuf is not empty at that point.
> 
> True. And I like that solution.
> 
> Thinking about your last sentence, do we care about printing an
> incomplete line at the end of the communication at all?

I'd think so. This is very unlikely to happen in practice, but that might be useful to diagnose potential problems.

Show 7 quoted lines

> If so, do we need to print such a line on every return path (i.e. on 
> protocol and remote errors as well)? If we do, and if we want to 
> implement partial line handling the way you suggested, we should 
> probably print that final line from a common return path. And if we 
> add such a path, we could reconsider using a non-static strbuf as 
> well, since we could simply strbuf_release() the output buffer in that 
> common code block. Opinions?

That makes sense to me.

Nicolas