Re: [PATCH v3 2/4] commit: replace the raw buffer with strbuf in read_graft_line

From: Patryk Obara <patryk.obara@gmail.com>
Date: Aug 18, 2017, 10:12 UTC
Message-ID: <CAJfL8+TRZQrfF9Y9PdBZTptEf_O9u9irVRzb0bBVAnTRga2xmw@mail.gmail.com>
In-Reply-To: <20170818062929.f4zitbtaeii4xiko@sigill.intra.peff.net>

Jeff King <peff@peff.net> wrote:

Show 5 quoted lines

> 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? :)

Show 6 quoted lines

> (…) I think short-circuiting like:
>
>   if (!line->len || line->buf[0] == '#')
>
> is better (I also think "!" instead of "== 0" is our usual style, but
> that's much less important).

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.

-- 
| ← Ceci n'est pas une pipe
Patryk Obara