{"thread":{"id":"20897","subject":"[RFC/PATCH 1/2] gitweb: extend &git_get_head_hash to be &git_get_hash","startedAt":"2009-09-10T21:20:27Z","lastAt":"2009-09-10T21:49:49Z","messageCount":2,"participants":["Mark Rada","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"122844","messageId":"4AA96D9B.6090003@mailservices.uwaterloo.ca","threadId":"20897","inReplyTo":null,"subject":"[RFC/PATCH 1/2] gitweb: extend &git_get_head_hash to be &git_get_hash","fromName":"Mark Rada","fromEmail":"marada@uwaterloo.ca","sentAt":"2009-09-10T21:20:27Z","receivedAt":"2009-09-10T21:20:27Z","isPatch":true,"sender":{"key":"marada@uwaterloo.ca","avatar":"https://avatars.githubusercontent.com/u/38430?v=4"},"body":"This adds an optional second argument to the routine which lets the\ncaller specify a treeish that can be translated to a hash id by\nrev-parse.\n\nTo maintain some backwards compatability, the second argument is\noptional and it will default to `HEAD' if not specified.\n\nSigned-off-by: Mark Rada <marada@uwaterloo.ca>\n---\n gitweb/gitweb.perl |   31 ++++++++++++++++---------------\n 1 files changed, 16 insertions(+), 15 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 24b2193..d650188 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1981,13 +1981,14 @@ sub quote_command {\n \t\tmap { my $a = $_; $a =~ s/(['!])/'\\\\$1'/g; \"'$a'\" } @_ );\n }\n \n-# get HEAD ref of given project as hash\n-sub git_get_head_hash {\n+# get object id of given project as full hash, defaults to HEAD\n+sub git_get_hash {\n \tmy $project = shift;\n+\tmy $hash = shift || 'HEAD';\n \tmy $o_git_dir = $git_dir;\n \tmy $retval = undef;\n \t$git_dir = \"$projectroot/$project\";\n-\tif (open my $fd, \"-|\", git_cmd(), \"rev-parse\", \"--verify\", \"HEAD\") {\n+\tif (open my $fd, '-|', git_cmd(), 'rev-parse', '--verify', \"$hash\") {\n \t\tmy $head = <$fd>;\n \t\tclose $fd;\n \t\tif (defined $head && $head =~ /^([0-9a-fA-F]{40})$/) {\n@@ -4737,7 +4738,7 @@ sub git_summary {\n }\n \n sub git_tag {\n-\tmy $head = git_get_head_hash($project);\n+\tmy $head = &git_get_hash($project);\n \tgit_header_html();\n \tgit_print_page_nav('','', $head,undef,$head);\n \tmy %tag = parse_tag($hash);\n@@ -4778,7 +4779,7 @@ sub git_blame {\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+\t$hash_base ||= &git_get_hash($project);\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@@ -4911,7 +4912,7 @@ HTML\n }\n \n sub git_tags {\n-\tmy $head = git_get_head_hash($project);\n+\tmy $head = &git_get_hash($project);\n \tgit_header_html();\n \tgit_print_page_nav('','', $head,undef,$head);\n \tgit_print_header_div('summary', $project);\n@@ -4924,7 +4925,7 @@ sub git_tags {\n }\n \n sub git_heads {\n-\tmy $head = git_get_head_hash($project);\n+\tmy $head = &git_get_hash($project);\n \tgit_header_html();\n \tgit_print_page_nav('','', $head,undef,$head);\n \tgit_print_header_div('summary', $project);\n@@ -4942,7 +4943,7 @@ sub git_blob_plain {\n \n \tif (!defined $hash) {\n \t\tif (defined $file_name) {\n-\t\t\tmy $base = $hash_base || git_get_head_hash($project);\n+\t\t\tmy $base = $hash_base || &git_get_hash($project);\n \t\t\t$hash = git_get_hash_by_path($base, $file_name, \"blob\")\n \t\t\t\tor die_error(404, \"Cannot find file\");\n \t\t} else {\n@@ -4994,7 +4995,7 @@ sub git_blob {\n \n \tif (!defined $hash) {\n \t\tif (defined $file_name) {\n-\t\t\tmy $base = $hash_base || git_get_head_hash($project);\n+\t\t\tmy $base = $hash_base || &git_get_hash($project);\n \t\t\t$hash = git_get_hash_by_path($base, $file_name, \"blob\")\n \t\t\t\tor die_error(404, \"Cannot find file\");\n \t\t} else {\n@@ -5197,7 +5198,7 @@ sub git_snapshot {\n \t}\n \n \tif (!defined $hash) {\n-\t\t$hash = git_get_head_hash($project);\n+\t\t$hash = &git_get_hash($project);\n \t}\n \n \tmy $name = $project;\n@@ -5229,7 +5230,7 @@ sub git_snapshot {\n }\n \n sub git_log {\n-\tmy $head = git_get_head_hash($project);\n+\tmy $head = &git_get_hash($project);\n \tif (!defined $hash) {\n \t\t$hash = $head;\n \t}\n@@ -5833,7 +5834,7 @@ sub git_patches {\n \n sub git_history {\n \tif (!defined $hash_base) {\n-\t\t$hash_base = git_get_head_hash($project);\n+\t\t$hash_base = &git_get_hash($project);\n \t}\n \tif (!defined $page) {\n \t\t$page = 0;\n@@ -5904,7 +5905,7 @@ sub git_search {\n \t\tdie_error(400, \"Text field is empty\");\n \t}\n \tif (!defined $hash) {\n-\t\t$hash = git_get_head_hash($project);\n+\t\t$hash = &git_get_hash($project);\n \t}\n \tmy %co = parse_commit($hash);\n \tif (!%co) {\n@@ -6159,7 +6160,7 @@ EOT\n }\n \n sub git_shortlog {\n-\tmy $head = git_get_head_hash($project);\n+\tmy $head = &git_get_hash($project);\n \tif (!defined $hash) {\n \t\t$hash = $head;\n \t}\n@@ -6486,7 +6487,7 @@ XML\n \n \tforeach my $pr (@list) {\n \t\tmy %proj = %$pr;\n-\t\tmy $head = git_get_head_hash($proj{'path'});\n+\t\tmy $head = &git_get_hash($proj{'path'});\n \t\tif (!defined $head) {\n \t\t\tnext;\n \t\t}\n-- \n1.6.4.2\n"},{"id":"122846","messageId":"200909102349.50988.jnareb@gmail.com","threadId":"20897","inReplyTo":"4AA96D9B.6090003@mailservices.uwaterloo.ca","subject":"Re: [RFC/PATCH 1/2] gitweb: extend &git_get_head_hash to be &git_get_hash","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-09-10T21:49:49Z","receivedAt":"2009-09-10T21:49:49Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 10 Sep 2009, Mark Rada wrote:\n\n> gitweb: extend &git_get_head_hash to be &git_get_hash\n\nShould be \"extend git_get_head_hash to be git_get_hash\", see below.\n\n>\n> This adds an optional second argument to the routine which lets the\n> caller specify a treeish that can be translated to a hash id by\n> rev-parse.\n\nMinor nit: we use \"tree-ish\" not \"treeish\" in documentation.\n\n> \n> To maintain some backwards compatability, the second argument is\n> optional and it will default to `HEAD' if not specified.\n\nWhy 'some'?  It does maintain backward compatibility.\n\nAlso i am not sure about git_get_hash defaulting to \"HEAD\". Perhaps\nit would be better to keep git_get_head_hash as warpper around \ngit_get_hash?  Or perhaps it would be better to explicitly pass\n\"HEAD\" as second parameter?\n\n> \n> Signed-off-by: Mark Rada <marada@uwaterloo.ca>\n\nIt looks like a good idea, even if it doesn't make much sense as\na standalone patch.\n\n> ---\n>  gitweb/gitweb.perl |   31 ++++++++++++++++---------------\n>  1 files changed, 16 insertions(+), 15 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 24b2193..d650188 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1981,13 +1981,14 @@ sub quote_command {\n>  \t\tmap { my $a = $_; $a =~ s/(['!])/'\\\\$1'/g; \"'$a'\" } @_ );\n>  }\n>  \n> -# get HEAD ref of given project as hash\n> -sub git_get_head_hash {\n> +# get object id of given project as full hash, defaults to HEAD\n> +sub git_get_hash {\n\nI'm not sure about naming (not that git_get_head_hash was best), as\nyou don't see from git_get_hash subroutine name that the first parameter\nis the name of repository (the git_get_head_hash at least hinted, if\nvery weekly, at it).\n\n>  \tmy $project = shift;\n> +\tmy $hash = shift || 'HEAD';\n>  \tmy $o_git_dir = $git_dir;\n>  \tmy $retval = undef;\n>  \t$git_dir = \"$projectroot/$project\";\n> -\tif (open my $fd, \"-|\", git_cmd(), \"rev-parse\", \"--verify\", \"HEAD\") {\n> +\tif (open my $fd, '-|', git_cmd(), 'rev-parse', '--verify', \"$hash\") {\n\nMinor nit: we don't need to quote (stringify) single variable here; \nusing \n  ..., '--verify', $hash)\nwould work as well.\n\n>  \t\tmy $head = <$fd>;\n>  \t\tclose $fd;\n>  \t\tif (defined $head && $head =~ /^([0-9a-fA-F]{40})$/) {\n> @@ -4737,7 +4738,7 @@ sub git_summary {\n>  }\n>  \n>  sub git_tag {\n> -\tmy $head = git_get_head_hash($project);\n> +\tmy $head = &git_get_hash($project);\n\nOne very rarely needs to use explicit \"&\" subroutine calling syntax;\nI think only when you want to circumvent subroutine prototype (which\nis different from function/procedure prototypes in other languages).\nCurrent practice is to not use it.\n\nBesides it it not used anywhere else in gitweb (consistency).\n\n>  \tgit_header_html();\n>  \tgit_print_page_nav('','', $head,undef,$head);\n>  \tmy %tag = parse_tag($hash);\n> @@ -4778,7 +4779,7 @@ sub git_blame {\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> +\t$hash_base ||= &git_get_hash($project);\n\nSame as above: use \"git_get_hash($project)\" or \"git_get_hash($project, 'HEAD');\"\n\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> @@ -4911,7 +4912,7 @@ HTML\n>  }\n>  \n>  sub git_tags {\n> -\tmy $head = git_get_head_hash($project);\n> +\tmy $head = &git_get_hash($project);\n\nSame as above: use \"git_get_hash($project)\"\n\n>  \tgit_header_html();\n>  \tgit_print_page_nav('','', $head,undef,$head);\n>  \tgit_print_header_div('summary', $project);\n> @@ -4924,7 +4925,7 @@ sub git_tags {\n>  }\n>  \n>  sub git_heads {\n> -\tmy $head = git_get_head_hash($project);\n> +\tmy $head = &git_get_hash($project);\n\nSame as above: use \"git_get_hash($project)\"\n\n>  \tgit_header_html();\n>  \tgit_print_page_nav('','', $head,undef,$head);\n>  \tgit_print_header_div('summary', $project);\n> @@ -4942,7 +4943,7 @@ sub git_blob_plain {\n>  \n>  \tif (!defined $hash) {\n>  \t\tif (defined $file_name) {\n> -\t\t\tmy $base = $hash_base || git_get_head_hash($project);\n> +\t\t\tmy $base = $hash_base || &git_get_hash($project);\n\nSame as above: use \"git_get_hash($project)\"\n[...]\n\n-- \nJakub Narebski\nPoland\n"}]}