Re: [PATCH 1/1] git-grep: improve the --show-function behaviour

From: René Scharfe <l.s.r@web.de>
Date: Sep 12, 2023, 18:07 UTC
Message-ID: <df05f761-c498-6930-bfd8-265f7e23d8ee@web.de>
In-Reply-To: <20230912135124.GA11315@redhat.com>

Am 12.09.23 um 15:51 schrieb Oleg Nesterov:

Show 6 quoted lines

> On 09/12, Oleg Nesterov wrote:
>>
>> René, Junio,
>>
>> I don't like the fact we can't understand each other ;) Could you
>> please explain why do you think this patch should update the docs?

Good thinking!  And thank you for the examples and you patience!

Show 27 quoted lines

>> Please forget about my patch for the moment. Lets start from the very
>> beginning:
>>
>> 	-p::
>> 	--show-function::
>> 		Show the preceding line that contains the function name of
>> 		the match, unless the matching line is a function name itself.
>>
>> and in my opinion, it is the current behaviour that doesn't match the
>> documentation.>>
>> -------------------------------------------------------------------------
>>
>> 	$ cat TEST1.c
>> 	void func1()
>> 	{
>> 	}
>> 	void func2()
>> 	{
>> 	}
>>
>> 	$ git grep --untracked -pn func2 TEST1.c
>> 	TEST1.c=1=void func1()
>> 	TEST1.c:4:void func2()
>>
>> in this case the matching line is "void func2()" and it is also a function
>> name itself, in this case git-grep should not show "=void func1()" which is
>> "the preceding line that contains the function name of the match.

Makes sense.

>> But it does.

Drat! ;)

The option -p came from diff(1). I thought diff change lines equivalent to grep match lines and hunk headers to the new = lines. What does diff do with the example file?

   $ diff -U0 -p TEST1.c <(sed s/2/3/ TEST1.c)
   --- TEST1.c	2023-09-12 17:59:04
   +++ /dev/fd/11	2023-09-12 18:54:03
   @@ -4 +4 @@ void func1()
   -void func2()
   +void func3()

It shows func1 in the hunk header, i.e. the next function line before the changed line. So that is at least consistent between git grep and the original -p.

The analogy breaks when it comes to how often a function line is shown: hunk headers for multiple changes in the same function show the same function line, but git grep doesn't repeat function lines. I did that on purpose, possibly because doesn't have an equivalent of hunk headers and only allows selecting and showing a subset of the lines of a file.

Except it actually does have the -- lines for separating match contexts, which are kind of similar. That looks a bit weird currently:

   $ git grep --untracked -C1 -np func2 TEST1.c
   TEST1.c=1=void func1()
   --
   TEST1.c-3-}
   TEST1.c:4:void func2()
   TEST1.c-5-{

The function line is separated from the match plus context, but you could argue that they belong together.

Show 7 quoted lines

>> So perhaps git-grep needs another change, something like
>>
>> 	if (match_funcname(opt, gs, bol, end_of_line(...)))
>> 		return;
>>
>> at the start of show_funcname_line(), but my patch does not change this
>> behaviour.

Yes, to make it match the documentation it would need something like that. (Though I'd add a match_funcname() call before the show_funcname_line() call in grep_source_1() instead, as it already has the eol value.)

Show 18 quoted lines

>>
>> --------------------------------------------------------------------------
>>
>> 	$ cat TEST2.c
>> 	void func(xxx)
>> 	{
>> 		use(xxx);
>> 	}
>>
>> 	$ git grep --untracked -pn xxx TEST2.c
>> 	TEST2.c:1:void func(xxx)
>> 	TEST2.c:3:      use(xxx)
>>
>> the 2nd match is use(xxx) and it is not a function name itself, in this
>> case git-grep should "Show the preceding line that contains the function
>> name of the match.
>>
>> But it doesn't.

The corresponding function line (line 1) is shown, as a match (with a colon). No function line is shown for the first match because it doesn't have any, being the first line of the file. So this matches the documentation at least.

Show 5 quoted lines

>> To me, this behaviour looks as
>>
>> 		Show the preceding line that contains the function name of
>> 		the match, unless the _PREVIOUS_ matching line is a function
>> 		name itself.

To me it looks like:

		Show the preceding line that contains the function name of
		the match.

("Show" meaning "show once", not "show for each match again and again".)

Or:

		Show the preceding line that contains the function name of
		the match, unless it is already shown for a different
		reason, e.g. as a match or as the function line of a
		previous match.

Show 14 quoted lines

>> Now, with my patch we have
>>
>> 	$ ./git grep --untracked -pn xxx TEST2.c
>> 	TEST2.c:1:void func(xxx)
>> 	TEST2.c=1=void func(xxx)
>> 	TEST2.c:3:      use(xxx);
>>
>> and unless I am totatlly confused this does match the documentation.
>
> So, just in case, please see V2 below. In my opinion it _fixes_ the
> current behaviour. With this patch
>
> 	$ ./git grep --untracked -pn func2 TEST1.c
> 	TEST1.c:4:void func2()

Indeed that matches the letter of the documentation.

> 	$ ./git grep --untracked -pn xxx TEST2.c
> 	TEST2.c:1:void func(xxx)
> 	TEST2.c=1=void func(xxx)
> 	TEST2.c:3:      use(xxx);

That one as well.

> Or I am totally confused?

No, I think the documentation is wrong. I'd simply remove the part after the comma, but there are probably better options.

René