> [This mail was very strangely wrapped; I fixed this for readability]
Yeah, sorry, format=flowed
> In first test you check that _contents_ contain specific error message,
> but you do not check if HTTP status code matches it (it should, because
> of how die_error works). In second test you check HTTP status. If the
> t/t9501-gitweb-standalone-http-status.sh is to be about status, I guess
> that you should check HTTP status, and not contents of the page (which
> is more likely to change, e.g. due to some prettifying).
>
> In t9501 tests you need, I think, only the HTTP headers part, unless
> you want also to check that the contents matches. There was some sed
> script shown to extract only HTTP headers.
Yes, t9501 also checks for the specific error message, this was originally
how it had to be done in order to show what trap was being executed for
that first block of checks in the snapshot routine. The header does not
contain the specific error string that gitweb outputs though, only the
standard message (i.e. 404 Not Found), so I check the script output for
negative test cases.
>> Also, right now gitweb will not accept tags for hashes. This seems to be
>> because it passes the --verify option to rev-parse, but the output
>> from using and not using the verify option seems to be the same (other
>> than also accepting all tree-ishes). Could you let me know if there is
>> a good reason not to take off the --verify option? Otherwise, I would
>> like to take it off in the next version of this patch.
>
> Errr, what?
>
> $ 5096:[gitweb/web@git]# git rev-parse --verify v1.5.0
> 6db027ffe03210324939b3dd655c4223ca023b45
> $ git rev-parse --verify refs/tags/v1.5.0
> 6db027ffe03210324939b3dd655c4223ca023b45
>
> So it works as intended. The problem must be in some other place.
Let's just say the problem is not in the computer. :(
--
Mark Rada (ferrous26)
marada@xxxxxxxxxxxx
--->8---
Makes things nicer in cases when you hand craft the snapshot URL but
make a typo in defining the hash variable (e.g. netx instead of next);
you will now get an error message instead of a broken tarball.
To maintain backwards compatibility, git_get_head_hash is now a wrapper
for git_get_full_hash, as suggested by Jakub Narebski.
Tests for t9501 are included to demonstrate changed functionality.
Signed-off-by: Mark Rada <marada@xxxxxxxxxxxx>
---
gitweb/gitweb.perl | 13 ++++++++++---
t/t9501-gitweb-standalone-http-status.sh | 29 +++++++++++++++++++++++++++++
2 files changed, 39 insertions(+), 3 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 24b2193..e9cac8d 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1983,11 +1983,17 @@ sub quote_command {
# get HEAD ref of given project as hash
sub git_get_head_hash {
+ return git_get_full_hash(shift, 'HEAD');
+}
+
+# given a project and tree-ish, returns full hash
+sub git_get_full_hash {
my $project = shift;
+ my $hash = shift;
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})$/) {
@@ -5196,8 +5202,9 @@ sub git_snapshot {
die_error(403, "Unsupported snapshot format");
}
- if (!defined $hash) {
- $hash = git_get_head_hash($project);
+ my $snapshot = git_get_full_hash($project, $hash);
+ if (!$snapshot) {
+ die_error(404, 'Hash id was not valid');
}
my $name = $project;
diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
index d0ff21d..632007e 100644
--- a/t/t9501-gitweb-standalone-http-status.sh
+++ b/t/t9501-gitweb-standalone-http-status.sh
@@ -75,4 +75,33 @@ test_expect_success \
test_debug 'cat gitweb.output'
+# ----------------------------------------------------------------------
+# snapshot hash ids
+
+test_expect_success \
+ 'snapshots: good treeish id' \
+ 'gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+ grep "Status: 200 OK" gitweb.output'
+test_debug 'cat gitweb.output'
+
+test_expect_success \
+ 'snapshots: bad treeish id' \
+ 'gitweb_run "p=.git;a=snapshot;h=frizzumFrazzum;sf=tgz" &&
+ grep "404 - Hash id was not valid" gitweb.output'
+test_debug 'cat gitweb.output'
+
+test_expect_success \
+ 'snapshots: good object id' \
+ 'ID=`git rev-parse --verify HEAD` &&
+ gitweb_run "p=.git;a=snapshot;h=$ID;sf=tgz" &&
+ grep "Status: 200 OK" gitweb.output'
+test_debug 'cat gitweb.output'
+
+test_expect_success \
+ 'snapshots: bad object id' \
+ 'gitweb_run "p=.git;a=snapshot;h=abcdef01234;sf=tgz" &&
+ grep "404 - Hash id was not valid" gitweb.output'
+test_debug 'cat gitweb.output'
+
+
test_done
--
1.6.4.2
--
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