{"thread":{"id":"20898","subject":"[RFC/PATCH 2/2] gitweb: check given hash before trying to create snapshot","startedAt":"2009-09-10T21:20:47Z","lastAt":"2009-09-11T16:42:34Z","messageCount":4,"participants":["Mark Rada","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"122845","messageId":"4AA96DAF.4070200@mailservices.uwaterloo.ca","threadId":"20898","inReplyTo":null,"subject":"[RFC/PATCH 2/2] gitweb: check given hash before trying to create snapshot","fromName":"Mark Rada","fromEmail":"marada@uwaterloo.ca","sentAt":"2009-09-10T21:20:47Z","receivedAt":"2009-09-10T21:20:47Z","isPatch":true,"sender":{"key":"marada@uwaterloo.ca","avatar":"https://avatars.githubusercontent.com/u/38430?v=4"},"body":"This patch is dual purposed.\n\nFirst, it makes things nicer in cases when you hand craft the snapshot\nURL but make a typo (e.g. netx instead of next); you will now get an\nerror message instead of a broken tarball.\n\nSecond, any given treeish will always be translated to the full length,\nunambiguous, hash id; this will be useful for things like creating\nunique names for snapshot caches.\n\nThis patch includes test for t9501 to demonstrate the changed\nfunctionality.\n\nSigned-off-by: Mark Rada <marada@uwaterloo.ca>\n---\n gitweb/gitweb.perl                       |    5 +++--\n t/t9501-gitweb-standalone-http-status.sh |   26 ++++++++++++++++++++++++++\n 2 files changed, 29 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex d650188..4ae960c 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5197,8 +5197,9 @@ sub git_snapshot {\n \t\tdie_error(403, \"Unsupported snapshot format\");\n \t}\n \n-\tif (!defined $hash) {\n-\t\t$hash = &git_get_hash($project);\n+\tmy $snapshot = &git_get_hash($project, $hash);\n+\tif (!$snapshot) {\n+\t\tdie_error(400, \"Not a valid hash id: $hash\");\n \t}\n \n \tmy $name = $project;\ndiff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh\nindex d0ff21d..4f8f147 100644\n--- a/t/t9501-gitweb-standalone-http-status.sh\n+++ b/t/t9501-gitweb-standalone-http-status.sh\n@@ -75,4 +75,30 @@ test_expect_success \\\n test_debug 'cat gitweb.output'\n \n \n+test_expect_success \\\n+\t'snapshots: bad treeish id' \\\n+\t'gitweb_run \"p=.git;a=snapshot;h=frizzumFrazzum;sf=tgz\" &&\n+\tgrep \"400 - Not a valid hash id:\" gitweb.output'\n+test_debug 'cat gitweb.output'\n+\n+test_expect_success \\\n+\t'snapshots: good treeish id' \\\n+\t'gitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n+\tgrep \"Status: 200 OK\" gitweb.output'\n+test_debug 'cat gitweb.output'\n+\n+test_expect_success \\\n+\t'snapshots: good object id' \\\n+\t'ID=`git rev-parse --verify HEAD` &&\n+\tgitweb_run \"p=.git;a=snapshot;h=$ID;sf=tgz\" &&\n+\tgrep \"Status: 200 OK\" gitweb.output'\n+test_debug 'cat gitweb.output'\n+\n+test_expect_success \\\n+\t'snapshots: bad object id' \\\n+\t'gitweb_run \"p=.git;a=snapshot;h=abcdef01234;sf=tgz\" &&\n+\tgrep \"400 - Not a valid hash id:\" gitweb.output'\n+test_debug 'cat gitweb.output'\n+\n+\n test_done\n-- \n1.6.4.2\n"},{"id":"122877","messageId":"200909110952.50536.jnareb@gmail.com","threadId":"20898","inReplyTo":"4AA96DAF.4070200@mailservices.uwaterloo.ca","subject":"Re: [RFC/PATCH 2/2] gitweb: check given hash before trying to create snapshot","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-09-11T07:52:48Z","receivedAt":"2009-09-11T07:52:48Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 10 Sep 2009, Mark Rada wrote:\n\n> This patch is dual purposed.\n> \n> First, it makes things nicer in cases when you hand craft the snapshot\n> URL but make a typo (e.g. netx instead of next); you will now get an\n> error message instead of a broken tarball.\n\nThis is a very good idea.\n\n> \n> Second, any given treeish will always be translated to the full length,\n> unambiguous, hash id; this will be useful for things like creating\n> unique names for snapshot caches.\n\nBut this is not a good idea, IMHO.\n\nFirst, it introduces feature that nobody uses (at least yet); we can\nintroduce this feature when it is needed instead.\n\nSecond, I'd rather have better names for snapshots than using full SHA-1.\nFor snapshot of 'v1.5.0' of repository 'repo.git' I'd prefer for snapshot\nto be named 'repo-v1.5.0', and for snapshot of 'next' branch of the same\nproject to be named for example 'repo-next-20090909', or perhaps\n'repo-next-2009-09-10T09:16:18' or 'repo-next-20090909-g5f6b0ff',\nor 'repo-v1.6.5-rc0-164-g5f6b0ff'.\n\nI'm not sure what would be the best name of snapshot of given \nsubdirectory...\n\n\nIn short: I'd rather not improve on bad design of using full SHA-1\nin snapshot name.\n\n> \n> This patch includes test for t9501 to demonstrate the changed\n> functionality.\n> \n> Signed-off-by: Mark Rada <marada@uwaterloo.ca>\n> ---\n>  gitweb/gitweb.perl                       |    5 +++--\n>  t/t9501-gitweb-standalone-http-status.sh |   26 ++++++++++++++++++++++++++\n>  2 files changed, 29 insertions(+), 2 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index d650188..4ae960c 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -5197,8 +5197,9 @@ sub git_snapshot {\n>  \t\tdie_error(403, \"Unsupported snapshot format\");\n>  \t}\n>  \n> -\tif (!defined $hash) {\n> -\t\t$hash = &git_get_hash($project);\n> +\tmy $snapshot = &git_get_hash($project, $hash);\n\nSame comment as for PATCH 1/2: don't use '&' subroutine call if it\nis not required.\n\n\n> +\tif (!$snapshot) {\n> +\t\tdie_error(400, \"Not a valid hash id: $hash\");\n\nNote that we don't use user input in _any_ of other error messages;\nyou would probably need to sanitize $hash.\n\nBy the way, wouldn't 404 (Not Found) be a better error code?\n\n>  \t}\n>  \n>  \tmy $name = $project;\n> diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh\n> index d0ff21d..4f8f147 100644\n> --- a/t/t9501-gitweb-standalone-http-status.sh\n> +++ b/t/t9501-gitweb-standalone-http-status.sh\n> @@ -75,4 +75,30 @@ test_expect_success \\\n>  test_debug 'cat gitweb.output'\n>  \n>  \n> +test_expect_success \\\n> +\t'snapshots: bad treeish id' \\\n> +\t'gitweb_run \"p=.git;a=snapshot;h=frizzumFrazzum;sf=tgz\" &&\n> +\tgrep \"400 - Not a valid hash id:\" gitweb.output'\n> +test_debug 'cat gitweb.output'\n> +\n> +test_expect_success \\\n> +\t'snapshots: good treeish id' \\\n> +\t'gitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n> +\tgrep \"Status: 200 OK\" gitweb.output'\n> +test_debug 'cat gitweb.output'\n\nWhy you don't check for \"Status: 400\" too?\n\n> +\n> +test_expect_success \\\n> +\t'snapshots: good object id' \\\n> +\t'ID=`git rev-parse --verify HEAD` &&\n> +\tgitweb_run \"p=.git;a=snapshot;h=$ID;sf=tgz\" &&\n> +\tgrep \"Status: 200 OK\" gitweb.output'\n> +test_debug 'cat gitweb.output'\n> +\n> +test_expect_success \\\n> +\t'snapshots: bad object id' \\\n> +\t'gitweb_run \"p=.git;a=snapshot;h=abcdef01234;sf=tgz\" &&\n> +\tgrep \"400 - Not a valid hash id:\" gitweb.output'\n> +test_debug 'cat gitweb.output'\n> +\n> +\n>  test_done\n> -- \n> 1.6.4.2\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"122895","messageId":"9513F576-4154-4281-8545-81841D59B766@mailservices.uwaterloo.ca","threadId":"20898","inReplyTo":"200909110952.50536.jnareb@gmail.com","subject":"Re: [RFC/PATCH 2/2] gitweb: check given hash before trying to create snapshot","fromName":"Mark Rada","fromEmail":"marada@uwaterloo.ca","sentAt":"2009-09-11T15:44:04Z","receivedAt":"2009-09-11T15:44:04Z","isPatch":true,"sender":{"key":"marada@uwaterloo.ca","avatar":"https://avatars.githubusercontent.com/u/38430?v=4"},"body":"\nOn 2009-09-11, at 3:52 AM, Jakub Narebski wrote:\n\n> Second, I'd rather have better names for snapshots than using full  \n> SHA-1.\n> For snapshot of 'v1.5.0' of repository 'repo.git' I'd prefer for  \n> snapshot\n> to be named 'repo-v1.5.0', and for snapshot of 'next' branch of the  \n> same\n> project to be named for example 'repo-next-20090909', or perhaps\n> 'repo-next-2009-09-10T09:16:18' or 'repo-next-20090909-g5f6b0ff',\n> or 'repo-v1.6.5-rc0-164-g5f6b0ff'.\n\nAh, yeah, well, I let $hash still hold the originally passed value,  \nwhich\nwould be used to create the outputted file name (unless overwritten by  \nthe\nclient by using curl -o or something). Then $snapshot holds the full  \nhash.\nThis way, if you were to type something like\n\nhttp://git.kernel.org/?p=git/git.git;a=snapshot;h=next;sf=tgz\n\ninto your browser window, you would get git.git-next.tar.gz back, but  \nthe\nbackend could look up something like\n\ngit-5f6b0ffff13f5cd762d0a5a4e1c4dede58e8a537.tar.gz\n\nusing the $snapshot variable in some hypothetical cache (or even  \nwithout the\ncache it won't mangle the nicer name $hash might have).\n\nAlso, right now gitweb will not accept tags for hashes. This seems to be\nbecause it passes the --verify option to rev-parse, but the output  \nfrom using\nand not using the verify option seems to be the same (other than also  \naccepting\nall tree-ishes). Could you let me know if there is a good reason not  \nto take\noff the --verify option? Otherwise, I would like to take it off in the  \nnext\nversion of this patch.\n\nYour point about adding the short hash to snapshots of branch heads is  \ncool,\nI'll try that for the next version of the patch.\n\n\n>> \t}\n>>\n>> \tmy $name = $project;\n>> diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501- \n>> gitweb-standalone-http-status.sh\n>> index d0ff21d..4f8f147 100644\n>> --- a/t/t9501-gitweb-standalone-http-status.sh\n>> +++ b/t/t9501-gitweb-standalone-http-status.sh\n>> @@ -75,4 +75,30 @@ test_expect_success \\\n>> test_debug 'cat gitweb.output'\n>>\n>>\n>> +test_expect_success \\\n>> +\t'snapshots: bad treeish id' \\\n>> +\t'gitweb_run \"p=.git;a=snapshot;h=frizzumFrazzum;sf=tgz\" &&\n>> +\tgrep \"400 - Not a valid hash id:\" gitweb.output'\n>> +test_debug 'cat gitweb.output'\n>> +\n>> +test_expect_success \\\n>> +\t'snapshots: good treeish id' \\\n>> +\t'gitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n>> +\tgrep \"Status: 200 OK\" gitweb.output'\n>> +test_debug 'cat gitweb.output'\n>\n> Why you don't check for \"Status: 400\" too?\n\nI'm not sure which test you are referring to (I think the second). The\nsecond test is valid and should return a nice .git-master.tar.gz\ntarball.\n\n\n>> Second, any given treeish will always be translated to the full  \n>> length,\n>> unambiguous, hash id; this will be useful for things like creating\n>> unique names for snapshot caches.\n>\n> But this is not a good idea, IMHO.\n>\n> First, it introduces feature that nobody uses (at least yet); we can\n> introduce this feature when it is needed instead.\n\nSorry for promoting vapourware, I did originally rip this patch out from\nsomething else. I removed the comment from the v2 commit message.\n\n\n\n--\nMark Rada (ferrous26)\nmarada@uwaterloo.ca\n"},{"id":"122900","messageId":"200909111842.36465.jnareb@gmail.com","threadId":"20898","inReplyTo":"9513F576-4154-4281-8545-81841D59B766@mailservices.uwaterloo.ca","subject":"Re: [RFC/PATCH 2/2] gitweb: check given hash before trying to create snapshot","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-09-11T16:42:34Z","receivedAt":"2009-09-11T16:42:34Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"[This mail was very strangely wrapped; I fixed this for readability]\n\nOn Fri, 11 Sep 2009, Mark Rada wrote:\n> On 2009-09-11, at 3:52 AM, Jakub Narebski wrote:\n> \n>> Second, I'd rather have better names for snapshots than using full  SHA-1.\n>> For snapshot of 'v1.5.0' of repository 'repo.git' I'd prefer for snapshot\n>> to be named 'repo-v1.5.0', and for snapshot of 'next' branch of the  \n>> same project to be named for example 'repo-next-20090909', or perhaps\n>> 'repo-next-2009-09-10T09:16:18' or 'repo-next-20090909-g5f6b0ff',\n>> or 'repo-v1.6.5-rc0-164-g5f6b0ff'.\n> \n> Ah, yeah, well, I let $hash still hold the originally passed value,  \n> which would be used to create the outputted file name (unless\n> overwritten by the client by using curl -o or something).\n\nOr by specifying different file name than proposed by browser.\n\n> Then $snapshot holds the full hash. This way, if you were to type\n> something like \n> \n> http://git.kernel.org/?p=git/git.git;a=snapshot;h=next;sf=tgz\n> \n> into your browser window, you would get git.git-next.tar.gz back, but  \n> the backend could look up something like\n> \n> git-5f6b0ffff13f5cd762d0a5a4e1c4dede58e8a537.tar.gz\n> \n> using the $snapshot variable in some hypothetical cache (or even  \n> without the cache it won't mangle the nicer name $hash might have).\n\nO.K.\n\n> \n> Also, right now gitweb will not accept tags for hashes. This seems to be\n> because it passes the --verify option to rev-parse, but the output  \n> from using and not using the verify option seems to be the same (other\n> than also accepting all tree-ishes). Could you let me know if there is\n> a good reason not to take off the --verify option? Otherwise, I would\n> like to take it off in the next version of this patch.\n\nErrr, what?\n\n  $ 5096:[gitweb/web@git]# git rev-parse --verify v1.5.0            \n  6db027ffe03210324939b3dd655c4223ca023b45\n  $ git rev-parse --verify refs/tags/v1.5.0\n  6db027ffe03210324939b3dd655c4223ca023b45\n\nSo it works as intended.  The problem must be in some other place.\n\nThe '--verify' option is needed because git-rev-parse would otherwise\npass parameters it does not understand 'as is'.  Compare\n\n  $ git rev-parse --verify v9.9.9  2>/dev/null\n\n  $ git rev-parse          v9.9.9  2>/dev/null\n  v9.9.9\n\n> \n> Your point about adding the short hash to snapshots of branch heads is\n> cool, I'll try that for the next version of the patch.\n\nI think it would be better left for a _separate_ patch, as it is\nseparate feature (and I guess more complicated one).\n\n>>> diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501- \n>>> gitweb-standalone-http-status.sh\n>>> index d0ff21d..4f8f147 100644\n>>> --- a/t/t9501-gitweb-standalone-http-status.sh\n>>> +++ b/t/t9501-gitweb-standalone-http-status.sh\n>>> @@ -75,4 +75,30 @@ test_expect_success \\\n>>> test_debug 'cat gitweb.output'\n>>>\n>>>\n>>> +test_expect_success \\\n>>> +\t'snapshots: bad treeish id' \\\n>>> +\t'gitweb_run \"p=.git;a=snapshot;h=frizzumFrazzum;sf=tgz\" &&\n>>> +\tgrep \"400 - Not a valid hash id:\" gitweb.output'\n>>> +test_debug 'cat gitweb.output'\n>>> +\n>>> +test_expect_success \\\n>>> +\t'snapshots: good treeish id' \\\n>>> +\t'gitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n>>> +\tgrep \"Status: 200 OK\" gitweb.output'\n>>> +test_debug 'cat gitweb.output'\n>>\n>> Why you don't check for \"Status: 400\" too?\n> \n> I'm not sure which test you are referring to (I think the second). The\n> second test is valid and should return a nice .git-master.tar.gz\n> tarball.\n\nThe output of CGI script like gitweb (and therefore gitweb.output file,\nas it is generated now) contains both HTTP headers, separated by single\nempty CRLF delimited line from the proper output of a script.\n\nIn first test you check that _contents_ contain specific error message,\nbut you do not check if HTTP status code matches it (it should, because\nof how die_error works).  In second test you check HTTP status.  If the\nt/t9501-gitweb-standalone-http-status.sh is to be about status, I guess\nthat you should check HTTP status, and not contents of the page (which\nis more likely to change, e.g. due to some prettifying).\n\nIn t9501 tests you need, I think, only the HTTP headers part, unless\nyou want also to check that the contents matches.  There was some sed\nscript shown to extract only HTTP headers.\n \n>>> Second, any given treeish will always be translated to the full length,\n>>> unambiguous, hash id; this will be useful for things like creating\n>>> unique names for snapshot caches.\n>>\n>> But this is not a good idea, IMHO.\n>>\n>> First, it introduces feature that nobody uses (at least yet); we can\n>> introduce this feature when it is needed instead.\n> \n> Sorry for promoting vapourware, I did originally rip this patch out from\n> something else. I removed the comment from the v2 commit message.\n\nAh. O.K. then.\n\n-- \nJakub Narebski\nPoland\n"}]}