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@xxxxxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html