On Fri, Aug 18, 2017 at 12:12:37PM +0200, Patryk Obara wrote: > Jeff King <peff@xxxxxxxx> wrote: > > > AFAICT this is only here to avoid having to s/buf/line->buf/ in the rest > > of the function. But I think we should just make that change (you > > already did in some of the spots). And IMHO we should do the same for > > line->len. When there are two names for the same value, it increases the > > chances of a bug where the two end up diverging. > > My motivation was rather to keep patch(es) as small as possible because every > line using buf will be replaced in a later patch in series. But it will make > commit better (it will stand on its own), so why not to do it? :) Ah, I didn't notice those lines went away. That does make it less bad, but I do think it's easier to review if each commit stands on its own. In some cases, if it's really painful to do the intermediate cleanup, I might say something in the commit message like "this leaves X that is not ideal, but we'll be getting rid of it soon anyway". But in this case I think just creating that intermediate state is simple enough. > Ah, I only replaced comparison to NULL terminator with length check because > I thought it better shows intention of the code and I didn't notice, that > reversing order will result in better code overall. > > I will include both changes in v4. Thanks. -Peff
