Re: [PATCH v5 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

From: Taylor Blau <me@ttaylorr.com>
Date: May 9, 2018, 23:48 UTC
Message-ID: <20180509234815.GA68889@syl.local>
In-Reply-To: <CACsJy8DLthWhxH6a7zwCvdirkuntanHasXZwfdADeo7iV8X79w@mail.gmail.com>

On Wed, May 09, 2018 at 06:17:20PM +0200, Duy Nguyen wrote:

Show 19 quoted lines

> On Wed, May 9, 2018 at 4:13 AM, Taylor Blau <me@ttaylorr.com> wrote:
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index 5f32d2ce84..f9f516dfc4 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -829,6 +829,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> >                             GREP_PATTERN_TYPE_PCRE),
> >                 OPT_GROUP(""),
> >                 OPT_BOOL('n', "line-number", &opt.linenum, N_("show line numbers")),
> > +               OPT_BOOL(0, "column", &opt.columnnum, N_("show column number of first match")),
>
> Two things to consider:
>
> - do we ever want columnar output in git-grep? Something like "git
> grep --column -l" could make sense (if you don't have very large
> worktree). --column is currently used for column output in git-branch,
> git-tag and git-status, which makes me think maybe we should reserve
> "--column" for that purpose and use another name here, even if we
> don't ever want column output in git-grep, for consistency.

I think that this is a valid concern. I had a similar thought when adding 'git config --color' (as a new type specifier) that we might be squatting on '--color' and instead opted for '[--type=<type>]'.

I don't feel that the tradeoff between '--column' as a good name and the concern that we _might_ want to output in a columnar format in 'git grep' someday warrants the change.

> - If this is to help git-jump only and rarely manually specified on
> command line, you could add the flag PARSE_OPT_NOCOMPLETE to hide it
> from "git grep --<tab>" completion. You would need to use OPT_BOOL_F()
> instead of OPT_BOOL() in order to add extra flags.

I believe that this option is worth auto-completing. Its primarily motivated for use within 'git-jump', but I feel as if it would be useful for other callers, as well.

Thanks, Taylor