Re: [PATCH] Refactor recv_sideband()

From: Nicolas Pitre <nico@fluxnic.net>
Date: Jun 14, 2016, 17:09 UTC
Message-ID: <alpine.LFD.2.20.1606141245490.1714@knanqh.ubzr>
In-Reply-To: <alpine.LFD.2.20.1606131704060.1714@knanqh.ubzr>

On Mon, 13 Jun 2016, Nicolas Pitre wrote:

Show 14 quoted lines

> 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.

Show 5 quoted lines

> > 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