{"thread":{"id":"60221","subject":"[PATCH 1/1] git-grep: improve the --show-function behaviour","startedAt":"2023-09-11T21:38:07Z","lastAt":"2023-09-17T16:46:55Z","messageCount":15,"participants":["Oleg Nesterov","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"481673","messageId":"20230911121211.GA17401@redhat.com","threadId":"60221","inReplyTo":"20230911121126.GA17383@redhat.com","subject":"[PATCH 1/1] git-grep: improve the --show-function behaviour","fromName":"Oleg Nesterov","fromEmail":"oleg@redhat.com","sentAt":"2023-09-11T12:12:11Z","receivedAt":"2023-09-11T21:38:07Z","isPatch":true,"sender":{"key":"oleg@redhat.com","avatar":null},"body":"show_funcname_line() returns when \"lno <= opt->last_shown\" and this\nis not right in that the ->last_shown line (which matched the pattern)\ncan also have the actual function name we need to report.\n\nChange this code to check \"lno < opt->last_shown\". While at it, move\nthis check up to avoid the unnecessary \"find the previous bol\" loop.\n\nNote that --lno can't underflow, lno==0 is not possible in this loop.\n\nSimple test-case:\n\n\t$ cat TEST.c\n\tvoid func(void);\n\n\tvoid func1(xxx)\n\t{\n\t\tuse1(xxx);\n\t}\n\n\tvoid func2(xxx)\n\t{\n\t\tuse2(xxx);\n\t}\n\n\t$ git grep --untracked -pn xxx TEST.c\n\nbefore the patch:\n\n\tTEST.c=1=void func(void);\n\tTEST.c:3:void func1(xxx)\n\tTEST.c:5: use1(xxx);\n\tTEST.c:8:void func2(xxx)\n\tTEST.c:10: use2(xxx);\n\nafter the patch:\n\n\tTEST.c=1=void func(void);\n\tTEST.c:3:void func1(xxx)\n\tTEST.c=3=void func1(xxx)\n\tTEST.c:5: use1(xxx);\n\tTEST.c:8:void func2(xxx)\n\tTEST.c=8=void func2(xxx)\n\tTEST.c:10: use2(xxx);\n\nwhich looks much better to me.\n\nSigned-off-by: Oleg Nesterov \n---\n grep.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 0904d55b24..7cad8352f4 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1350,12 +1350,11 @@ static void show_funcname_line(struct grep_opt *opt, struct grep_source *gs,\n \twhile (bol > gs->buf) {\n \t\tconst char *eol = --bol;\n \n+\t\tif (--lno < opt->last_shown)\n+\t\t\tbreak;\n+\n \t\twhile (bol > gs->buf && bol[-1] != '\\n')\n \t\t\tbol--;\n-\t\tlno--;\n-\n-\t\tif (lno <= opt->last_shown)\n-\t\t\tbreak;\n \n \t\tif (match_funcname(opt, gs, bol, eol)) {\n \t\t\tshow_line(opt, bol, eol, gs->name, lno, 0, '=');\n-- \n2.25.1.362.g51ebf55\n\n\n"},{"id":"481677","messageId":"e214eb9c-7576-f8f5-ef1d-3828affd47d8@web.de","threadId":"60221","inReplyTo":"20230911121211.GA17401@redhat.com","subject":"Re: [PATCH 1/1] git-grep: improve the --show-function behaviour","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-11T20:11:48Z","receivedAt":"2023-09-11T21:38:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.09.23 um 14:12 schrieb Oleg Nesterov:\n> show_funcname_line() returns when \"lno <= opt->last_shown\" and this\n> is not right in that the ->last_shown line (which matched the pattern)\n> can also have the actual function name we need to report.\n>\n> Change this code to check \"lno < opt->last_shown\". While at it, move\n> this check up to avoid the unnecessary \"find the previous bol\" loop.\n>\n> Note that --lno can't underflow, lno==0 is not possible in this loop.\n>\n> Simple test-case:\n>\n> \t$ cat TEST.c\n> \tvoid func(void);\n>\n> \tvoid func1(xxx)\n> \t{\n> \t\tuse1(xxx);\n> \t}\n>\n> \tvoid func2(xxx)\n> \t{\n> \t\tuse2(xxx);\n> \t}\n>\n> \t$ git grep --untracked -pn xxx TEST.c\n>\n> before the patch:\n>\n> \tTEST.c=1=void func(void);\n> \tTEST.c:3:void func1(xxx)\n> \tTEST.c:5: use1(xxx);\n> \tTEST.c:8:void func2(xxx)\n> \tTEST.c:10: use2(xxx);\n>\n> after the patch:\n>\n> \tTEST.c=1=void func(void);\n> \tTEST.c:3:void func1(xxx)\n> \tTEST.c=3=void func1(xxx)\n> \tTEST.c:5: use1(xxx);\n> \tTEST.c:8:void func2(xxx)\n> \tTEST.c=8=void func2(xxx)\n> \tTEST.c:10: use2(xxx);\n>\n> which looks much better to me.\n\nInteresting idea to treat function lines as annotations of matches\ninstead of as special context. Showing lines twice feels wasteful, but\nat least for -p it might be justifiable from that angle. Wouldn't you\nhave to repeat function line 3 before the match in line 8, though?\n\nThe reason for not repeating a matched function line was that it\ndoesn't add much information under the assumption that it's easy to\nidentify function lines visually. I assume this still holds, but\nperhaps it doesn't for more complicated languages?\n\nI have to admit that I almost never use --show-function, but instead\nuse the related --function-context a lot. So my practical experience\nwith the former option is very limited.\n\n> grep.c | 7 +++----\n> 1 file changed, 3 insertions(+), 4 deletions(-)\n\nThe patch would need to update Documentation/git-grep.txt as well to\nreflect the changed output.\n\nRené\n"},{"id":"481718","messageId":"20230911121126.GA17383@redhat.com","threadId":"60221","inReplyTo":null,"subject":"[PATCH 0/1] git-grep: improve the --show-function behaviour","fromName":"Oleg Nesterov","fromEmail":"oleg@redhat.com","sentAt":"2023-09-11T12:11:26Z","receivedAt":"2023-09-11T23:02:40Z","isPatch":true,"sender":{"key":"oleg@redhat.com","avatar":null},"body":"Hello,\n\nI have never looked into the git sources before, so I am spamming\na lot of people found in git-log grep.c\n\nYesterday I tried to teach my text editor to use \"git grep -p\" and\nI noticed that it doesn't work as I'd expect. The changelog has a\nsimple test-case, but let me also provide \"real life\" example.\n\nWhen I do\n\n\t:git-grep -pw pid kernel/sys.c\n\nin my editor before this patch, I get\n\n\tkernel/sys.c 224 sys_setpriority struct pid *pgrp;\n\tkernel/sys.c 294 sys_getpriority struct pid *pgrp;\n\tkernel/sys.c 952 * Note, despite the name, this returns the tgid not the pid. The tgid and\n\tkernel/sys.c 953 * the pid are identical unless CLONE_THREAD was specified on clone() in\n\tkernel/sys.c 963 /* Thread ID - the internal kernel \"pid\" */\n\tkernel/sys.c 977 sys_getppid int pid;\n\tkernel/sys.c 980 sys_getppid pid = task_tgid_vnr(rcu_dereference(current->real_parent));\n\tkernel/sys.c 983 sys_getppid return pid;\n\tkernel/sys.c 1073 SYSCALL_DEFINE2(setpgid, pid_t, pid, pid_t, pgid)\n\tkernel/sys.c 1077 sys_times struct pid *pgrp;\n\tkernel/sys.c 1080 sys_times if (!pid)\n\tkernel/sys.c 1081 sys_times pid = task_pid_vnr(group_leader);\n\tkernel/sys.c 1083 sys_times pgid = pid;\n\tkernel/sys.c 1094 sys_times p = find_task_by_vpid(pid);\n\tkernel/sys.c 1120 sys_times if (pgid != pid) {\n\tkernel/sys.c 1144 static int do_getpgid(pid_t pid)\n\tkernel/sys.c 1147 sys_times struct pid *grp;\n\tkernel/sys.c 1151 sys_times if (!pid)\n\tkernel/sys.c 1155 sys_times p = find_task_by_vpid(pid);\n\tkernel/sys.c 1172 SYSCALL_DEFINE1(getpgid, pid_t, pid)\n\tkernel/sys.c 1174 sys_times return do_getpgid(pid);\n\tkernel/sys.c 1186 SYSCALL_DEFINE1(getsid, pid_t, pid)\n\tkernel/sys.c 1189 sys_getpgrp struct pid *sid;\n\tkernel/sys.c 1193 sys_getpgrp if (!pid)\n\tkernel/sys.c 1197 sys_getpgrp p = find_task_by_vpid(pid);\n\tkernel/sys.c 1214 static void set_special_pids(struct pid *pid)\n\tkernel/sys.c 1218 sys_getpgrp if (task_session(curr) != pid)\n\tkernel/sys.c 1219 sys_getpgrp change_pid(curr, PIDTYPE_SID, pid);\n\tkernel/sys.c 1221 sys_getpgrp if (task_pgrp(curr) != pid)\n\tkernel/sys.c 1222 sys_getpgrp change_pid(curr, PIDTYPE_PGID, pid);\n\tkernel/sys.c 1228 ksys_setsid struct pid *sid = task_pid(group_leader);\n\tkernel/sys.c 1684 SYSCALL_DEFINE4(prlimit64, pid_t, pid, unsigned int, resource,\n\tkernel/sys.c 1705 check_prlimit_permission tsk = pid ? find_task_by_vpid(pid) : current;\n\nAnd only the first 5 funcnames are correct.\n\nAfter this patch I get\n\n\tkernel/sys.c 224 sys_setpriority struct pid *pgrp;\n\tkernel/sys.c 294 sys_getpriority struct pid *pgrp;\n\tkernel/sys.c 952 * Note, despite the name, this returns the tgid not the pid. The tgid and\n\tkernel/sys.c 953 * the pid are identical unless CLONE_THREAD was specified on clone() in\n\tkernel/sys.c 963 /* Thread ID - the internal kernel \"pid\" */\n\tkernel/sys.c 977 sys_getppid int pid;\n\tkernel/sys.c 980 sys_getppid pid = task_tgid_vnr(rcu_dereference(current->real_parent));\n\tkernel/sys.c 983 sys_getppid return pid;\n\tkernel/sys.c 1073 SYSCALL_DEFINE2(setpgid, pid_t, pid, pid_t, pgid)\n\tkernel/sys.c 1077 sys_setpgid struct pid *pgrp;\n\tkernel/sys.c 1080 sys_setpgid if (!pid)\n\tkernel/sys.c 1081 sys_setpgid pid = task_pid_vnr(group_leader);\n\tkernel/sys.c 1083 sys_setpgid pgid = pid;\n\tkernel/sys.c 1094 sys_setpgid p = find_task_by_vpid(pid);\n\tkernel/sys.c 1120 sys_setpgid if (pgid != pid) {\n\tkernel/sys.c 1144 static int do_getpgid(pid_t pid)\n\tkernel/sys.c 1147 do_getpgid struct pid *grp;\n\tkernel/sys.c 1151 do_getpgid if (!pid)\n\tkernel/sys.c 1155 do_getpgid p = find_task_by_vpid(pid);\n\tkernel/sys.c 1172 SYSCALL_DEFINE1(getpgid, pid_t, pid)\n\tkernel/sys.c 1174 sys_getpgid return do_getpgid(pid);\n\tkernel/sys.c 1186 SYSCALL_DEFINE1(getsid, pid_t, pid)\n\tkernel/sys.c 1189 sys_getsid struct pid *sid;\n\tkernel/sys.c 1193 sys_getsid if (!pid)\n\tkernel/sys.c 1197 sys_getsid p = find_task_by_vpid(pid);\n\tkernel/sys.c 1214 static void set_special_pids(struct pid *pid)\n\tkernel/sys.c 1218 set_special_pids if (task_session(curr) != pid)\n\tkernel/sys.c 1219 set_special_pids change_pid(curr, PIDTYPE_SID, pid);\n\tkernel/sys.c 1221 set_special_pids if (task_pgrp(curr) != pid)\n\tkernel/sys.c 1222 set_special_pids change_pid(curr, PIDTYPE_PGID, pid);\n\tkernel/sys.c 1228 ksys_setsid struct pid *sid = task_pid(group_leader);\n\tkernel/sys.c 1684 SYSCALL_DEFINE4(prlimit64, pid_t, pid, unsigned int, resource,\n\tkernel/sys.c 1705 sys_prlimit64 tsk = pid ? find_task_by_vpid(pid) : current;\n\nand everythig looks correct.\n\nOleg.\n\n"},{"id":"481725","messageId":"20230911215416.GA15714@redhat.com","threadId":"60221","inReplyTo":"e214eb9c-7576-f8f5-ef1d-3828affd47d8@web.de","subject":"Re: [PATCH 1/1] git-grep: improve the --show-function behaviour","fromName":"Oleg Nesterov","fromEmail":"oleg@redhat.com","sentAt":"2023-09-11T21:54:16Z","receivedAt":"2023-09-12T01:40:27Z","isPatch":true,"sender":{"key":"oleg@redhat.com","avatar":null},"body":"Hi René,\n\nThanks for feedback. I am already sleeping but let me try to reply anyway,\neven if I don't really understand you.\n\nOn 09/11, René Scharfe wrote:\n>\n> Am 11.09.23 um 14:12 schrieb Oleg Nesterov:\n> > show_funcname_line() returns when \"lno <= opt->last_shown\" and this\n> > is not right in that the ->last_shown line (which matched the pattern)\n> > can also have the actual function name we need to report.\n> >\n> > Change this code to check \"lno < opt->last_shown\". While at it, move\n> > this check up to avoid the unnecessary \"find the previous bol\" loop.\n> >\n> > Note that --lno can't underflow, lno==0 is not possible in this loop.\n> >\n> > Simple test-case:\n> >\n> > \t$ cat TEST.c\n> > \tvoid func(void);\n> >\n> > \tvoid func1(xxx)\n> > \t{\n> > \t\tuse1(xxx);\n> > \t}\n> >\n> > \tvoid func2(xxx)\n> > \t{\n> > \t\tuse2(xxx);\n> > \t}\n> >\n> > \t$ git grep --untracked -pn xxx TEST.c\n> >\n> > before the patch:\n> >\n> > \tTEST.c=1=void func(void);\n> > \tTEST.c:3:void func1(xxx)\n> > \tTEST.c:5: use1(xxx);\n> > \tTEST.c:8:void func2(xxx)\n> > \tTEST.c:10: use2(xxx);\n> >\n> > after the patch:\n> >\n> > \tTEST.c=1=void func(void);\n> > \tTEST.c:3:void func1(xxx)\n> > \tTEST.c=3=void func1(xxx)\n> > \tTEST.c:5: use1(xxx);\n> > \tTEST.c:8:void func2(xxx)\n> > \tTEST.c=8=void func2(xxx)\n> > \tTEST.c:10: use2(xxx);\n> >\n> > which looks much better to me.\n>\n> Interesting idea to treat function lines as annotations of matches\n> instead of as special context.\n\nSorry, I don't understand... Let me repeat I am not familiar with this\ncode and terminology. Could you spell please?\n\n> Showing lines twice feels wasteful, but\n> at least for -p it might be justifiable from that angle.\n\nJust in case... say, \"func1\" is reported twice only when it is really\nneeded. From the \"after the patch\" output above:\n\n\tTEST.c:3:void func1(xxx)\n\nthis is what we already have without this patch\n\n\tTEST.c=3=void func1(xxx)\n\nthis is what we have with this patch because the next\n\n\tTEST.c:5: use1(xxx);\n\nline needs the proper funcname line, and without this patch it would be\n\"void func()\" which has nothing to do with use1(xxx),\n\nIf I do, say,\n\n\t./git grep --untracked -pn func1 TEST.c\n\nthen (with or without this patch) the output is\n\n\tTEST.c=1=void func(void);\n\tTEST.c:3:void func1(xxx)\n\nin this case there is no reason to report \"=void func1(xxx)\".\n\n\n> Wouldn't you\n> have to repeat function line 3 before the match in line 8, though?\n\nWhy?\n\n> The reason for not repeating a matched function line was that it\n> doesn't add much information under the assumption that it's easy to\n> identify function lines visually.\n\nBut it is not. Lets look again at the \"before the patch:\" output above,\n\n \tTEST.c=1=void func(void);\n \tTEST.c:3:void func1(xxx)\n \tTEST.c:5: use1(xxx);\n \tTEST.c:8:void func2(xxx)\n \tTEST.c:10: use2(xxx);\n\nit looks as if every \"xxx\" match is inside the (unrelated) func().\n\nOK, \"visually\" you can also notice the \"void funcX(xxx)\" declarations\nand understand whats going on.\n\nBut a) I don't think this is always easy, and b) it is certainly not\neasy when you use \"git-grep -p\" in scripts. Please see 0/1.\n\n> The patch would need to update Documentation/git-grep.txt as well to\n> reflect the changed output.\n\nHmm... From Documentation/git-grep.txt:\n\n\t-p::\n\t--show-function::\n\t\tShow the preceding line that contains the function name of\n\t\tthe match, unless the matching line is a function name itself.\n\t\t...\n\nthis is still true after this patch. How do you think I should update this\nsection?\n\nOleg.\n\n"},{"id":"481726","messageId":"xmqq34zktk4h.fsf@gitster.g","threadId":"60221","inReplyTo":"20230911121211.GA17401@redhat.com","subject":"Re: [PATCH 1/1] git-grep: improve the --show-function behaviour","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-11T22:34:06Z","receivedAt":"2023-09-12T01:41:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oleg Nesterov writes:\n\n> \t$ git grep --untracked -pn xxx TEST.c\n>\n> before the patch:\n>\n> \tTEST.c=1=void func(void);\n> \tTEST.c:3:void func1(xxx)\n> \tTEST.c:5: use1(xxx);\n> \tTEST.c:8:void func2(xxx)\n> \tTEST.c:10: use2(xxx);\n>\n> after the patch:\n>\n> \tTEST.c=1=void func(void);\n> \tTEST.c:3:void func1(xxx)\n> \tTEST.c=3=void func1(xxx)\n> \tTEST.c:5: use1(xxx);\n> \tTEST.c:8:void func2(xxx)\n> \tTEST.c=8=void func2(xxx)\n> \tTEST.c:10: use2(xxx);\n>\n> which looks much better to me.\n\nThe \"better\" is often subjective. The former is showing what is\ngoing on in the TEST.c code very clearly without wasting valuable\nvertical screen real estate, at least to me. If we want to adopt\nthe proposed behaviour, which I would recommend against, the same\npatch should update the documentation, which currently says\n\n Show the preceding line that contains the function name of the\n match, unless the matching line is a function name itself.\n\nwhich has allowed the users to depend on the current behaviour for\npractically forever since the feature was introduced by 2944e4e6\n(grep: add option -p/--show-function, 2009-07-02).\n\nAs René said, I think -p/--show-function is a rather less used\noption in modern Git where \"--function-context\", which back in\n2944e4e6 did not exist, tend to be a much more useful option, so the\nfallout from such a change may be small, but it still is a backward\nincompatible behaviour change that needs to be handled with care.\n\nThanks.\n\n> Signed-off-by: Oleg Nesterov \n> ---\n> grep.c | 7 +++----\n> 1 file changed, 3 insertions(+), 4 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index 0904d55b24..7cad8352f4 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -1350,12 +1350,11 @@ static void show_funcname_line(struct grep_opt *opt, struct grep_source *gs,\n> \twhile (bol > gs->buf) {\n> \t\tconst char *eol = --bol;\n> \n> +\t\tif (--lno < opt->last_shown)\n> +\t\t\tbreak;\n> +\n> \t\twhile (bol > gs->buf && bol[-1] != '\\n')\n> \t\t\tbol--;\n> -\t\tlno--;\n> -\n> -\t\tif (lno <= opt->last_shown)\n> -\t\t\tbreak;\n> \n> \t\tif (match_funcname(opt, gs, bol, eol)) {\n> \t\t\tshow_line(opt, bol, eol, gs->name, lno, 0, '=');\n"},{"id":"481732","messageId":"20230911231756.GA2840@redhat.com","threadId":"60221","inReplyTo":"xmqq34zktk4h.fsf@gitster.g","subject":"Re: [PATCH 1/1] git-grep: improve the --show-function behaviour","fromName":"Oleg Nesterov","fromEmail":"oleg@redhat.com","sentAt":"2023-09-11T23:17:56Z","receivedAt":"2023-09-12T02:20:05Z","isPatch":true,"sender":{"key":"oleg@redhat.com","avatar":null},"body":"On 09/11, Junio C Hamano wrote:\n>\n> Oleg Nesterov writes:\n>\n> > \t$ git grep --untracked -pn xxx TEST.c\n> >\n> > before the patch:\n> >\n> > \tTEST.c=1=void func(void);\n> > \tTEST.c:3:void func1(xxx)\n> > \tTEST.c:5: use1(xxx);\n> > \tTEST.c:8:void func2(xxx)\n> > \tTEST.c:10: use2(xxx);\n> >\n> > after the patch:\n> >\n> > \tTEST.c=1=void func(void);\n> > \tTEST.c:3:void func1(xxx)\n> > \tTEST.c=3=void func1(xxx)\n> > \tTEST.c:5: use1(xxx);\n> > \tTEST.c:8:void func2(xxx)\n> > \tTEST.c=8=void func2(xxx)\n> > \tTEST.c:10: use2(xxx);\n> >\n> > which looks much better to me.\n>\n> The \"better\" is often subjective.\n\nSure. that is why I added \"to me\".\n\n> The former is showing what is\n> going on in the TEST.c code very clearly without wasting valuable\n> vertical screen real estate, at least to me.\n\nvery clearly? As you probably understand this is subjective as well.\nBut yes, you too added \"at least to me\" ;)\n\nHowever, certainly this is not true when you use git-grep in scripts,\nplease see 0/1.\n\n> If we want to adopt\n> the proposed behaviour, which I would recommend against, the same\n> patch should update the documentation, which currently says\n>\n> Show the preceding line that contains the function name of the\n> match, unless the matching line is a function name itself.\n\nAnd I still don't think this patch changes the documented behaviour.\nSee my reply to Rene.\n\nAgain, if you do\n\n\t./git grep -pn --untracked func1 TEST.c\n\nwith this patch applied, the output is still\n\n\tTEST.c=1=void func(void);\n\tTEST.c:3:void func1(xxx)\n\nwhich iiuc matches the documentation above.\n\nNow,\n\n\t./git grep -pn --untracked xxx TEST.c\n\nadds the additional\n\n\tTEST.c=3=void func1(xxx)\n\t...\n\tTEST.c=8=void func2(xxx)\n\nbut how does this contradict with the documentation above?\n\nthe matching lines are use1(xxx) and use2(xxx), there are NOT\n\"the matching line is a function name itself\".\n\n> As René said, I think -p/--show-function is a rather less used\n> option in modern Git where \"--function-context\", which back in\n> 2944e4e6 did not exist, tend to be a much more useful option,\n\nWell, not to me. And you know, I am a git user too ;)\n\n> but it still is a backward\n> incompatible behaviour change that needs to be handled with care.\n\nAnd this is what I still don't understand.\n\n> Thanks.\n\nThanks,\n\nOleg.\n\n"},{"id":"481760","messageId":"20230912130429.GA9982@redhat.com","threadId":"60221","inReplyTo":"20230911231756.GA2840@redhat.com","subject":"Re: [PATCH 1/1] git-grep: improve the --show-function behaviour","fromName":"Oleg Nesterov","fromEmail":"oleg@redhat.com","sentAt":"2023-09-12T13:04:29Z","receivedAt":"2023-09-12T13:08:04Z","isPatch":true,"sender":{"key":"oleg@redhat.com","avatar":null},"body":"René, Junio,\n\nI don't like the fact we can't understand each other ;) Could you\nplease explain why do you think this patch should update the docs?\n\nPlease forget about my patch for the moment. Lets start from the very\nbeginning:\n\n\t-p::\n\t--show-function::\n\t\tShow the preceding line that contains the function name of\n\t\tthe match, unless the matching line is a function name itself.\n\nand in my opinion, it is the current behaviour that doesn't match the\ndocumentation.\n\n-------------------------------------------------------------------------\n\n\t$ cat TEST1.c\n\tvoid func1()\n\t{\n\t}\n\tvoid func2()\n\t{\n\t}\n\n\t$ git grep --untracked -pn func2 TEST1.c\n\tTEST1.c=1=void func1()\n\tTEST1.c:4:void func2()\n\nin this case the matching line is \"void func2()\" and it is also a function\nname itself, in this case git-grep should not show \"=void func1()\" which is\n\"the preceding line that contains the function name of the match.\n\nBut it does. So perhaps git-grep needs another change, something like\n\n\tif (match_funcname(opt, gs, bol, end_of_line(...)))\n\t\treturn;\n\nat the start of show_funcname_line(), but my patch does not change this\nbehaviour.\n\n--------------------------------------------------------------------------\n\n\t$ cat TEST2.c\n\tvoid func(xxx)\n\t{\n\t\tuse(xxx);\n\t}\n\n\t$ git grep --untracked -pn xxx TEST2.c\n\tTEST2.c:1:void func(xxx)\n\tTEST2.c:3: use(xxx)\n\nthe 2nd match is use(xxx) and it is not a function name itself, in this\ncase git-grep should \"Show the preceding line that contains the function\nname of the match.\n\nBut it doesn't. To me, this behaviour looks as\n\n\t\tShow the preceding line that contains the function name of\n\t\tthe match, unless the _PREVIOUS_ matching line is a function\n\t\tname itself.\n\nNow, with my patch we have\n\n\t$ ./git grep --untracked -pn xxx TEST2.c\n\tTEST2.c:1:void func(xxx)\n\tTEST2.c=1=void func(xxx)\n\tTEST2.c:3: use(xxx);\n\nand unless I am totatlly confused this does match the documentation.\n\nOleg.\n\n"},{"id":"481764","messageId":"20230912135124.GA11315@redhat.com","threadId":"60221","inReplyTo":"20230912130429.GA9982@redhat.com","subject":"Re: [PATCH 1/1] git-grep: improve the --show-function behaviour","fromName":"Oleg Nesterov","fromEmail":"oleg@redhat.com","sentAt":"2023-09-12T13:51:24Z","receivedAt":"2023-09-12T13:53:34Z","isPatch":true,"sender":{"key":"oleg@redhat.com","avatar":null},"body":"On 09/12, Oleg Nesterov wrote:\n>\n> René, Junio,\n>\n> I don't like the fact we can't understand each other ;) Could you\n> please explain why do you think this patch should update the docs?\n>\n> Please forget about my patch for the moment. Lets start from the very\n> beginning:\n>\n> \t-p::\n> \t--show-function::\n> \t\tShow the preceding line that contains the function name of\n> \t\tthe match, unless the matching line is a function name itself.\n>\n> and in my opinion, it is the current behaviour that doesn't match the\n> documentation.\n>\n> -------------------------------------------------------------------------\n>\n> \t$ cat TEST1.c\n> \tvoid func1()\n> \t{\n> \t}\n> \tvoid func2()\n> \t{\n> \t}\n>\n> \t$ git grep --untracked -pn func2 TEST1.c\n> \tTEST1.c=1=void func1()\n> \tTEST1.c:4:void func2()\n>\n> in this case the matching line is \"void func2()\" and it is also a function\n> name itself, in this case git-grep should not show \"=void func1()\" which is\n> \"the preceding line that contains the function name of the match.\n>\n> But it does. So perhaps git-grep needs another change, something like\n>\n> \tif (match_funcname(opt, gs, bol, end_of_line(...)))\n> \t\treturn;\n>\n> at the start of show_funcname_line(), but my patch does not change this\n> behaviour.\n>\n> --------------------------------------------------------------------------\n>\n> \t$ cat TEST2.c\n> \tvoid func(xxx)\n> \t{\n> \t\tuse(xxx);\n> \t}\n>\n> \t$ git grep --untracked -pn xxx TEST2.c\n> \tTEST2.c:1:void func(xxx)\n> \tTEST2.c:3: use(xxx)\n>\n> the 2nd match is use(xxx) and it is not a function name itself, in this\n> case git-grep should \"Show the preceding line that contains the function\n> name of the match.\n>\n> But it doesn't. To me, this behaviour looks as\n>\n> \t\tShow the preceding line that contains the function name of\n> \t\tthe match, unless the _PREVIOUS_ matching line is a function\n> \t\tname itself.\n>\n> Now, with my patch we have\n>\n> \t$ ./git grep --untracked -pn xxx TEST2.c\n> \tTEST2.c:1:void func(xxx)\n> \tTEST2.c=1=void func(xxx)\n> \tTEST2.c:3: use(xxx);\n>\n> and unless I am totatlly confused this does match the documentation.\n\nSo, just in case, please see V2 below. In my opinion it _fixes_ the\ncurrent behaviour. With this patch\n\n\t$ ./git grep --untracked -pn func2 TEST1.c\n\tTEST1.c:4:void func2()\n\n\t$ ./git grep --untracked -pn xxx TEST2.c\n\tTEST2.c:1:void func(xxx)\n\tTEST2.c=1=void func(xxx)\n\tTEST2.c:3: use(xxx);\n\nOr I am totally confused?\n\nOleg.\n---\n\ndiff --git a/grep.c b/grep.c\nindex 0904d55b24..c240c4bfa1 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1347,15 +1347,19 @@ static int match_funcname(struct grep_opt *opt, struct grep_source *gs,\n static void show_funcname_line(struct grep_opt *opt, struct grep_source *gs,\n \t\t\t const char *bol, unsigned lno)\n {\n+\tunsigned long left = bol - gs->buf;\n+\n+\tif (match_funcname(opt, gs, bol, end_of_line(bol, &left)))\n+\t\treturn;\n+\n \twhile (bol > gs->buf) {\n \t\tconst char *eol = --bol;\n \n+\t\tif (--lno < opt->last_shown)\n+\t\t\tbreak;\n+\n \t\twhile (bol > gs->buf && bol[-1] != '\\n')\n \t\t\tbol--;\n-\t\tlno--;\n-\n-\t\tif (lno <= opt->last_shown)\n-\t\t\tbreak;\n \n \t\tif (match_funcname(opt, gs, bol, eol)) {\n \t\t\tshow_line(opt, bol, eol, gs->name, lno, 0, '=');\n\n"},{"id":"481783","messageId":"df05f761-c498-6930-bfd8-265f7e23d8ee@web.de","threadId":"60221","inReplyTo":"20230912135124.GA11315@redhat.com","subject":"Re: [PATCH 1/1] git-grep: improve the --show-function behaviour","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-12T18:07:06Z","receivedAt":"2023-09-12T18:07:39Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 12.09.23 um 15:51 schrieb Oleg Nesterov:\n> On 09/12, Oleg Nesterov wrote:\n>>\n>> René, Junio,\n>>\n>> I don't like the fact we can't understand each other ;) Could you\n>> please explain why do you think this patch should update the docs?\n\nGood thinking! And thank you for the examples and you patience!\n\n>> Please forget about my patch for the moment. Lets start from the very\n>> beginning:\n>>\n>> \t-p::\n>> \t--show-function::\n>> \t\tShow the preceding line that contains the function name of\n>> \t\tthe match, unless the matching line is a function name itself.\n>>\n>> and in my opinion, it is the current behaviour that doesn't match the\n>> documentation.>>\n>> -------------------------------------------------------------------------\n>>\n>> \t$ cat TEST1.c\n>> \tvoid func1()\n>> \t{\n>> \t}\n>> \tvoid func2()\n>> \t{\n>> \t}\n>>\n>> \t$ git grep --untracked -pn func2 TEST1.c\n>> \tTEST1.c=1=void func1()\n>> \tTEST1.c:4:void func2()\n>>\n>> in this case the matching line is \"void func2()\" and it is also a function\n>> name itself, in this case git-grep should not show \"=void func1()\" which is\n>> \"the preceding line that contains the function name of the match.\n\nMakes sense.\n\n>> But it does.\n\nDrat! ;)\n\nThe option -p came from diff(1). I thought diff change lines equivalent\nto grep match lines and hunk headers to the new = lines. What does diff\ndo with the example file?\n\n $ diff -U0 -p TEST1.c <(sed s/2/3/ TEST1.c)\n --- TEST1.c\t2023-09-12 17:59:04\n +++ /dev/fd/11\t2023-09-12 18:54:03\n @@ -4 +4 @@ void func1()\n -void func2()\n +void func3()\n\nIt shows func1 in the hunk header, i.e. the next function line before\nthe changed line. So that is at least consistent between git grep and\nthe original -p.\n\nThe analogy breaks when it comes to how often a function line is shown:\nhunk headers for multiple changes in the same function show the same\nfunction line, but git grep doesn't repeat function lines. I did that\non purpose, possibly because doesn't have an equivalent of hunk headers\nand only allows selecting and showing a subset of the lines of a file.\n\nExcept it actually does have the -- lines for separating match contexts,\nwhich are kind of similar. That looks a bit weird currently:\n\n $ git grep --untracked -C1 -np func2 TEST1.c\n TEST1.c=1=void func1()\n --\n TEST1.c-3-}\n TEST1.c:4:void func2()\n TEST1.c-5-{\n\nThe function line is separated from the match plus context, but you\ncould argue that they belong together.\n\n>> So perhaps git-grep needs another change, something like\n>>\n>> \tif (match_funcname(opt, gs, bol, end_of_line(...)))\n>> \t\treturn;\n>>\n>> at the start of show_funcname_line(), but my patch does not change this\n>> behaviour.\n\nYes, to make it match the documentation it would need something like\nthat. (Though I'd add a match_funcname() call before the\nshow_funcname_line() call in grep_source_1() instead, as it already has\nthe eol value.)\n\n>>\n>> --------------------------------------------------------------------------\n>>\n>> \t$ cat TEST2.c\n>> \tvoid func(xxx)\n>> \t{\n>> \t\tuse(xxx);\n>> \t}\n>>\n>> \t$ git grep --untracked -pn xxx TEST2.c\n>> \tTEST2.c:1:void func(xxx)\n>> \tTEST2.c:3: use(xxx)\n>>\n>> the 2nd match is use(xxx) and it is not a function name itself, in this\n>> case git-grep should \"Show the preceding line that contains the function\n>> name of the match.\n>>\n>> But it doesn't.\n\nThe corresponding function line (line 1) is shown, as a match (with a\ncolon). No function line is shown for the first match because it\ndoesn't have any, being the first line of the file. So this matches the\ndocumentation at least.\n\n>> To me, this behaviour looks as\n>>\n>> \t\tShow the preceding line that contains the function name of\n>> \t\tthe match, unless the _PREVIOUS_ matching line is a function\n>> \t\tname itself.\n\nTo me it looks like:\n\n\t\tShow the preceding line that contains the function name of\n\t\tthe match.\n\n(\"Show\" meaning \"show once\", not \"show for each match again and again\".)\n\nOr:\n\n\t\tShow the preceding line that contains the function name of\n\t\tthe match, unless it is already shown for a different\n\t\treason, e.g. as a match or as the function line of a\n\t\tprevious match.\n\n>> Now, with my patch we have\n>>\n>> \t$ ./git grep --untracked -pn xxx TEST2.c\n>> \tTEST2.c:1:void func(xxx)\n>> \tTEST2.c=1=void func(xxx)\n>> \tTEST2.c:3: use(xxx);\n>>\n>> and unless I am totatlly confused this does match the documentation.\n>\n> So, just in case, please see V2 below. In my opinion it _fixes_ the\n> current behaviour. With this patch\n>\n> \t$ ./git grep --untracked -pn func2 TEST1.c\n> \tTEST1.c:4:void func2()\n\nIndeed that matches the letter of the documentation.\n\n> \t$ ./git grep --untracked -pn xxx TEST2.c\n> \tTEST2.c:1:void func(xxx)\n> \tTEST2.c=1=void func(xxx)\n> \tTEST2.c:3: use(xxx);\n\nThat one as well.\n\n> Or I am totally confused?\n\nNo, I think the documentation is wrong. I'd simply remove the part\nafter the comma, but there are probably better options.\n\nRené\n"},{"id":"481796","messageId":"xmqq1qf2lxrm.fsf@gitster.g","threadId":"60221","inReplyTo":"df05f761-c498-6930-bfd8-265f7e23d8ee@web.de","subject":"Re: [PATCH 1/1] git-grep: improve the --show-function behaviour","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-13T00:31:09Z","receivedAt":"2023-09-13T00:31:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>>> To me, this behaviour looks as\n>>>\n>>> \t\tShow the preceding line that contains the function name of\n>>> \t\tthe match, unless the _PREVIOUS_ matching line is a function\n>>> \t\tname itself.\n>\n> To me it looks like:\n>\n> \t\tShow the preceding line that contains the function name of\n> \t\tthe match.\n>\n> (\"Show\" meaning \"show once\", not \"show for each match again and again\".)\n>\n> Or:\n>\n> \t\tShow the preceding line that contains the function name of\n> \t\tthe match, unless it is already shown for a different\n> \t\treason, e.g. as a match or as the function line of a\n> \t\tprevious match.\n\nWow, that was a mouthful, but matches my understanding. I naïvely\nthought \"when showing a hit, we may add the line that matches the\nfunction header pattern before the hit even that header line does\nnot hit the grep pattern. But if the header line does hit the grep\npattern, we do not bother show the same thing twice.\" was a\nreasonable goal to have.\n\n> Indeed that matches the letter of the documentation.\n>\n>> \t$ ./git grep --untracked -pn xxx TEST2.c\n>> \tTEST2.c:1:void func(xxx)\n>> \tTEST2.c=1=void func(xxx)\n>> \tTEST2.c:3: use(xxx);\n>\n> That one as well.\n>\n>> Or I am totally confused?\n>\n> No, I think the documentation is wrong. I'd simply remove the part\n> after the comma, but there are probably better options.\n\nDocumentation may not match the behaviour, but do we know what the\nbehaviour we want is? To me, the last example that shows the same\nline twice (one as a real hit, the other because of \"-p\") looks a\nbit counter-intuitive for the purpose of \"help me locate where the\ngrep hits are\". I dunno.\n"},{"id":"481802","messageId":"20230913094638.GA535@redhat.com","threadId":"60221","inReplyTo":"xmqq1qf2lxrm.fsf@gitster.g","subject":"Re: [PATCH 1/1] git-grep: improve the --show-function behaviour","fromName":"Oleg Nesterov","fromEmail":"oleg@redhat.com","sentAt":"2023-09-13T09:46:39Z","receivedAt":"2023-09-13T09:48:23Z","isPatch":true,"sender":{"key":"oleg@redhat.com","avatar":null},"body":"I think I should trim CC to not spam the people who are not\ninterested in this discussion...\n\nOn 09/12, Junio C Hamano wrote:\n>\n> Documentation may not match the behaviour, but do we know what the\n> behaviour we want is? To me, the last example that shows the same\n> line twice (one as a real hit, the other because of \"-p\") looks a\n> bit counter-intuitive for the purpose of \"help me locate where the\n> grep hits are\". I dunno.\n\nI have another opinion. To me the 2nd \"=...\" marker does help to\nunderstand the hit location. But this doesn't matter.\n\nLet me repeat: scripts.\n\nI tried to explain this in 0/1 and in my other replies, but lets\nstart from the very beginning once again.\n\nI've never used git-grep with -p/-n and most probably never will.\nBut 3 days ago my text editor (vi clone) started to use \"grep -pn\".\n\n\t$ cat -n TEST.c\n\n\t 1\tvoid func1(struct pid *);\n\t 2\t\n\t 3\tvoid func2(struct pid *pid)\n\t 4\t{\n\t 5\t\tuse1(pid);\n\t 6\t}\n\t 7\t\n\t 8\tvoid func3(struct pid *pid)\n\t 9\t{\n\t 10\t\tuse2(pid);\n\t 11\t}\n\n\nwhen I do\n\n\t:git-grep --untracked -pn pid TEST.c\n\nin my editor it calls the script which parses the output from git-grep\nand puts this\n\n\t
\n\tTEST.c                      1                          void func1(struct pid *);\n\tTEST.c                      3                          void func2(struct pid *pid)\n\tTEST.c                      5 func2                    use1(pid);\n\tTEST.c                      8                          void func3(struct pid *pid)\n\tTEST.c                     10 func3                    use2(pid);\n\t
\n\nhtml to the text buffer which is nicely displayed as\n\n\tTEST.c 1 void func1(struct pid *);\n\tTEST.c 3 void func2(struct pid *pid)\n\tTEST.c 5 func2 use1(pid);\n\tTEST.c 8 void func3(struct pid *pid)\n\tTEST.c 10 func3 use2(pid);\n\nand I can use Ctrl-] to jump to the file/function/location.\n\nAnd this script is very simple, it parses the output line-by-line. When\nit sees the \"=\" marker it does some minimal post-processing, records the\nfunction name to display it in the 3rd column later, and goes to the next\nline.\n\nBut without my patch, in this case I get\n\n\tTEST.c 1 void func1(struct pid *);\n\tTEST.c 3 void func2(struct pid *pid)\n\tTEST.c 5 use1(pid);\n\tTEST.c 8 void func3(struct pid *pid)\n\tTEST.c 10 use2(pid);\n\nbecause the output from git-grep\n\n\t$ git grep --untracked -pn pid TEST.c\n\tTEST.c:1:void func1(struct pid *);\n\tTEST.c:3:void func2(struct pid *pid)\n\tTEST.c:5: use1(pid);\n\tTEST.c:8:void func3(struct pid *pid)\n\tTEST.c:10: use2(pid);\n\ndoesn't have the \"=...\" markers at all.\n\nBut TEST.c is just the trivial/artificial example. From 0/1,\n\nWhen I do\n\n\t:git-grep -pw pid kernel/sys.c\n\nin my editor without this patch, I get\n\n\tkernel/sys.c 224 sys_setpriority struct pid *pgrp;\n\tkernel/sys.c 294 sys_getpriority struct pid *pgrp;\n\tkernel/sys.c 952 * Note, despite the name, this returns the tgid not the pid. The tgid and\n\tkernel/sys.c 953 * the pid are identical unless CLONE_THREAD was specified on clone() in\n\tkernel/sys.c 963 /* Thread ID - the internal kernel \"pid\" */\n\tkernel/sys.c 977 sys_getppid int pid;\n\tkernel/sys.c 980 sys_getppid pid = task_tgid_vnr(rcu_dereference(current->real_parent));\n\tkernel/sys.c 983 sys_getppid return pid;\n\tkernel/sys.c 1073 SYSCALL_DEFINE2(setpgid, pid_t, pid, pid_t, pgid)\n\tkernel/sys.c 1077 sys_times struct pid *pgrp;\n\tkernel/sys.c 1080 sys_times if (!pid)\n\tkernel/sys.c 1081 sys_times pid = task_pid_vnr(group_leader);\n\tkernel/sys.c 1083 sys_times pgid = pid;\n\tkernel/sys.c 1094 sys_times p = find_task_by_vpid(pid);\n\tkernel/sys.c 1120 sys_times if (pgid != pid) {\n\tkernel/sys.c 1144 static int do_getpgid(pid_t pid)\n\tkernel/sys.c 1147 sys_times struct pid *grp;\n\tkernel/sys.c 1151 sys_times if (!pid)\n\tkernel/sys.c 1155 sys_times p = find_task_by_vpid(pid);\n\tkernel/sys.c 1172 SYSCALL_DEFINE1(getpgid, pid_t, pid)\n\tkernel/sys.c 1174 sys_times return do_getpgid(pid);\n\tkernel/sys.c 1186 SYSCALL_DEFINE1(getsid, pid_t, pid)\n\tkernel/sys.c 1189 sys_getpgrp struct pid *sid;\n\tkernel/sys.c 1193 sys_getpgrp if (!pid)\n\tkernel/sys.c 1197 sys_getpgrp p = find_task_by_vpid(pid);\n\tkernel/sys.c 1214 static void set_special_pids(struct pid *pid)\n\tkernel/sys.c 1218 sys_getpgrp if (task_session(curr) != pid)\n\tkernel/sys.c 1219 sys_getpgrp change_pid(curr, PIDTYPE_SID, pid);\n\tkernel/sys.c 1221 sys_getpgrp if (task_pgrp(curr) != pid)\n\tkernel/sys.c 1222 sys_getpgrp change_pid(curr, PIDTYPE_PGID, pid);\n\tkernel/sys.c 1228 ksys_setsid struct pid *sid = task_pid(group_leader);\n\tkernel/sys.c 1684 SYSCALL_DEFINE4(prlimit64, pid_t, pid, unsigned int, resource,\n\tkernel/sys.c 1705 check_prlimit_permission tsk = pid ? find_task_by_vpid(pid) : current;\n\nAnd only the first 5 funcnames are correct.\n\nAnd note that this case is very simple too (I mostly use :git-grep to scan\nthe whole linux kernel tree), but even in this simple case I don't think it\nmakes sense to use \"git-grep -pn\" directly, the output is hardly readable\n(at least to me) with or without my patch.\n\nOleg.\n\n"},{"id":"481803","messageId":"20230913101501.GC535@redhat.com","threadId":"60221","inReplyTo":"df05f761-c498-6930-bfd8-265f7e23d8ee@web.de","subject":"Re: [PATCH 1/1] git-grep: improve the --show-function behaviour","fromName":"Oleg Nesterov","fromEmail":"oleg@redhat.com","sentAt":"2023-09-13T10:15:01Z","receivedAt":"2023-09-13T10:16:49Z","isPatch":true,"sender":{"key":"oleg@redhat.com","avatar":null},"body":"On 09/12, René Scharfe wrote:\n>\n> >> So perhaps git-grep needs another change, something like\n> >>\n> >> \tif (match_funcname(opt, gs, bol, end_of_line(...)))\n> >> \t\treturn;\n> >>\n> >> at the start of show_funcname_line(), but my patch does not change this\n> >> behaviour.\n>\n> Yes, to make it match the documentation it would need something like\n> that. (Though I'd add a match_funcname() call before the\n> show_funcname_line() call in grep_source_1() instead, as it already has\n> the eol value.)\n\nYes, I too thought about this. Except I thought that it makes sense to\npass the additional \"unsigned eol\" argument to show_funcname_line().\nBut in any case show_pre_context() will need to calculate eol.\n\nHowever this is just a minor detail, I am fine either way.\n\n> > So, just in case, please see V2 below. In my opinion it _fixes_ the\n> > current behaviour. With this patch\n> >\n> > \t$ ./git grep --untracked -pn func2 TEST1.c\n> > \tTEST1.c:4:void func2()\n>\n> Indeed that matches the letter of the documentation.\n>\n> > \t$ ./git grep --untracked -pn xxx TEST2.c\n> > \tTEST2.c:1:void func(xxx)\n> > \tTEST2.c=1=void func(xxx)\n> > \tTEST2.c:3: use(xxx);\n>\n> That one as well.\n\nSo. Can I assume you agree with my patch ? ;)\n\n> No, I think the documentation is wrong.\n\nWell, to me it looks good, but only after this patch.\n\nOleg.\n\n"},{"id":"481848","messageId":"29ea79e7-87fd-fa27-e9df-d626e27c7c74@web.de","threadId":"60221","inReplyTo":"xmqq1qf2lxrm.fsf@gitster.g","subject":"Re: [PATCH 1/1] git-grep: improve the --show-function behaviour","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-14T19:34:34Z","receivedAt":"2023-09-14T19:34:57Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"\n\nAm 13.09.23 um 02:31 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>>>> To me, this behaviour looks as\n>>>>\n>>>> \t\tShow the preceding line that contains the function name of\n>>>> \t\tthe match, unless the _PREVIOUS_ matching line is a function\n>>>> \t\tname itself.\n>>\n>> To me it looks like:\n>>\n>> \t\tShow the preceding line that contains the function name of\n>> \t\tthe match.\n>>\n>> (\"Show\" meaning \"show once\", not \"show for each match again and again\".)\n>>\n>> Or:\n>>\n>> \t\tShow the preceding line that contains the function name of\n>> \t\tthe match, unless it is already shown for a different\n>> \t\treason, e.g. as a match or as the function line of a\n>> \t\tprevious match.\n>\n> Wow, that was a mouthful, but matches my understanding. I naïvely\n> thought \"when showing a hit, we may add the line that matches the\n> function header pattern before the hit even that header line does\n> not hit the grep pattern. But if the header line does hit the grep\n> pattern, we do not bother show the same thing twice.\" was a\n> reasonable goal to have.\n\nI agree, and that's probably why I included the \"unless the matching\nline is a function name itself\" part. Not sure why the code doesn't\nagree. A test for that aspect would have been nice. *ahem*\n\nRené\n"},{"id":"481849","messageId":"9203cd46-6a81-38e4-f191-da0b51e238c1@web.de","threadId":"60221","inReplyTo":"20230913094638.GA535@redhat.com","subject":"Re: [PATCH 1/1] git-grep: improve the --show-function behaviour","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-14T19:34:48Z","receivedAt":"2023-09-14T19:35:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.09.23 um 11:46 schrieb Oleg Nesterov:\n> I think I should trim CC to not spam the people who are not\n> interested in this discussion...\n>\n> On 09/12, Junio C Hamano wrote:\n>>\n>> Documentation may not match the behaviour, but do we know what the\n>> behaviour we want is? To me, the last example that shows the same\n>> line twice (one as a real hit, the other because of \"-p\") looks a\n>> bit counter-intuitive for the purpose of \"help me locate where the\n>> grep hits are\". I dunno.\n>\n> I have another opinion. To me the 2nd \"=...\" marker does help to\n> understand the hit location. But this doesn't matter.\n\nYou see it as another layer of information, as an annotation, an\nadditional line containing meta-information. I saw them as context\nlines, i.e. lines from the original file shown in the original order\nwithout duplication, like - lines, with the only place for meta-\ninformation being the marker character itself.\n\n> Let me repeat: scripts.\n>\n> I tried to explain this in 0/1 and in my other replies, but lets\n> start from the very beginning once again.\n>\n> I've never used git-grep with -p/-n and most probably never will.\n> But 3 days ago my text editor (vi clone) started to use \"grep -pn\".\n>\n> \t$ cat -n TEST.c\n>\n> \t 1\tvoid func1(struct pid *);\n> \t 2\n> \t 3\tvoid func2(struct pid *pid)\n> \t 4\t{\n> \t 5\t\tuse1(pid);\n> \t 6\t}\n> \t 7\n> \t 8\tvoid func3(struct pid *pid)\n> \t 9\t{\n> \t 10\t\tuse2(pid);\n> \t 11\t}\n>\n>\n> when I do\n>\n> \t:git-grep --untracked -pn pid TEST.c\n>\n> in my editor it calls the script which parses the output from git-grep\n> and puts this\n>\n> \t
\n> \tTEST.c                      1                          void func1(struct pid *);\n> \tTEST.c                      3                          void func2(struct pid *pid)\n> \tTEST.c                      5 func2                    use1(pid);\n> \tTEST.c                      8                          void func3(struct pid *pid)\n> \tTEST.c                     10 func3                    use2(pid);\n> \t
\n>\n> html to the text buffer which is nicely displayed as\n>\n> \tTEST.c 1 void func1(struct pid *);\n> \tTEST.c 3 void func2(struct pid *pid)\n> \tTEST.c 5 func2 use1(pid);\n> \tTEST.c 8 void func3(struct pid *pid)\n> \tTEST.c 10 func3 use2(pid);\n>\n> and I can use Ctrl-] to jump to the file/function/location.\n>\n> And this script is very simple, it parses the output line-by-line. When\n> it sees the \"=\" marker it does some minimal post-processing, records the\n> function name to display it in the 3rd column later, and goes to the next\n> line.\n>\n> But without my patch, in this case I get\n>\n> \tTEST.c 1 void func1(struct pid *);\n> \tTEST.c 3 void func2(struct pid *pid)\n> \tTEST.c 5 use1(pid);\n> \tTEST.c 8 void func3(struct pid *pid)\n> \tTEST.c 10 use2(pid);\n>\n> because the output from git-grep\n>\n> \t$ git grep --untracked -pn pid TEST.c\n> \tTEST.c:1:void func1(struct pid *);\n> \tTEST.c:3:void func2(struct pid *pid)\n> \tTEST.c:5: use1(pid);\n> \tTEST.c:8:void func3(struct pid *pid)\n> \tTEST.c:10: use2(pid);\n>\n> doesn't have the \"=...\" markers at all.\n\nSure, that's a problem. You could easily check whether a match is also\na function line according to the default heuristic (does the line start\nwith a letter, dollar sign or underscore?), e.g. with a glob like\n\"*:*:[a-zA-Z$_]*\". If git grep uses more sophisticated rules then it\nbecomes impractical -- there are some impressive regexes in userdiff.c\nand the script would have to figure out which language the file is\nconfigured to be for Git in the first place.\n\n> But TEST.c is just the trivial/artificial example. From 0/1,\n>\n> When I do\n>\n> \t:git-grep -pw pid kernel/sys.c\n>\n> in my editor without this patch, I get\n>\n> \tkernel/sys.c 224 sys_setpriority struct pid *pgrp;\n> \tkernel/sys.c 294 sys_getpriority struct pid *pgrp;\n> \tkernel/sys.c 952 * Note, despite the name, this returns the tgid not the pid. The tgid and\n> \tkernel/sys.c 953 * the pid are identical unless CLONE_THREAD was specified on clone() in\n> \tkernel/sys.c 963 /* Thread ID - the internal kernel \"pid\" */\n> \tkernel/sys.c 977 sys_getppid int pid;\n> \tkernel/sys.c 980 sys_getppid pid = task_tgid_vnr(rcu_dereference(current->real_parent));\n> \tkernel/sys.c 983 sys_getppid return pid;\n> \tkernel/sys.c 1073 SYSCALL_DEFINE2(setpgid, pid_t, pid, pid_t, pgid)\n> \tkernel/sys.c 1077 sys_times struct pid *pgrp;\n> \tkernel/sys.c 1080 sys_times if (!pid)\n> \tkernel/sys.c 1081 sys_times pid = task_pid_vnr(group_leader);\n> \tkernel/sys.c 1083 sys_times pgid = pid;\n> \tkernel/sys.c 1094 sys_times p = find_task_by_vpid(pid);\n> \tkernel/sys.c 1120 sys_times if (pgid != pid) {\n> \tkernel/sys.c 1144 static int do_getpgid(pid_t pid)\n> \tkernel/sys.c 1147 sys_times struct pid *grp;\n> \tkernel/sys.c 1151 sys_times if (!pid)\n> \tkernel/sys.c 1155 sys_times p = find_task_by_vpid(pid);\n> \tkernel/sys.c 1172 SYSCALL_DEFINE1(getpgid, pid_t, pid)\n> \tkernel/sys.c 1174 sys_times return do_getpgid(pid);\n> \tkernel/sys.c 1186 SYSCALL_DEFINE1(getsid, pid_t, pid)\n> \tkernel/sys.c 1189 sys_getpgrp struct pid *sid;\n> \tkernel/sys.c 1193 sys_getpgrp if (!pid)\n> \tkernel/sys.c 1197 sys_getpgrp p = find_task_by_vpid(pid);\n> \tkernel/sys.c 1214 static void set_special_pids(struct pid *pid)\n> \tkernel/sys.c 1218 sys_getpgrp if (task_session(curr) != pid)\n> \tkernel/sys.c 1219 sys_getpgrp change_pid(curr, PIDTYPE_SID, pid);\n> \tkernel/sys.c 1221 sys_getpgrp if (task_pgrp(curr) != pid)\n> \tkernel/sys.c 1222 sys_getpgrp change_pid(curr, PIDTYPE_PGID, pid);\n> \tkernel/sys.c 1228 ksys_setsid struct pid *sid = task_pid(group_leader);\n> \tkernel/sys.c 1684 SYSCALL_DEFINE4(prlimit64, pid_t, pid, unsigned int, resource,\n> \tkernel/sys.c 1705 check_prlimit_permission tsk = pid ? find_task_by_vpid(pid) : current;\n>\n> And only the first 5 funcnames are correct.\n\nWell, your script turns \"SYSCALL_DEFINE3(setpriority, [...]\" into\n\"sys_setpriority\" etc., so it is already knows a lot about function lines.\nIt could be made to take a second look at match lines, I guess.\n\nBut a general solution would require git grep to somehow report both aspects\nof matching function lines. That's easy if we allow ourselves to duplicate\nlines. This is strange enough to warrant making it a new output format I\nthink.\n\nAnother possibility is to switch the precedence of : and =. With match\ncoloring it would still be possible to identify most positive matches in\nfunction interactively, but not negative matches (-v) in function lines.\nProbably not the best choice, since grep is primarily about finding\nmatching lines; function line info comes second.\n\nCan we use two markers, i.e. both : and =? No idea what that might break.\n\nThere is a Unicode symbol named colon equals, which looks like this: ≔\nWe added the =, so I guess we could add that thing as well. But is the\nworld prepared for Unicode output? Not sure. If we need to stay in the\nASCII table the same idea could be implemented with a different character\nlike # or ;.\n\n> And note that this case is very simple too (I mostly use :git-grep to scan\n> the whole linux kernel tree), but even in this simple case I don't think it\n> makes sense to use \"git-grep -pn\" directly, the output is hardly readable\n> (at least to me) with or without my patch.\n\nSo with the patch below this would look like this:\n\nkernel/sys.c=218=SYSCALL_DEFINE3(setpriority, int, which, int, who, int, niceval)\nkernel/sys.c:224: struct pid *pgrp;\nkernel/sys.c=288=SYSCALL_DEFINE2(getpriority, int, which, int, who)\nkernel/sys.c:294: struct pid *pgrp;\nkernel/sys.c=943=SYSCALL_DEFINE1(setfsgid, gid_t, gid)\nkernel/sys.c:952: * Note, despite the name, this returns the tgid not the pid. The tgid and\nkernel/sys.c:953: * the pid are identical unless CLONE_THREAD was specified on clone() in\nkernel/sys.c=958=SYSCALL_DEFINE0(getpid)\nkernel/sys.c:963:/* Thread ID - the internal kernel \"pid\" */\nkernel/sys.c=975=SYSCALL_DEFINE0(getppid)\nkernel/sys.c:977: int pid;\nkernel/sys.c:980: pid = task_tgid_vnr(rcu_dereference(current->real_parent));\nkernel/sys.c:983: return pid;\nkernel/sys.c#1073#SYSCALL_DEFINE2(setpgid, pid_t, pid, pid_t, pgid)\nkernel/sys.c:1077: struct pid *pgrp;\nkernel/sys.c:1080: if (!pid)\nkernel/sys.c:1081: pid = task_pid_vnr(group_leader);\nkernel/sys.c:1083: pgid = pid;\nkernel/sys.c:1094: p = find_task_by_vpid(pid);\nkernel/sys.c:1120: if (pgid != pid) {\nkernel/sys.c#1144#static int do_getpgid(pid_t pid)\nkernel/sys.c:1147: struct pid *grp;\nkernel/sys.c:1151: if (!pid)\nkernel/sys.c:1155: p = find_task_by_vpid(pid);\nkernel/sys.c#1172#SYSCALL_DEFINE1(getpgid, pid_t, pid)\nkernel/sys.c:1174: return do_getpgid(pid);\nkernel/sys.c#1186#SYSCALL_DEFINE1(getsid, pid_t, pid)\nkernel/sys.c:1189: struct pid *sid;\nkernel/sys.c:1193: if (!pid)\nkernel/sys.c:1197: p = find_task_by_vpid(pid);\nkernel/sys.c#1214#static void set_special_pids(struct pid *pid)\nkernel/sys.c:1218: if (task_session(curr) != pid)\nkernel/sys.c:1219: change_pid(curr, PIDTYPE_SID, pid);\nkernel/sys.c:1221: if (task_pgrp(curr) != pid)\nkernel/sys.c:1222: change_pid(curr, PIDTYPE_PGID, pid);\nkernel/sys.c=1225=int ksys_setsid(void)\nkernel/sys.c:1228: struct pid *sid = task_pid(group_leader);\nkernel/sys.c#1684#SYSCALL_DEFINE4(prlimit64, pid_t, pid, unsigned int, resource,\nkernel/sys.c:1705: tsk = pid ? find_task_by_vpid(pid) : current;\n\nIt uses # for matches that happen to be function lines, and doesn't show\na previous function line for those anymore. Usable?\n\nRené\n\n\ndiff --git a/grep.c b/grep.c\nindex fc2d0c837a..a08da5cdcb 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1681,6 +1681,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \t\t\tgoto next_line;\n \t\t}\n \t\tif (hit && (opt->max_count < 0 || count < opt->max_count)) {\n+\t\t\tchar sign = ':';\n \t\t\tcount++;\n \t\t\tif (opt->status_only)\n \t\t\t\treturn 1;\n@@ -1697,12 +1698,14 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \t\t\t\topt->output(opt, \" matches\\n\", 9);\n \t\t\t\treturn 1;\n \t\t\t}\n+\t\t\tif (opt->funcname && match_funcname(opt, gs, bol, eol))\n+\t\t\t\tsign = '#';\n \t\t\t/* Hit at this line. If we haven't shown the\n \t\t\t * pre-context lines, we would need to show them.\n \t\t\t */\n \t\t\tif (opt->pre_context || opt->funcbody)\n \t\t\t\tshow_pre_context(opt, gs, bol, eol, lno);\n-\t\t\telse if (opt->funcname)\n+\t\t\telse if (opt->funcname && sign == ':')\n \t\t\t\tshow_funcname_line(opt, gs, bol, lno);\n \t\t\tcno = opt->invert ? icol : col;\n \t\t\tif (cno < 0) {\n@@ -1715,7 +1718,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \t\t\t\t */\n \t\t\t\tcno = 0;\n \t\t\t}\n-\t\t\tshow_line(opt, bol, eol, gs->name, lno, cno + 1, ':');\n+\t\t\tshow_line(opt, bol, eol, gs->name, lno, cno + 1, sign);\n \t\t\tlast_hit = lno;\n \t\t\tif (opt->funcbody)\n \t\t\t\tshow_function = 1;\n"},{"id":"481941","messageId":"20230917164418.GA20932@redhat.com","threadId":"60221","inReplyTo":"9203cd46-6a81-38e4-f191-da0b51e238c1@web.de","subject":"Re: [PATCH 1/1] git-grep: improve the --show-function behaviour","fromName":"Oleg Nesterov","fromEmail":"oleg@redhat.com","sentAt":"2023-09-17T16:44:18Z","receivedAt":"2023-09-17T16:46:55Z","isPatch":true,"sender":{"key":"oleg@redhat.com","avatar":null},"body":"René, sorry for late reply,\n\nOn 09/14, René Scharfe wrote:\n>\n> Am 13.09.23 um 11:46 schrieb Oleg Nesterov:\n> >\n> > I have another opinion. To me the 2nd \"=...\" marker does help to\n> > understand the hit location. But this doesn't matter.\n>\n> You see it as another layer of information, as an annotation, an\n> additional line containing meta-information. I saw them as context\n> lines, i.e. lines from the original file shown in the original order\n> without duplication, like - lines, with the only place for meta-\n> information being the marker character itself.\n\nYes,\n\n> > But without my patch, in this case I get\n> >\n> > \tTEST.c 1 void func1(struct pid *);\n> > \tTEST.c 3 void func2(struct pid *pid)\n> > \tTEST.c 5 use1(pid);\n> > \tTEST.c 8 void func3(struct pid *pid)\n> > \tTEST.c 10 use2(pid);\n> >\n> > because the output from git-grep\n> >\n> > \t$ git grep --untracked -pn pid TEST.c\n> > \tTEST.c:1:void func1(struct pid *);\n> > \tTEST.c:3:void func2(struct pid *pid)\n> > \tTEST.c:5: use1(pid);\n> > \tTEST.c:8:void func3(struct pid *pid)\n> > \tTEST.c:10: use2(pid);\n> >\n> > doesn't have the \"=...\" markers at all.\n>\n> Sure, that's a problem. You could easily check whether a match is also\n> a function line according to the default heuristic\n\nYes, but...\n\n> there are some impressive regexes in userdiff.c\n> and the script would have to figure out which language the file is\n> configured to be for Git in the first place.\n\nYes, and this is what I'd like to avoid, I do not want to duplicate the\nbuiltin_drivers[] logic.\n\n> > in my editor without this patch, I get\n> >\n> > \tkernel/sys.c 224 sys_setpriority struct pid *pgrp;\n\n[...snip...]\n\n> Well, your script turns \"SYSCALL_DEFINE3(setpriority, [...]\" into\n> \"sys_setpriority\" etc., so it is already knows a lot about function lines.\n\nNo, not a lot ;)\n\nBut yes sure, I can adapt this script to the current behaviour. In fact I\ncan even change it to not use \"-p\", the script can read the file backwards\nitself (but of course I'd prefer to not do this).\n\nNevermind. Thanks for discussion. If I can't convince maintainers - lets\nforget this patch. Although I will probably keep it (and another one I\nhad from the very begginning) for myself, it works for me.\n\n> Can we use two markers, i.e. both : and =? No idea what that might break.\n\n...\n\n> So with the patch below this would look like this:\n\n...\n\n> kernel/sys.c#1073#SYSCALL_DEFINE2(setpgid, pid_t, pid, pid_t, pgid)\n\nThis works for me too. So please CC me if you ever push this change ;)\n\nThanks,\n\nOleg.\n\n"}]}