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

From: Oleg Nesterov <oleg@redhat.com>
Date: Sep 17, 2023, 16:44 UTC
Message-ID: <20230917164418.GA20932@redhat.com>
In-Reply-To: <9203cd46-6a81-38e4-f191-da0b51e238c1@web.de>

René, sorry for late reply,

On 09/14, René Scharfe wrote:

Show 11 quoted lines

>
> Am 13.09.23 um 11:46 schrieb Oleg Nesterov:
> >
> > I have another opinion. To me the 2nd "=..." marker does help to
> > understand the hit location. But this doesn't matter.
>
> You see it as another layer of information, as an annotation, an
> additional line containing meta-information.  I saw them as context
> lines, i.e. lines from the original file shown in the original order
> without duplication, like - lines, with the only place for meta-
> information being the marker character itself.

Yes,

Show 21 quoted lines

> > But without my patch, in this case I get
> >
> > 	TEST.c                      1                          void func1(struct pid *);
> > 	TEST.c                      3                          void func2(struct pid *pid)
> > 	TEST.c                      5                          use1(pid);
> > 	TEST.c                      8                          void func3(struct pid *pid)
> > 	TEST.c                     10                          use2(pid);
> >
> > because the output from git-grep
> >
> > 	$ git grep --untracked -pn pid TEST.c
> > 	TEST.c:1:void func1(struct pid *);
> > 	TEST.c:3:void func2(struct pid *pid)
> > 	TEST.c:5:       use1(pid);
> > 	TEST.c:8:void func3(struct pid *pid)
> > 	TEST.c:10:      use2(pid);
> >
> > doesn't have the "=..." markers at all.
>
> Sure, that's a problem.  You could easily check whether a match is also
> a function line according to the default heuristic

Yes, but...

> there are some impressive regexes in userdiff.c
> and the script would have to figure out which language the file is
> configured to be for Git in the first place.

Yes, and this is what I'd like to avoid, I do not want to duplicate the builtin_drivers[] logic.

> > in my editor without this patch, I get
> >
> > 	kernel/sys.c              224 sys_setpriority          struct pid *pgrp;

[...snip...]

> Well, your script turns "SYSCALL_DEFINE3(setpriority, [...]" into
> "sys_setpriority" etc., so it is already knows a lot about function lines.

No, not a lot ;)

But yes sure, I can adapt this script to the current behaviour. In fact I can even change it to not use "-p", the script can read the file backwards itself (but of course I'd prefer to not do this).

Nevermind. Thanks for discussion. If I can't convince maintainers - lets forget this patch. Although I will probably keep it (and another one I had from the very begginning) for myself, it works for me.

> Can we use two markers, i.e. both : and =?  No idea what that might break.

...

> So with the patch below this would look like this:

...

> kernel/sys.c#1073#SYSCALL_DEFINE2(setpgid, pid_t, pid, pid_t, pgid)

This works for me too. So please CC me if you ever push this change ;)

Thanks,

Oleg.