phpdoc diff in git -L is not the correct one

phpdoc diff in git -L is not the correct one

[Grégoire PARIS]

[-- Attachment #1: Type: text/plain, Size: 411 bytes --]

Hello,

I have recently found out about git -L , which is great! I think I have 
found a
bug in it though: the diff is correct on the method itself, but changes 
in the
phpdoc of the method do not seem to be taken into account, while changes 
in the
phpdoc of the method that follows the one I care about show up in the 
diff. I
have attached a bug report generated with git bugreport.

Regards,

-- 
greg0ire


[-- Attachment #2: git-bugreport-2020-11-14-1402.txt --]
[-- Type: text/plain, Size: 1489 bytes --]

Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)

echo '*.php diff=php' > ~/.config/git/attributes
git clone git@github.com:doctrine/instantiator.git
cd instantiator
git log -L :instantiate:src/Doctrine/Instantiator/Instantiator.php

What did you expect to happen? (Expected behavior)

See changes that happened in the phpdoc of the instantiate() method appear in
the log.

What happened instead? (Actual behavior)

I saw changes that happened in the phpdoc of the buildAndCacheFromFactory()
method, which is the one right after the instantiate() method

What's different between what you expected and what actually happened?

The phpdoc being diffed.

Anything else you want to add:

Showing changes of the phpdoc of the method might not be easy to implement, but
I think not showing changes of the phpdoc of the next method should be.

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.

[System Info]
git version:
git version 2.28.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 5.8.16-300.fc33.x86_64 #1 SMP Mon Oct 19 13:18:33 UTC 2020 x86_64
compiler info: gnuc: 10.2
libc info: glibc: 2.32
$SHELL (typically, interactive shell): /bin/zsh


[Enabled Hooks]
pre-commit
post-commit
post-checkout
post-merge
pre-push
post-rewrite

Re: phpdoc diff in git -L is not the correct one

[Martin Ågren]

Hi greg0ire,

On Sat, 14 Nov 2020 at 14:28, Grégoire PARIS <postmaster@greg0ire.fr> wrote:
> I have recently found out about git -L , which is great! I think I have
> found a
> bug in it though: the diff is correct on the method itself, but changes
> in the
> phpdoc of the method do not seem to be taken into account, while changes
> in the
> phpdoc of the method that follows the one I care about show up in the
> diff. I
> have attached a bug report generated with git bugreport.

This seems to be behaving like documented. Quoting the man-page:

  If :<funcname> is given in place of <start> and <end>, it is a regular
  expression that denotes the range from the first funcname line that
  matches <funcname>, up to the next funcname line.

That range is exactly what you're seeing.

Now, I can certainly understand your wish of peeking backwards to
include the phpdoc for that function. You can do that using something
like

  git log -L46,76:src/Doctrine/Instantiator/Instantiator.php

but it's obviously a bit more involved to figure out which (approximate)
numbers to give.

One way of *only* looking backwards might be to use a regex for the
<start>, then a negative offset for <end>:

  git log -L/instantiate\(/,-14:src/Doctrine/Instantiator/Instantiator.php

Alas, this also requires coming up with a decent guess for how far back
to look. I can't seem to find a way of using a regex for <end> and
searching backwards -- I imagine it could be something like "-/regex/".
Anyway, that would just solve half your problem: You'd see the
documentation evolve but not the implementation.

In the end I think your best option right now is to give explicit line
numbers for <end> and <start>.

Martin

Re: phpdoc diff in git -L is not the correct one

[Grégoire PARIS]

Hi Martin,

thanks for your answer!

> In the end I think your best option right now is to give explicit line
> numbers for <end> and <start>.
That is indeed what I currently do, I plugged that to vim's visual 
selection with

   vnoremap <leader>l :<c-u>exe '!git log -L' 
line("'<").','.line("'>").':'.expand('%')<CR>

and it works great!

I was wondering if that was maybe an issue with the PHP regex, but with your
explanation I understand a bit more what the issue might be: The man 
page you
are quoting seems to say that the regex can only work on a single line as
opposed to a code block (for probably very good reasons), which means 
there is
no hope to include the phpdoc in the regex, or to fix this issue I suppose.

I also suppose the issue is the same for any other language that has 
documentation above function declarations.

Thanks for taking the time to answer my question.

--

greg0ire

Re: phpdoc diff in git -L is not the correct one

[Martin Ågren]

Hi,

On Sat, 14 Nov 2020 at 16:55, Grégoire PARIS <postmaster@greg0ire.fr> wrote:

> > In the end I think your best option right now is to give explicit line
> > numbers for <end> and <start>.
> That is indeed what I currently do, I plugged that to vim's visual
> selection with
>
>    vnoremap <leader>l :<c-u>exe '!git log -L'
> line("'<").','.line("'>").':'.expand('%')<CR>
>
> and it works great!

Great!

> I also suppose the issue is the same for any other language that has
> documentation above function declarations.

Yeah, you'll see the same thing for C files, e.g., using this in the
repo of Git itself:

  git log -L :strbuf_swap:strbuf.h

It will follow the lines from the function definition all the way down
to the next function, but just as you saw in your example, it will not
match the comment immediately *before* the function. That is, these
lines will be followed:

  https://github.com/git/git/blob/v2.29.2/strbuf.h#L125-L138

Martin

Re: phpdoc diff in git -L is not the correct one

[René Scharfe]

Am 14.11.20 um 18:35 schrieb Martin Ågren:
> Hi,
>
> On Sat, 14 Nov 2020 at 16:55, Grégoire PARIS <postmaster@greg0ire.fr> wrote:
>
>>> In the end I think your best option right now is to give explicit line
>>> numbers for <end> and <start>.
>> That is indeed what I currently do, I plugged that to vim's visual
>> selection with
>>
>>    vnoremap <leader>l :<c-u>exe '!git log -L'
>> line("'<").','.line("'>").':'.expand('%')<CR>
>>
>> and it works great!
>
> Great!
>
>> I also suppose the issue is the same for any other language that has
>> documentation above function declarations.
>
> Yeah, you'll see the same thing for C files, e.g., using this in the
> repo of Git itself:
>
>   git log -L :strbuf_swap:strbuf.h
>
> It will follow the lines from the function definition all the way down
> to the next function, but just as you saw in your example, it will not
> match the comment immediately *before* the function. That is, these
> lines will be followed:
>
>   https://github.com/git/git/blob/v2.29.2/strbuf.h#L125-L138

The --function-context options of git diff and git grep try to show
comments by including non-empty lines before function lines.  This
heuristic might work for -L :funcname:file as well (patch below), but
breaks seven tests in each of t8001-annotate.sh, t8002-blame.sh and
t8012-blame-colors.sh.

René

---
 line-range.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/line-range.c b/line-range.c
index 9b50583dc0..5d55fa255f 100644
--- a/line-range.c
+++ b/line-range.c
@@ -163,6 +163,13 @@ static const char *find_funcname_matching_regexp(xdemitconf_t *xecfg, const char
 	}
 }

+static int is_empty_line(const char *bol, const char *eol)
+{
+	while (bol < eol && isspace(*bol))
+		bol++;
+	return bol == eol;
+}
+
 static const char *parse_range_funcname(
 	const char *arg, nth_line_fn_t nth_line_cb,
 	void *cb_data, long lines, long anchor, long *begin, long *end,
@@ -233,6 +240,18 @@ static const char *parse_range_funcname(
 		(*end)++;
 	}

+	/*
+	 * Include non-empty, non-funcname lines before the found
+	 * funcname line, as they probably contain related comments.
+	 */
+	while (*begin > 0) {
+		const char *bol = nth_line_cb(cb_data, *begin - 1);
+		const char *eol = nth_line_cb(cb_data, *begin);
+		if (is_empty_line(bol, eol) || match_funcname(xecfg, bol, eol))
+			break;
+		(*begin)--;
+	}
+
 	regfree(&regexp);
 	free(xecfg);
 	free(pattern);
--
2.29.2

Re: phpdoc diff in git -L is not the correct one

[Grégoire PARIS]

Hi René!

Sorry for not seeing your answer before!

On 11/15/20 12:40 AM, René Scharfe wrote:
>
> The --function-context options of git diff and git grep try to show
> comments by including non-empty lines before function lines.

That would indeed do the first part of the job. Do you think something 
similarcould be done to remove non-empty lines before the next funcname?

>    This
> heuristic might work for -L :funcname:file as well (patch below), but
> breaks seven tests in each of t8001-annotate.sh, t8002-blame.sh and
> t8012-blame-colors.sh.

I haven't written C in 10 literal years but I think I managed to apply 
this patch, and something looks wrong: it's looking too "far" before: 
See for instance: --- commit 1a8a640f87cad94d36713f45e5e257de20930171 
Author: Michael Moravec <me@majkl.me> Date: Mon Mar 5 04:01:58 2018 
+0100 Upgrade to Doctrine CS 4.0 diff --git 
a/src/Doctrine/Instantiator/Instantiator.php 
b/src/Doctrine/Instantiator/Instantiator.php --- 
a/src/Doctrine/Instantiator/Instantiator.php +++ 
b/src/Doctrine/Instantiator/Instantiator.php @@ -31,12 +33,14 @@ */ 
private static $cachedInstantiators = []; /** - * @var object[] of 
objects that can directly be cloned, indexed by class name + * Array of 
objects that can directly be cloned, indexed by class name. + * + * @var 
object[] */ private static $cachedCloneables = []; /** * {@inheritDoc} 
*/ public function instantiate($className) --- Here it's picking changes 
in the phpdoc of the property that precedes `instantiate`, (when using 
git log 
-L/instantiate\(/,-14:src/Doctrine/Instantiator/Instantiator.php) What's 
wrong? -- greg0ire

Re: phpdoc diff in git -L is not the correct one

[Martin Ågren]

On Wed, 18 Nov 2020 at 09:17, Grégoire PARIS <postmaster@greg0ire.fr> wrote:
> On 11/15/20 12:40 AM, René Scharfe wrote:
> >
> > The --function-context options of git diff and git grep try to show
> > comments by including non-empty lines before function lines.

> >    This
> > heuristic might work for -L :funcname:file as well (patch below), but
> > breaks seven tests in each of t8001-annotate.sh, t8002-blame.sh and
> > t8012-blame-colors.sh.
>
> I haven't written C in 10 literal years but I think I managed to apply
> this patch, and something looks wrong: it's looking too "far" before:
> See for instance: --- commit 1a8a640f87cad94d36713f45e5e257de20930171
> Author: Michael Moravec <me@majkl.me> Date: Mon Mar 5 04:01:58 2018
> +0100 Upgrade to Doctrine CS 4.0 diff --git
> a/src/Doctrine/Instantiator/Instantiator.php
> b/src/Doctrine/Instantiator/Instantiator.php ---
> a/src/Doctrine/Instantiator/Instantiator.php +++
> b/src/Doctrine/Instantiator/Instantiator.php @@ -31,12 +33,14 @@ */
> private static $cachedInstantiators = []; /** - * @var object[] of
> objects that can directly be cloned, indexed by class name + * Array of
> objects that can directly be cloned, indexed by class name. + * + * @var
> object[] */ private static $cachedCloneables = []; /** * {@inheritDoc}
> */ public function instantiate($className) --- Here it's picking changes
> in the phpdoc of the property that precedes `instantiate`, (when using
> git log
> -L/instantiate\(/,-14:src/Doctrine/Instantiator/Instantiator.php) What's
> wrong? -- greg0ire

So to reproduce, it's first something like this?

  git clone https://github.com/doctrine/instantiator.git
  cd instantiator
  echo '*.php diff=php' >>.gitattributes

Then this?

  git log -L/instantiate\(/,-14:src/Doctrine/Instantiator/Instantiator.php

Or should that be the following?

  git log -L :instantiate:src/Doctrine/Instantiator/Instantiator.php

I played around a little, but couldn't seem to hit 1a8a640f87 as you
mentioned.

Martin