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

Am 12.09.23 um 15:51 schrieb Oleg Nesterov:

On 09/12, Oleg Nesterov wrote:

quoted

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!

quoted

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.

quoted

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.

quoted

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.)

quoted

--------------------------------------------------------------------------

	$ 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.

quoted

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.

quoted

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é