{"thread":{"id":"16659","subject":"[PATCH 0/3] gitweb: Improve git_blame in preparation for incremental blame","startedAt":"2008-12-09T22:43:23Z","lastAt":"2008-12-17T08:34:55Z","messageCount":31,"participants":["Jakub Narebski","Nanako Shiraishi","Luben Tuikov","Junio C Hamano","Petr Baudis"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"97457","messageId":"20081209223703.28106.29198.stgit@localhost.localdomain","threadId":"16659","inReplyTo":null,"subject":"[PATCH 0/3] gitweb: Improve git_blame in preparation for incremental blame","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-09T22:43:23Z","receivedAt":"2008-12-09T22:43:23Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"The following series implements a few improvements to git_blame code\nand 'blame' view output to prepare for WIP/RFC patch adding incremental\nblame output to gitweb using AJAX (JavaScript and XMLHttpRequest); the\ncode in question is based on code by Petr Baudis from 26 Aug 2007\n http://permalink.gmane.org/gmane.comp.version-control.git/56657\nwhich in turn was based on Fredrik Kuivinen proof of concept patch.\n\n\nThe first patch in series (moving id to tr element) is needed in\nblame_incremental, and it makes it easier to use DOM to manipulate\ngitweb blame output.\n\nSecond patch is about what I have noticed when examining git_blame\ncode.\n\nThe last patch is not necessarily required; but please tell me if it\nis to be accepted or to be dropped, to know whether to base\nincremental blame on it.\n\n---\nJakub Narebski (3):\n gitweb: A bit of code cleanup in git_blame()\n gitweb: Cache $parent_commit info in git_blame()\n gitweb: Move 'lineno' id from link to row element in git_blame\n\n gitweb/gitweb.perl | 84 ++++++++++++++++++++++++++++++----------------------\n 1 files changed, 49 insertions(+), 35 deletions(-)\n\n-- \nJakub Narebski\nShadeHawk on #git\nPoland\n"},{"id":"97458","messageId":"20081209224330.28106.18301.stgit@localhost.localdomain","threadId":"16659","inReplyTo":"20081209223703.28106.29198.stgit@localhost.localdomain","subject":"[PATCH 1/3] gitweb: Move 'lineno' id from link to row element in git_blame","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-09T22:46:16Z","receivedAt":"2008-12-09T22:46:16Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Move l ID from link element inside table row (inside\ncell element for column with line numbers), to encompassing table\nrow element. It was done to make it easier to manipulate result HTML\nwith DOM, and to be able write 'blame_incremental' view with the same,\nor nearly the same result.\n\nSigned-off-by: Jakub Narebski \n---\nFor blame_incremental I need easy way to manipulate rows of blame\ntable, to add information about blamed commits as it arrives.\n\nSo there it is.\n\n gitweb/gitweb.perl | 3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 6eb370d..1b800f4 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -4645,7 +4645,7 @@ HTML\n \t\tif ($group_size) {\n \t\t\t$current_color = ++$current_color % $num_colors;\n \t\t}\n-\t\tprint \"\\n\";\n+\t\tprint \"\\n\";\n \t\tif ($group_size) {\n \t\t\tprint \" $parent_commit);\n \t\tprint \"\";\n \t\tprint $cgi->a({ -href => \"$blamed#l$orig_lineno\",\n-\t\t -id => \"l$lineno\",\n \t\t -class => \"linenr\" },\n \t\t esc_html($lineno));\n \t\tprint \"\";\n"},{"id":"97459","messageId":"20081209224622.28106.89325.stgit@localhost.localdomain","threadId":"16659","inReplyTo":"20081209223703.28106.29198.stgit@localhost.localdomain","subject":"[PATCH 2/3] gitweb: Cache $parent_commit info in git_blame()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-09T22:48:08Z","receivedAt":"2008-12-09T22:48:08Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Luben Tuikov changed 'lineno' link from leading to commit which lead\nto current version of given block of lines, to leading to parent of\nthis commit in 244a70e (Blame \"linenr\" link jumps to previous state at\n\"orig_lineno\"). This supposedly made data mining possible (or just\nbetter).\n\nUnfortunately the implementation in 244a70e used one call for\ngit-rev-parse to find parent revision per line in file, instead of\nusing long lived \"git cat-file --batch-check\" (which might not existed\nthen), or changing validate_refname to validate_revision and made it\naccept ^, ^^, ^^^ etc. syntax.\n\nThis patch attempts to migitate issue a bit by caching $parent_commit\ninfo in %metainfo, which makes gitweb to call git-rev-parse only once\nper unique commit in blame output.\n\nSigned-off-by: Jakub Narebski \n---\nThat is what I have noticed during browsing git_blame() code.\n\nWe can change it to even more effective implementation (like the ones\nproposed above in the commit message) later.\n\nIndenting is cause for artifically large diff\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 1b800f4..916396a 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -4657,11 +4657,17 @@ HTML\n \t\t\t esc_html($rev));\n \t\t\tprint \"\\n\";\n \t\t}\n-\t\topen (my $dd, \"-|\", git_cmd(), \"rev-parse\", \"$full_rev^\")\n-\t\t\tor die_error(500, \"Open git-rev-parse failed\");\n-\t\tmy $parent_commit = <$dd>;\n-\t\tclose $dd;\n-\t\tchomp($parent_commit);\n+\t\tmy $parent_commit;\n+\t\tif (!exists $meta->{'parent'}) {\n+\t\t\topen (my $dd, \"-|\", git_cmd(), \"rev-parse\", \"$full_rev^\")\n+\t\t\t\tor die_error(500, \"Open git-rev-parse failed\");\n+\t\t\t$parent_commit = <$dd>;\n+\t\t\tclose $dd;\n+\t\t\tchomp($parent_commit);\n+\t\t\t$meta->{'parent'} = $parent_commit;\n+\t\t} else {\n+\t\t\t$parent_commit = $meta->{'parent'};\n+\t\t}\n \t\tmy $blamed = href(action => 'blame',\n \t\t file_name => $meta->{'filename'},\n \t\t hash_base => $parent_commit);\n"},{"id":"97460","messageId":"20081209224814.28106.83387.stgit@localhost.localdomain","threadId":"16659","inReplyTo":"20081209223703.28106.29198.stgit@localhost.localdomain","subject":"[PATCH 3/3] gitweb: A bit of code cleanup in git_blame()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-09T22:48:51Z","receivedAt":"2008-12-09T22:48:51Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Among others: \n * move variable declaration closer to the place it is set and used,\n if possible,\n * uniquify and simplify coding style a bit, which includes removing\n unnecessary '()'.\n * check type only if $hash was defined, as otherwise from the way\n git_get_hash_by_path() is called (and works), we know that it is\n a blob,\n * use modern calling convention for git-blame,\n * remove unused variable,\n * don't use implicit variables ($_),\n * add some comments\n\nSigned-off-by: Jakub Narebski \n---\nNot stricly necessary... but the code looked not very nice\n\n gitweb/gitweb.perl | 65 ++++++++++++++++++++++++++++++----------------------\n 1 files changed, 37 insertions(+), 28 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 916396a..68aa3f8 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -4575,28 +4575,32 @@ sub git_tag {\n }\n \n sub git_blame {\n-\tmy $fd;\n-\tmy $ftype;\n-\n+\t# permissions\n \tgitweb_check_feature('blame')\n-\t or die_error(403, \"Blame view not allowed\");\n+\t\tor die_error(403, \"Blame view not allowed\");\n \n+\t# error checking\n \tdie_error(400, \"No file name given\") unless $file_name;\n \t$hash_base ||= git_get_head_hash($project);\n-\tdie_error(404, \"Couldn't find base commit\") unless ($hash_base);\n+\tdie_error(404, \"Couldn't find base commit\") unless $hash_base;\n \tmy %co = parse_commit($hash_base)\n \t\tor die_error(404, \"Commit not found\");\n \tif (!defined $hash) {\n \t\t$hash = git_get_hash_by_path($hash_base, $file_name, \"blob\")\n \t\t\tor die_error(404, \"Error looking up file\");\n+\t} else {\n+\t\tmy $ftype = git_get_type($hash);\n+\t\tif ($ftype !~ \"blob\") {\n+\t\t\tdie_error(400, \"Object is not a blob\");\n+\t\t}\n \t}\n-\t$ftype = git_get_type($hash);\n-\tif ($ftype !~ \"blob\") {\n-\t\tdie_error(400, \"Object is not a blob\");\n-\t}\n-\topen ($fd, \"-|\", git_cmd(), \"blame\", '-p', '--',\n-\t $file_name, $hash_base)\n+\n+\t# run git-blame --porcelain\n+\topen my $fd, \"-|\", git_cmd(), \"blame\", '-p',\n+\t\t$hash_base, '--', $file_name\n \t\tor die_error(500, \"Open git-blame failed\");\n+\n+\t# page header\n \tgit_header_html();\n \tmy $formats_nav =\n \t\t$cgi->a({-href => href(action=>\"blob\", -replay=>1)},\n@@ -4610,40 +4614,43 @@ sub git_blame {\n \tgit_print_page_nav('','', $hash_base,$co{'tree'},$hash_base, $formats_nav);\n \tgit_print_header_div('commit', esc_html($co{'title'}), $hash_base);\n \tgit_print_page_path($file_name, $ftype, $hash_base);\n-\tmy @rev_color = (qw(light2 dark2));\n+\n+\t# page body\n+\tmy @rev_color = qw(light2 dark2);\n \tmy $num_colors = scalar(@rev_color);\n \tmy $current_color = 0;\n-\tmy $last_rev;\n+\tmy %metainfo = ();\n+\n \tprint <\n \n \n HTML\n-\tmy %metainfo = ();\n-\twhile (1) {\n-\t\t$_ = <$fd>;\n-\t\tlast unless defined $_;\n+ LINE:\n+\twhile (my $line = <$fd>) {\n+\t\tchomp $line;\n+\t\t# the header: []\n+\t\t# no for subsequent lines in group of lines\n \t\tmy ($full_rev, $orig_lineno, $lineno, $group_size) =\n-\t\t /^([0-9a-f]{40}) (\\d+) (\\d+)(?: (\\d+))?$/;\n+\t\t ($line =~ /^([0-9a-f]{40}) (\\d+) (\\d+)(?: (\\d+))?$/);\n \t\tif (!exists $metainfo{$full_rev}) {\n \t\t\t$metainfo{$full_rev} = {};\n \t\t}\n \t\tmy $meta = $metainfo{$full_rev};\n-\t\twhile (<$fd>) {\n-\t\t\tlast if (s/^\\t//);\n-\t\t\tif (/^(\\S+) (.*)$/) {\n+\t\twhile (my $data = <$fd>) {\n+\t\t\tchomp $data;\n+\t\t\tlast if ($data =~ s/^\\t//); # contents of line\n+\t\t\tif ($data =~ /^(\\S+) (.*)$/) {\n \t\t\t\t$meta->{$1} = $2;\n \t\t\t}\n \t\t}\n-\t\tmy $data = $_;\n-\t\tchomp $data;\n-\t\tmy $rev = substr($full_rev, 0, 8);\n+\t\tmy $short_rev = substr($full_rev, 0, 8);\n \t\tmy $author = $meta->{'author'};\n-\t\tmy %date = parse_date($meta->{'author-time'},\n-\t\t $meta->{'author-tz'});\n+\t\tmy %date =\n+\t\t\tparse_date($meta->{'author-time'}, $meta->{'author-tz'});\n \t\tmy $date = $date{'iso-tz'};\n \t\tif ($group_size) {\n-\t\t\t$current_color = ++$current_color % $num_colors;\n+\t\t\t$current_color = ($current_color + 1) % $num_colors;\n \t\t}\n \t\tprint \"\\n\";\n \t\tif ($group_size) {\n@@ -4654,7 +4661,7 @@ HTML\n \t\t\tprint $cgi->a({-href => href(action=>\"commit\",\n \t\t\t hash=>$full_rev,\n \t\t\t file_name=>$file_name)},\n-\t\t\t esc_html($rev));\n+\t\t\t esc_html($short_rev));\n \t\t\tprint \"\\n\";\n \t\t}\n \t\tmy $parent_commit;\n@@ -4683,6 +4690,8 @@ HTML\n \tprint \"\";\n \tclose $fd\n \t\tor print \"Reading blob failed\\n\";\n+\n+\t# page footer\n \tgit_footer_html();\n }\n \n"},{"id":"97464","messageId":"ghn8jv$hg9$1@ger.gmane.org","threadId":"16659","inReplyTo":"20081209224814.28106.83387.stgit@localhost.localdomain","subject":"Re: [PATCH 3/3] gitweb: A bit of code cleanup in git_blame()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-10T02:13:24Z","receivedAt":"2008-12-10T02:13:24Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jakub Narebski wrote:\n\nI'm sorry, there should be\n\n + my $ftype = \"blob\";\n>         if (!defined $hash) {\n>                 $hash = git_get_hash_by_path($hash_base, $file_name, \"blob\")\n>                         or die_error(404, \"Error looking up file\");\n> +       } else {\n> +               $ftype = git_get_type($hash);\n> +               if ($ftype !~ \"blob\") {\n> +                       die_error(400, \"Object is not a blob\");\n> +               }\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"97465","messageId":"20081210124901.6117@nanako3.lavabit.com","threadId":"16659","inReplyTo":"20081209224622.28106.89325.stgit@localhost.localdomain","subject":"Re: [PATCH 2/3] gitweb: Cache $parent_commit info in git_blame()","fromName":"Nanako Shiraishi","fromEmail":"nanako3@lavabit.com","sentAt":"2008-12-10T03:49:01Z","receivedAt":"2008-12-10T03:49:01Z","isPatch":true,"sender":{"key":"nanako3@lavabit.com","avatar":"https://gravatar.com/avatar/3777b9e201c5883a62b1a6fdf7c53f2d712d1d80989146063ea861e33aad72a8?d=mp&s=160"},"body":"Quoting Jakub Narebski :\n\n> Unfortunately the implementation in 244a70e used one call for\n> git-rev-parse to find parent revision per line in file, instead of\n> using long lived \"git cat-file --batch-check\" (which might not existed\n> then), or changing validate_refname to validate_revision and made it\n> accept ^, ^^, ^^^ etc. syntax.\n\nCould you substantiate why this is \"Unfortunate\"? Is the new implementation faster? By how much?\n\nWhen \"previous\" commit information is available in the output from \"git blame\", can you make use of it?\n\n-- \nNanako Shiraishi\nhttp://ivory.ap.teacup.com/nanako3/\n"},{"id":"97466","messageId":"661413.30278.qm@web31807.mail.mud.yahoo.com","threadId":"16659","inReplyTo":"20081209224330.28106.18301.stgit@localhost.localdomain","subject":"Re: [PATCH 1/3] gitweb: Move 'lineno' id from link to row element in git_blame","fromName":"Luben Tuikov","fromEmail":"ltuikov@yahoo.com","sentAt":"2008-12-10T05:55:18Z","receivedAt":"2008-12-10T05:55:18Z","isPatch":true,"sender":{"key":"ltuikov@yahoo.com","avatar":null},"body":"\n--- On Tue, 12/9/08, Jakub Narebski wrote:\n> From: Jakub Narebski \n> Subject: [PATCH 1/3] gitweb: Move 'lineno' id from link to row element in git_blame\n> To: git@vger.kernel.org\n> Cc: \"Luben Tuikov\" , \"Jakub Narebski\" \n> Date: Tuesday, December 9, 2008, 2:46 PM\n> Move l ID from link element\n> inside table row (inside\n> cell element for column with line numbers), to encompassing\n> table\n> row element. It was done to make it easier to manipulate\n> result HTML\n> with DOM, and to be able write 'blame_incremental'\n> view with the same,\n> or nearly the same result.\n> \n> Signed-off-by: Jakub Narebski \nAcked-by: Luben Tuikov \n\n Luben\n\n> ---\n> For blame_incremental I need easy way to manipulate rows of\n> blame\n> table, to add information about blamed commits as it\n> arrives.\n> \n> So there it is.\n> \n> gitweb/gitweb.perl | 3 +--\n> 1 files changed, 1 insertions(+), 2 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 6eb370d..1b800f4 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -4645,7 +4645,7 @@ HTML\n> \t\tif ($group_size) {\n> \t\t\t$current_color = ++$current_color % $num_colors;\n> \t\t}\n> -\t\tprint \" class=\\\"$rev_color[$current_color]\\\">\\n\";\n> +\t\tprint \" class=\\\"$rev_color[$current_color]\\\">\\n\";\n> \t\tif ($group_size) {\n> \t\t\tprint \" class=\\\"sha1\\\"\";\n> \t\t\tprint \" title=\\\"\". esc_html($author)\n> . \", $date\\\"\";\n> @@ -4667,7 +4667,6 @@ HTML\n> \t\t hash_base => $parent_commit);\n> \t\tprint \" class=\\\"linenr\\\">\";\n> \t\tprint $cgi->a({ -href =>\n> \"$blamed#l$orig_lineno\",\n> -\t\t -id => \"l$lineno\",\n> \t\t -class => \"linenr\" },\n> \t\t esc_html($lineno));\n> \t\tprint \"\";\n"},{"id":"97467","messageId":"182871.96175.qm@web31804.mail.mud.yahoo.com","threadId":"16659","inReplyTo":"20081209224622.28106.89325.stgit@localhost.localdomain","subject":"Re: [PATCH 2/3] gitweb: Cache $parent_commit info in git_blame()","fromName":"Luben Tuikov","fromEmail":"ltuikov@yahoo.com","sentAt":"2008-12-10T06:20:33Z","receivedAt":"2008-12-10T06:20:33Z","isPatch":true,"sender":{"key":"ltuikov@yahoo.com","avatar":null},"body":"\n--- On Tue, 12/9/08, Jakub Narebski wrote:\n> From: Jakub Narebski \n> Subject: [PATCH 2/3] gitweb: Cache $parent_commit info in git_blame()\n> To: git@vger.kernel.org\n> Cc: \"Luben Tuikov\" , \"Jakub Narebski\" \n> Date: Tuesday, December 9, 2008, 2:48 PM\n> Luben Tuikov changed 'lineno' link from leading to\n> commit which lead\n> to current version of given block of lines, to leading to\n> parent of\n> this commit in 244a70e (Blame \"linenr\" link jumps\n> to previous state at\n> \"orig_lineno\"). This supposedly made data mining\n> possible (or just\n> better).\n\nBefore 244a70e, clicking on linenr links would display\nthe same commit id as displayed to the left, which is no\ndifferent than the block of lines displayed, thus data\nmining was impossible, i.e. I had to manually (commands)\ngo back in history to see how this line or block of lines\ndeveloped and/or changed.\n\n244a70e didn't make data mining perfect, just possible.\n\n> This patch attempts to migitate issue a bit by caching\n> $parent_commit\n> info in %metainfo, which makes gitweb to call git-rev-parse\n> only once\n> per unique commit in blame output.\n\nHave you tested this patch that it gives the same commit chain\nas before it?\n\n Luben\n\n\n> \n> Signed-off-by: Jakub Narebski \n> ---\n> That is what I have noticed during browsing git_blame()\n> code.\n\nWhat?\n\n> We can change it to even more effective implementation\n> (like the ones\n> proposed above in the commit message) later.\n\nWhere?\n\n> \n> Indenting is cause for artifically large diff\n> \n> gitweb/gitweb.perl | 16 +++++++++++-----\n> 1 files changed, 11 insertions(+), 5 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 1b800f4..916396a 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -4657,11 +4657,17 @@ HTML\n> \t\t\t esc_html($rev));\n> \t\t\tprint \"\\n\";\n> \t\t}\n> -\t\topen (my $dd, \"-|\", git_cmd(),\n> \"rev-parse\", \"$full_rev^\")\n> -\t\t\tor die_error(500, \"Open git-rev-parse\n> failed\");\n> -\t\tmy $parent_commit = <$dd>;\n> -\t\tclose $dd;\n> -\t\tchomp($parent_commit);\n> +\t\tmy $parent_commit;\n> +\t\tif (!exists $meta->{'parent'}) {\n> +\t\t\topen (my $dd, \"-|\", git_cmd(),\n> \"rev-parse\", \"$full_rev^\")\n> +\t\t\t\tor die_error(500, \"Open git-rev-parse\n> failed\");\n> +\t\t\t$parent_commit = <$dd>;\n> +\t\t\tclose $dd;\n> +\t\t\tchomp($parent_commit);\n> +\t\t\t$meta->{'parent'} = $parent_commit;\n> +\t\t} else {\n> +\t\t\t$parent_commit = $meta->{'parent'};\n> +\t\t}\n> \t\tmy $blamed = href(action => 'blame',\n> \t\t file_name =>\n> $meta->{'filename'},\n> \t\t hash_base => $parent_commit);\n"},{"id":"97468","messageId":"910359.52983.qm@web31807.mail.mud.yahoo.com","threadId":"16659","inReplyTo":"20081209224814.28106.83387.stgit@localhost.localdomain","subject":"Re: [PATCH 3/3] gitweb: A bit of code cleanup in git_blame()","fromName":"Luben Tuikov","fromEmail":"ltuikov@yahoo.com","sentAt":"2008-12-10T06:24:49Z","receivedAt":"2008-12-10T06:24:49Z","isPatch":true,"sender":{"key":"ltuikov@yahoo.com","avatar":null},"body":"\n--- On Tue, 12/9/08, Jakub Narebski wrote:\n> From: Jakub Narebski \n> Subject: [PATCH 3/3] gitweb: A bit of code cleanup in git_blame()\n> To: git@vger.kernel.org\n> Cc: \"Luben Tuikov\" , \"Jakub Narebski\" \n> Date: Tuesday, December 9, 2008, 2:48 PM\n> Among others: \n> * move variable declaration closer to the place it is set\n> and used,\n> if possible,\n> * uniquify and simplify coding style a bit, which includes\n> removing\n> unnecessary '()'.\n> * check type only if $hash was defined, as otherwise from\n> the way\n> git_get_hash_by_path() is called (and works), we know\n> that it is\n> a blob,\n> * use modern calling convention for git-blame,\n> * remove unused variable,\n> * don't use implicit variables ($_),\n> * add some comments\n> \n> Signed-off-by: Jakub Narebski \nAcked-by: Luben Tuikov \n\nLooks good.\n\n> ---\n> Not stricly necessary... but the code looked not very nice\n> \n> gitweb/gitweb.perl | 65\n> ++++++++++++++++++++++++++++++----------------------\n> 1 files changed, 37 insertions(+), 28 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 916396a..68aa3f8 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -4575,28 +4575,32 @@ sub git_tag {\n> }\n> \n> sub git_blame {\n> -\tmy $fd;\n> -\tmy $ftype;\n> -\n> +\t# permissions\n> \tgitweb_check_feature('blame')\n> -\t or die_error(403, \"Blame view not\n> allowed\");\n> +\t\tor die_error(403, \"Blame view not allowed\");\n> \n> +\t# error checking\n> \tdie_error(400, \"No file name given\") unless\n> $file_name;\n> \t$hash_base ||= git_get_head_hash($project);\n> -\tdie_error(404, \"Couldn't find base commit\")\n> unless ($hash_base);\n> +\tdie_error(404, \"Couldn't find base commit\")\n> unless $hash_base;\n> \tmy %co = parse_commit($hash_base)\n> \t\tor die_error(404, \"Commit not found\");\n> \tif (!defined $hash) {\n> \t\t$hash = git_get_hash_by_path($hash_base, $file_name,\n> \"blob\")\n> \t\t\tor die_error(404, \"Error looking up file\");\n> +\t} else {\n> +\t\tmy $ftype = git_get_type($hash);\n> +\t\tif ($ftype !~ \"blob\") {\n> +\t\t\tdie_error(400, \"Object is not a blob\");\n> +\t\t}\n> \t}\n> -\t$ftype = git_get_type($hash);\n> -\tif ($ftype !~ \"blob\") {\n> -\t\tdie_error(400, \"Object is not a blob\");\n> -\t}\n> -\topen ($fd, \"-|\", git_cmd(), \"blame\",\n> '-p', '--',\n> -\t $file_name, $hash_base)\n> +\n> +\t# run git-blame --porcelain\n> +\topen my $fd, \"-|\", git_cmd(),\n> \"blame\", '-p',\n> +\t\t$hash_base, '--', $file_name\n> \t\tor die_error(500, \"Open git-blame failed\");\n> +\n> +\t# page header\n> \tgit_header_html();\n> \tmy $formats_nav =\n> \t\t$cgi->a({-href =>\n> href(action=>\"blob\", -replay=>1)},\n> @@ -4610,40 +4614,43 @@ sub git_blame {\n> \tgit_print_page_nav('','',\n> $hash_base,$co{'tree'},$hash_base, $formats_nav);\n> \tgit_print_header_div('commit',\n> esc_html($co{'title'}), $hash_base);\n> \tgit_print_page_path($file_name, $ftype, $hash_base);\n> -\tmy @rev_color = (qw(light2 dark2));\n> +\n> +\t# page body\n> +\tmy @rev_color = qw(light2 dark2);\n> \tmy $num_colors = scalar(@rev_color);\n> \tmy $current_color = 0;\n> -\tmy $last_rev;\n> +\tmy %metainfo = ();\n> +\n> \tprint <
\n>
CommitLineData
\n> \n> \n> HTML\n> -\tmy %metainfo = ();\n> -\twhile (1) {\n> -\t\t$_ = <$fd>;\n> -\t\tlast unless defined $_;\n> + LINE:\n> +\twhile (my $line = <$fd>) {\n> +\t\tchomp $line;\n> +\t\t# the header: lineno> []\n> +\t\t# no for subsequent lines in\n> group of lines\n> \t\tmy ($full_rev, $orig_lineno, $lineno, $group_size) =\n> -\t\t /^([0-9a-f]{40}) (\\d+) (\\d+)(?:\n> (\\d+))?$/;\n> +\t\t ($line =~ /^([0-9a-f]{40}) (\\d+) (\\d+)(?:\n> (\\d+))?$/);\n> \t\tif (!exists $metainfo{$full_rev}) {\n> \t\t\t$metainfo{$full_rev} = {};\n> \t\t}\n> \t\tmy $meta = $metainfo{$full_rev};\n> -\t\twhile (<$fd>) {\n> -\t\t\tlast if (s/^\\t//);\n> -\t\t\tif (/^(\\S+) (.*)$/) {\n> +\t\twhile (my $data = <$fd>) {\n> +\t\t\tchomp $data;\n> +\t\t\tlast if ($data =~ s/^\\t//); # contents of line\n> +\t\t\tif ($data =~ /^(\\S+) (.*)$/) {\n> \t\t\t\t$meta->{$1} = $2;\n> \t\t\t}\n> \t\t}\n> -\t\tmy $data = $_;\n> -\t\tchomp $data;\n> -\t\tmy $rev = substr($full_rev, 0, 8);\n> +\t\tmy $short_rev = substr($full_rev, 0, 8);\n> \t\tmy $author = $meta->{'author'};\n> -\t\tmy %date = parse_date($meta->{'author-time'},\n> -\t\t $meta->{'author-tz'});\n> +\t\tmy %date =\n> +\t\t\tparse_date($meta->{'author-time'},\n> $meta->{'author-tz'});\n> \t\tmy $date = $date{'iso-tz'};\n> \t\tif ($group_size) {\n> -\t\t\t$current_color = ++$current_color % $num_colors;\n> +\t\t\t$current_color = ($current_color + 1) % $num_colors;\n> \t\t}\n> \t\tprint \" class=\\\"$rev_color[$current_color]\\\">\\n\";\n> \t\tif ($group_size) {\n> @@ -4654,7 +4661,7 @@ HTML\n> \t\t\tprint $cgi->a({-href =>\n> href(action=>\"commit\",\n> \t\t\t hash=>$full_rev,\n> \t\t\t \n> file_name=>$file_name)},\n> -\t\t\t esc_html($rev));\n> +\t\t\t esc_html($short_rev));\n> \t\t\tprint \"\\n\";\n> \t\t}\n> \t\tmy $parent_commit;\n> @@ -4683,6 +4690,8 @@ HTML\n> \tprint \"\";\n> \tclose $fd\n> \t\tor print \"Reading blob failed\\n\";\n> +\n> +\t# page footer\n> \tgit_footer_html();\n> }\n"},{"id":"97474","messageId":"7voczk1l7q.fsf@gitster.siamese.dyndns.org","threadId":"16659","inReplyTo":"ghn8jv$hg9$1@ger.gmane.org","subject":"Re: [PATCH 3/3] gitweb: A bit of code cleanup in git_blame()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-10T08:35:37Z","receivedAt":"2008-12-10T08:35:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Jakub Narebski wrote:\n>\n> I'm sorry, there should be\n>\n> + my $ftype = \"blob\";\n>>         if (!defined $hash) {\n>>                 $hash = git_get_hash_by_path($hash_base, $file_name, \"blob\")\n>>                         or die_error(404, \"Error looking up file\");\n>> +       } else {\n>> +               $ftype = git_get_type($hash);\n>> +               if ($ftype !~ \"blob\") {\n>> +                       die_error(400, \"Object is not a blob\");\n>> +               }\n\nI will squash in the following and queue [1/3] and [3/3] to 'pu', as there\nseem to be a few comments on [2/3] that look worth addressing.\n\n gitweb/gitweb.perl | 6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git c/gitweb/gitweb.perl w/gitweb/gitweb.perl\nindex d491a1d..ccbf5d4 100755\n--- c/gitweb/gitweb.perl\n+++ w/gitweb/gitweb.perl\n@@ -4585,11 +4585,12 @@ sub git_blame {\n \tdie_error(404, \"Couldn't find base commit\") unless $hash_base;\n \tmy %co = parse_commit($hash_base)\n \t\tor die_error(404, \"Commit not found\");\n+\tmy $ftype = \"blob\";\n \tif (!defined $hash) {\n \t\t$hash = git_get_hash_by_path($hash_base, $file_name, \"blob\")\n \t\t\tor die_error(404, \"Error looking up file\");\n \t} else {\n-\t\tmy $ftype = git_get_type($hash);\n+\t\t$ftype = git_get_type($hash);\n \t\tif ($ftype !~ \"blob\") {\n \t\t\tdie_error(400, \"Object is not a blob\");\n \t\t}\n@@ -4637,7 +4638,8 @@ HTML\n \t\t\t$metainfo{$full_rev} = {};\n \t\t}\n \t\tmy $meta = $metainfo{$full_rev};\n-\t\twhile (my $data = <$fd>) {\n+\t\tmy $data;\n+\t\twhile ($data = <$fd>) {\n \t\t\tchomp $data;\n \t\t\tlast if ($data =~ s/^\\t//); # contents of line\n \t\t\tif ($data =~ /^(\\S+) (.*)$/) {\n"},{"id":"97484","messageId":"200812101439.18526.jnareb@gmail.com","threadId":"16659","inReplyTo":"20081210124901.6117@nanako3.lavabit.com","subject":"Re: [PATCH 2/3] gitweb: Cache $parent_commit info in git_blame()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-10T13:39:17Z","receivedAt":"2008-12-10T13:39:17Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 10 Dec 2008, Nanako Shiraishi wrote:\n> Quoting Jakub Narebski :\n> \n> > Unfortunately the implementation in 244a70e used one call for\n> > git-rev-parse to find parent revision per line in file, instead of\n> > using long lived \"git cat-file --batch-check\" (which might not existed\n> > then), or changing validate_refname to validate_revision and made it\n> > accept ^, ^^, ^^^ etc. syntax.\n> \n> Could you substantiate why this is \"Unfortunate\"?\n\nBecause it calls git-rev-parse once for _each line_, even if for lines\nin the group of neighbour lines blamed by same commit $parent_commit\nis the same, and even if you need to calculate $parent_commit only once\nper unique individual commit present in blame output.\n \n> Is the new implementation faster? By how much?\n\nFile | L[1] | C[2] || Time0[3] | Before[4] | After[4]\n====================================================================\nblob.h | 18 | 4 || 0m1.727s | 0m2.545s | 0m2.474s\nGIT-VERSION-GEN | 42 | 13 || 0m2.165s | 0m2.448s | 0m2.071s\nREADME | 46 | 6 || 0m1.593s | 0m2.727s | 0m2.242s\nrevision.c | 1923 | 121 || 0m2.357s | 0m30.365s | 0m7.028s\ngitweb/gitweb.perl | 6291 | 428 || 0m8.080s | 1m37.244s | 0m20.627s\n\nFile | L/C | Before/After\n=========================================\nblob.h | 4.5 | 1.03\nGIT-VERSION-GEN | 3.2 | 1.18\nREADME | 7.7 | 1.22\nrevision.c | 15.9 | 4.32\ngitweb/gitweb.perl | 14.7 | 4.71\n\nAs you can see the greater ratio of lines in file to unique commits\nin blame output, the greater gain from the new implementation.\n\nFootnotes:\n~~~~~~~~~~\n[1] Lines: \n $ wc -l \n[2] Individual commits in blame output:\n $ git blame -p | grep author-time | wc -l\n[3] Time for running \"git blame -p\" (user time, single run):\n $ time git blame -p >/dev/null\n[4] Time to run gitweb as Perl script from command line:\n $ gitweb-run.sh \"p=git.git;a=blame;f=\" > /dev/null 2>&1\n\nAppendix A:\n~~~~~~~~~~~\n#!/bin/bash\n\nexport GATEWAY_INTERFACE=\"CGI/1.1\"\nexport HTTP_ACCEPT=\"*/*\"\nexport REQUEST_METHOD=\"GET\"\nexport QUERY_STRING=\"\"$1\"\"\nexport PATH_INFO=\"\"$2\"\"\n\nexport GITWEB_CONFIG=\"/home/jnareb/git/gitweb/gitweb_config.perl\"\n\nperl -- /home/jnareb/git/gitweb/gitweb.perl\n\n# end of gitweb-run.sh\n\n\n> When \"previous\" commit information is available in the output from\n> \"git blame\", can you make use of it? \n\nYes, I could; but I don't think it got implemented.\n-- \nJakub Narebski\nPoland\n"},{"id":"97492","messageId":"200812101615.55340.jnareb@gmail.com","threadId":"16659","inReplyTo":"182871.96175.qm@web31804.mail.mud.yahoo.com","subject":"Re: [PATCH 2/3] gitweb: Cache $parent_commit info in git_blame()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-10T15:15:54Z","receivedAt":"2008-12-10T15:15:54Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 10 Dec 2008, Luben Tuikov wrote:\n> --- On Tue, 12/9/08, Jakub Narebski wrote:\n\n> > This patch attempts to migitate issue [of performance] a bit by\n> > caching $parent_commit info in %metainfo, which makes gitweb to\n> > call git-rev-parse only once per unique commit in blame output.\n> \n> Have you tested this patch that it gives the same commit chain\n> as before it?\n\nThe only difference between precious version and this patch is that\nnow, if you calculate sha-1 of $long_rev^, it is stored in \n$metainfo{$long_rev}{'parent'} and not calculated second time.\n\nBut I have checked that (at least for single example file) the blame\noutput is identical for before and after this patch.\n\n\n> > That is what I have noticed during browsing git_blame()\n> > code.\n> \n> What?\n\nWhat I have noticed? I have noticed this inefficiency.\nWhy I was browsing git_blame? To write git_blame_incremental...\n\nSee also my reply to Nanako Shiraishi with simple benchmark.\n\n> > We can change it to even more effective implementation\n> > (like the ones proposed above in the commit message) later.\n> \n> Where?\n\njn> Unfortunately the implementation in 244a70e used one call for\njn> git-rev-parse to find parent revision per line in file, instead of\njn> using long lived \"git cat-file --batch-check\" (which might not existed\njn> then), or changing validate_refname to validate_revision and made it\njn> accept ^, ^^, ^^^ etc. syntax.\n\nOne solution mentioned there is to open bidi pipe (like in Git::Repo\nin gitweb caching by Lea Wiemann) to \"git cat-file --batch-check\"\n(the '--batch-check' option was added to git-cat-file by Adam Roben\non Apr 23, 2008 in v1.5.6-rc0~8^2~9), feed it $long_rev^, and parse\nits output of the form:\n\n 926b07e694599d86cec668475071b32147c95034 commit 637\n\nsee manpage for git-cat-file(1). Unfortunately it seems like \ncommand_bidi_pipe doesn't work as _I_ expected...\n\n\nAnother solution mentioned there is to change validate_refname to\nvalidate_revision when checking script parameters (CGI query or\npath_info), with validate_revision being something like:\n\n sub validate_revision {\n \tmy $rev = shift;\n\treturn validate_refname(strip_rev_suffixes($rev));\n }\n\nor something like that, so we don't need to calculate $long_rev^,\nbut can pass \"$long_rev^\" as 'hb' parameter ($long_rev can in turn\nalso end in '^').\n\n-- \nJakub Narebski\nPoland\n"},{"id":"97509","messageId":"710873.22344.qm@web31806.mail.mud.yahoo.com","threadId":"16659","inReplyTo":"200812101615.55340.jnareb@gmail.com","subject":"Re: [PATCH 2/3] gitweb: Cache $parent_commit info in git_blame()","fromName":"Luben Tuikov","fromEmail":"ltuikov@yahoo.com","sentAt":"2008-12-10T20:05:30Z","receivedAt":"2008-12-10T20:05:30Z","isPatch":true,"sender":{"key":"ltuikov@yahoo.com","avatar":null},"body":"\n--- On Wed, 12/10/08, Jakub Narebski wrote:\n> > Have you tested this patch that it gives the same\n> commit chain\n> > as before it?\n> \n> The only difference between precious version and this patch\n> is that\n> now, if you calculate sha-1 of $long_rev^, it is stored in \n> $metainfo{$long_rev}{'parent'} and not calculated\n> second time.\n\nYes, I agree a patch to this effect would improve performance\nproportionally to the history of the lines of a file. So it's a\ngood thing, as most commits change a contiguous block of size more\nthan one line of a file.\n\n\"$parent_commit\" depends on \"$full_rev^\" which depends on \"$full_rev\".\nSo as soon as \"$full_rev\" != \"$old_full_rev\", you'd know that you need\nto update \"$parent_commit\". \"$old_full_rev\" needs to exist outside the\nscope of \"while (1)\". I didn't see this in the code or in the patch.\n\n> But I have checked that (at least for single example file)\n> the blame output is identical for before and after this patch.\n\nI've always tested it on something like \"gitweb.perl\", etc.\n\n Luben\n"},{"id":"97512","messageId":"20081210200908.16899.36727.stgit@localhost.localdomain","threadId":"16659","inReplyTo":"20081209223703.28106.29198.stgit@localhost.localdomain","subject":"[RFC/PATCH 4/3] gitweb: Incremental blame (proof of concept)","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-10T20:11:18Z","receivedAt":"2008-12-10T20:11:18Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"This is tweaked up version of Petr Baudis patch, which\nin turn was tweaked up version of Fredrik Kuivinen 's\nproof of concept patch. It adds 'blame_incremental' view, which\nincrementally displays line data in blame view using JavaScript (AJAX).\n\nThe original patch by Fredrik Kuivinen has been lightly tested in a\ncouple of browsers (Firefox, Mozilla, Konqueror, Galeon, Opera and IE6).\nThe next patch by Petr Baudis has been tested in Firefox and Epiphany.\nThis patch has been lightly tested in Mozilla 1.17.2 and Konqueror\n3.5.3.\n\nThis patch does not (contrary to the one by Petr Baudis) enable this\nview in gitweb: there are no links leading to 'blame_incremental'\naction. You would have to generate URL 'by hand' (e.g. changing 'blame'\nor 'blob' in gitweb URL to 'blame_incremental'). Having links in gitweb\nlead to this new action (e.g. by rewriting them like in previous patch),\nif JavaScript is enabled in browser, is left for later.\n\nLike earlier patch by Per Baudis it avoids code duplication, but it goes\none step further and use git_blame_common for ordinary blame view, for\nincremental blame, and for incremental blame data.\n\nHow the 'blame_incremental' view works:\n* gitweb generates initial info by putting file contents (from\n git-cat-file) together with line numbers in blame table\n* then gitweb makes web browser JavaScript engine call startBlame()\n function from blame.js\n* startBlame() opens connection to 'blame_data' view, which in turn\n calls \"git blame --incremental\" for a file, and streams output of\n git-blame to JavaScript (blame.js)\n* blame.js updates line info in blame view, coloring it, and updating\n progress info; note that it has to use 3 colors to ensure that\n different neighbour groups have different styles\n* when 'blame_data' ends, and blame.js finishes updating line info,\n it fixes colors to match (as far as possible) ordinary 'blame' view,\n and updates generating time info.\n\nThis code uses http://ajaxpatterns.org/HTTP_Streaming pattern.\n\nIt deals with streamed 'blame_data' server error by notifying about them\nin the progress info area (just in case).\n\nThis patch adds GITWEB_BLAMEJS compile configuration option, and\nmodifies git-instaweb.sh to take blame.js into account, but it does not\nupdate gitweb/README file (as it is only proof of concept patch). The\ncode for git-instaweb.sh was taken from Pasky's patch.\n\nWhile at it, this patch uniquifies td.dark and td.dark2 style: they\ndiffer only in that td.dark2 doesn't have style for :hover.\n\n\nThis patch also adds showing time (in seconds) it took to generate\na page in page footer (based on example code by Pasky), even though\nit is independent change, to be able to evaluate incremental blame in\ngitweb better. In proper patch series it would be independent commit;\nand it probably would provide fallback if Time::HiRes is not available\n(by e.g. not showing generating time info), even though this is\nunlikely.\n\nCc: Fredrik Kuivinen \nSigned-off-by: Petr Baudis \nSigned-off-by: Jakub Narebski \n---\nI'm sorry if you have received duplicate copy of this message...\n\nNOTE: This patch is RFC proof of concept patch!: it should be split\nonto many smaller patches for easy review (and bug finding) in version\nmeant to be applied.\n\n[Please tell me if some of info below should be put in the commit\n message instead of here]\n\nPatch by Petr Baudis this one is based on:\n http://permalink.gmane.org/gmane.comp.version-control.git/56657\n\nOriginal patch by Fredrik Kuivinen:\n http://article.gmane.org/gmane.comp.version-control.git/41361\n\nSnippet adding 'generated in' to gitweb, by Petr Baudis:\n http://article.gmane.org/gmane.comp.version-control.git/83306\n\nShould I post interdiff to Petr Baudis patch, and comments about\ndifference between them? This post is quite long as it is now...\n\n\nDifferences between 'blame' and 'blame_incremental' output:\n* Line number links in 'blame' lead to parent of blamed commit, i.e. to\n the view where given line _changed_; this behavior was introduced in\n 244a70e (Blame \"linenr\" link jumps to previous state at \"orig_lineno\")\n by Luben Tuikov on Jan 2008 to make data mining possible.\n\n Currently 'blame_incremental' lead to version at blamed commit; this\n would be hard to change without opening another stream, or without\n having gitweb accept ^ for 'hb' (hash_base) parameter.\n\n* The title attribute (shown in popup on mouseover) for \"sha1\" cell\n for 'blame_incremental' view looks like the following:\n 'Kay Sievers, 2005-08-07 19:49:46 +0000 (21:49:46 +0200)'\n more like the date format used in 'commit' view, rather than shorter\n 'Kay Sievers, 2005-08-07 21:49:46 +0200'\n\n This is a bit of accident, as in originla patch(es) there was error\n where the time and date shown were for UTC (GMT), and not for shown\n together timezone, i.e.\n 'Kay Sievers, 2005-08-07 19:49:46 +0200'\n and I have added local time instead of replacing it. But perhaps it\n is 'blame' view that should change format? \n\n* In 'blame_incremental' the cell (elements in blame table (currently commented out in the\n source), then row with column headings (with \\n\";\n \t\t}\n-\t\topen (my $dd, \"-|\", git_cmd(), \"rev-parse\", \"$full_rev^\")\n-\t\t\tor die_error(500, \"Open git-rev-parse failed\");\n-\t\tmy $parent_commit = <$dd>;\n-\t\tclose $dd;\n-\t\tchomp($parent_commit);\n+\t\tmy $parent_commit;\n+\t\tif (!exists $meta->{'parent'}) {\n+\t\t\topen (my $dd, \"-|\", git_cmd(), \"rev-parse\", \"$full_rev^\")\n+\t\t\t\tor die_error(500, \"Open git-rev-parse failed\");\n+\t\t\t$parent_commit = <$dd>;\n+\t\t\tclose $dd;\n+\t\t\tchomp($parent_commit);\n+\t\t\t$meta->{'parent'} = $parent_commit;\n+\t\t} else {\n+\t\t\t$parent_commit = $meta->{'parent'};\n+\t\t}\n \t\tmy $blamed = href(action => 'blame',\n \t\t file_name => $meta->{'filename'},\n \t\t hash_base => $parent_commit);\n-- \n1.6.0.4\n"},{"id":"97558","messageId":"7v3agvy1v3.fsf@gitster.siamese.dyndns.org","threadId":"16659","inReplyTo":"20081210200908.16899.36727.stgit@localhost.localdomain","subject":"Re: [RFC/PATCH 4/3] gitweb: Incremental blame (proof of concept)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-11T00:47:12Z","receivedAt":"2008-12-11T00:47:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> NOTE: This patch is RFC proof of concept patch!: it should be split\n> onto many smaller patches for easy review (and bug finding) in version\n> meant to be applied.\n\nHmm, the comments an RFC requests for would certainly be based on reviews\nof the patch in question, so if the patch is known to be unsuitable for\nreviewing, what would that tell us, I wonder ;-)?\n\nAmong the 700 lines added/deleted, 400 lines are from a single new file,\nso what may benefit from splitting would be the changes to gitweb.perl but\nit does not look so bad (I haven't really read the patch, though).\n\n> Differences between 'blame' and 'blame_incremental' output:\n\nHmm, are these by design in the sense that \"when people are getting\nincremental blame output, the normal blame output format is unsuitable for\nsuch and such reasons and that is why there have to be these differences\",\nor \"the code happens to produce slightly different results because it is\nimplemented differently; the differences are listed here as due\ndiligence\"?\n\n> P.P.S. What is the stance for copyrigth assesments in the files\n> for git code, like the ones in gitweb/gitweb.perl and gitweb/blame.js?\n\nThere is no copyright assignment. Everybody retains the own copyright on\ntheir own work.\n\n> P.P.P.S. Should I use Signed-off-by from Pasky and Fredrik if I based\n> my code on theirs, and if they all signed their patches?\n\nI think that is in line with what Certificate of Origin asks you to do.\n"},{"id":"97564","messageId":"200812110222.04663.jnareb@gmail.com","threadId":"16659","inReplyTo":"7v3agvy1v3.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC/PATCH 4/3] gitweb: Incremental blame (proof of concept)","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-11T01:22:03Z","receivedAt":"2008-12-11T01:22:03Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 11 Dec 2008, Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n> > NOTE: This patch is RFC proof of concept patch!: it should be split\n> > onto many smaller patches for easy review (and bug finding) in version\n> > meant to be applied.\n> \n> Hmm, the comments an RFC requests for would certainly be based on reviews\n> of the patch in question, so if the patch is known to be unsuitable for\n> reviewing, what would that tell us, I wonder ;-)?\n\nWell, you can apply patch and test how it works, for example if\nJavaScript code works in other browsers that have JavaScript and DOM\nsupport (Firefox, IE, Opera, Safari, Google Chrome)... Or what\nfeatures or what interface one would like to have...\n\n> Among the 700 lines added/deleted, 400 lines are from a single new file,\n> so what may benefit from splitting would be the changes to gitweb.perl but\n> it does not look so bad (I haven't really read the patch, though).\n\nThere are a few features which could be split in separate commits:\n * there are a few improvements to gitweb.css, independent of \n incremental blame view, like td.warning -> .warning\n * adding to gitweb writing how long it took to generate page should\n be made into separate commit, probably made optional, use better\n HTML style, and have some fallback if there is no Time::HiRes\n\n * progress report could be made into separate commit; I needed it to\n debug code, to check if it progress nicely, but it is not strictly\n required (but it is nice to have visual indicator of progress)\n * 3-coloring of blamed lines during adding blame info was added for\n the fun of it, and should probably be in separate commit\n * adding author initials a'la \"git gui blame\" while nice could also\n be put in separate commit, probably adding this feature also to\n ordinary 'blame' output\n\n[...] \n> > Differences between 'blame' and 'blame_incremental' output:\n> \n> Hmm, are these by design in the sense that \"when people are getting\n> incremental blame output, the normal blame output format is unsuitable for\n> such and such reasons and that is why there have to be these differences\",\n> or \"the code happens to produce slightly different results because it is\n> implemented differently; the differences are listed here as due\n> diligence\"?\n\nActually it is both. Some of differences are _currently_ not possible\nto resolve (parent commit 'lineno' links, split group of lines blamed\nby the same commit), some are coded differently (title attribute for\nsha1, rowspan=\"1\", author initials feature), and some are impossible\nin incremental blame at least during generation (zebra table) or does\nnot make sense in 'blame' view (progress indicators).\n\n> > P.P.S. What is the stance for copyrigth assesments in the files\n> > for git code, like the ones in gitweb/gitweb.perl and gitweb/blame.js?\n> \n> There is no copyright assignment. Everybody retains the own copyright on\n> their own work.\n\nErrr... I'm sorry, I haven't made myself clear. I wanted to ask what\nis the best practices about copyright statement lines like\n\n // Copyright (C) 2007, Fredrik Kuivinen \n\nand other results of \"git grep Copyright\": should it be added for\ninitial author, for main authors... I guess not for all authors.\n\n> > P.P.P.S. Should I use Signed-off-by from Pasky and Fredrik if I based\n> > my code on theirs, and if they all signed their patches?\n> \n> I think that is in line with what Certificate of Origin asks you to do.\n \nI was bit confused because Petr Baudis in his patch used Cc: and not\nSigned-off-by: to Fredrik Kuivinen...\n-- \nJakub Narebski\nPoland\n"},{"id":"97572","messageId":"506479.53667.qm@web31812.mail.mud.yahoo.com","threadId":"16659","inReplyTo":"200812110133.33124.jnareb@gmail.com","subject":"Re: [PATCH 2/3 (edit v2)] gitweb: Cache $parent_commit info in git_blame()","fromName":"Luben Tuikov","fromEmail":"ltuikov@yahoo.com","sentAt":"2008-12-11T04:08:25Z","receivedAt":"2008-12-11T04:08:25Z","isPatch":true,"sender":{"key":"ltuikov@yahoo.com","avatar":null},"body":"\n--- On Wed, 12/10/08, Jakub Narebski wrote:\n> Acked-by: Luben Tuikov \n> Signed-off-by: Jakub Narebski \n\nI've always seen \"Acked-by:\" follows \"Signed-off-by:\". Junio, has this\nchanged?\n\n Luben\n"},{"id":"97573","messageId":"7v8wqnwdiz.fsf@gitster.siamese.dyndns.org","threadId":"16659","inReplyTo":"506479.53667.qm@web31812.mail.mud.yahoo.com","subject":"Re: [PATCH 2/3 (edit v2)] gitweb: Cache $parent_commit info in git_blame()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-11T04:18:12Z","receivedAt":"2008-12-11T04:18:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luben Tuikov writes:\n\n> --- On Wed, 12/10/08, Jakub Narebski wrote:\n>> Acked-by: Luben Tuikov \n>> Signed-off-by: Jakub Narebski \n>\n> I've always seen \"Acked-by:\" follows \"Signed-off-by:\". Junio, has this\n> changed?\n\nI think the order is supposed to show the order of things happened. Jakub\nsigns off the patch, you Ack, and I see the patch and append my sign-off.\n\nYou saw the exact same patch text, said that looked Ok to you, and Jakub\nupdated the log message to present the change better and signed off the\nwhole thing again. You could say that there should be another, original,\nsign off by Jakub before your Ack, but I do not think it adds anything of\nvalue.\n\nIn any case, the change will be queued to 'pu'. It is great that it is a\ntrivial change that gives us great performance boost, and I wish all our\npatches are like that ;-).\n"},{"id":"97606","messageId":"m3r64ehba7.fsf@localhost.localdomain","threadId":"16659","inReplyTo":"20081210200908.16899.36727.stgit@localhost.localdomain","subject":"Re: [RFC/PATCH 4/3] gitweb: Incremental blame (proof of concept)","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-11T17:28:09Z","receivedAt":"2008-12-11T17:28:09Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jakub Narebski writes:\n\n> This is tweaked up version of Petr Baudis patch, which\n> in turn was tweaked up version of Fredrik Kuivinen 's\n> proof of concept patch. It adds 'blame_incremental' view, which\n> incrementally displays line data in blame view using JavaScript (AJAX).\n\n[...]\n> Patch by Petr Baudis this one is based on:\n> http://permalink.gmane.org/gmane.comp.version-control.git/56657\n> \n> Original patch by Fredrik Kuivinen:\n> http://article.gmane.org/gmane.comp.version-control.git/41361\n> \n> Snippet adding 'generated in' to gitweb, by Petr Baudis:\n> http://article.gmane.org/gmane.comp.version-control.git/83306\n> \n> Should I post interdiff to Petr Baudis patch, and comments about\n> difference between them? [...]\n\nHere is the list of differences between Petr Baudis patch and the one\nI have just send. No interdiff, as it is artificially large because\nprevious patch was based on much older version, so ranges does not\nmatch.\n\nBugs I have made:\n * I forgot to make some changes for git-instaweb.sh to have support\n for incremental blame, namely dependency of 'git-instaweb' target\n in Makefile on gitweb/blame.js, and lack of the following line in\n git-instaweb.sh:\n gitweb_blamejs $GIT_DIR/gitweb/blame.js\n\n * Pasky's patch added support for href(...,-partial_query=>1) extra\n parameter, which ensured that gitweb link had '?' in it, and used\n it to generate 'baseUrl' parameter for startBlame. I have\n misunderstood what baseUrl is about, and used $my_url there, while\n it is partial URL for blame links: it is projectUrl.\n\n Therefore links in blame table current 'blame_incremental' would\n not work. I'm sorry about that, I thought I have checked it...\n\nIntentionally omitted features:\n * In patch this one is based on there was fixBlameLinks() JavaScript\n function (put directly in the HTML head inside \\n!.\n+\t\t qq!\\n!;\n+\t}\n+\n \tgit_footer_html();\n }\n \n+sub git_blame {\n+\tgit_blame_common();\n+}\n+\n+sub git_blame_incremental {\n+\tgit_blame_common('incremental');\n+}\n+\n+sub git_blame_data {\n+\tgit_blame_common('data');\n+}\n+\n sub git_tags {\n \tmy $head = git_get_head_hash($project);\n \tgit_header_html();\n-- \n1.6.0.4\n"},{"id":"97883","messageId":"m34p16eny3.fsf_-_@localhost.localdomain","threadId":"16659","inReplyTo":"1229213850-12787-1-git-send-email-jnareb@gmail.com","subject":"[RFC] gitweb: Incremental blame - suggestions for improvements","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-14T16:11:54Z","receivedAt":"2008-12-14T16:11:54Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"A few suggestions for further improvement of blame_incremental:\n * Better support for data mining in 'blame_incremental' view,\n (for \"lineno\" links to (approximately) lead to previous version of\n line) would probably require for gitweb to accept \"^\" for 'hb'\n parameter (and perhaps resolve/parse it before saving to\n $hash_base). This would also help performance of 'blame' view.\n\n * Move some of processing to server, to 'blame_data' action, and for\n example send whole saved and processed info for a group of lines as\n JSON or as variation of \"git blame --incremental\" output.\n\n * Better error checking: not only check if scripting is turned on,\n but also if required methods like document.getElementById are\n available. Probably would require falback to *ugh* document.write.\n\n * Perhaps fallback on (slower) DOM methods if innerHTML is not\n available or doesn't work, which is the case for some versions of\n some browsers in strict XHTML (application/xml+html + XHTML DTD)\n mode.\n\n * Fallback on DOM Core methods of deleting cell if DOM HTML\n deleteCell method is not available; check that DOM Core way does\n not lead to memory leaks (by deleting element, but not its\n children).\n\n * Use 'one second spotlight' or other similar user interface patter\n to signal in more visible way than reaching 100% in progress info,\n and changing colors from 3-coloring to 2-color zebra in blame table\n that all blame data was received and entered.\n\n * Check in handleResponse if browser calls it on [each] server flush,\n by checking if it is called more than once with http.readyState == 3\n and with different http.responseText, and disable poll timer then.\n\n * Mark boundary commits (this applies both to 'blame' and\n 'blame_incremental' views), using UNDOCUMENTED \"boundary\" header.\n\n * A toy. Make progress bar indicator more smooth by interpolating\n changes between updates, so it moves smoothly. This would make it\n provide impression for user, not an accurate measure of blamed\n percentage.\n\n * A toy. Make sure that 3-coloring during updating blame table use\n all three colors with similar frequency, for example by using the\n following implementation of chooseColorNoFrom function:\n\n // return one of given possible colors\n // example: chooseColorNoFrom(2, 3) might be 2 or 3\n var colorsFreq = [0, 0, 0];\n // assumes that 1 <= arguments[i] <= colorsFreq.length\n function chooseColorNoFrom() {\n \t// choose the color which is least used\n \tvar colorNo = arguments[0];\n \tfor (var i = 1; i < arguments.length; i++) {\n \t\tif (colorsFreq[arguments[i]-1] < colorsFreq[colorNo-1]) {\n \t\t\tcolorNo = arguments[i];\n \t\t}\n \t}\n \tcolorsFreq[colorNo-1]++;\n \treturn colorNo;\n }\n\n\nDo you have your own suggestions? Comments?\nWould incremental blame help git-instaweb use?\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"98105","messageId":"20081217081300.GB3640@machine.or.cz","threadId":"16659","inReplyTo":"20081209224330.28106.18301.stgit@localhost.localdomain","subject":"Re: [PATCH 1/3] gitweb: Move 'lineno' id from link to row element in git_blame","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-12-17T08:13:00Z","receivedAt":"2008-12-17T08:13:00Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Tue, Dec 09, 2008 at 11:46:16PM +0100, Jakub Narebski wrote:\n> Move l ID from link element inside table row (inside\n> cell element for column with line numbers), to encompassing table\n> row element. It was done to make it easier to manipulate result HTML\n> with DOM, and to be able write 'blame_incremental' view with the same,\n> or nearly the same result.\n> \n> Signed-off-by: Jakub Narebski \n\nAcked-by: Petr Baudis \n"},{"id":"98106","messageId":"20081217081935.GC3640@machine.or.cz","threadId":"16659","inReplyTo":"200812110133.33124.jnareb@gmail.com","subject":"Re: [PATCH 2/3 (edit v2)] gitweb: Cache $parent_commit info in git_blame()","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-12-17T08:19:35Z","receivedAt":"2008-12-17T08:19:35Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Thu, Dec 11, 2008 at 01:33:29AM +0100, Jakub Narebski wrote:\n> Luben Tuikov changed 'lineno' link from leading to commit which gave\n> current version of given block of lines, to leading to parent of this\n> commit in 244a70e (Blame \"linenr\" link jumps to previous state at\n> \"orig_lineno\"). This made possible data mining using 'blame' view.\n> \n> The current implementation calls rev-parse once per each blamed line\n> to find parent revision of blamed commit, even when the same commit\n> appears more than once, which is inefficient.\n> \n> This patch attempts to mitigate this issue by storing (caching)\n> $parent_commit info in %metainfo, which makes gitweb call\n> git-rev-parse only once per each unique commit in blame output.\n> \n> \n> In the tables below you can see simple benchmark comparing gitweb\n> performance before and after this patch\n> \n> File | L[1] | C[2] || Time0[3] | Before[4] | After[4]\n> ====================================================================\n> blob.h | 18 | 4 || 0m1.727s | 0m2.545s | 0m2.474s\n> GIT-VERSION-GEN | 42 | 13 || 0m2.165s | 0m2.448s | 0m2.071s\n> README | 46 | 6 || 0m1.593s | 0m2.727s | 0m2.242s\n> revision.c | 1923 | 121 || 0m2.357s | 0m30.365s | 0m7.028s\n> gitweb/gitweb.perl | 6291 | 428 || 0m8.080s | 1m37.244s | 0m20.627s\n> \n> File | L/C | Before/After\n> =========================================\n> blob.h | 4.5 | 1.03\n> GIT-VERSION-GEN | 3.2 | 1.18\n> README | 7.7 | 1.22\n> revision.c | 15.9 | 4.32\n> gitweb/gitweb.perl | 14.7 | 4.71\n> \n> As you can see the greater ratio of lines in file to unique commits\n> in blame output, the greater gain from the new implementation.\n> \n> Footnotes:\n> ~~~~~~~~~~\n> [1] Lines:\n> $ wc -l \n> [2] Individual commits in blame output:\n> $ git blame -p | grep author-time | wc -l\n> [3] Time for running \"git blame -p\" (user time, single run):\n> $ time git blame -p >/dev/null\n> [4] Time to run gitweb as Perl script from command line:\n> $ gitweb-run.sh \"p=.git;a=blame;f=\" > /dev/null 2>&1\n> \n> The gitweb-run.sh script includes slightly modified (with adjusted\n> pathnames) code from gitweb_run() function from the test script\n> t/t9500-gitweb-standalone-no-errors.sh; gitweb config file\n> gitweb_config.perl contents (again up to adjusting pathnames; in\n> particular $projectroot variable should point to top directory of git\n> repository) can be found in the same place.\n> \n> \n> Alternate solutions:\n> ~~~~~~~~~~~~~~~~~~~~\n> Alternate solution would be to open bidi pipe to \"git cat-file\n> --batch-check\", (like in Git::Repo in gitweb caching by Lea Wiemann),\n> feed $long_rev^ to it, and parse its output which has the following\n> form:\n> \n> 926b07e694599d86cec668475071b32147c95034 commit 637\n> \n> This would mean one call to git-cat-file for the whole 'blame' view,\n> instead of one call to git-rev-parse per each unique commit in blame\n> output.\n> \n> \n> Yet another solution would be to change use of validate_refname() to\n> validate_revision() when checking script parameters (CGI query or\n> path_info), with validate_revision being something like the following:\n> \n> sub validate_revision {\n> my $rev = shift;\n> return validate_refname(strip_rev_suffixes($rev));\n> }\n> \n> so we don't need to calculate $long_rev^, but can pass \"$long_rev^\" as\n> 'hb' parameter.\n> \n> This solution has the advantage that it can be easily adapted to\n> future incremental blame output.\n> \n> Acked-by: Luben Tuikov \n> Signed-off-by: Jakub Narebski \n\nAcked-by: Petr Baudis \n\n(though I think the commit message is total overkill for such an obvious\nchange ;-)\n"},{"id":"98111","messageId":"7v63ljp5cg.fsf@gitster.siamese.dyndns.org","threadId":"16659","inReplyTo":"20081217081935.GC3640@machine.or.cz","subject":"Re: [PATCH 2/3 (edit v2)] gitweb: Cache $parent_commit info in git_blame()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-17T08:34:55Z","receivedAt":"2008-12-17T08:34:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Petr Baudis writes:\n\n> (though I think the commit message is total overkill for such an obvious\n> change ;-)\n\nI think so too; the performance figures are nice but there is no need to\ntalk about what is not implemented.\n"}]}
CommitLineData
) with sha1 has rowspan\n attribute set even if it is at its default value rowspan=\"1\".\n This should be very easy to fix.\n\n* The 'blame_incremental' view has new feature inspired by output of\n \"git gui blame \", that if group of lines blamed to the same\n commit has more than two lines, then below shortened sha-1 of a\n commit it is shown shortened author (initials, in uppercase).\n\n If you think it is worth it, this feature should be fairly easy to\n port to ordinary non-incremental 'blame' view.\n\n* Sometimes \"git blame --porcelain\" and \"git blame --incremental\" can\n give different output. Compare for example 'blame' and\n 'blame_incremental' view for GIT-VERSION-GEN in git.git repository,\n and note that in 'blame_incremental' the last two groups are for the\n same commit (compare bottom parts of pages). 'blame_incremental'\n currently does not consolidate groups; if it did that, it should do\n it before fixColors()\n\n* During filling blame info 'blame_incremental' view uses three colors\n (three styles: color1, color2, color3) instead of two color zebra\n coloring (two styles: light2, dark2) to ensure that the color of\n current group is different from both of its neighbours.\n\n This is non-issue: this is fixed at the end (although intermediate\n versions of blame.js script didn't fo fixColors() to make it easier\n to check if the 3-coloring is correct).\n\n* The 'blame_incremental' view contains unique progress bar and\n progress report; perhaps they should vanish after succesfull loading\n of all data?\n\n\nBugs and suspected bugs in Mozilla 1.17.2 (my main browser); perhaps\nthose got corrected, as 1.17.2 is ancient (Gecko/20050923) version:\n* HTML 4.01 Transitional specification at W3C states only ID and NAME\n tokens must begin with a letter ([A-Za-z]); it looks like class\n named \"1\" or \"2\" or \"3\" has style specified by CSS ignored.\n\n* With XHTML 1.0 DTD and application/xhtml+xml content type, if there\n were
elements) was not\n visible.\n\n* With XHTML 1.0 DTD and application/xhtml+xml content type, if there\n was an error in JavaScript, instead of having it visible as error\n message in JavaScript Console, Mozilla behaved as if the script\n wasn't there at all.\n\n* With XHTML 1.0 DTD and application/xhtml+xml content type, setting\n innerHTML doesn't work: it raises cryptic JavaScript exception:\n\n Component returned failure code: 0x80004005 (NS_ERROR_FAILURE)\n [nsIDOMNSHTMLElement.innerHTML]\t\n\n Correct solution would be to use only DOM, or rather depending on\n what web browser supports, use either .innerHTML (which is\n proprietary Microsoft extension) or DOM (which is standard but not\n all browser use it). Current *workaround* is to simply always use\n 'text/html' content type, and HTML 4.01 DTD.\n\n I wonder whether innerHTML or DOM is faster; and how many of web\n browser implements other similar properties like innerText,\n outerHTML and insertAdjacentHTML.\n\n* XHTML 1.0 doesn't allow for trick with using HTML comments to hide\n contents of \\n!.\n+\t\t qq!\\n!;\n+\t}\n+\n \tgit_footer_html();\n }\n \n+sub git_blame {\n+\tgit_blame_common();\n+}\n+\n+sub git_blame_incremental {\n+\tgit_blame_common('incremental');\n+}\n+\n+sub git_blame_data {\n+\tgit_blame_common('data');\n+}\n+\n sub git_tags {\n \tmy $head = git_get_head_hash($project);\n \tgit_header_html();\n\n-- \nStacked GIT 0.14.3\ngit version 1.6.0.4\n"},{"id":"97517","messageId":"7v7i67zsgj.fsf@gitster.siamese.dyndns.org","threadId":"16659","inReplyTo":"200812101439.18526.jnareb@gmail.com","subject":"Re: [PATCH 2/3] gitweb: Cache $parent_commit info in git_blame()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-10T20:27:24Z","receivedAt":"2008-12-10T20:27:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> On Wed, 10 Dec 2008, Nanako Shiraishi wrote:\n>> Quoting Jakub Narebski :\n>> \n>> > Unfortunately the implementation in 244a70e used one call for\n>> > git-rev-parse to find parent revision per line in file, instead of\n>> > using long lived \"git cat-file --batch-check\" (which might not existed\n>> > then), or changing validate_refname to validate_revision and made it\n>> > accept ^, ^^, ^^^ etc. syntax.\n>> \n>> Could you substantiate why this is \"Unfortunate\"?\n>\n> Because it calls git-rev-parse once for _each line_, even if for lines\n> in the group of neighbour lines blamed by same commit $parent_commit\n> is the same, and even if you need to calculate $parent_commit only once\n> per unique individual commit present in blame output.\n\nIt probably was obvious that this was meant as a patch for better\nperformance without changing the functionality. I tend to think the\npresentation wasn't so great, though.\n\n Luben Tuikov changed 'lineno' link from leading to commit which lead\n to current version of given block of lines, to leading to parent of\n this commit in 244a70e (Blame \"linenr\" link jumps to previous state at\n \"orig_lineno\"). This supposedly made data mining possible (or just\n better).\n\nOther than \"supposedly\" which I do not think should be there, I think this\nis a great opening paragraph to establish the context.\n\n Unfortunately the implementation in 244a70e used one call for\n git-rev-parse to find parent revision per line in file, instead of\n using long lived \"git cat-file --batch-check\" (which might not existed\n then), or changing validate_refname to validate_revision and made it\n accept ^, ^^, ^^^ etc. syntax.\n\nBut I do not think this is such a great second paragraph that states what\nproblem it tries to solve. I'd say something like this instead:\n\n The current implementation calls rev-parse once per line to find\n parent revision of blamed commit, even when the same commit\n appears more than once, which is inefficient.\n\nAnd then the outline of the solution:\n\n This patch attempts to migitate issue a bit by caching $parent_commit\n info in %metainfo, which makes gitweb to call git-rev-parse only once\n per unique commit in blame output.\n\nwhich is very good, except that I do not think you need to say \"a bit\".\nAnd have your benchmark (two tables and footnotes) after this outline of\nthe solution.\n\nI think the first part of \"Unfortunately\" paragraph can be dropped\n(because that is already in the first half of problem description) and the\nrest can come as the last paragraph as \"Possible future enhancements\".\n\n> Appendix A:\n> ~~~~~~~~~~~\n> #!/bin/bash\n>\n> export GATEWAY_INTERFACE=\"CGI/1.1\"\n> export HTTP_ACCEPT=\"*/*\"\n> export REQUEST_METHOD=\"GET\"\n> export QUERY_STRING=\"\"$1\"\"\n> export PATH_INFO=\"\"$2\"\"\n>\n> export GITWEB_CONFIG=\"/home/jnareb/git/gitweb/gitweb_config.perl\"\n>\n> perl -- /home/jnareb/git/gitweb/gitweb.perl\n>\n> # end of gitweb-run.sh\n\nI'd suggest making a separate patch to add \"gitweb-run.sh\" in contrib/ so\nthat other people can use it when checking their changes to gitweb. The\nscript might need some more polishing, though. For example, it is not\nvery obvious if you have *_config.perl only to customize for your\nenvironment (e.g. where the test repositories are) or you need to have\nsome overrides in there when you are running gitweb as a standalone script.\n\nTo recap, I think the commit log for this patch would have been much\neasier to read if it were presented in this order:\n\n\ta paragraph to establish the context;\n\n\ta paragraph to state what problem it tries to solve;\n\n a paragraph (or more) to explain the solution; and finally\n\n\ta paragraph to discuss possible future enhancements.\n"},{"id":"97520","messageId":"200812102203.30486.jnareb@gmail.com","threadId":"16659","inReplyTo":"710873.22344.qm@web31806.mail.mud.yahoo.com","subject":"Re: [PATCH 2/3] gitweb: Cache $parent_commit info in git_blame()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-10T21:03:28Z","receivedAt":"2008-12-10T21:03:28Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 10 Dec 2008, Luben Tuikov wrote:\n> --- On Wed, 12/10/08, Jakub Narebski wrote:\n\nSide note: is it Yahoo web mail interface that removes attributions?\n\n> > > Have you tested this patch that it gives the same commit chain\n> > > as before it?\n> > \n> > The only difference between precious version and this patch\n> > is that now, if you calculate sha-1 of $long_rev^, it is stored in \n> > $metainfo{$long_rev}{'parent'} and not calculated second time.\n> \n> Yes, I agree a patch to this effect would improve performance\n> proportionally to the history of the lines of a file.\n\nOr rather proportionally to the ratio of number of lines of a file\nto number of unique commits (not groups of lines) which are blamed\nfor given contents of a file.\n\n> So it's a good thing, as most commits change a contiguous block\n> of size more than one line of a file.\n> \n> \"$parent_commit\" depends on \"$full_rev^\" which depends on \"$full_rev\".\n> So as soon as \"$full_rev\" != \"$old_full_rev\", you'd know that you need\n> to update \"$parent_commit\". \"$old_full_rev\" needs to exist outside\n> the scope of \"while (1)\". I didn't see this in the code or in the\n> patch. \n\nYou don't need $old_full_rev. We have to save data about commit in\n%metainfo hash because information about commit appears in \n\"git blame --porcelain\" output _once_ per commit. So we use the same\ncache to store $full_rev^ in $meta{'parent'}, which means storing\nit in $metainfo{$full_rev}{'parent'}.\n\nNow if the commit we saved this info about appears again in git-blame\noutput, be it in group of lines for which the same commit is blamed,\nor later in unrelated chunk, we use saved info.\n\nLet me try to explain it using the following diagram:\n\n Commit N Line Original code This patch\n ------------------------------------------------------\n 3a4046 1 xxx rev-parse 3a4046^ rev-parse 3a4046^\n 2 xxx rev-parse 3a4046^ $mi{3a4046}{'parent'}\n 3 xxx rev-parse 3a4046^ $mi{3a4046}{'parent'}\n f47c19 5 xxx rev-parse f47c19^ rev-parse f47c19^\n 6 xxx rev-parse f47c19^ $mi{f47c19}{'parent'}\n 3a4046 7 xxx rev-parse 3a4046^ $mi{3a4046}{'parent'} <--\n 8 xxx rev-parse 3a4046^ $mi{3a4046}{'parent'}\n\nwhere \"rev-parse 3a4046^\" means call to git-rev-parse, and $mi{}\naccessing $metainfo{$full_rev} (via $meta).\n \nIn place marked by arrow '<--' you don't need to calculate 3a4046^\nagain...\n\n> > But I have checked that (at least for single example file)\n> > the blame output is identical for before and after this patch.\n> \n> I've always tested it on something like \"gitweb.perl\", etc.\n\nI've checked it on blob.h. Other good example is README (with boundary\ncommits) and GIT-VERSION-GEN (with different output between git-blame\n--porcelain and --incremental), both of which take much less time than\ngitweb/gitweb.perl (see benchmarks in other post).\n\n-- \nJakub Narebski\nPoland\n"},{"id":"97522","messageId":"755680.28599.qm@web31804.mail.mud.yahoo.com","threadId":"16659","inReplyTo":"200812102203.30486.jnareb@gmail.com","subject":"Re: [PATCH 2/3] gitweb: Cache $parent_commit info in git_blame()","fromName":"Luben Tuikov","fromEmail":"ltuikov@yahoo.com","sentAt":"2008-12-10T21:15:02Z","receivedAt":"2008-12-10T21:15:02Z","isPatch":true,"sender":{"key":"ltuikov@yahoo.com","avatar":null},"body":"\n--- On Wed, 12/10/08, Jakub Narebski wrote:\n\n> You don't need $old_full_rev. We have to save data\n> about commit in\n> %metainfo hash because information about commit appears in \n\nOh, yeah, the hash... Then, it looks correct.\n\nAcked-by: Luben Tuikov \n"},{"id":"97554","messageId":"200812110133.33124.jnareb@gmail.com","threadId":"16659","inReplyTo":"7v7i67zsgj.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 2/3 (edit v2)] gitweb: Cache $parent_commit info in git_blame()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-11T00:33:29Z","receivedAt":"2008-12-11T00:33:29Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Luben Tuikov changed 'lineno' link from leading to commit which gave\ncurrent version of given block of lines, to leading to parent of this\ncommit in 244a70e (Blame \"linenr\" link jumps to previous state at\n\"orig_lineno\"). This made possible data mining using 'blame' view.\n\nThe current implementation calls rev-parse once per each blamed line\nto find parent revision of blamed commit, even when the same commit\nappears more than once, which is inefficient.\n\nThis patch attempts to mitigate this issue by storing (caching)\n$parent_commit info in %metainfo, which makes gitweb call\ngit-rev-parse only once per each unique commit in blame output.\n\n\nIn the tables below you can see simple benchmark comparing gitweb\nperformance before and after this patch\n\nFile | L[1] | C[2] || Time0[3] | Before[4] | After[4]\n====================================================================\nblob.h | 18 | 4 || 0m1.727s | 0m2.545s | 0m2.474s\nGIT-VERSION-GEN | 42 | 13 || 0m2.165s | 0m2.448s | 0m2.071s\nREADME | 46 | 6 || 0m1.593s | 0m2.727s | 0m2.242s\nrevision.c | 1923 | 121 || 0m2.357s | 0m30.365s | 0m7.028s\ngitweb/gitweb.perl | 6291 | 428 || 0m8.080s | 1m37.244s | 0m20.627s\n\nFile | L/C | Before/After\n=========================================\nblob.h | 4.5 | 1.03\nGIT-VERSION-GEN | 3.2 | 1.18\nREADME | 7.7 | 1.22\nrevision.c | 15.9 | 4.32\ngitweb/gitweb.perl | 14.7 | 4.71\n\nAs you can see the greater ratio of lines in file to unique commits\nin blame output, the greater gain from the new implementation.\n\nFootnotes:\n~~~~~~~~~~\n[1] Lines:\n $ wc -l \n[2] Individual commits in blame output:\n $ git blame -p | grep author-time | wc -l\n[3] Time for running \"git blame -p\" (user time, single run):\n $ time git blame -p >/dev/null\n[4] Time to run gitweb as Perl script from command line:\n $ gitweb-run.sh \"p=.git;a=blame;f=\" > /dev/null 2>&1\n\nThe gitweb-run.sh script includes slightly modified (with adjusted\npathnames) code from gitweb_run() function from the test script\nt/t9500-gitweb-standalone-no-errors.sh; gitweb config file\ngitweb_config.perl contents (again up to adjusting pathnames; in\nparticular $projectroot variable should point to top directory of git\nrepository) can be found in the same place.\n\n\nAlternate solutions:\n~~~~~~~~~~~~~~~~~~~~\nAlternate solution would be to open bidi pipe to \"git cat-file\n--batch-check\", (like in Git::Repo in gitweb caching by Lea Wiemann),\nfeed $long_rev^ to it, and parse its output which has the following\nform:\n\n 926b07e694599d86cec668475071b32147c95034 commit 637\n\nThis would mean one call to git-cat-file for the whole 'blame' view,\ninstead of one call to git-rev-parse per each unique commit in blame\noutput.\n\n\nYet another solution would be to change use of validate_refname() to\nvalidate_revision() when checking script parameters (CGI query or\npath_info), with validate_revision being something like the following:\n\n sub validate_revision {\n my $rev = shift;\n return validate_refname(strip_rev_suffixes($rev));\n }\n\nso we don't need to calculate $long_rev^, but can pass \"$long_rev^\" as\n'hb' parameter.\n\nThis solution has the advantage that it can be easily adapted to\nfuture incremental blame output.\n\nAcked-by: Luben Tuikov \nSigned-off-by: Jakub Narebski \n---\nOn Wed, 10 Dec 2008, Junio C Hamano wrote:\n\n> To recap, I think the commit log for this patch would have been much\n> easier to read if it were presented in this order:\n> \n> a paragraph to establish the context;\n> \n> a paragraph to state what problem it tries to solve;\n> \n> a paragraph (or more) to explain the solution; and finally\n> \n> a paragraph to discuss possible future enhancements.\n\nLike this?\n\nOnly commit message has changed.\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 1b800f4..916396a 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -4657,11 +4657,17 @@ HTML\n \t\t\t esc_html($rev));\n \t\t\tprint \"