From: Jeff King <peff@peff.net>
Date: Fri, 18 Aug 2017 11:50:17 GMT
Subject: Re: [PATCH v3 2/4] commit: replace the raw buffer with strbuf in read_graft_line
Message-ID: <20170818115017.cji4r5nrdetumtqo@sigill.intra.peff.net>
In-Reply-To: <CAJfL8+TRZQrfF9Y9PdBZTptEf_O9u9irVRzb0bBVAnTRga2xmw@mail.gmail.com>

On Fri, Aug 18, 2017 at 12:12:37PM +0200, Patryk Obara wrote:

> Jeff King <peff@peff.net> 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