Re: [RFC/PATCH 2/2] gitweb: check given hash before trying to create snapshot
- From
Mark Rada <marada@uwaterloo.ca>
- Date
- Sep 11, 2009, 15:44 UTC
- Message-ID
- <9513F576-4154-4281-8545-81841D59B766@mailservices.uwaterloo.ca>
- In-Reply-To
- <200909110952.50536.jnareb@gmail.com>
On 2009-09-11, at 3:52 AM, Jakub Narebski wrote:
Show 9 quoted lines
> 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.
Show 25 quoted lines
>> } >> >> 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.
Show 9 quoted lines
>> 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