From: Junio C Hamano <hidden> Date: 2023-09-12 01:41:17
Oleg Nesterov [off-list ref] writes:
$ 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.
The "better" is often subjective. The former is showing what is
going on in the TEST.c code very clearly without wasting valuable
vertical screen real estate, at least to me. If we want to adopt
the proposed behaviour, which I would recommend against, the same
patch should update the documentation, which currently says
Show the preceding line that contains the function name of the
match, unless the matching line is a function name itself.
which has allowed the users to depend on the current behaviour for
practically forever since the feature was introduced by 2944e4e6
(grep: add option -p/--show-function, 2009-07-02).
As René said, I think -p/--show-function is a rather less used
option in modern Git where "--function-context", which back in
2944e4e6 did not exist, tend to be a much more useful option, so the
fallout from such a change may be small, but it still is a backward
incompatible behaviour change that needs to be handled with care.
Thanks.
$ 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.
The "better" is often subjective.
Sure. that is why I added "to me".
The former is showing what is
going on in the TEST.c code very clearly without wasting valuable
vertical screen real estate, at least to me.
very clearly? As you probably understand this is subjective as well.
But yes, you too added "at least to me" ;)
However, certainly this is not true when you use git-grep in scripts,
please see 0/1.
If we want to adopt
the proposed behaviour, which I would recommend against, the same
patch should update the documentation, which currently says
Show the preceding line that contains the function name of the
match, unless the matching line is a function name itself.
And I still don't think this patch changes the documented behaviour.
See my reply to Rene.
Again, if you do
./git grep -pn --untracked func1 TEST.c
with this patch applied, the output is still
TEST.c=1=void func(void);
TEST.c:3:void func1(xxx)
which iiuc matches the documentation above.
Now,
./git grep -pn --untracked xxx TEST.c
adds the additional
TEST.c=3=void func1(xxx)
...
TEST.c=8=void func2(xxx)
but how does this contradict with the documentation above?
the matching lines are use1(xxx) and use2(xxx), there are NOT
"the matching line is a function name itself".
As René said, I think -p/--show-function is a rather less used
option in modern Git where "--function-context", which back in
2944e4e6 did not exist, tend to be a much more useful option,
Well, not to me. And you know, I am a git user too ;)
but it still is a backward
incompatible behaviour change that needs to be handled with care.
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?
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.
But it does. 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.
--------------------------------------------------------------------------
$ 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. 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.
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.
Oleg.
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?
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.
But it does. 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.
--------------------------------------------------------------------------
$ 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. 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.
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()
$ ./git grep --untracked -pn xxx TEST2.c
TEST2.c:1:void func(xxx)
TEST2.c=1=void func(xxx)
TEST2.c:3: use(xxx);
Or I am totally confused?
Oleg.
---
From: René Scharfe <hidden> Date: 2023-09-12 18:07:39
Am 12.09.23 um 15:51 schrieb Oleg Nesterov:
On 09/12, Oleg Nesterov wrote:
quoted
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!
quoted
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.
quoted
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.
quoted
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.)
quoted
--------------------------------------------------------------------------
$ 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.
quoted
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.
quoted
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.
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.)
Yes, I too thought about this. Except I thought that it makes sense to
pass the additional "unsigned eol" argument to show_funcname_line().
But in any case show_pre_context() will need to calculate eol.
However this is just a minor detail, I am fine either way.
quoted
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.