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

[Mark Rada <marada@uwaterloo.ca>]

On 2009-09-11, at 3:52 AM, Jakub Narebski wrote:

> 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'.

Ah, yeah, well, I let $hash still hold the originally passed value,  
which
would be used to create the outputted file name (unless overwritten by  
the
client by using curl -o or something). Then $snapshot holds the full  
hash.
This way, if you were to type something like

http://git.kernel.org/?p=git/git.git;a=snapshot;h=next;sf=tgz

into your browser window, you would get git.git-next.tar.gz back, but  
the
backend could look up something like

git-5f6b0ffff13f5cd762d0a5a4e1c4dede58e8a537.tar.gz

using the $snapshot variable in some hypothetical cache (or even  
without the
cache it won't mangle the nicer name $hash might have).

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.

Your point about adding the short hash to snapshots of branch heads is  
cool,
I'll try that for the next version of the patch.


>> 	}
>>
>> 	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?

I'm not sure which test you are referring to (I think the second). The
second test is valid and should return a nice .git-master.tar.gz
tarball.


>> 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.

Sorry for promoting vapourware, I did originally rip this patch out from
something else. I removed the comment from the v2 commit message.



--
Mark Rada (ferrous26)
marada@uwaterloo.ca