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

From: Jeff King <peff@peff.net>
Date: Sep 21, 2021, 01:39 UTC
Message-ID: <YUk3zwuse56v76ze@coredump.intra.peff.net>
In-Reply-To: <YUk0OEXg36QXrkDm@coredump.intra.peff.net>

On Mon, Sep 20, 2021 at 09:24:08PM -0400, Jeff King wrote:

Show 11 quoted lines

> > +	buf = (char *)line;
> > +	eol = buf + linelen;
> 
> OK, so we got rid of the copy of "line", which is nice. But we are
> casting away const-ness, which is a potential red flag (is somebody
> going to modify this string, even though we promised our caller we would
> not?). We'd probably want a comment to explain why we are doing so, and
> why it is OK (e.g., if somebody in the call stack modifies it
> temporarily).
> 
> More on this in a moment.

The root of the issue is that grep_next_match() takes a non-const buffer, and so on. And indeed, it _does_ eventually get modified, although only temporarily. I think we can clean that up, though.

Here are two patches I prepared on top of your series to show what's possible, though I think we should do one of:

  - put them at the front of your series (with the appropriate
    adjustments) as preparatory cleanup

  - keep them separate. You can put a comment above the cast to mention
    what's going on and why it's OK for now, and then later when they're
    both merged, we can remove that cast.

The second option creates a little extra work for the maintainer (they both touch match_one_patter(), so there will be some textual conflicts). But it does mean we avoid a dependencies; the cleanups don't derail your series, nor does your series hold up the cleanups. So I could go either way.

  [1/2]: grep: stop modifying buffer in strip_timestamp
  [2/2]: grep: mark "haystack" buffers as const

 grep.c   | 30 ++++++++++++------------------
 grep.h   |  3 ++-
 pretty.c |  6 +++---
 3 files changed, 17 insertions(+), 22 deletions(-)

-Peff