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

[Jakub Narebski <jnareb@gmail.com>]

On Thu, 10 Sep 2009, Mark Rada wrote:

> This patch is dual purposed.
> 
> First, it makes things nicer in cases when you hand craft the snapshot
> URL but make a typo (e.g. netx instead of next); you will now get an
> error message instead of a broken tarball.

This is a very good idea.

> 
> Second, any given treeish will always be translated to the full length,
> unambiguous, hash id; this will be useful for things like creating
> unique names for snapshot caches.

But this is not a good idea, IMHO.

First, it introduces feature that nobody uses (at least yet); we can
introduce this feature when it is needed instead.

Second, I'd rather have better names for snapshots than using full SHA-1.
For snapshot of 'v1.5.0' of repository 'repo.git' I'd prefer for snapshot
to be named 'repo-v1.5.0', and for snapshot of 'next' branch of the same
project to be named for example 'repo-next-20090909', or perhaps
'repo-next-2009-09-10T09:16:18' or 'repo-next-20090909-g5f6b0ff',
or 'repo-v1.6.5-rc0-164-g5f6b0ff'.

I'm not sure what would be the best name of snapshot of given 
subdirectory...


In short: I'd rather not improve on bad design of using full SHA-1
in snapshot name.

> 
> This patch includes test for t9501 to demonstrate the changed
> functionality.
> 
> Signed-off-by: Mark Rada <marada@uwaterloo.ca>
> ---
>  gitweb/gitweb.perl                       |    5 +++--
>  t/t9501-gitweb-standalone-http-status.sh |   26 ++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index d650188..4ae960c 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -5197,8 +5197,9 @@ sub git_snapshot {
>  		die_error(403, "Unsupported snapshot format");
>  	}
>  
> -	if (!defined $hash) {
> -		$hash = &git_get_hash($project);
> +	my $snapshot = &git_get_hash($project, $hash);

Same comment as for PATCH 1/2: don't use '&' subroutine call if it
is not required.


> +	if (!$snapshot) {
> +		die_error(400, "Not a valid hash id: $hash");

Note that we don't use user input in _any_ of other error messages;
you would probably need to sanitize $hash.

By the way, wouldn't 404 (Not Found) be a better error code?

>  	}
>  
>  	my $name = $project;
> diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
> index d0ff21d..4f8f147 100644
> --- a/t/t9501-gitweb-standalone-http-status.sh
> +++ b/t/t9501-gitweb-standalone-http-status.sh
> @@ -75,4 +75,30 @@ test_expect_success \
>  test_debug 'cat gitweb.output'
>  
>  
> +test_expect_success \
> +	'snapshots: bad treeish id' \
> +	'gitweb_run "p=.git;a=snapshot;h=frizzumFrazzum;sf=tgz" &&
> +	grep "400 - Not a valid hash id:" gitweb.output'
> +test_debug 'cat gitweb.output'
> +
> +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'

Why you don't check for "Status: 400" too?

> +
> +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 "400 - Not a valid hash id:" gitweb.output'
> +test_debug 'cat gitweb.output'
> +
> +
>  test_done
> -- 
> 1.6.4.2
> 
> 

-- 
Jakub Narebski
Poland