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

[Oleg Nesterov <oleg@redhat.com>]

I think I should trim CC to not spam the people who are not
interested in this discussion...

On 09/12, Junio C Hamano wrote:
>
> Documentation may not match the behaviour, but do we know what the
> behaviour we want is?  To me, the last example that shows the same
> line twice (one as a real hit, the other because of "-p") looks a
> bit counter-intuitive for the purpose of "help me locate where the
> grep hits are".  I dunno.

I have another opinion. To me the 2nd "=..." marker does help to
understand the hit location. But this doesn't matter.

Let me repeat: scripts.

I tried to explain this in 0/1 and in my other replies, but lets
start from the very beginning once again.

I've never used git-grep with -p/-n and most probably never will.
But 3 days ago my text editor (vi clone) started to use "grep -pn".

	$ cat -n TEST.c

	     1	void func1(struct pid *);
	     2	
	     3	void func2(struct pid *pid)
	     4	{
	     5		use1(pid);
	     6	}
	     7	
	     8	void func3(struct pid *pid)
	     9	{
	    10		use2(pid);
	    11	}


when I do

	:git-grep --untracked -pn pid TEST.c

in my editor it calls the script which parses the output from git-grep
and puts this

	<pre>
	<a href="TEST.c?1">TEST.c                  </a>    1 <b>                        </b> void func1(struct <i>pid</i> *);
	<a href="TEST.c?3">TEST.c                  </a>    3 <b>                        </b> void func2(struct <i>pid</i> *<i>pid</i>)
	<a href="TEST.c?5">TEST.c                  </a>    5 <b>func2                   </b> use1(<i>pid</i>);
	<a href="TEST.c?8">TEST.c                  </a>    8 <b>                        </b> void func3(struct <i>pid</i> *<i>pid</i>)
	<a href="TEST.c?10">TEST.c                  </a>   10 <b>func3                   </b> use2(<i>pid</i>);
	</pre>

html to the text buffer which is nicely displayed as

	TEST.c                      1                          void func1(struct pid *);
	TEST.c                      3                          void func2(struct pid *pid)
	TEST.c                      5 func2                    use1(pid);
	TEST.c                      8                          void func3(struct pid *pid)
	TEST.c                     10 func3                    use2(pid);

and I can use Ctrl-] to jump to the file/function/location.

And this script is very simple, it parses the output line-by-line. When
it sees the "=" marker it does some minimal post-processing, records the
function name to display it in the 3rd column later, and goes to the next
line.

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.

But TEST.c is just the trivial/artificial example. From 0/1,

When I do

	:git-grep -pw pid kernel/sys.c

in my editor without this patch, I get

	kernel/sys.c              224 sys_setpriority          struct pid *pgrp;
	kernel/sys.c              294 sys_getpriority          struct pid *pgrp;
	kernel/sys.c              952                          * Note, despite the name, this returns the tgid not the pid.  The tgid and
	kernel/sys.c              953                          * the pid are identical unless CLONE_THREAD was specified on clone() in
	kernel/sys.c              963                          /* Thread ID - the internal kernel "pid" */
	kernel/sys.c              977 sys_getppid              int pid;
	kernel/sys.c              980 sys_getppid              pid = task_tgid_vnr(rcu_dereference(current->real_parent));
	kernel/sys.c              983 sys_getppid              return pid;
	kernel/sys.c             1073                          SYSCALL_DEFINE2(setpgid, pid_t, pid, pid_t, pgid)
	kernel/sys.c             1077 sys_times                struct pid *pgrp;
	kernel/sys.c             1080 sys_times                if (!pid)
	kernel/sys.c             1081 sys_times                pid = task_pid_vnr(group_leader);
	kernel/sys.c             1083 sys_times                pgid = pid;
	kernel/sys.c             1094 sys_times                p = find_task_by_vpid(pid);
	kernel/sys.c             1120 sys_times                if (pgid != pid) {
	kernel/sys.c             1144                          static int do_getpgid(pid_t pid)
	kernel/sys.c             1147 sys_times                struct pid *grp;
	kernel/sys.c             1151 sys_times                if (!pid)
	kernel/sys.c             1155 sys_times                p = find_task_by_vpid(pid);
	kernel/sys.c             1172                          SYSCALL_DEFINE1(getpgid, pid_t, pid)
	kernel/sys.c             1174 sys_times                return do_getpgid(pid);
	kernel/sys.c             1186                          SYSCALL_DEFINE1(getsid, pid_t, pid)
	kernel/sys.c             1189 sys_getpgrp              struct pid *sid;
	kernel/sys.c             1193 sys_getpgrp              if (!pid)
	kernel/sys.c             1197 sys_getpgrp              p = find_task_by_vpid(pid);
	kernel/sys.c             1214                          static void set_special_pids(struct pid *pid)
	kernel/sys.c             1218 sys_getpgrp              if (task_session(curr) != pid)
	kernel/sys.c             1219 sys_getpgrp              change_pid(curr, PIDTYPE_SID, pid);
	kernel/sys.c             1221 sys_getpgrp              if (task_pgrp(curr) != pid)
	kernel/sys.c             1222 sys_getpgrp              change_pid(curr, PIDTYPE_PGID, pid);
	kernel/sys.c             1228 ksys_setsid              struct pid *sid = task_pid(group_leader);
	kernel/sys.c             1684                          SYSCALL_DEFINE4(prlimit64, pid_t, pid, unsigned int, resource,
	kernel/sys.c             1705 check_prlimit_permission tsk = pid ? find_task_by_vpid(pid) : current;

And only the first 5 funcnames are correct.

And note that this case is very simple too (I mostly use :git-grep to scan
the whole linux kernel tree), but even in this simple case I don't think it
makes sense to use "git-grep -pn" directly, the output is hardly readable
(at least to me) with or without my patch.

Oleg.