Re: [RFC/PATCH 1/2] gitweb: extend &git_get_head_hash to be &git_get_hash

[Jakub Narebski <jnareb@gmail.com>]

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.

> ---
>  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).

>  	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.

>  		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).

>  	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');"

>  	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)"

>  	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)"

>  	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