Re: [PATCH v2 2/6] grep.c: take column number as argument to show_line()

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 24, 2018, 04:27 UTC
Message-ID: <20180424042739.GA82093@syl.local>
In-Reply-To: <CAPig+cRXkSrPHPyEEhp6_ndRBNW3hE7HkspSk1atPSE5pn_sMw@mail.gmail.com>

On Mon, Apr 23, 2018 at 03:34:21AM -0400, Eric Sunshine wrote:

Show 18 quoted lines

> On Mon, Apr 23, 2018 at 3:27 AM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> > On Mon, Apr 23 2018, Eric Sunshine wrote:
> >> One important issue I noticed is that patch 3/7 neglects to update
> >> grep.c:init_grep_defaults() to initialize opt.color_columnno.
> >
> > I think this is fine for fields that are 0 by default, since the struct
> >  is already zero'd out. See my e62ba43244 ("grep: remove redundant
> >  double assignment to 0", 2017-06-29) for some prior art.
>
> Indeed, I wasn't worried about opt.columnnum, which is fine being
> zero'd out by the memset(). What I was concerned about was
> opt.color_columnno; the corresponding opt.color_lineno is handled
> explicitly in init_grep_defaults():
>
>     color_set(opt->color_lineno, "");
>
> I'd expect opt.color_columnno to be treated likewise.

I agree with Ævar and Eric, we should certainly zero-out opt->color_lineno in grep.c's init_grep_defaults().

I recall doing this in v1, but I think that I must have dropped this part of the patch on the floor. In either case, I have amended my local copy to include this color_set() invocation and will include it in v3 (which I hope to send later this evening).

Thanks, Taylor