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@xxxxxxxxxxxx>
> ---
> 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
--
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