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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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


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

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]