Re: [RFC/PATCH 1/2] gitweb: extend &git_get_head_hash to be &git_get_hash
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Sep 10, 2009, 21:49 UTC
- Message-ID
- <200909102349.50988.jnareb@gmail.com>
- In-Reply-To
- <4AA96D9B.6090003@mailservices.uwaterloo.ca>
On Thu, 10 Sep 2009, Mark Rada wrote:
> gitweb: extend &git_get_head_hash to be &git_get_hash
Should be "extend git_get_head_hash to be git_get_hash", see below.
> > This adds an optional second argument to the routine which lets the > caller specify a treeish that can be translated to a hash id by > rev-parse.
Minor nit: we use "tree-ish" not "treeish" in documentation.
> > To maintain some backwards compatability, the second argument is > optional and it will default to `HEAD' if not specified.
Why 'some'? It does maintain backward compatibility.
Also i am not sure about git_get_hash defaulting to "HEAD". Perhaps it would be better to keep git_get_head_hash as warpper around git_get_hash? Or perhaps it would be better to explicitly pass "HEAD" as second parameter?
> > Signed-off-by: Mark Rada <marada@uwaterloo.ca>
It looks like a good idea, even if it doesn't make much sense as a standalone patch.
Show 16 quoted lines
> ---
> gitweb/gitweb.perl | 31 ++++++++++++++++---------------
> 1 files changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 24b2193..d650188 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1981,13 +1981,14 @@ sub quote_command {
> map { my $a = $_; $a =~ s/(['!])/'\\$1'/g; "'$a'" } @_ );
> }
>
> -# get HEAD ref of given project as hash
> -sub git_get_head_hash {
> +# get object id of given project as full hash, defaults to HEAD
> +sub git_get_hash {I'm not sure about naming (not that git_get_head_hash was best), as you don't see from git_get_hash subroutine name that the first parameter is the name of repository (the git_get_head_hash at least hinted, if very weekly, at it).
Show 7 quoted lines
> my $project = shift;
> + my $hash = shift || 'HEAD';
> my $o_git_dir = $git_dir;
> my $retval = undef;
> $git_dir = "$projectroot/$project";
> - if (open my $fd, "-|", git_cmd(), "rev-parse", "--verify", "HEAD") {
> + if (open my $fd, '-|', git_cmd(), 'rev-parse', '--verify', "$hash") {Minor nit: we don't need to quote (stringify) single variable here; using ..., '--verify', $hash) would work as well.
Show 9 quoted lines
> my $head = <$fd>;
> close $fd;
> if (defined $head && $head =~ /^([0-9a-fA-F]{40})$/) {
> @@ -4737,7 +4738,7 @@ sub git_summary {
> }
>
> sub git_tag {
> - my $head = git_get_head_hash($project);
> + my $head = &git_get_hash($project);One very rarely needs to use explicit "&" subroutine calling syntax; I think only when you want to circumvent subroutine prototype (which is different from function/procedure prototypes in other languages). Current practice is to not use it.
Besides it it not used anywhere else in gitweb (consistency).
Show 9 quoted lines
> git_header_html();
> git_print_page_nav('','', $head,undef,$head);
> my %tag = parse_tag($hash);
> @@ -4778,7 +4779,7 @@ sub git_blame {
>
> # error checking
> die_error(400, "No file name given") unless $file_name;
> - $hash_base ||= git_get_head_hash($project);
> + $hash_base ||= &git_get_hash($project);Same as above: use "git_get_hash($project)" or "git_get_hash($project, 'HEAD');"
Show 9 quoted lines
> die_error(404, "Couldn't find base commit") unless $hash_base;
> my %co = parse_commit($hash_base)
> or die_error(404, "Commit not found");
> @@ -4911,7 +4912,7 @@ HTML
> }
>
> sub git_tags {
> - my $head = git_get_head_hash($project);
> + my $head = &git_get_hash($project);Same as above: use "git_get_hash($project)"
Show 9 quoted lines
> git_header_html();
> git_print_page_nav('','', $head,undef,$head);
> git_print_header_div('summary', $project);
> @@ -4924,7 +4925,7 @@ sub git_tags {
> }
>
> sub git_heads {
> - my $head = git_get_head_hash($project);
> + my $head = &git_get_hash($project);Same as above: use "git_get_hash($project)"
Show 9 quoted lines
> git_header_html();
> git_print_page_nav('','', $head,undef,$head);
> git_print_header_div('summary', $project);
> @@ -4942,7 +4943,7 @@ sub git_blob_plain {
>
> if (!defined $hash) {
> if (defined $file_name) {
> - my $base = $hash_base || git_get_head_hash($project);
> + my $base = $hash_base || &git_get_hash($project);Same as above: use "git_get_hash($project)" [...]
-- Jakub Narebski Poland