Am 12.09.23 um 15:51 schrieb Oleg Nesterov:
> On 09/12, Oleg Nesterov wrote:
>>
>> 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!
>> 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.
>> 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.
>> 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.)
>>
>> --------------------------------------------------------------------------
>>
>> $ 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.
>> 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.
>> 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é
