{"thread":{"id":"54642","subject":"phpdoc diff in git -L is not the correct one","startedAt":"2020-11-14T13:25:43Z","lastAt":"2020-11-18T19:19:04Z","messageCount":7,"participants":["Grégoire PARIS","Martin Ågren","René Scharfe"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"409954","messageId":"348a2a4a-dfdb-190b-edac-01e9ad4c2d4d@greg0ire.fr","threadId":"54642","inReplyTo":null,"subject":"phpdoc diff in git -L is not the correct one","fromName":"Grégoire PARIS","fromEmail":"postmaster@greg0ire.fr","sentAt":"2020-11-14T13:19:01Z","receivedAt":"2020-11-14T13:25:43Z","isPatch":false,"sender":{"key":"postmaster@greg0ire.fr","avatar":"https://avatars.githubusercontent.com/u/657779?v=4"},"body":"Hello,\n\nI have recently found out about git -L , which is great! I think I have \nfound a\nbug in it though: the diff is correct on the method itself, but changes \nin the\nphpdoc of the method do not seem to be taken into account, while changes \nin the\nphpdoc of the method that follows the one I care about show up in the \ndiff. I\nhave attached a bug report generated with git bugreport.\n\nRegards,\n\n-- \ngreg0ire\n\n\n\nThank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\necho '*.php diff=php' > ~/.config/git/attributes\ngit clone git@github.com:doctrine/instantiator.git\ncd instantiator\ngit log -L :instantiate:src/Doctrine/Instantiator/Instantiator.php\n\nWhat did you expect to happen? (Expected behavior)\n\nSee changes that happened in the phpdoc of the instantiate() method appear in\nthe log.\n\nWhat happened instead? (Actual behavior)\n\nI saw changes that happened in the phpdoc of the buildAndCacheFromFactory()\nmethod, which is the one right after the instantiate() method\n\nWhat's different between what you expected and what actually happened?\n\nThe phpdoc being diffed.\n\nAnything else you want to add:\n\nShowing changes of the phpdoc of the method might not be easy to implement, but\nI think not showing changes of the phpdoc of the next method should be.\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n[System Info]\ngit version:\ngit version 2.28.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 5.8.16-300.fc33.x86_64 #1 SMP Mon Oct 19 13:18:33 UTC 2020 x86_64\ncompiler info: gnuc: 10.2\nlibc info: glibc: 2.32\n$SHELL (typically, interactive shell): /bin/zsh\n\n\n[Enabled Hooks]\npre-commit\npost-commit\npost-checkout\npost-merge\npre-push\npost-rewrite\n"},{"id":"409956","messageId":"CAN0heSrU5zzgR_FDZcEopPP2EmSQnraZXO4v8Smx8=fWcXa0uQ@mail.gmail.com","threadId":"54642","inReplyTo":"348a2a4a-dfdb-190b-edac-01e9ad4c2d4d@greg0ire.fr","subject":"Re: phpdoc diff in git -L is not the correct one","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-14T15:32:07Z","receivedAt":"2020-11-14T15:32:23Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi greg0ire,\n\nOn Sat, 14 Nov 2020 at 14:28, Grégoire PARIS wrote:\n> I have recently found out about git -L , which is great! I think I have\n> found a\n> bug in it though: the diff is correct on the method itself, but changes\n> in the\n> phpdoc of the method do not seem to be taken into account, while changes\n> in the\n> phpdoc of the method that follows the one I care about show up in the\n> diff. I\n> have attached a bug report generated with git bugreport.\n\nThis seems to be behaving like documented. Quoting the man-page:\n\n If : is given in place of and , it is a regular\n expression that denotes the range from the first funcname line that\n matches , up to the next funcname line.\n\nThat range is exactly what you're seeing.\n\nNow, I can certainly understand your wish of peeking backwards to\ninclude the phpdoc for that function. You can do that using something\nlike\n\n git log -L46,76:src/Doctrine/Instantiator/Instantiator.php\n\nbut it's obviously a bit more involved to figure out which (approximate)\nnumbers to give.\n\nOne way of *only* looking backwards might be to use a regex for the\n, then a negative offset for :\n\n git log -L/instantiate\\(/,-14:src/Doctrine/Instantiator/Instantiator.php\n\nAlas, this also requires coming up with a decent guess for how far back\nto look. I can't seem to find a way of using a regex for and\nsearching backwards -- I imagine it could be something like \"-/regex/\".\nAnyway, that would just solve half your problem: You'd see the\ndocumentation evolve but not the implementation.\n\nIn the end I think your best option right now is to give explicit line\nnumbers for and .\n\nMartin\n"},{"id":"409958","messageId":"CAN0heSr9=_So-sUhQN86vawBEkJhnrHHsd3OcSYZMZ-idZGFAQ@mail.gmail.com","threadId":"54642","inReplyTo":"e666e806-d8c6-0b8d-c583-e4a8ee0ee806@greg0ire.fr","subject":"Re: phpdoc diff in git -L is not the correct one","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-14T17:35:02Z","receivedAt":"2020-11-14T17:35:20Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi,\n\nOn Sat, 14 Nov 2020 at 16:55, Grégoire PARIS wrote:\n\n> > In the end I think your best option right now is to give explicit line\n> > numbers for and .\n> That is indeed what I currently do, I plugged that to vim's visual\n> selection with\n>\n> vnoremap l :exe '!git log -L'\n> line(\"'<\").','.line(\"'>\").':'.expand('%')\n>\n> and it works great!\n\nGreat!\n\n> I also suppose the issue is the same for any other language that has\n> documentation above function declarations.\n\nYeah, you'll see the same thing for C files, e.g., using this in the\nrepo of Git itself:\n\n git log -L :strbuf_swap:strbuf.h\n\nIt will follow the lines from the function definition all the way down\nto the next function, but just as you saw in your example, it will not\nmatch the comment immediately *before* the function. That is, these\nlines will be followed:\n\n https://github.com/git/git/blob/v2.29.2/strbuf.h#L125-L138\n\nMartin\n"},{"id":"409963","messageId":"e666e806-d8c6-0b8d-c583-e4a8ee0ee806@greg0ire.fr","threadId":"54642","inReplyTo":"CAN0heSrU5zzgR_FDZcEopPP2EmSQnraZXO4v8Smx8=fWcXa0uQ@mail.gmail.com","subject":"Re: phpdoc diff in git -L is not the correct one","fromName":"Grégoire PARIS","fromEmail":"postmaster@greg0ire.fr","sentAt":"2020-11-14T15:55:22Z","receivedAt":"2020-11-14T19:32:03Z","isPatch":false,"sender":{"key":"postmaster@greg0ire.fr","avatar":"https://avatars.githubusercontent.com/u/657779?v=4"},"body":"Hi Martin,\n\nthanks for your answer!\n\n> In the end I think your best option right now is to give explicit line\n> numbers for and .\nThat is indeed what I currently do, I plugged that to vim's visual \nselection with\n\n   vnoremap l :exe '!git log -L' \nline(\"'<\").','.line(\"'>\").':'.expand('%')\n\nand it works great!\n\nI was wondering if that was maybe an issue with the PHP regex, but with your\nexplanation I understand a bit more what the issue might be: The man \npage you\nare quoting seems to say that the regex can only work on a single line as\nopposed to a code block (for probably very good reasons), which means \nthere is\nno hope to include the phpdoc in the regex, or to fix this issue I suppose.\n\nI also suppose the issue is the same for any other language that has \ndocumentation above function declarations.\n\nThanks for taking the time to answer my question.\n\n--\n\ngreg0ire\n\n"},{"id":"409968","messageId":"5a2d70eb-da75-ff59-8ac1-6ba81e1632da@web.de","threadId":"54642","inReplyTo":"CAN0heSr9=_So-sUhQN86vawBEkJhnrHHsd3OcSYZMZ-idZGFAQ@mail.gmail.com","subject":"Re: phpdoc diff in git -L is not the correct one","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-11-14T23:40:51Z","receivedAt":"2020-11-14T23:41:21Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 14.11.20 um 18:35 schrieb Martin Ågren:\n> Hi,\n>\n> On Sat, 14 Nov 2020 at 16:55, Grégoire PARIS wrote:\n>\n>>> In the end I think your best option right now is to give explicit line\n>>> numbers for and .\n>> That is indeed what I currently do, I plugged that to vim's visual\n>> selection with\n>>\n>> vnoremap l :exe '!git log -L'\n>> line(\"'<\").','.line(\"'>\").':'.expand('%')\n>>\n>> and it works great!\n>\n> Great!\n>\n>> I also suppose the issue is the same for any other language that has\n>> documentation above function declarations.\n>\n> Yeah, you'll see the same thing for C files, e.g., using this in the\n> repo of Git itself:\n>\n> git log -L :strbuf_swap:strbuf.h\n>\n> It will follow the lines from the function definition all the way down\n> to the next function, but just as you saw in your example, it will not\n> match the comment immediately *before* the function. That is, these\n> lines will be followed:\n>\n> https://github.com/git/git/blob/v2.29.2/strbuf.h#L125-L138\n\nThe --function-context options of git diff and git grep try to show\ncomments by including non-empty lines before function lines. This\nheuristic might work for -L :funcname:file as well (patch below), but\nbreaks seven tests in each of t8001-annotate.sh, t8002-blame.sh and\nt8012-blame-colors.sh.\n\nRené\n\n---\n line-range.c | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/line-range.c b/line-range.c\nindex 9b50583dc0..5d55fa255f 100644\n--- a/line-range.c\n+++ b/line-range.c\n@@ -163,6 +163,13 @@ static const char *find_funcname_matching_regexp(xdemitconf_t *xecfg, const char\n \t}\n }\n\n+static int is_empty_line(const char *bol, const char *eol)\n+{\n+\twhile (bol < eol && isspace(*bol))\n+\t\tbol++;\n+\treturn bol == eol;\n+}\n+\n static const char *parse_range_funcname(\n \tconst char *arg, nth_line_fn_t nth_line_cb,\n \tvoid *cb_data, long lines, long anchor, long *begin, long *end,\n@@ -233,6 +240,18 @@ static const char *parse_range_funcname(\n \t\t(*end)++;\n \t}\n\n+\t/*\n+\t * Include non-empty, non-funcname lines before the found\n+\t * funcname line, as they probably contain related comments.\n+\t */\n+\twhile (*begin > 0) {\n+\t\tconst char *bol = nth_line_cb(cb_data, *begin - 1);\n+\t\tconst char *eol = nth_line_cb(cb_data, *begin);\n+\t\tif (is_empty_line(bol, eol) || match_funcname(xecfg, bol, eol))\n+\t\t\tbreak;\n+\t\t(*begin)--;\n+\t}\n+\n \tregfree(®exp);\n \tfree(xecfg);\n \tfree(pattern);\n--\n2.29.2\n"},{"id":"410210","messageId":"a205cb0c-95ec-7f9e-0dea-8fd5b4bc694f@greg0ire.fr","threadId":"54642","inReplyTo":"5a2d70eb-da75-ff59-8ac1-6ba81e1632da@web.de","subject":"Re: phpdoc diff in git -L is not the correct one","fromName":"Grégoire PARIS","fromEmail":"postmaster@greg0ire.fr","sentAt":"2020-11-18T08:17:49Z","receivedAt":"2020-11-18T08:17:54Z","isPatch":false,"sender":{"key":"postmaster@greg0ire.fr","avatar":"https://avatars.githubusercontent.com/u/657779?v=4"},"body":"Hi René!\n\nSorry for not seeing your answer before!\n\nOn 11/15/20 12:40 AM, René Scharfe wrote:\n>\n> The --function-context options of git diff and git grep try to show\n> comments by including non-empty lines before function lines.\n\nThat would indeed do the first part of the job. Do you think something \nsimilarcould be done to remove non-empty lines before the next funcname?\n\n> This\n> heuristic might work for -L :funcname:file as well (patch below), but\n> breaks seven tests in each of t8001-annotate.sh, t8002-blame.sh and\n> t8012-blame-colors.sh.\n\nI haven't written C in 10 literal years but I think I managed to apply \nthis patch, and something looks wrong: it's looking too \"far\" before: \nSee for instance: --- commit 1a8a640f87cad94d36713f45e5e257de20930171 \nAuthor: Michael Moravec Date: Mon Mar 5 04:01:58 2018 \n+0100 Upgrade to Doctrine CS 4.0 diff --git \na/src/Doctrine/Instantiator/Instantiator.php \nb/src/Doctrine/Instantiator/Instantiator.php --- \na/src/Doctrine/Instantiator/Instantiator.php +++ \nb/src/Doctrine/Instantiator/Instantiator.php @@ -31,12 +33,14 @@ */ \nprivate static $cachedInstantiators = []; /** - * @var object[] of \nobjects that can directly be cloned, indexed by class name + * Array of \nobjects that can directly be cloned, indexed by class name. + * + * @var \nobject[] */ private static $cachedCloneables = []; /** * {@inheritDoc} \n*/ public function instantiate($className) --- Here it's picking changes \nin the phpdoc of the property that precedes `instantiate`, (when using \ngit log \n-L/instantiate\\(/,-14:src/Doctrine/Instantiator/Instantiator.php) What's \nwrong? -- greg0ire\n\n"},{"id":"410264","messageId":"CAN0heSrqs9GLo6DA8aWGG8JON4NmGCEiw63pR0BJRZHRTZOaFA@mail.gmail.com","threadId":"54642","inReplyTo":"a205cb0c-95ec-7f9e-0dea-8fd5b4bc694f@greg0ire.fr","subject":"Re: phpdoc diff in git -L is not the correct one","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-18T19:18:44Z","receivedAt":"2020-11-18T19:19:04Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Wed, 18 Nov 2020 at 09:17, Grégoire PARIS wrote:\n> On 11/15/20 12:40 AM, René Scharfe wrote:\n> >\n> > The --function-context options of git diff and git grep try to show\n> > comments by including non-empty lines before function lines.\n\n> > This\n> > heuristic might work for -L :funcname:file as well (patch below), but\n> > breaks seven tests in each of t8001-annotate.sh, t8002-blame.sh and\n> > t8012-blame-colors.sh.\n>\n> I haven't written C in 10 literal years but I think I managed to apply\n> this patch, and something looks wrong: it's looking too \"far\" before:\n> See for instance: --- commit 1a8a640f87cad94d36713f45e5e257de20930171\n> Author: Michael Moravec Date: Mon Mar 5 04:01:58 2018\n> +0100 Upgrade to Doctrine CS 4.0 diff --git\n> a/src/Doctrine/Instantiator/Instantiator.php\n> b/src/Doctrine/Instantiator/Instantiator.php ---\n> a/src/Doctrine/Instantiator/Instantiator.php +++\n> b/src/Doctrine/Instantiator/Instantiator.php @@ -31,12 +33,14 @@ */\n> private static $cachedInstantiators = []; /** - * @var object[] of\n> objects that can directly be cloned, indexed by class name + * Array of\n> objects that can directly be cloned, indexed by class name. + * + * @var\n> object[] */ private static $cachedCloneables = []; /** * {@inheritDoc}\n> */ public function instantiate($className) --- Here it's picking changes\n> in the phpdoc of the property that precedes `instantiate`, (when using\n> git log\n> -L/instantiate\\(/,-14:src/Doctrine/Instantiator/Instantiator.php) What's\n> wrong? -- greg0ire\n\nSo to reproduce, it's first something like this?\n\n git clone https://github.com/doctrine/instantiator.git\n cd instantiator\n echo '*.php diff=php' >>.gitattributes\n\nThen this?\n\n git log -L/instantiate\\(/,-14:src/Doctrine/Instantiator/Instantiator.php\n\nOr should that be the following?\n\n git log -L :instantiate:src/Doctrine/Instantiator/Instantiator.php\n\nI played around a little, but couldn't seem to hit 1a8a640f87 as you\nmentioned.\n\nMartin\n"}]}