{"thread":{"id":"33443","subject":"[PATCH 4/4] gitweb: Fix broken blob action parameters on blob/commitdiff pages","startedAt":"2013-04-08T20:10:21Z","lastAt":"2013-04-09T20:17:09Z","messageCount":3,"participants":["Jürgen Kreileder","Jakub Narębski"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"213601","messageId":"m2vc7wbx76.fsf@blackdown.de","threadId":"33443","inReplyTo":null,"subject":"[PATCH 4/4] gitweb: Fix broken blob action parameters on blob/commitdiff pages","fromName":"Jürgen Kreileder","fromEmail":"jk@blackdown.de","sentAt":"2013-04-08T20:10:21Z","receivedAt":"2013-04-08T20:10:21Z","isPatch":true,"sender":{"key":"jk@blackdown.de","avatar":"https://avatars.githubusercontent.com/u/4390544?v=4"},"body":"Fix broken blob action parameters on blobdiff and commitdiff pages by\nexplicitly passing variables instead of relying on global ones.\n\n(The broken parameters on blob links lead to blob pages which show the\nblob but with a hash instead of a commit message and have broken\nblob_plain (404 - Cannot find file) and tree links (404 - Reading tree\nfailed))\n\nSigned-off-by: Jürgen Kreileder <jk@blackdown.de>\n---\n gitweb/gitweb.perl |   15 ++++++++-------\n 1 file changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 1309196..995e8fb 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2258,7 +2258,7 @@ sub format_extended_diff_header_line {\n \n # format from-file/to-file diff header\n sub format_diff_from_to_header {\n-\tmy ($from_line, $to_line, $diffinfo, $from, $to, @parents) = @_;\n+\tmy ($from_line, $to_line, $diffinfo, $from, $to, $hash, @parents) = @_;\n \tmy $line;\n \tmy $result = '';\n \n@@ -2324,7 +2324,7 @@ sub format_diff_from_to_header {\n \n # create note for patch simplified by combined diff\n sub format_diff_cc_simplified {\n-\tmy ($diffinfo, @parents) = @_;\n+\tmy ($diffinfo, $hash, @parents) = @_;\n \tmy $result = '';\n \n \t$result .= \"<div class=\\\"diff header\\\">\" .\n@@ -3592,7 +3592,8 @@ sub parse_ls_tree_line {\n \n # generates _two_ hashes, references to which are passed as 2 and 3 argument\n sub parse_from_to_diffinfo {\n-\tmy ($diffinfo, $from, $to, @parents) = @_;\n+\tmy ($diffinfo, $from, $to, $hash, @parents) = @_;\n+\tmy ($hash_parent) = $parents[0];\n \n \tif ($diffinfo->{'nparents'}) {\n \t\t# combined diff\n@@ -5312,7 +5313,7 @@ sub git_patchset_body {\n \t\t\tif ($is_combined) {\n \t\t\t\twhile ($to_name ne $diffinfo->{'to_file'}) {\n \t\t\t\t\tprint \"<div class=\\\"patch\\\" id=\\\"patch\". ($patch_idx+1) .\"\\\">\\n\" .\n-\t\t\t\t\t      format_diff_cc_simplified($diffinfo, @hash_parents) .\n+\t\t\t\t\t      format_diff_cc_simplified($diffinfo, $hash, @hash_parents) .\n \t\t\t\t\t      \"</div>\\n\";  # class=\"patch\"\n \n \t\t\t\t\t$patch_idx++;\n@@ -5324,7 +5325,7 @@ sub git_patchset_body {\n \t\t\t}\n \n \t\t\t# modifies %from, %to hashes\n-\t\t\tparse_from_to_diffinfo($diffinfo, \\%from, \\%to, @hash_parents);\n+\t\t\tparse_from_to_diffinfo($diffinfo, \\%from, \\%to, $hash, @hash_parents);\n \n \t\t\t# this is first patch for raw difftree line with $patch_idx index\n \t\t\t# we index @$difftree array from 0, but number patches from 1\n@@ -5367,7 +5368,7 @@ sub git_patchset_body {\n \n \t\tprint format_diff_from_to_header($last_patch_line, $patch_line,\n \t\t                                 $diffinfo, \\%from, \\%to,\n-\t\t                                 @hash_parents);\n+\t\t                                 $hash, @hash_parents);\n \n \t\t# the patch itself\n \tLINE:\n@@ -5404,7 +5405,7 @@ sub git_patchset_body {\n \n \t\t# generate anchor for \"patch\" links in difftree / whatchanged part\n \t\tprint \"<div class=\\\"patch\\\" id=\\\"patch\". ($patch_idx+1) .\"\\\">\\n\" .\n-\t\t      format_diff_cc_simplified($diffinfo, @hash_parents) .\n+\t\t      format_diff_cc_simplified($diffinfo, $hash, @hash_parents) .\n \t\t      \"</div>\\n\";  # class=\"patch\"\n \n \t\t$patch_number++;\n-- \n1.7.10.4\n"},{"id":"213666","messageId":"51643307.9090603@gmail.com","threadId":"33443","inReplyTo":"m2vc7wbx76.fsf@blackdown.de","subject":"Re: [PATCH 4/4] gitweb: Fix broken blob action parameters on blob/commitdiff pages","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2013-04-09T15:25:59Z","receivedAt":"2013-04-09T15:25:59Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 08.04.2013 22:10, Jürgen Kreileder pisze:\n\n> Fix broken blob action parameters on blobdiff and commitdiff pages by\n> explicitly passing variables instead of relying on global ones.\n\nDo I understand it correctly that those variables (e.g. $hash variable\nin git_patchset_body in second chunk below, after this change passed\nas parameter to format_diff_cc_simplified()) can be filled in then,\nor at least adjusted correctly?\n\n> (The broken parameters on blob links lead to blob pages which show the\n> blob but with a hash instead of a commit message and have broken\n> blob_plain (404 - Cannot find file) and tree links (404 - Reading tree\n> failed))\n> \n> Signed-off-by: Jürgen Kreileder <jk@blackdown.de>\n\nI wonder how we missed this.  Does this happen always, or in some\nspecific conditions?\n\nAnyway, this change is a good change.  Internal subroutines should not\nuse global variables.\n\nI hope that in the future we would get rid of most global variables...\n\nAcked-by: Jakub Narebski <jnareb@gmail.com>\n\n[not tested]\n\n>  # create note for patch simplified by combined diff\n>  sub format_diff_cc_simplified {\n> -\tmy ($diffinfo, @parents) = @_;\n> +\tmy ($diffinfo, $hash, @parents) = @_;\n>  \tmy $result = '';\n>  \n>  \t$result .= \"<div class=\\\"diff header\\\">\" .\n[...]\n> @@ -5404,7 +5405,7 @@ sub git_patchset_body {\n>  \n>  \t\t# generate anchor for \"patch\" links in difftree / whatchanged part\n>  \t\tprint \"<div class=\\\"patch\\\" id=\\\"patch\". ($patch_idx+1) .\"\\\">\\n\" .\n> -\t\t      format_diff_cc_simplified($diffinfo, @hash_parents) .\n> +\t\t      format_diff_cc_simplified($diffinfo, $hash, @hash_parents) .\n>  \t\t      \"</div>\\n\";  # class=\"patch\"\n>  \n>  \t\t$patch_number++;\n> \n\n\n-- \nJakub Narębski\n"},{"id":"213712","messageId":"m2d2u3a27u.fsf@zahir.fritz.box","threadId":"33443","inReplyTo":"51643307.9090603@gmail.com","subject":"Re: [PATCH 4/4] gitweb: Fix broken blob action parameters on blob/commitdiff pages","fromName":"Jürgen Kreileder","fromEmail":"jk@blackdown.de","sentAt":"2013-04-09T20:17:09Z","receivedAt":"2013-04-09T20:17:09Z","isPatch":true,"sender":{"key":"jk@blackdown.de","avatar":"https://avatars.githubusercontent.com/u/4390544?v=4"},"body":"Jakub Narębski <jnareb@gmail.com> writes:\n\n> W dniu 08.04.2013 22:10, Jürgen Kreileder pisze:\n>\n>> Fix broken blob action parameters on blobdiff and commitdiff pages by\n>> explicitly passing variables instead of relying on global ones.\n>\n> Do I understand it correctly that those variables (e.g. $hash variable\n> in git_patchset_body in second chunk below, after this change passed\n> as parameter to format_diff_cc_simplified()) can be filled in then,\n> or at least adjusted correctly?\n\nYep.\n\n>> (The broken parameters on blob links lead to blob pages which show the\n>> blob but with a hash instead of a commit message and have broken\n>> blob_plain (404 - Cannot find file) and tree links (404 - Reading tree\n>> failed))\n>\n> I wonder how we missed this.  Does this happen always, or in some\n> specific conditions?\n\nJust having a hash in the diff header and missing navigation links for\nlhs links in diffs happen pretty much all the time:\nE.g. follow any a/... link on http://repo.or.cz/w/alt-git.git/commitdiff/5bda18c186e455f8e65f976d3bf333ab1f4b5b53\n\n(Btw, on\nhttp://repo.or.cz/w/alt-git.git/blobdiff/6466fbbeef67ec398f6f5d5b5da5d6f107a647b2..5bda18c186e455f8e65f976d3bf333ab1f4b5b53:/GIT-VERSION-GEN\nboth the a/ und b/ link are broken.  No idea if my patch fixes that, I\ndon't use pathinfo because I've had similar problems with it when I\ntested it a year ago).\n\nBroken a/ and b/ + broken tree + broken blob_plain links are more\nrare.  I see them on\nhttps://git.blackdown.de/old.cgi?p=contactalbum.git;a=blobdiff;f=Classes/ContactAlbum.h;h=8f2b6a772ba0a4530e0c441db53ae5bbc0f2e277;hp=03c43ea01a3a88bbce3c73df9bfde64ee7a13717;hb=cc4eaa64c2b399dd9bdbf1f67f6d621aa24df5f8;hpb=43ff97d2c247517f43d83314aaa215e9f61d5d0c;js=1\nfor example:\nWhen following either a/ or b/, all links in \"[contactalbum.git] /\nClasses / ContactAlbum.h\" are broken.\n\nWith my patch I don't see these problems:\nhttps://git.blackdown.de/?p=contactalbum.git;a=blobdiff;f=Classes/ContactAlbum.h;h=8f2b6a772ba0a4530e0c441db53ae5bbc0f2e277;hp=03c43ea01a3a88bbce3c73df9bfde64ee7a13717;hb=cc4eaa64c2b399dd9bdbf1f67f6d621aa24df5f8;hpb=43ff97d2c247517f43d83314aaa215e9f61d5d0c;js=1\n\n\n        Juergen\n"}]}