* phpdoc diff in git -L is not the correct one @ 2020-11-14 13:19 Grégoire PARIS 2020-11-14 15:32 ` Martin Ågren 0 siblings, 1 reply; 7+ messages in thread From: Grégoire PARIS @ 2020-11-14 13:19 UTC (permalink / raw) To: git [-- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: phpdoc diff in git -L is not the correct one 2020-11-14 13:19 phpdoc diff in git -L is not the correct one Grégoire PARIS @ 2020-11-14 15:32 ` Martin Ågren 2020-11-14 15:55 ` Grégoire PARIS 0 siblings, 1 reply; 7+ messages in thread From: Martin Ågren @ 2020-11-14 15:32 UTC (permalink / raw) To: Grégoire PARIS; +Cc: Git Mailing List 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: phpdoc diff in git -L is not the correct one 2020-11-14 15:32 ` Martin Ågren @ 2020-11-14 15:55 ` Grégoire PARIS 2020-11-14 17:35 ` Martin Ågren 0 siblings, 1 reply; 7+ messages in thread From: Grégoire PARIS @ 2020-11-14 15:55 UTC (permalink / raw) To: Martin Ågren; +Cc: Git Mailing List 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: phpdoc diff in git -L is not the correct one 2020-11-14 15:55 ` Grégoire PARIS @ 2020-11-14 17:35 ` Martin Ågren 2020-11-14 23:40 ` René Scharfe 0 siblings, 1 reply; 7+ messages in thread From: Martin Ågren @ 2020-11-14 17:35 UTC (permalink / raw) To: Grégoire PARIS; +Cc: Git Mailing List 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: phpdoc diff in git -L is not the correct one 2020-11-14 17:35 ` Martin Ågren @ 2020-11-14 23:40 ` René Scharfe 2020-11-18 8:17 ` Grégoire PARIS 0 siblings, 1 reply; 7+ messages in thread From: René Scharfe @ 2020-11-14 23:40 UTC (permalink / raw) To: Martin Ågren, Grégoire PARIS; +Cc: Git Mailing List 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(®exp); free(xecfg); free(pattern); -- 2.29.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: phpdoc diff in git -L is not the correct one 2020-11-14 23:40 ` René Scharfe @ 2020-11-18 8:17 ` Grégoire PARIS 2020-11-18 19:18 ` Martin Ågren 0 siblings, 1 reply; 7+ messages in thread From: Grégoire PARIS @ 2020-11-18 8:17 UTC (permalink / raw) To: René Scharfe, Martin Ågren; +Cc: Git Mailing List 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: phpdoc diff in git -L is not the correct one 2020-11-18 8:17 ` Grégoire PARIS @ 2020-11-18 19:18 ` Martin Ågren 0 siblings, 0 replies; 7+ messages in thread From: Martin Ågren @ 2020-11-18 19:18 UTC (permalink / raw) To: Grégoire PARIS; +Cc: René Scharfe, Git Mailing List 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-11-18 19:19 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-14 13:19 phpdoc diff in git -L is not the correct one Grégoire PARIS 2020-11-14 15:32 ` Martin Ågren 2020-11-14 15:55 ` Grégoire PARIS 2020-11-14 17:35 ` Martin Ågren 2020-11-14 23:40 ` René Scharfe 2020-11-18 8:17 ` Grégoire PARIS 2020-11-18 19:18 ` Martin Ågren
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).