* [RFC/PATCH 1/2] gitweb: extend &git_get_head_hash to be &git_get_hash
@ 2009-09-10 21:20 Mark Rada
2009-09-10 21:49 ` Jakub Narebski
0 siblings, 1 reply; 2+ messages in thread
From: Mark Rada @ 2009-09-10 21:20 UTC (permalink / raw)
To: git; +Cc: Jakub Narebski, Junio C Hamano
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.
To maintain some backwards compatability, the second argument is
optional and it will default to `HEAD' if not specified.
Signed-off-by: Mark Rada <marada@uwaterloo.ca>
---
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 {
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") {
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);
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);
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);
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);
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);
$hash = git_get_hash_by_path($base, $file_name, "blob")
or die_error(404, "Cannot find file");
} else {
@@ -4994,7 +4995,7 @@ sub git_blob {
if (!defined $hash) {
if (defined $file_name) {
- my $base = $hash_base || git_get_head_hash($project);
+ my $base = $hash_base || &git_get_hash($project);
$hash = git_get_hash_by_path($base, $file_name, "blob")
or die_error(404, "Cannot find file");
} else {
@@ -5197,7 +5198,7 @@ sub git_snapshot {
}
if (!defined $hash) {
- $hash = git_get_head_hash($project);
+ $hash = &git_get_hash($project);
}
my $name = $project;
@@ -5229,7 +5230,7 @@ sub git_snapshot {
}
sub git_log {
- my $head = git_get_head_hash($project);
+ my $head = &git_get_hash($project);
if (!defined $hash) {
$hash = $head;
}
@@ -5833,7 +5834,7 @@ sub git_patches {
sub git_history {
if (!defined $hash_base) {
- $hash_base = git_get_head_hash($project);
+ $hash_base = &git_get_hash($project);
}
if (!defined $page) {
$page = 0;
@@ -5904,7 +5905,7 @@ sub git_search {
die_error(400, "Text field is empty");
}
if (!defined $hash) {
- $hash = git_get_head_hash($project);
+ $hash = &git_get_hash($project);
}
my %co = parse_commit($hash);
if (!%co) {
@@ -6159,7 +6160,7 @@ EOT
}
sub git_shortlog {
- my $head = git_get_head_hash($project);
+ my $head = &git_get_hash($project);
if (!defined $hash) {
$hash = $head;
}
@@ -6486,7 +6487,7 @@ XML
foreach my $pr (@list) {
my %proj = %$pr;
- my $head = git_get_head_hash($proj{'path'});
+ my $head = &git_get_hash($proj{'path'});
if (!defined $head) {
next;
}
--
1.6.4.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [RFC/PATCH 1/2] gitweb: extend &git_get_head_hash to be &git_get_hash
2009-09-10 21:20 [RFC/PATCH 1/2] gitweb: extend &git_get_head_hash to be &git_get_hash Mark Rada
@ 2009-09-10 21:49 ` Jakub Narebski
0 siblings, 0 replies; 2+ messages in thread
From: Jakub Narebski @ 2009-09-10 21:49 UTC (permalink / raw)
To: Mark Rada; +Cc: git, Junio C Hamano
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2009-09-10 21:49 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-09-10 21:20 [RFC/PATCH 1/2] gitweb: extend &git_get_head_hash to be &git_get_hash Mark Rada
2009-09-10 21:49 ` Jakub Narebski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).