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

[Jakub Narebski <jnareb@gmail.com>]

[This mail was very strangely wrapped; I fixed this for readability]

On Fri, 11 Sep 2009, Mark Rada wrote:
> 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).

Or by specifying different file name than proposed by browser.

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

O.K.

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

The '--verify' option is needed because git-rev-parse would otherwise
pass parameters it does not understand 'as is'.  Compare

  $ git rev-parse --verify v9.9.9  2>/dev/null

  $ git rev-parse          v9.9.9  2>/dev/null
  v9.9.9

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

I think it would be better left for a _separate_ patch, as it is
separate feature (and I guess more complicated one).

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

The output of CGI script like gitweb (and therefore gitweb.output file,
as it is generated now) contains both HTTP headers, separated by single
empty CRLF delimited line from the proper output of a script.

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

Ah. O.K. then.

-- 
Jakub Narebski
Poland