Re: [PATCH v5 2/2] pretty: colorize pattern matches in commit messages

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 17, 2021, 22:15 UTC
Message-ID: <CAPig+cTD22MM1vjXMSjVcNXfVWfqDgT7rDp1j-e_Fko33osATA@mail.gmail.com>
In-Reply-To: <YUUQA1qNCzY7Vx+j@coredump.intra.peff.net>

On Fri, Sep 17, 2021 at 6:01 PM Jeff King <peff@peff.net> wrote:

Show 12 quoted lines

> On Fri, Sep 17, 2021 at 05:14:56PM -0400, Hamza Mahfooz wrote:
> > On Fri, Sep 17 2021 at 03:10:12 AM -0400, Eric Sunshine
> > <sunshine@sunshineco.com> wrote:
> > > `buf` and `eol` seem like an accident waiting to happen...
> > > ... because strbuf_grow() may reallocate the underlying buffer, which
> > > means that `buf` and `eol` will end up pointing at freed memory, which
> > > will be accessed by the next call to grep_next_match().
> >
> > I don't see how it's problematic, since `tmp_sb` isn't modified after `buf`
> > is initialized (until strbuf_release() is called, of course).
>
> Yes, you are correct.

Indeed, I got confused by the multiple strbufs with similar names. However...

Show 17 quoted lines

> However, I do think the code would be much clearer
> if you skipped the strbuf entirely, like:
>
>   line_as_string = xmemdupz(line, linelen);
>   ...
>   buf = line_as_string;
>   eol = buf + linelen;
>
> which makes it much clearer that you don't intend to modify it further
> (especially with all those other calls operating on the _other_ strbuf,
> it's hard to see immediately that this is the case).
>
> The "as_string" name is assuming the purpose is to get a NUL-terminated
> string. I'm not sure why we need one, though, since we pass the buf/eol
> pointers to grep_next_match(). A comment above the xmemdupz() line might
> be a good place to explain that (which would help especially if the
> reasons change later and we can get rid of it).

... as Peff points out, it indeed is unclear why a copy of `line` is needed at all. It seems like a simple:

    buf = line;
    eol = buf + linelen;

would more than suffice without making any copies. If you do that, then the:

    if (buf != line) {

later in the code -- and which I questioned in my review as making no sense -- suddenly makes sense.