{"thread":{"id":"13779","subject":"[PATCH] Avoid errors from git-rev-parse in gitweb blame (take 2)","startedAt":"2008-06-03T12:58:10Z","lastAt":"2008-06-03T14:59:08Z","messageCount":3,"participants":["Rafael Garcia-Suarez","Lea Wiemann"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"78491","messageId":"1212497890-4101-1-git-send-email-rgarciasuarez@gmail.com","threadId":"13779","inReplyTo":null,"subject":"[PATCH] Avoid errors from git-rev-parse in gitweb blame (take 2)","fromName":"Rafael Garcia-Suarez","fromEmail":"rgarciasuarez@gmail.com","sentAt":"2008-06-03T12:58:10Z","receivedAt":"2008-06-03T12:58:10Z","isPatch":true,"sender":{"key":"rgarciasuarez@gmail.com","avatar":null},"body":"git-rev-parse will abort with an error message on stderr when passed a\nnon-existent revision spec, such as \"deadbeef^\" where deadbeef has no\nparent. Using the --revs-only parameter makes this error go away, while\nretaining functionality, keeping the web server error log nice and clean.\n\nMoreover, when there is no parent commit, direct the blame at the first\ncommit featuring the file, that is itself. This unbreaks the link on the\nline number when the corresponding line had never been modified.\n\nFinally, to avoid forking git-rev-parse too many times, cache its\nresults in a new hash %parent_commits.\n\nSigned-off-by: Rafael Garcia-Suarez <rgarciasuarez@gmail.com>\n---\n gitweb/gitweb.perl |   16 +++++++++++-----\n 1 files changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 55fb100..828cf45 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -4189,6 +4189,7 @@ sub git_blame2 {\n <tr><th>Commit</th><th>Line</th><th>Data</th></tr>\n HTML\n \tmy %metainfo = ();\n+\tmy %parent_commits = ();\n \twhile (1) {\n \t\t$_ = <$fd>;\n \t\tlast unless defined $_;\n@@ -4226,11 +4227,16 @@ HTML\n \t\t\t              esc_html($rev));\n \t\t\tprint \"</td>\\n\";\n \t\t}\n-\t\topen (my $dd, \"-|\", git_cmd(), \"rev-parse\", \"$full_rev^\")\n-\t\t\tor die_error(undef, \"Open git-rev-parse failed\");\n-\t\tmy $parent_commit = <$dd>;\n-\t\tclose $dd;\n-\t\tchomp($parent_commit);\n+\t\tif (!exists $parent_commits{$full_rev}) {\n+\t\t    # --revs-only avoids an error message if $full_rev has no parent\n+\t\t    open (my $dd, \"-|\", git_cmd(), \"rev-parse\", '--revs-only', \"$full_rev^\")\n+\t\t\t    or die_error(undef, \"Open git-rev-parse failed\");\n+\t\t    # set the $parent_commit to $full_rev if it has no parent\n+\t\t    $parent_commits{$full_rev} = <$dd> || $full_rev;\n+\t\t    chomp($parent_commits{$full_rev});\n+\t\t    close $dd;\n+\t\t}\n+\t\tmy $parent_commit = $parent_commits{$full_rev};\n \t\tmy $blamed = href(action => 'blame',\n \t\t                  file_name => $meta->{'filename'},\n \t\t                  hash_base => $parent_commit);\n-- \n1.5.6.rc1\n"},{"id":"78504","messageId":"484553E8.4050007@gmail.com","threadId":"13779","inReplyTo":"1212497890-4101-1-git-send-email-rgarciasuarez@gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame (take 2)","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-03T14:23:36Z","receivedAt":"2008-06-03T14:23:36Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Rafael Garcia-Suarez wrote:\n> Finally, to avoid forking git-rev-parse too many times, cache its\n> results in a new hash %parent_commits.\n\nI'm not too happy with this:\n\n1) Minor point: I'm working on caching for the backend right now (IOW, \nbasically what you're doing, just centralized in a separate module), so \nyou're essentially duplicating work, and you're making it (a little) \nharder for me to refactor gitweb since I have to rip out your cache \ncode.  Those few lines won't hurt, but in general I suggest that nobody \nmake any larger efforts to cache stuff in gitweb for the next few weeks.\n\n2) Major point: You're still forking a lot.  The Right Thing is to \ncondense everything into a single call -- I believe \"git-rev-list \n--parents --no-walk hash hash hash...\" is correct and easily parsable. \nIts output seems to be lines of\n     hash parent_1 parent_2 ... parent_n\nwith n >= 0.  Can you implement that?  It would be very useful and also \nreusable for me!\n\n-- Lea\n\nP.S.: I believe that the usual way to post follow-up patches is to label \nthem [PATCH vN] for N >= 2 in the subject (since \"take 2\" shouldn't be \npart of the commit message), and to send them as In-reply-to a message \nin the original thread -- just provide git-send-email with the \nMessage-ID of the message you want to reply to.  </nitpick>\n"},{"id":"78513","messageId":"b77c1dce0806030759w48045055if7636e950b14c977@mail.gmail.com","threadId":"13779","inReplyTo":"484553E8.4050007@gmail.com","subject":"Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame (take 2)","fromName":"Rafael Garcia-Suarez","fromEmail":"rgarciasuarez@gmail.com","sentAt":"2008-06-03T14:59:08Z","receivedAt":"2008-06-03T14:59:08Z","isPatch":true,"sender":{"key":"rgarciasuarez@gmail.com","avatar":null},"body":"2008/6/3 Lea Wiemann <lewiemann@gmail.com>:\n> Rafael Garcia-Suarez wrote:\n>>\n>> Finally, to avoid forking git-rev-parse too many times, cache its\n>> results in a new hash %parent_commits.\n>\n> I'm not too happy with this:\n>\n> 1) Minor point: I'm working on caching for the backend right now (IOW,\n> basically what you're doing, just centralized in a separate module), so\n> you're essentially duplicating work, and you're making it (a little) harder\n> for me to refactor gitweb since I have to rip out your cache code.  Those\n> few lines won't hurt, but in general I suggest that nobody make any larger\n> efforts to cache stuff in gitweb for the next few weeks.\n\nSorry for that. I failed to consider that your work was applying there as well.\n\n> 2) Major point: You're still forking a lot.  The Right Thing is to condense\n> everything into a single call -- I believe \"git-rev-list --parents --no-walk\n> hash hash hash...\" is correct and easily parsable. Its output seems to be\n> lines of\n>    hash parent_1 parent_2 ... parent_n\n> with n >= 0.  Can you implement that?  It would be very useful and also\n> reusable for me!\n\nOk, I see what you mean -- but I suppose that's the kind of\ninformation that would be nicely abstracted out somewhere. At least I\nwould put it in a sub. And that sub would fit perfectly in your\ncaching module, too! Looking at the source, a generic git-rev-list\nwrapper could be used.\n\nIf you want I can produce a patch against your version, but I'm afraid\nI'll end up being a bit short on time, so I might be slow to do it.\n\n> P.S.: I believe that the usual way to post follow-up patches is to label\n> them [PATCH vN] for N >= 2 in the subject (since \"take 2\" shouldn't be part\n> of the commit message), and to send them as In-reply-to a message in the\n> original thread -- just provide git-send-email with the Message-ID of the\n> message you want to reply to.  </nitpick>\n>\n\nOk, noted. Also, next time, I'll send smaller patches.\n"}]}