{"thread":{"id":"21067","subject":"[PATCH v5 1/2] gitweb: check given hash before trying to create snapshot","startedAt":"2009-09-26T17:46:08Z","lastAt":"2009-10-01T08:13:57Z","messageCount":2,"participants":["Mark Rada","Jakub Narebski"],"isPatch":true,"patchVersion":5,"patchTotal":2},"messages":[{"id":"123850","messageId":"4ABE5360.8090204@mailservices.uwaterloo.ca","threadId":"21067","inReplyTo":null,"subject":"[PATCH v5 1/2] gitweb: check given hash before trying to create snapshot","fromName":"Mark Rada","fromEmail":"marada@uwaterloo.ca","sentAt":"2009-09-26T17:46:08Z","receivedAt":"2009-09-26T17:46:08Z","isPatch":true,"sender":{"key":"marada@uwaterloo.ca","avatar":"https://avatars.githubusercontent.com/u/38430?v=4"},"body":"Makes things nicer in cases when you hand craft the snapshot URL but\nmake a typo in defining the hash variable (e.g. netx instead of next);\nyou will now get an error message instead of a broken tarball.\n\nTests for t9501 are included to demonstrate added functionality.\n\nSigned-off-by: Mark Rada <marada@uwaterloo.ca>\n---\n\n\tChanges since v4:\n\t\t- used Jakub's suggestion for checking hash validity\n\t\t\t- moved git_get_full_hash to the second commit\n\t\t- changed test cases format, suggested by Junio\n\t\t- added another test case for tagged objects due to the\n\t\t  bug Junio pointed out\n\n\tSorry it's been a while since the v4, school started and I got\n\tburied under a whole lot of other things I had to take care of\n\tfirst. I've got time now, so further fix ups will happen in a\n\tmore reasonable time frame (but hopefully aren't needed!).\n\n\n gitweb/gitweb.perl                       |    7 ++++-\n t/t9501-gitweb-standalone-http-status.sh |   39 ++++++++++++++++++++++++++++++\n 2 files changed, 44 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 24b2193..8d4a2ae 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5196,8 +5196,11 @@ sub git_snapshot {\n \t\tdie_error(403, \"Unsupported snapshot format\");\n \t}\n \n-\tif (!defined $hash) {\n-\t\t$hash = git_get_head_hash($project);\n+\tmy $type = git_get_type(\"$hash^{}\");\n+\tif (!$type) {\n+\t\tdie_error(404, 'Object does not exist');\n+\t}  elsif ($type eq 'blob') {\n+\t\tdie_error(400, 'Object is not a tree-ish');\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..0688a57 100644\n--- a/t/t9501-gitweb-standalone-http-status.sh\n+++ b/t/t9501-gitweb-standalone-http-status.sh\n@@ -75,4 +75,43 @@ test_expect_success \\\n test_debug 'cat gitweb.output'\n \n \n+# ----------------------------------------------------------------------\n+# snapshot hash ids\n+\n+test_expect_success 'snapshots: good tree-ish id' '\n+\tgitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n+\tgrep \"Status: 200 OK\" gitweb.output\n+'\n+test_debug 'cat gitweb.output'\n+\n+test_expect_success 'snapshots: bad tree-ish id' '\n+\tgitweb_run \"p=.git;a=snapshot;h=frizzumFrazzum;sf=tgz\" &&\n+\tgrep \"404 - Object does not exist\" gitweb.output\n+'\n+test_debug 'cat gitweb.output'\n+\n+test_expect_success 'snapshots: bad tree-ish id (tagged object)' '\n+\techo object > tag-object &&\n+\tgit add tag-object &&\n+\tgit commit -m \"Object to be tagged\" &&\n+\tgit tag tagged-object `git hash-object tag-object` &&\n+\tgitweb_run \"p=.git;a=snapshot;h=tagged-object;sf=tgz\" &&\n+\tgrep \"400 - Object is not a tree-ish\" gitweb.output\n+'\n+test_debug 'cat gitweb.output'\n+\n+test_expect_success 'snapshots: good object id' '\n+\tID=`git rev-parse --verify HEAD` &&\n+\tgitweb_run \"p=.git;a=snapshot;h=$ID;sf=tgz\" &&\n+\tgrep \"Status: 200 OK\" gitweb.output\n+'\n+test_debug 'cat gitweb.output'\n+\n+test_expect_success 'snapshots: bad object id' '\n+\tgitweb_run \"p=.git;a=snapshot;h=abcdef01234;sf=tgz\" &&\n+\tgrep \"404 - Object does not exist\" gitweb.output\n+'\n+test_debug 'cat gitweb.output'\n+\n+\n test_done\n-- \n1.6.4.GIT\n"},{"id":"124070","messageId":"200910011013.58899.jnareb@gmail.com","threadId":"21067","inReplyTo":"4ABE5360.8090204@mailservices.uwaterloo.ca","subject":"Re: [PATCH v5 1/2] gitweb: check given hash before trying to create snapshot","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-10-01T08:13:57Z","receivedAt":"2009-10-01T08:13:57Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 26 Sep 2009, Mark Rada wrote:\n\n> Makes things nicer in cases when you hand craft the snapshot URL but\n> make a typo in defining the hash variable (e.g. netx instead of next);\n> you will now get an error message instead of a broken tarball.\n> \n> Tests for t9501 are included to demonstrate added functionality.\n> \n> Signed-off-by: Mark Rada <marada@uwaterloo.ca>\n> ---\n\nVery nice, and I think robust solution.\n\nAcked-by: Jakub Narebski <jnareb@gmail.com>\n\n[...]\n> diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh\n> index d0ff21d..0688a57 100644\n> --- a/t/t9501-gitweb-standalone-http-status.sh\n> +++ b/t/t9501-gitweb-standalone-http-status.sh\n\nBTW. the rest of test scripts are executable, but not this one? Why?\n(But correcting this should be done, if needed, in separate commit).\n\n> @@ -75,4 +75,43 @@ test_expect_success \\\n>  test_debug 'cat gitweb.output'\n>  \n>  \n> +# ----------------------------------------------------------------------\n> +# snapshot hash ids\n> +\n> +test_expect_success 'snapshots: good tree-ish id' '\n> +\tgitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n> +\tgrep \"Status: 200 OK\" gitweb.output\n> +'\n> +test_debug 'cat gitweb.output'\n> +\n\nWhat *could* be improved (but don't *need to*) is to check also HTTP\nstatus and not only formatted error message:\n\n> +test_expect_success 'snapshots: bad tree-ish id' '\n> +\tgitweb_run \"p=.git;a=snapshot;h=frizzumFrazzum;sf=tgz\" &&\n  +\tgrep \"Status: 404 Not Found\" gitweb.output &&\n> +\tgrep \"404 - Object does not exist\" gitweb.output\n> +'\n> +test_debug 'cat gitweb.output'\n\nAnd similarly in other cases.  But it is not something required.\nI think what it is now is good enough.\n\n-- \nJakub Narebski\nPoland\n"}]}