Re: [PATCH v4 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'
On Mon, May 07, 2018 at 11:13:12PM +0900, Junio C Hamano wrote: > Taylor Blau writes: > > > diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt > > index 18b494731f..5409a24399 100644 > > --- a/Documentation/git-grep.txt > > +++ b/Documentation/git-grep.txt > > @@ -13,7 +13,7 @@ SYNOPSIS > >[-v | --invert-match] [-h|-H] [--full-name] > >[-E | --extended-regexp] [-G | --basic-regexp] > >[-P | --perl-regexp] > > - [-F | --fixed-strings] [-n | --line-number] > > + [-F | --fixed-strings] [-n | --line-number] [--column] > >[-l | --files-with-matches] [-L | --files-without-match] > >[(-O | --open-files-in-pager) []] > >[-z | --null] > > @@ -169,6 +169,9 @@ providing this option will cause it to die. > > --line-number:: > > Prefix the line number to matching lines. > > > > +--column:: > > + Prefix the 1-indexed column number of the first match on non-context > > lines. > > + > > Two questions. > > - It is fine that the leftmost column is 1, but what does this >number count? The number of bytes on the same line before the >first byte of the hit (plus 1)? The display width of the initial >non-matching part of the line (plus 1) on a fixed-width terminal? >The number of "characters"? Something else? The count is the byte offset from the 1-index (which is the beginning of the line, as you noted). Incidentally, Peff and I chatted briefly offline about this, and agree that it makes the most sense, since (1) Vim treats it correctly, and (2) we can't be sure of options like display width, character count, etc., without knowing the character encoding. Nonetheless, other folks in this thread seem to be curious about this as well. I'll add it to the documentation for --column in Documentation/git-grep.txt. > - Does --column combined with -v make any sense? If not, shouldn't >the command error out when both are given at the same time? I hadn't thought of this. They do not work together, since 'git grep -v --column' would require us to either (1) not output the column number, or (2) output '0', or some other non-value. I think that both (1) and (2) require callers to complicate their scripts to understand either approach. As such, I think that we should die() here, and add a test in t7810 to ensure that that's indeed what happens. Does this seem sensible to include in v5? Thanks, Taylor
Re: [PATCH v4 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'
On Sun, May 06, 2018 at 07:56:42PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Sat, May 05 2018, Taylor Blau wrote:
>
>
> > + test_expect_success "grep -w $L (with --{line,column}-number)" '
>
> It's now --column in v4 but this still refers to v3 --column-number.
Thanks, I certainly missed this one.
Thanks,
Taylor
Re: [PATCH v4 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'
On Sat, May 05, 2018 at 08:15:03AM +0200, Duy Nguyen wrote: > On Sat, May 5, 2018 at 4:43 AM, Taylor Blau wrote: > > Teach 'git-grep(1)' a new option, '--column', to show the column > > number of the first match on a non-context line. > > Why? Or put it another way, what is this option used for? Only > git-jump? (which should also be mentioned here if true) Good question. My primary intention is giving 'contrib/git-jump/git-jump' the information it needs in order to tell $EDITOR how to seek to the relevant position within a line. I have amended this patch to include the relevant bits, and will attach in v5. Thanks, Taylor
Re: [PATCH v4 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'
Taylor Blau writes: > diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt > index 18b494731f..5409a24399 100644 > --- a/Documentation/git-grep.txt > +++ b/Documentation/git-grep.txt > @@ -13,7 +13,7 @@ SYNOPSIS > [-v | --invert-match] [-h|-H] [--full-name] > [-E | --extended-regexp] [-G | --basic-regexp] > [-P | --perl-regexp] > -[-F | --fixed-strings] [-n | --line-number] > +[-F | --fixed-strings] [-n | --line-number] [--column] > [-l | --files-with-matches] [-L | --files-without-match] > [(-O | --open-files-in-pager) []] > [-z | --null] > @@ -169,6 +169,9 @@ providing this option will cause it to die. > --line-number:: > Prefix the line number to matching lines. > > +--column:: > + Prefix the 1-indexed column number of the first match on non-context > lines. > + Two questions. - It is fine that the leftmost column is 1, but what does this number count? The number of bytes on the same line before the first byte of the hit (plus 1)? The display width of the initial non-matching part of the line (plus 1) on a fixed-width terminal? The number of "characters"? Something else? - Does --column combined with -v make any sense? If not, shouldn't the command error out when both are given at the same time?
Re: [PATCH v4 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'
On Sat, May 05 2018, Taylor Blau wrote:
> + test_expect_success "grep -w $L (with --{line,column}-number)" '
It's now --column in v4 but this still refers to v3 --column-number.
Re: [PATCH v4 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'
Hi Taylor
On 05/05/18 03:43, Taylor Blau wrote:
>
> Teach 'git-grep(1)' a new option, '--column', to show the column
> number of the first match on a non-context line.
>
> For example:
>
> $ git grep -n --column foo | head -n3
> .clang-format:51:14:# myFunction(foo, bar, baz);
> .clang-format:64:7:# int foo();
> .clang-format:75:8:# void foo()
>
> Signed-off-by: Taylor Blau
> ---
> Documentation/git-grep.txt | 5 -
> builtin/grep.c | 1 +
> t/t7810-grep.sh| 22 ++
> 3 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> index 18b494731f..5409a24399 100644
> --- a/Documentation/git-grep.txt
> +++ b/Documentation/git-grep.txt
> @@ -13,7 +13,7 @@ SYNOPSIS
> [-v | --invert-match] [-h|-H] [--full-name]
> [-E | --extended-regexp] [-G | --basic-regexp]
> [-P | --perl-regexp]
> -[-F | --fixed-strings] [-n | --line-number]
> +[-F | --fixed-strings] [-n | --line-number] [--column]
> [-l | --files-with-matches] [-L | --files-without-match]
> [(-O | --open-files-in-pager) []]
> [-z | --null]
> @@ -169,6 +169,9 @@ providing this option will cause it to die.
> --line-number::
> Prefix the line number to matching lines.
>
> +--column::
> + Prefix the 1-indexed column number of the first match on non-context
> lines.
> +
I think it would be useful to explain what the column number actually is
so that users know how to consume it. Is it a count of bytes, multi-byte
characters or graphemes? It would probably be worth testing with a file
that contains multi-byte characters to check for future regressions.
Best Wishes
Phillip
> -l::
> --files-with-matches::
> --name-only::
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 5f32d2ce84..5c83f17759 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")),
> OPT_NEGBIT('h', NULL, &opt.pathname, N_("don't show
> filenames"), 1),
> OPT_BIT('H', NULL, &opt.pathname, N_("show filenames"), 1),
> OPT_NEGBIT(0, "full-name", &opt.relative,
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 1797f632a3..a03c3416e7 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -99,6 +99,28 @@ do
> test_cmp expected actual
> '
>
> + test_expect_success "grep -w $L (with --column)" '
> + {
> + echo ${HC}file:5:foo mmap bar
> + echo ${HC}file:14:foo_mmap bar mmap
> + echo ${HC}file:5:foo mmap bar_mmap
> + echo ${HC}file:14:foo_mmap bar mmap baz
> + } >expected &&
> + git grep --column -w -e mmap $H >actual &&
> + test_cmp expected actual
> + '
> +
> + test_expect_success "grep -w $L (with --{line,column}-number)" '
> + {
> + echo ${HC}file:1:5:foo mmap bar
> + echo ${HC}file:3:14:foo_mmap bar mmap
> + echo ${HC}file:4:5:foo mmap bar_mmap
> + echo ${HC}file:5:14:foo_mmap bar mmap baz
> + } >expected &&
> + git grep -n --column -w -e mmap $H >actual &&
> + test_cmp expected actual
> + '
> +
> test_expect_success "grep -w $L" '
> {
> echo ${HC}file:1:foo mmap bar
>
Re: [PATCH v4 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'
On Sat, May 5, 2018 at 4:43 AM, Taylor Blau wrote: > Teach 'git-grep(1)' a new option, '--column', to show the column > number of the first match on a non-context line. Why? Or put it another way, what is this option used for? Only git-jump? (which should also be mentioned here if true) > > For example: > > $ git grep -n --column foo | head -n3 > .clang-format:51:14:# myFunction(foo, bar, baz); > .clang-format:64:7:# int foo(); > .clang-format:75:8:# void foo() > -- Duy
