Re: [PATCH] Refactor recv_sideband()

[Nicolas Pitre <nico@fluxnic.net>]

On Mon, 13 Jun 2016, Nicolas Pitre wrote:

> On Mon, 13 Jun 2016, Lukas Fleischer wrote:
> 
> > Improve the readability of recv_sideband() significantly by replacing
> > fragile buffer manipulations with more sophisticated format strings.
> > Also, reorganize the overall control flow, remove some superfluous
> > variables and replace a custom implementation of strpbrk() with a call
> > to the standard C library function.
> > 
> > Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
> 
> The previous code was a total abomination, even if I happen to know who 
> wrote it.
> 
> Acked-by: Nicolas Pitre <nico@fluxnic.net>

I just looked again at all the contraptions _I_ wrote (not Junio's) for 
a reason why I went to such extremes in making this code co complicated.

One aspect that is now lost with your patch is the atomic nature of the 
write.  See commit ed1902ef5c for the explanation.  You could probably 
use sprintf() into a temporary buffer and write it in one go to avoid 
segmented writes from the C library. It's probably not worth having that 
complex code just to avoid a string copy.

> > I had a really hard time reading and understanding this function when I
> > came up with my last patch. What I ended up with is almost a complete
> > rewrite of recv_sideband() and I find the end result to be much more
> > readable than what we have now. Given that this is quite invasive, it
> > would be good to have some more eyes and opinions...

I'd also suggest you look at "git log --author=Pitre sideband.c" output 
where I documented other examples of messed up displays that led to 
the current code so you could confirm your patch covers them.


Nicolas