threads / discuss / 54642

phpdoc diff in git -L is not the correct one

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

## tl;dr

7 messages between Nov 14, 2020 and Nov 18, 2020.

replies: 6people: 3as markdown or json

Grégoire PARIS· Nov 14, 2020, 13:19 UTC · lore
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



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
Martin Ågren· Nov 14, 2020, 15:32 UTC · re: Grégoire PARIS · lore

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

Hi greg0ire,
On Sat, 14 Nov 2020 at 14:28, Grégoire PARIS <postmaster@greg0ire.fr> wrote:
Show 9 quoted lines
> 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
Grégoire PARIS· Nov 14, 2020, 15:55 UTC · re: Martin Ågren · lore

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

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
Martin Ågren· Nov 14, 2020, 17:35 UTC · re: Grégoire PARIS · lore

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

Hi,
On Sat, 14 Nov 2020 at 16:55, Grégoire PARIS <postmaster@greg0ire.fr> wrote:
Show 9 quoted lines
> > 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
René Scharfe· Nov 14, 2020, 23:40 UTC · re: Martin Ågren · lore

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

Am 14.11.20 um 18:35 schrieb Martin Ågren:
Show 30 quoted lines
> 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
Grégoire PARIS· Nov 18, 2020, 08:17 UTC · re: René Scharfe · lore

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

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
Martin Ågren· Nov 18, 2020, 19:18 UTC · re: Grégoire PARIS · lore

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

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.
Show 23 quoted 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

← back to recent threads