[PATCH v5 1/2] gitweb: check given hash before trying to create snapshot

[PATCH v5 1/2] gitweb: check given hash before trying to create snapshot

[Mark Rada]

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.

Tests for t9501 are included to demonstrate added functionality.

Signed-off-by: Mark Rada <marada@uwaterloo.ca>
---

	Changes since v4:
		- used Jakub's suggestion for checking hash validity
			- moved git_get_full_hash to the second commit
		- changed test cases format, suggested by Junio
		- added another test case for tagged objects due to the
		  bug Junio pointed out

	Sorry it's been a while since the v4, school started and I got
	buried under a whole lot of other things I had to take care of
	first. I've got time now, so further fix ups will happen in a
	more reasonable time frame (but hopefully aren't needed!).


 gitweb/gitweb.perl                       |    7 ++++-
 t/t9501-gitweb-standalone-http-status.sh |   39 ++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 24b2193..8d4a2ae 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5196,8 +5196,11 @@ sub git_snapshot {
 		die_error(403, "Unsupported snapshot format");
 	}
 
-	if (!defined $hash) {
-		$hash = git_get_head_hash($project);
+	my $type = git_get_type("$hash^{}");
+	if (!$type) {
+		die_error(404, 'Object does not exist');
+	}  elsif ($type eq 'blob') {
+		die_error(400, 'Object is not a tree-ish');
 	}
 
 	my $name = $project;
diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
index d0ff21d..0688a57 100644
--- a/t/t9501-gitweb-standalone-http-status.sh
+++ b/t/t9501-gitweb-standalone-http-status.sh
@@ -75,4 +75,43 @@ test_expect_success \
 test_debug 'cat gitweb.output'
 
 
+# ----------------------------------------------------------------------
+# snapshot hash ids
+
+test_expect_success 'snapshots: good tree-ish 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 tree-ish id' '
+	gitweb_run "p=.git;a=snapshot;h=frizzumFrazzum;sf=tgz" &&
+	grep "404 - Object does not exist" gitweb.output
+'
+test_debug 'cat gitweb.output'
+
+test_expect_success 'snapshots: bad tree-ish id (tagged object)' '
+	echo object > tag-object &&
+	git add tag-object &&
+	git commit -m "Object to be tagged" &&
+	git tag tagged-object `git hash-object tag-object` &&
+	gitweb_run "p=.git;a=snapshot;h=tagged-object;sf=tgz" &&
+	grep "400 - Object is not a tree-ish" 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 - Object does not exist" gitweb.output
+'
+test_debug 'cat gitweb.output'
+
+
 test_done
-- 
1.6.4.GIT

Re: [PATCH v5 1/2] gitweb: check given hash before trying to create snapshot

[Jakub Narebski]

On Sat, 26 Sep 2009, Mark Rada wrote:

> 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.
> 
> Tests for t9501 are included to demonstrate added functionality.
> 
> Signed-off-by: Mark Rada <marada@uwaterloo.ca>
> ---

Very nice, and I think robust solution.

Acked-by: Jakub Narebski <jnareb@gmail.com>

[...]
> diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
> index d0ff21d..0688a57 100644
> --- a/t/t9501-gitweb-standalone-http-status.sh
> +++ b/t/t9501-gitweb-standalone-http-status.sh

BTW. the rest of test scripts are executable, but not this one? Why?
(But correcting this should be done, if needed, in separate commit).

> @@ -75,4 +75,43 @@ test_expect_success \
>  test_debug 'cat gitweb.output'
>  
>  
> +# ----------------------------------------------------------------------
> +# snapshot hash ids
> +
> +test_expect_success 'snapshots: good tree-ish id' '
> +	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
> +	grep "Status: 200 OK" gitweb.output
> +'
> +test_debug 'cat gitweb.output'
> +

What *could* be improved (but don't *need to*) is to check also HTTP
status and not only formatted error message:

> +test_expect_success 'snapshots: bad tree-ish id' '
> +	gitweb_run "p=.git;a=snapshot;h=frizzumFrazzum;sf=tgz" &&
  +	grep "Status: 404 Not Found" gitweb.output &&
> +	grep "404 - Object does not exist" gitweb.output
> +'
> +test_debug 'cat gitweb.output'

And similarly in other cases.  But it is not something required.
I think what it is now is good enough.

-- 
Jakub Narebski
Poland