--function-context shows wrong function in chunk

--function-context shows wrong function in chunk

[Zach Riggle]

When using --function-context, the function that is claimed at the top
of the diff does not match the actual function.

Note that the change exists in main, but the hunk header
(terminology?) shows other_routine.

$ git --version
git version 2.27.0

$ git diff -b --function-context
diff --git i/example.c w/example.c
index d87b59b..346e2a7 100644
--- i/example.c
+++ w/example.c
@@ -4,5 +4,5 @@ int other_routine() {
 }

 int main() {
-
+    puts("Hello, world!");
 }

Zach Riggle

Re: --function-context shows wrong function in chunk

[Junio C Hamano]

Zach Riggle <zachriggle@gmail.com> writes:

> When using --function-context, the function that is claimed at the top
> of the diff does not match the actual function.
>
> Note that the change exists in main, but the hunk header
> (terminology?) shows other_routine.
>
> $ git --version
> git version 2.27.0
>
> $ git diff -b --function-context
> diff --git i/example.c w/example.c
> index d87b59b..346e2a7 100644
> --- i/example.c
> +++ w/example.c
> @@ -4,5 +4,5 @@ int other_routine() {
>  }
>
>  int main() {
> -
> +    puts("Hello, world!");
>  }
>
> Zach Riggle

I think it is possible to modify the "find the line that match
xfuncname pattern" logic to start scanning backwards from the first
actual change (i.e. the blank line in the preimage of the patch
inside "int main() {" function in your example) and make the hunk
header say "int main() {" instead of "int other_routine() {".

I however doubt that such a change makes any sense.  In fact, I find
the sample output above both quite logical and also even desirable.

It is logical because the first thing we see in the hunk, "}", is at
the end of "int other_routine() {" function; it does not conclude
the "int main() {" function.  Saying "int main() {" there on the
hunk header line would be misleading and confusing.  It sends a
wrong signal that there was such a line _before_ the first line we
see in this hunk, which is definitely not.

It is desirable because it gives more information than saying the
illogical "main".  The reader can see what the routine before the
beginning of main function we see in the hunk is.  In the above
example it may not matter, but if we see "return -1;" at the end of
that function and a call to other_routine() from main(), e.g.

        @@ ... @@ int other_routine() {
            return -1;
        }
        int main() {
        -   int i = other_routine();
        +   int i = other_routine() ? 1 : 0;
            printf("%d\n", i);
        }

it would be more informative than having "int main() {" there.

Re: --function-context shows wrong function in chunk

[Zach Riggle]

This happens even with -U0 which does not include the e.g. trailing
bit of other_routine.

$ git diff -U0 -b --function-context
diff --git i/example.c w/example.c
index d87b59b..346e2a7 100644
--- i/example.c
+++ w/example.c
@@ -6,3 +6,3 @@ int other_routine() {
 int main() {
-
+    puts("Hello, world!");
 }

Zach Riggle

On Fri, Jul 10, 2020 at 3:12 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Zach Riggle <zachriggle@gmail.com> writes:
>
> > When using --function-context, the function that is claimed at the top
> > of the diff does not match the actual function.
> >
> > Note that the change exists in main, but the hunk header
> > (terminology?) shows other_routine.
> >
> > $ git --version
> > git version 2.27.0
> >
> > $ git diff -b --function-context
> > diff --git i/example.c w/example.c
> > index d87b59b..346e2a7 100644
> > --- i/example.c
> > +++ w/example.c
> > @@ -4,5 +4,5 @@ int other_routine() {
> >  }
> >
> >  int main() {
> > -
> > +    puts("Hello, world!");
> >  }
> >
> > Zach Riggle
>
> I think it is possible to modify the "find the line that match
> xfuncname pattern" logic to start scanning backwards from the first
> actual change (i.e. the blank line in the preimage of the patch
> inside "int main() {" function in your example) and make the hunk
> header say "int main() {" instead of "int other_routine() {".
>
> I however doubt that such a change makes any sense.  In fact, I find
> the sample output above both quite logical and also even desirable.
>
> It is logical because the first thing we see in the hunk, "}", is at
> the end of "int other_routine() {" function; it does not conclude
> the "int main() {" function.  Saying "int main() {" there on the
> hunk header line would be misleading and confusing.  It sends a
> wrong signal that there was such a line _before_ the first line we
> see in this hunk, which is definitely not.
>
> It is desirable because it gives more information than saying the
> illogical "main".  The reader can see what the routine before the
> beginning of main function we see in the hunk is.  In the above
> example it may not matter, but if we see "return -1;" at the end of
> that function and a call to other_routine() from main(), e.g.
>
>         @@ ... @@ int other_routine() {
>             return -1;
>         }
>         int main() {
>         -   int i = other_routine();
>         +   int i = other_routine() ? 1 : 0;
>             printf("%d\n", i);
>         }
>
> it would be more informative than having "int main() {" there.

Re: --function-context shows wrong function in chunk

[Junio C Hamano]

Zach Riggle <zachriggle@gmail.com> writes:

[administrivia: do not top-post]

>> I think it is possible to modify the "find the line that match
>> xfuncname pattern" logic to start scanning backwards from the first
>> actual change (i.e. the blank line in the preimage of the patch
>> inside "int main() {" function in your example) and make the hunk
>> header say "int main() {" instead of "int other_routine() {".
>>
>> I however doubt that such a change makes any sense.  In fact, I find
>> the sample output above both quite logical and also even desirable.

> This happens even with -U0 which does not include the e.g. trailing
> bit of other_routine.

Sure, it does not change the equation, though.  We are finding the
line that matches the xfuncname pattern before the first line of the
hunk, which is "int other_routine() {" in your example.

> $ git diff -U0 -b --function-context
> diff --git i/example.c w/example.c
> index d87b59b..346e2a7 100644
> --- i/example.c
> +++ w/example.c
> @@ -6,3 +6,3 @@ int other_routine() {
>  int main() {
> -
> +    puts("Hello, world!");
>  }

In your updated example, the line "int main() {" is the first line
of the hunk, and not a line before it.

If we said "int main() {" there, it would send a wrong signal that
there is another such line before the "int main() {" we see in the
hunk.