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

From: Oleg Nesterov <oleg@redhat.com>
Date: Sep 11, 2023, 21:54 UTC
Message-ID: <20230911215416.GA15714@redhat.com>
In-Reply-To: <e214eb9c-7576-f8f5-ef1d-3828affd47d8@web.de>

Hi René,

Thanks for feedback. I am already sleeping but let me try to reply anyway, even if I don't really understand you.

On 09/11, René Scharfe wrote:

Show 50 quoted lines

>
> Am 11.09.23 um 14:12 schrieb Oleg Nesterov:
> > show_funcname_line() returns when "lno <= opt->last_shown" and this
> > is not right in that the ->last_shown line (which matched the pattern)
> > can also have the actual function name we need to report.
> >
> > Change this code to check "lno < opt->last_shown". While at it, move
> > this check up to avoid the unnecessary "find the previous bol" loop.
> >
> > Note that --lno can't underflow, lno==0 is not possible in this loop.
> >
> > Simple test-case:
> >
> > 	$ cat TEST.c
> > 	void func(void);
> >
> > 	void func1(xxx)
> > 	{
> > 		use1(xxx);
> > 	}
> >
> > 	void func2(xxx)
> > 	{
> > 		use2(xxx);
> > 	}
> >
> > 	$ git grep --untracked -pn xxx TEST.c
> >
> > before the patch:
> >
> > 	TEST.c=1=void func(void);
> > 	TEST.c:3:void func1(xxx)
> > 	TEST.c:5:       use1(xxx);
> > 	TEST.c:8:void func2(xxx)
> > 	TEST.c:10:      use2(xxx);
> >
> > after the patch:
> >
> > 	TEST.c=1=void func(void);
> > 	TEST.c:3:void func1(xxx)
> > 	TEST.c=3=void func1(xxx)
> > 	TEST.c:5:       use1(xxx);
> > 	TEST.c:8:void func2(xxx)
> > 	TEST.c=8=void func2(xxx)
> > 	TEST.c:10:      use2(xxx);
> >
> > which looks much better to me.
>
> Interesting idea to treat function lines as annotations of matches
> instead of as special context.

Sorry, I don't understand... Let me repeat I am not familiar with this code and terminology. Could you spell please?

> Showing lines twice feels wasteful, but
> at least for -p it might be justifiable from that angle.

Just in case... say, "func1" is reported twice only when it is really needed. From the "after the patch" output above:

	TEST.c:3:void func1(xxx)

this is what we already have without this patch

	TEST.c=3=void func1(xxx)

this is what we have with this patch because the next

	TEST.c:5:       use1(xxx);

line needs the proper funcname line, and without this patch it would be "void func()" which has nothing to do with use1(xxx),

If I do, say,

	./git grep --untracked -pn func1 TEST.c

then (with or without this patch) the output is

	TEST.c=1=void func(void);
	TEST.c:3:void func1(xxx)

in this case there is no reason to report "=void func1(xxx)".

> Wouldn't you
> have to repeat function line 3 before the match in line 8, though?

Why?

> The reason for not repeating a matched function line was that it
> doesn't add much information under the assumption that it's easy to
> identify function lines visually.

But it is not. Lets look again at the "before the patch:" output above,

 	TEST.c=1=void func(void);
 	TEST.c:3:void func1(xxx)
 	TEST.c:5:       use1(xxx);
 	TEST.c:8:void func2(xxx)
 	TEST.c:10:      use2(xxx);

it looks as if every "xxx" match is inside the (unrelated) func().

OK, "visually" you can also notice the "void funcX(xxx)" declarations and understand whats going on.

But a) I don't think this is always easy, and b) it is certainly not easy when you use "git-grep -p" in scripts. Please see 0/1.

> The patch would need to update Documentation/git-grep.txt as well to
> reflect the changed output.

Hmm... From Documentation/git-grep.txt:

	-p::
	--show-function::
		Show the preceding line that contains the function name of
		the match, unless the matching line is a function name itself.
		...

this is still true after this patch. How do you think I should update this section?

Oleg.