{"thread":{"id":"42235","subject":"[PATCH] gitweb: fix link to parent diff with pathinfo","startedAt":"2016-05-06T10:19:38Z","lastAt":"2016-05-25T22:20:32Z","messageCount":8,"participants":["Richard Braun","Junio C Hamano","Jakub Narębski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"285679","messageId":"1462529978-31322-1-git-send-email-rbraun@sceen.net","threadId":"42235","inReplyTo":null,"subject":"[PATCH] gitweb: fix link to parent diff with pathinfo","fromName":"Richard Braun","fromEmail":"rbraun@sceen.net","sentAt":"2016-05-06T10:19:38Z","receivedAt":"2016-05-06T10:19:38Z","isPatch":true,"sender":{"key":"rbraun@sceen.net","avatar":null},"body":"Signed-off-by: Richard Braun \n---\n gitweb/gitweb.perl | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 05d7910..f7f7936 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1423,7 +1423,12 @@ sub href {\n \t\t\tdelete $params{'hash'};\n \t\t\tdelete $params{'hash_base'};\n \t\t} elsif (defined $params{'hash'}) {\n-\t\t\t$href .= esc_path_info($params{'hash'});\n+\t\t\tif (defined $params{'hash_parent'}) {\n+\t\t\t\t$href .= esc_path_info($params{'hash_parent'});\n+\t\t\t\tdelete $params{'hash_parent'};\n+\t\t\t} else {\n+\t\t\t\t$href .= esc_path_info($params{'hash'});\n+\t\t\t}\n \t\t\tdelete $params{'hash'};\n \t\t}\n \n-- \n2.1.4\n"},{"id":"285787","messageId":"xmqqmvo225fg.fsf@gitster.mtv.corp.google.com","threadId":"42235","inReplyTo":"1462529978-31322-1-git-send-email-rbraun@sceen.net","subject":"Re: [PATCH] gitweb: fix link to parent diff with pathinfo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-06T22:21:55Z","receivedAt":"2016-05-06T22:21:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Richard Braun writes:\n\n> Signed-off-by: Richard Braun \n> ---\n\nCould you justify your change with a bit more than \"fix\"? That is,\n\n gitweb, when used with PATH_INFO, shows a link to parent diff\n like [fill in the blank]. However, it is wrong because [fill in\n the blank].\n\n Make it show it like [fill in the blank]. Because [fill in the\n blank], delete 'hash_parent' element from the %params hash once\n we used it; otherwise [fill in the blank to describe \"this bad\n thing happens\"].\n\nor something like that.\n\nThanks.\n\n> gitweb/gitweb.perl | 7 ++++++-\n> 1 file changed, 6 insertions(+), 1 deletion(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 05d7910..f7f7936 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1423,7 +1423,12 @@ sub href {\n> \t\t\tdelete $params{'hash'};\n> \t\t\tdelete $params{'hash_base'};\n> \t\t} elsif (defined $params{'hash'}) {\n> -\t\t\t$href .= esc_path_info($params{'hash'});\n> +\t\t\tif (defined $params{'hash_parent'}) {\n> +\t\t\t\t$href .= esc_path_info($params{'hash_parent'});\n> +\t\t\t\tdelete $params{'hash_parent'};\n> +\t\t\t} else {\n> +\t\t\t\t$href .= esc_path_info($params{'hash'});\n> +\t\t\t}\n> \t\t\tdelete $params{'hash'};\n> \t\t}\n"},{"id":"285792","messageId":"1462579902-18907-1-git-send-email-rbraun@sceen.net","threadId":"42235","inReplyTo":"xmqqmvo225fg.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] gitweb: fix link to parent diff with pathinfo","fromName":"Richard Braun","fromEmail":"rbraun@sceen.net","sentAt":"2016-05-07T00:11:42Z","receivedAt":"2016-05-07T00:11:42Z","isPatch":true,"sender":{"key":"rbraun@sceen.net","avatar":null},"body":"Gitweb, when used with PATH_INFO, shows a link to parent diff\nlike http://somedomain/somerepo.git/commitdiff/somehash?hp=parenthash.\nThat link reports \"400 - Invalid hash parameter\".\n\nAs I understand it, it should instead directly point to the parent diff,\ni.e. turn it into http://somedomain/somerepo.git/commitdiff/parenthash,\nand delete 'hash_parent' element from the %params hash once we used it,\notherwise the '?hp=parenthash' string is appended.\n\nSigned-off-by: Richard Braun \n---\n gitweb/gitweb.perl | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 05d7910..f7f7936 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1423,7 +1423,12 @@ sub href {\n \t\t\tdelete $params{'hash'};\n \t\t\tdelete $params{'hash_base'};\n \t\t} elsif (defined $params{'hash'}) {\n-\t\t\t$href .= esc_path_info($params{'hash'});\n+\t\t\tif (defined $params{'hash_parent'}) {\n+\t\t\t\t$href .= esc_path_info($params{'hash_parent'});\n+\t\t\t\tdelete $params{'hash_parent'};\n+\t\t\t} else {\n+\t\t\t\t$href .= esc_path_info($params{'hash'});\n+\t\t\t}\n \t\t\tdelete $params{'hash'};\n \t\t}\n \n-- \n2.1.4\n"},{"id":"287433","messageId":"xmqq37p75nif.fsf@gitster.mtv.corp.google.com","threadId":"42235","inReplyTo":"1462579902-18907-1-git-send-email-rbraun@sceen.net","subject":"Re: [PATCH] gitweb: fix link to parent diff with pathinfo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-24T18:17:28Z","receivedAt":"2016-05-24T18:17:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Richard Braun writes:\n\n> Gitweb, when used with PATH_INFO, shows a link to parent diff\n> like http://somedomain/somerepo.git/commitdiff/somehash?hp=parenthash.\n> That link reports \"400 - Invalid hash parameter\".\n>\n> As I understand it, it should instead directly point to the parent diff,\n> i.e. turn it into http://somedomain/somerepo.git/commitdiff/parenthash,\n> and delete 'hash_parent' element from the %params hash once we used it,\n> otherwise the '?hp=parenthash' string is appended.\n>\n> Signed-off-by: Richard Braun \n> ---\n\nPinging...\n\n> gitweb/gitweb.perl | 7 ++++++-\n> 1 file changed, 6 insertions(+), 1 deletion(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 05d7910..f7f7936 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1423,7 +1423,12 @@ sub href {\n> \t\t\tdelete $params{'hash'};\n> \t\t\tdelete $params{'hash_base'};\n> \t\t} elsif (defined $params{'hash'}) {\n> -\t\t\t$href .= esc_path_info($params{'hash'});\n> +\t\t\tif (defined $params{'hash_parent'}) {\n> +\t\t\t\t$href .= esc_path_info($params{'hash_parent'});\n> +\t\t\t\tdelete $params{'hash_parent'};\n> +\t\t\t} else {\n> +\t\t\t\t$href .= esc_path_info($params{'hash'});\n> +\t\t\t}\n> \t\t\tdelete $params{'hash'};\n> \t\t}\n"},{"id":"287434","messageId":"20160524182623.GA8562@shattrath","threadId":"42235","inReplyTo":"xmqq37p75nif.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] gitweb: fix link to parent diff with pathinfo","fromName":"Richard Braun","fromEmail":"rbraun@sceen.net","sentAt":"2016-05-24T18:26:23Z","receivedAt":"2016-05-24T18:26:23Z","isPatch":true,"sender":{"key":"rbraun@sceen.net","avatar":null},"body":"On Tue, May 24, 2016 at 11:17:28AM -0700, Junio C Hamano wrote:\n> Pinging...\n\nHum, see [1].\n\nTell me if I need to resend.\n\n-- \nRichard Braun\nPacte Novation / Novasys Ingénierie\n\n[1] http://git.661346.n2.nabble.com/PATCH-gitweb-fix-link-to-parent-diff-with-pathinfo-td7655482.html\n"},{"id":"287437","messageId":"xmqqtwhn47xs.fsf@gitster.mtv.corp.google.com","threadId":"42235","inReplyTo":"20160524182623.GA8562@shattrath","subject":"Re: [PATCH] gitweb: fix link to parent diff with pathinfo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-24T18:39:11Z","receivedAt":"2016-05-24T18:39:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Richard Braun writes:\n\n> On Tue, May 24, 2016 at 11:17:28AM -0700, Junio C Hamano wrote:\n>> Pinging...\n>\n> Hum, see [1].\n>\n> Tell me if I need to resend.\n\nSorry, check the \"To:\" field of the message you are responding to.\nThe ping was not meant to (and was not addressed to) you. It asked\nfor comments from an area expert.\n"},{"id":"287509","messageId":"CANQwDwdKaVDGDxZZma25WcV+ea90YtFhDedLwv2KNZt6jhKVAQ@mail.gmail.com","threadId":"42235","inReplyTo":"xmqq37p75nif.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] gitweb: fix link to parent diff with pathinfo","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-05-25T20:33:32Z","receivedAt":"2016-05-25T20:33:32Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, May 24, 2016 at 8:17 PM, Junio C Hamano wrote:\n> Richard Braun writes:\n>\n>> Gitweb, when used with PATH_INFO, shows a link to parent diff\n>> like http://somedomain/somerepo.git/commitdiff/somehash?hp=parenthash.\n>> That link reports \"400 - Invalid hash parameter\".\n\nRichard, at which view is this bad link present? Err... never mind, I see that\ngitweb uses link to 'commitdiff' view with 'hash_parent' parameter set only\n..in two places (well, perhaps there are some which get hash_parent from\n-replay, but I doubt it): the \"commit\" view (link to each parent commit)\nand in \"commitdiff\" view for octopus merges (e.g. \"pu\" in git.git).\n\nThe problem is not with ?hp=parenthash, but with /somehash part, which\nsomehow got invalid (from the error message). I have checked using\nhttp://repo.or.cz/git.git, and while it has the bug (i.e. uses '?hp=...' instead\nof path_info), there is no \"400 - Invalid has parameter\" error.\n\n>> As I understand it, it should instead directly point to the parent diff,\n>> i.e. turn it into http://somedomain/somerepo.git/commitdiff/parenthash,\n\nActually, the correct path_info based URI would be\n\n http://somedomain/somerepo.git/commitdiff/parenthash..somehash\n\nJust like href() does with hash_parent_base and hash_base for blobdiff.\nUrgh... href() is a bit of mess, now I see it when I am not current.\n\n>> and delete 'hash_parent' element from the %params hash once we used it,\n>> otherwise the '?hp=parenthash' string is appended.\n\nThat's correct: the unstated rule of href is that if it went into path_info,\nit is deleted (not everything can be expressed with path_info), the rest\ngoes into query parameters. So without deleting the element, it would\nbe duplicated.\n\nNote that using query parameter when we can use path_info is a minor\nerror; URL should work anyway (and I don't see why it doesn't - somewhat\nthe 'hash' parameter got incorrect...).\n\n>>\n>> Signed-off-by: Richard Braun \n>> ---\n>\n> Pinging...\n\nI'm sorry, I didn't notice it was meant for me. Simple \"Jakub,...\"\nwould be enough.\n\nOn Tue, May 24, 2016 at 8:39 PM, Junio C Hamano wrote:\n> Richard Braun writes:\n>\n>> On Tue, May 24, 2016 at 11:17:28AM -0700, Junio C Hamano wrote:\n>>> Pinging...\n>>\n>> Hum, see [1].\n>>\n>> Tell me if I need to resend.\n>\n> Sorry, check the \"To:\" field of the message you are responding to.\n> The ping was not meant to (and was not addressed to) you. It asked\n> for comments from an area expert.\n\nOnly this made me realize that you are expecting *my* response.\n\n>> gitweb/gitweb.perl | 7 ++++++-\n>> 1 file changed, 6 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index 05d7910..f7f7936 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -1423,7 +1423,12 @@ sub href {\n>> delete $params{'hash'};\n>> delete $params{'hash_base'};\n>> } elsif (defined $params{'hash'}) {\n>> - $href .= esc_path_info($params{'hash'});\n>> + if (defined $params{'hash_parent'}) {\n>> + $href .= esc_path_info($params{'hash_parent'});\n>> + delete $params{'hash_parent'};\n>> + } else {\n>> + $href .= esc_path_info($params{'hash'});\n>> + }\n\nThis should read _something_ like this\n\n+ if (defined $params{'hash_parent'}) {\n+ $href .=\nesc_path_info($params{'hash_parent'}) . '..';\n+ delete $params{'hash_parent'};\n+ }\n+ $href .= esc_path_info($params{'hash'});\n+ delete $params{'hash'};\n\nOtherwise you would get correct link in your situation with\nbad 'hash' parameter, but not the view that was requested;\nit would not be diff between current and given parent, but\ncommitdiff for parent (to grandparent(s)).\n\nRichard, thanks for finding a problematic thing, but you\nneed to search more for a true fix.\n\nRegards\n-- \nJakub Narebski\n"},{"id":"287520","messageId":"20160525222032.GA18076@shattrath","threadId":"42235","inReplyTo":"CANQwDwdKaVDGDxZZma25WcV+ea90YtFhDedLwv2KNZt6jhKVAQ@mail.gmail.com","subject":"Re: [PATCH] gitweb: fix link to parent diff with pathinfo","fromName":"Richard Braun","fromEmail":"rbraun@sceen.net","sentAt":"2016-05-25T22:20:32Z","receivedAt":"2016-05-25T22:20:32Z","isPatch":true,"sender":{"key":"rbraun@sceen.net","avatar":null},"body":"On Wed, May 25, 2016 at 10:33:32PM +0200, Jakub Narębski wrote:\n> Richard, thanks for finding a problematic thing, but you\n> need to search more for a true fix.\n\nNoted, I'll get back to you soon (hopefully not too late).\n\n-- \nRichard Braun\n"}]}