{"thread":{"id":"21068","subject":"[PATCH v5 2/2] gitweb: append short hash ids to snapshot files","startedAt":"2009-09-26T17:46:21Z","lastAt":"2009-10-13T23:46:40Z","messageCount":4,"participants":["Mark Rada","Jakub Narebski"],"isPatch":true,"patchVersion":5,"patchTotal":2},"messages":[{"id":"123851","messageId":"4ABE536D.3070705@mailservices.uwaterloo.ca","threadId":"21068","inReplyTo":null,"subject":"[PATCH v5 2/2] gitweb: append short hash ids to snapshot files","fromName":"Mark Rada","fromEmail":"marada@uwaterloo.ca","sentAt":"2009-09-26T17:46:21Z","receivedAt":"2009-09-26T17:46:21Z","isPatch":true,"sender":{"key":"marada@uwaterloo.ca","avatar":"https://avatars.githubusercontent.com/u/38430?v=4"},"body":"Teach gitweb how to produce nicer snapshot names by only using the\nshort hash id. If clients make requests using a tree-ish that is not a\npartial or full SHA-1 hash, then the short hash will also be appended\nto whatever they asked for.\n\nThis also includes tests cases for t9502-gitweb-standalone-parse-output.\n\nSigned-off-by: Mark Rada <marada@uwaterloo.ca>\n---\n\n\n\tChanges since v4:\n\t\t- moved git_get_full_hash into this commit\n\t\t- changed test case format, suggested by Junio\n\t\t- explicity request at least a length of 7 for short hashes\n\n\n gitweb/gitweb.perl                        |   40 +++++++++++++++--\n t/t9502-gitweb-standalone-parse-output.sh |   67 +++++++++++++++++++++++++++++\n 2 files changed, 103 insertions(+), 4 deletions(-)\n create mode 100644 t/t9502-gitweb-standalone-parse-output.sh\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 8d4a2ae..bc132a5 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1983,14 +1983,39 @@ sub quote_command {\n \n # get HEAD ref of given project as hash\n sub git_get_head_hash {\n+\treturn git_get_full_hash(shift, 'HEAD');\n+}\n+\n+sub git_get_full_hash {\n \tmy $project = shift;\n+\tmy $hash = shift;\n \tmy $o_git_dir = $git_dir;\n \tmy $retval = undef;\n \t$git_dir = \"$projectroot/$project\";\n-\tif (open my $fd, \"-|\", git_cmd(), \"rev-parse\", \"--verify\", \"HEAD\") {\n-\t\tmy $head = <$fd>;\n+\tif (open my $fd, '-|', git_cmd(), 'rev-parse', '--verify', $hash) {\n+\t\t$hash = <$fd>;\n \t\tclose $fd;\n-\t\tif (defined $head && $head =~ /^([0-9a-fA-F]{40})$/) {\n+\t\tif (defined $hash && $hash =~ /^([0-9a-fA-F]{40})$/) {\n+\t\t\t$retval = $1;\n+\t\t}\n+\t}\n+\tif (defined $o_git_dir) {\n+\t\t$git_dir = $o_git_dir;\n+\t}\n+\treturn $retval;\n+}\n+\n+# try and get a shorter hash id\n+sub git_get_short_hash {\n+\tmy $project = shift;\n+\tmy $hash = shift;\n+\tmy $o_git_dir = $git_dir;\n+\tmy $retval = undef;\n+\t$git_dir = \"$projectroot/$project\";\n+\tif (open my $fd, '-|', git_cmd(), 'rev-parse', '--short=7', $hash) {\n+\t\t$hash = <$fd>;\n+\t\tclose $fd;\n+\t\tif (defined $hash && $hash =~ /^([0-9a-fA-F]{7,})$/) {\n \t\t\t$retval = $1;\n \t\t}\n \t}\n@@ -5203,6 +5228,13 @@ sub git_snapshot {\n \t\tdie_error(400, 'Object is not a tree-ish');\n \t}\n \n+\n+\tmy $full_hash = git_get_full_hash($project, $hash);\n+\tif ($full_hash =~ /^$hash/) {\n+\t\t$hash = git_get_short_hash($project, $hash);\n+\t} else {\n+\t\t$hash .= '-' . git_get_short_hash($project, $hash);\n+\t}\n \tmy $name = $project;\n \t$name =~ s,([^/])/*\\.git$,$1,;\n \t$name = basename($name);\n@@ -5213,7 +5245,7 @@ sub git_snapshot {\n \t$cmd = quote_command(\n \t\tgit_cmd(), 'archive',\n \t\t\"--format=$known_snapshot_formats{$format}{'format'}\",\n-\t\t\"--prefix=$name/\", $hash);\n+\t\t\"--prefix=$name/\", $full_hash);\n \tif (exists $known_snapshot_formats{$format}{'compressor'}) {\n \t\t$cmd .= ' | ' . quote_command(@{$known_snapshot_formats{$format}{'compressor'}});\n \t}\ndiff --git a/t/t9502-gitweb-standalone-parse-output.sh b/t/t9502-gitweb-standalone-parse-output.sh\nnew file mode 100644\nindex 0000000..5f2b1d5\n--- /dev/null\n+++ b/t/t9502-gitweb-standalone-parse-output.sh\n@@ -0,0 +1,67 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2009 Mark Rada\n+#\n+\n+test_description='gitweb as standalone script (parsing script output).\n+\n+This test runs gitweb (git web interface) as a CGI script from the\n+commandline, and checks that it produces the correct output, either\n+in the HTTP header or the actual script output.'\n+\n+\n+. ./gitweb-lib.sh\n+\n+# ----------------------------------------------------------------------\n+# snapshot file name\n+\n+test_commit \\\n+\t'SnapshotFileTests' \\\n+\t'i can has snapshot?'\n+\n+test_expect_success 'snapshots: give full hash' '\n+\tID=`git rev-parse --verify HEAD` &&\n+\tgitweb_run \"p=.git;a=snapshot;h=$ID;sf=tgz\" &&\n+\tID=`git rev-parse --short HEAD` &&\n+\tgrep \".git-$ID.tar.gz\" gitweb.output\n+'\n+test_debug 'cat gitweb.output'\n+\n+test_expect_success 'snapshots: give short hash' '\n+\tID=`git rev-parse --short HEAD` &&\n+\tgitweb_run \"p=.git;a=snapshot;h=$ID;sf=tgz\" &&\n+\tgrep \".git-$ID.tar.gz\" gitweb.output\n+'\n+test_debug 'cat gitweb.output'\n+\n+test_expect_success 'snapshots: give almost full hash' '\n+\tID=`git rev-parse --short=30 HEAD` &&\n+\tgitweb_run \"p=.git;a=snapshot;h=$ID;sf=tgz\" &&\n+\tID=`git rev-parse --short HEAD` &&\n+\tgrep \".git-$ID.tar.gz\" gitweb.output\n+'\n+test_debug 'cat gitweb.output'\n+\n+test_expect_success 'snapshots: give HEAD tree-ish' '\n+\tgitweb_run \"p=.git;a=snapshot;h=HEAD;sf=tgz\" &&\n+\tID=`git rev-parse --short HEAD` &&\n+\tgrep \".git-HEAD-$ID.tar.gz\" gitweb.output\n+'\n+test_debug 'cat gitweb.output'\n+\n+test_expect_success 'snapshots: give branch name tree-ish' '\n+\tgitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n+\tID=`git rev-parse --short master` &&\n+\tgrep \".git-master-$ID.tar.gz\" gitweb.output\n+'\n+test_debug 'cat gitweb.output'\n+\n+test_expect_success 'snapshots: give tag tree-ish' '\n+\tgitweb_run \"p=.git;a=snapshot;h=SnapshotFileTests;sf=tgz\" &&\n+\tID=`git rev-parse --short SnapshotFileTests` &&\n+\tgrep \".git-SnapshotFileTests-$ID.tar.gz\" gitweb.output\n+'\n+test_debug 'cat gitweb.output'\n+\n+\n+test_done\n-- \n1.6.4.GIT\n"},{"id":"124237","messageId":"200910051206.18943.jnareb@gmail.com","threadId":"21068","inReplyTo":"4ABE536D.3070705@mailservices.uwaterloo.ca","subject":"Re: [PATCH v5 2/2] gitweb: append short hash ids to snapshot files","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-10-05T10:06:17Z","receivedAt":"2009-10-05T10:06:17Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"I am sorry for being late with review of this patch.\n\nOn Sat, 26 Sep 2009, Mark Rada wrote:\n\n> Teach gitweb how to produce nicer snapshot names by only using the\n> short hash id.\n\nA few questions (which I think should be answered in this commit message).\n\nFirst, what was original behaviour of 'snapshot' action?  Did gitweb\nalways convert 'h' (hash) parameter to full SHA-1?\n\nSecond, do you preserve 'snapshot' behavior that it generated archive\nwhich unpacks to the directory with the same name as basename of (proposed)\narchive name?  I mean here that \"repo-2cc6859e.tar.gz\" unpacks to\n\"repo-2cc6859e/\" directory.  I guess it does, but this should be\nmentioned in the commit message.\n\n>               If clients make requests using a tree-ish that is not a\n> partial or full SHA-1 hash, then the short hash will also be appended\n> to whatever they asked for.\n\nHere example would be a good idea.  I guess this means that if one is\nrequesting for snapshot of 'next' or 'v1.6.0' of 'repo.git' project,\none would get 'repo-next-2cc6859.tar.gz' or 'repo-v1.6.0-2cc6859.tar.gz'\nas [proposed] snapshot filename.\n\n> \n> This also includes tests cases for t9502-gitweb-standalone-parse-output.\n\nI am not sure if it shouldn't be rather t9502-gitweb-standalone-snapshot\ntest; see comments below for whys.\n\n> \n> Signed-off-by: Mark Rada <marada@uwaterloo.ca>\n> ---\n> \n> \n> \tChanges since v4:\n> \t\t- moved git_get_full_hash into this commit\n> \t\t- changed test case format, suggested by Junio\n> \t\t- explicity request at least a length of 7 for short hashes\n> \n> \n>  gitweb/gitweb.perl                        |   40 +++++++++++++++--\n>  t/t9502-gitweb-standalone-parse-output.sh |   67 +++++++++++++++++++++++++++++\n>  2 files changed, 103 insertions(+), 4 deletions(-)\n>  create mode 100644 t/t9502-gitweb-standalone-parse-output.sh\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 8d4a2ae..bc132a5 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1983,14 +1983,39 @@ sub quote_command {\n>  \n>  # get HEAD ref of given project as hash\n>  sub git_get_head_hash {\n> +\treturn git_get_full_hash(shift, 'HEAD');\n> +}\n\nGood.  This means that we don't need to change code that do not use\nnew feature (new subroutines).  It's nice to cater to backward \ncompatibility, especially if it is so low cost as this one.\n\n> +\n> +sub git_get_full_hash {\n>  \tmy $project = shift;\n> +\tmy $hash = shift;\n\nI think it might be good idea here to default to 'HEAD', i.e.\n\n  +\tmy $hash = shift || 'HEAD';\n\n>  \tmy $o_git_dir = $git_dir;\n>  \tmy $retval = undef;\n>  \t$git_dir = \"$projectroot/$project\";\n> -\tif (open my $fd, \"-|\", git_cmd(), \"rev-parse\", \"--verify\", \"HEAD\") {\n> -\t\tmy $head = <$fd>;\n> +\tif (open my $fd, '-|', git_cmd(), 'rev-parse', '--verify', $hash) {\n> +\t\t$hash = <$fd>;\n>  \t\tclose $fd;\n> -\t\tif (defined $head && $head =~ /^([0-9a-fA-F]{40})$/) {\n> +\t\tif (defined $hash && $hash =~ /^([0-9a-fA-F]{40})$/) {\n> +\t\t\t$retval = $1;\n> +\t\t}\n\nI guess that you use \"$retval = $1;\" instead of just \"$retval = $hash;\"\nbecause of similarities with git_get_short_hash, isn't it?  Or it is just\nfollowing earlier code?\n\n> +\t}\n> +\tif (defined $o_git_dir) {\n> +\t\t$git_dir = $o_git_dir;\n> +\t}\n> +\treturn $retval;\n> +}\n> +\n> +# try and get a shorter hash id\n> +sub git_get_short_hash {\n> +\tmy $project = shift;\n> +\tmy $hash = shift;\n> +\tmy $o_git_dir = $git_dir;\n> +\tmy $retval = undef;\n> +\t$git_dir = \"$projectroot/$project\";\n> +\tif (open my $fd, '-|', git_cmd(), 'rev-parse', '--short=7', $hash) {\n> +\t\t$hash = <$fd>;\n> +\t\tclose $fd;\n> +\t\tif (defined $hash && $hash =~ /^([0-9a-fA-F]{7,})$/) {\n>  \t\t\t$retval = $1;\n>  \t\t}\n>  \t}\n\nNote that git_get_full_hash (which additionally does verification) and\ngit_get_short_hash share much of code.  Perhaps it might be worth to\navoid code duplication somehow?  On the other hand it might be not worth\nto complicate code by trying to extract common parts here...\n\n> @@ -5203,6 +5228,13 @@ sub git_snapshot {\n>  \t\tdie_error(400, 'Object is not a tree-ish');\n>  \t}\n>  \n> +\n> +\tmy $full_hash = git_get_full_hash($project, $hash);\n> +\tif ($full_hash =~ /^$hash/) {\n> +\t\t$hash = git_get_short_hash($project, $hash);\n> +\t} else {\n> +\t\t$hash .= '-' . git_get_short_hash($project, $hash);\n> +\t}\n\nI think we might want to avoid calling git_get_full_hash (and extra call\nto \"git rev-parse\" command, which is extra fork) if we know in advance\nthat  $full_hash =~ /^$hash/  can't be true, i.e. if $hash doesn't match\n/^[0-9a-fA-F]+$/.  That would require that we continue to use $hash\nand not $full_hash, see comment for the chunk below.\n\nBTW do you think that having better name (nicer name in the case\nwhen $hash is full SHA-1, or name which describes exact version as \nin the case when $hash is branch name or just 'HEAD') is worth\nslight extra cost of \"git rev-parse --abbrev=7\"?\n\n>  \tmy $name = $project;\n>  \t$name =~ s,([^/])/*\\.git$,$1,;\n>  \t$name = basename($name);\n> @@ -5213,7 +5245,7 @@ sub git_snapshot {\n>  \t$cmd = quote_command(\n>  \t\tgit_cmd(), 'archive',\n>  \t\t\"--format=$known_snapshot_formats{$format}{'format'}\",\n> -\t\t\"--prefix=$name/\", $hash);\n> +\t\t\"--prefix=$name/\", $full_hash);\n\nWhy this change?\n\n>  \tif (exists $known_snapshot_formats{$format}{'compressor'}) {\n>  \t\t$cmd .= ' | ' . quote_command(@{$known_snapshot_formats{$format}{'compressor'}});\n>  \t}\n> diff --git a/t/t9502-gitweb-standalone-parse-output.sh b/t/t9502-gitweb-standalone-parse-output.sh\n> new file mode 100644\n> index 0000000..5f2b1d5\n> --- /dev/null\n> +++ b/t/t9502-gitweb-standalone-parse-output.sh\n> @@ -0,0 +1,67 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2009 Mark Rada\n> +#\n> +\n> +test_description='gitweb as standalone script (parsing script output).\n> +\n> +This test runs gitweb (git web interface) as a CGI script from the\n> +commandline, and checks that it produces the correct output, either\n> +in the HTTP header or the actual script output.'\n\nCurrently all tests here are about 'snapshot' action.  They are quite\nspecific, and they do require some knowledge about chosen archive format.\nI think it would be better to put snapshot test into separate test,\ni.e. in 't/t9502-gitweb-standalone-snapshot.sh'.\n\n> +\n> +\n> +. ./gitweb-lib.sh\n> +\n> +# ----------------------------------------------------------------------\n> +# snapshot file name\n> +\n> +test_commit \\\n> +\t'SnapshotFileTests' \\\n> +\t'i can has snapshot?'\n\nErrr... with filename [cutely] called 'i can has snapshot?' you would\nhave, I guess, problems with tests on MS Windows, where IIRC '?' is\nforbidden in filenames.\n\nPerhaps\n\n  +test_commit \\\n  +\t'Initial commit' \\\n  +\t'foo'\n\nor\n\n  +test_commit \\\n  +\t'SnapshotFileTests' \\\n  +\t'foo' 'i can has snapshot?'\n\n\n> +\n\nIn the test below you use \"git rev-parse --verify HEAD\" and\n\"git rev-parse --short HEAD\" over and over.  I think it would be better\nto calculate them upfront:\n\n  +test_expect_success 'calculate full and short ids' '\n  +\tFULLID= $(git rev-parse --verify  HEAD) &&\n  +\tSHORTID=$(git rev-parse --short=7 HEAD)\n  +'\n\n> +test_expect_success 'snapshots: give full hash' '\n\nYou test here that giving full has works, and that gitweb uses short hash\nin file name.  Better name would therefore be something like\n\n  +test_expect_success 'snapshots: give full hash, get short hash' '\n\n\n> +\tID=`git rev-parse --verify HEAD` &&\n> +\tgitweb_run \"p=.git;a=snapshot;h=$ID;sf=tgz\" &&\n\nHere I had to remember that 'tgz' snapshot format is enabled by default.\nI think it could be better to explicitly enable it in preparation step.\n\n> +\tID=`git rev-parse --short HEAD` &&\n> +\tgrep \".git-$ID.tar.gz\" gitweb.output\n\nHere had to think a bit that gitweb.output consists both of HTTP headers,\nand of response body, and you are grepping here in the HTTP headers part.\nIt would be better solution for gitweb_run to split gitweb.output into\ngitweb.headers and gitweb.body (perhaps if requested by setting some\nvariable, e.g. GITWEB_SPLIT_OUTPUT).\n\nIt can be done using the following lines:\n\n\tsed    -e '/^\\r$/'      <gitweb.output >gitweb.headers\n\tsed -n -e '0,/^\\r$/!p'  <gitweb.output >gitweb.body\n\n\t# gitweb.headers is used to parse http headers\n\t# gitweb.body is response without http headers\n\nBut the second one uses GNU sed extension; I don't know how to write\nit in more portable way.\n\nThen\n\n> +\tgrep \".git-$ID.tar.gz\" gitweb.output\n\nwould be\n\n  +\tgrep \".git-$ID.tar.gz\" gitweb.headers\n\nNote that this would mean that t/t9501-gitweb-standalone-http-status.sh\nshould also be updated to use gitweb.headers and gitweb.body\n\n> +'\n> +test_debug 'cat gitweb.output'\n\nNot a good idea in current state.  gitweb.output contains binary part,\nand generally it is not a good idea to output binary files (which can\ncontain ANSI escape sequences) to terminal.\n\n> +\n> +test_expect_success 'snapshots: give short hash' '\n\n  +test_expect_success 'snapshots: give short hash, get short hash' '\n\n> +\tID=`git rev-parse --short HEAD` &&\n\nGitweb uses '--short=7'.  Shouldn't you use the same option here?\n\n> +\tgitweb_run \"p=.git;a=snapshot;h=$ID;sf=tgz\" &&\n> +\tgrep \".git-$ID.tar.gz\" gitweb.output\n> +'\n> +test_debug 'cat gitweb.output'\n> +\n> +test_expect_success 'snapshots: give almost full hash' '\n\n  +test_expect_success 'snapshots: give almost full hash, get short hash' '\n\n> +\tID=`git rev-parse --short=30 HEAD` &&\n> +\tgitweb_run \"p=.git;a=snapshot;h=$ID;sf=tgz\" &&\n> +\tID=`git rev-parse --short HEAD` &&\n> +\tgrep \".git-$ID.tar.gz\" gitweb.output\n> +'\n> +test_debug 'cat gitweb.output'\n> +\n> +test_expect_success 'snapshots: give HEAD tree-ish' '\n\n  +test_expect_success 'snapshots: give HEAD, get HEAD-<short hash>' '\n\n> +\tgitweb_run \"p=.git;a=snapshot;h=HEAD;sf=tgz\" &&\n> +\tID=`git rev-parse --short HEAD` &&\n> +\tgrep \".git-HEAD-$ID.tar.gz\" gitweb.output\n> +'\n> +test_debug 'cat gitweb.output'\n> +\n> +test_expect_success 'snapshots: give branch name tree-ish' '\n\n  +test_expect_success 'snapshots: give branch name, get <branch name>-<short hash>' '\n\n> +\tgitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n> +\tID=`git rev-parse --short master` &&\n> +\tgrep \".git-master-$ID.tar.gz\" gitweb.output\n> +'\n> +test_debug 'cat gitweb.output'\n> +\n> +test_expect_success 'snapshots: give tag tree-ish' '\n\n  +test_expect_success 'snapshots: give tag name, get <tag name>-<short hash>' '\n\n> +\tgitweb_run \"p=.git;a=snapshot;h=SnapshotFileTests;sf=tgz\" &&\n> +\tID=`git rev-parse --short SnapshotFileTests` &&\n> +\tgrep \".git-SnapshotFileTests-$ID.tar.gz\" gitweb.output\n> +'\n> +test_debug 'cat gitweb.output'\n\nNote that to avoid ambiguities currently gitweb uses refs/heads/master\nand refs/tags/SnapshotFileTests... but dealing with this issue should be\nleft, I think, for separate commit.\n\n> +\n> +\n> +test_done\n> -- \n> 1.6.4.GIT\n> \n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"124710","messageId":"4AD34C93.20605@mailservices.uwaterloo.ca","threadId":"21068","inReplyTo":"200910051206.18943.jnareb@gmail.com","subject":"Re: [PATCH v5 2/2] gitweb: append short hash ids to snapshot files","fromName":"Mark Rada","fromEmail":"marada@uwaterloo.ca","sentAt":"2009-10-12T15:34:43Z","receivedAt":"2009-10-12T15:34:43Z","isPatch":true,"sender":{"key":"marada@uwaterloo.ca","avatar":"https://avatars.githubusercontent.com/u/38430?v=4"},"body":"On 09-10-05 6:06 AM, Jakub Narebski wrote:\n\n>>  \tmy $o_git_dir = $git_dir;\n>>  \tmy $retval = undef;\n>>  \t$git_dir = \"$projectroot/$project\";\n>> -\tif (open my $fd, \"-|\", git_cmd(), \"rev-parse\", \"--verify\", \"HEAD\") {\n>> -\t\tmy $head = <$fd>;\n>> +\tif (open my $fd, '-|', git_cmd(), 'rev-parse', '--verify', $hash) {\n>> +\t\t$hash = <$fd>;\n>>  \t\tclose $fd;\n>> -\t\tif (defined $head && $head =~ /^([0-9a-fA-F]{40})$/) {\n>> +\t\tif (defined $hash && $hash =~ /^([0-9a-fA-F]{40})$/) {\n>> +\t\t\t$retval = $1;\n>> +\t\t}\n> \n> I guess that you use \"$retval = $1;\" instead of just \"$retval = $hash;\"\n> because of similarities with git_get_short_hash, isn't it?  Or it is just\n> following earlier code?\n\nYeah, it is following earlier code, I did not change it, though the diff\nseems to think I added it, perhaps this is a bug with diff?\n\n>> +\t}\n>> +\tif (defined $o_git_dir) {\n>> +\t\t$git_dir = $o_git_dir;\n>> +\t}\n>> +\treturn $retval;\n>> +}\n>> +\n>> +# try and get a shorter hash id\n>> +sub git_get_short_hash {\n>> +\tmy $project = shift;\n>> +\tmy $hash = shift;\n>> +\tmy $o_git_dir = $git_dir;\n>> +\tmy $retval = undef;\n>> +\t$git_dir = \"$projectroot/$project\";\n>> +\tif (open my $fd, '-|', git_cmd(), 'rev-parse', '--short=7', $hash) {\n>> +\t\t$hash = <$fd>;\n>> +\t\tclose $fd;\n>> +\t\tif (defined $hash && $hash =~ /^([0-9a-fA-F]{7,})$/) {\n>>  \t\t\t$retval = $1;\n>>  \t\t}\n>>  \t}\n> \n> Note that git_get_full_hash (which additionally does verification) and\n> git_get_short_hash share much of code.  Perhaps it might be worth to\n> avoid code duplication somehow?  On the other hand it might be not worth\n> to complicate code by trying to extract common parts here...\n\nHmm, I think it might be a good idea to just write a generic routine\nthat takes a hash length as an extra parameter. Then the short and full\nhash fetching routines can just acts as wrappers.\n\n>> @@ -5203,6 +5228,13 @@ sub git_snapshot {\n>>  \t\tdie_error(400, 'Object is not a tree-ish');\n>>  \t}\n>>  \n>> +\n>> +\tmy $full_hash = git_get_full_hash($project, $hash);\n>> +\tif ($full_hash =~ /^$hash/) {\n>> +\t\t$hash = git_get_short_hash($project, $hash);\n>> +\t} else {\n>> +\t\t$hash .= '-' . git_get_short_hash($project, $hash);\n>> +\t}\n> \n> I think we might want to avoid calling git_get_full_hash (and extra call\n> to \"git rev-parse\" command, which is extra fork) if we know in advance\n> that  $full_hash =~ /^$hash/  can't be true, i.e. if $hash doesn't match\n> /^[0-9a-fA-F]+$/.  That would require that we continue to use $hash\n> and not $full_hash, see comment for the chunk below.\n> \n> BTW do you think that having better name (nicer name in the case\n> when $hash is full SHA-1, or name which describes exact version as \n> in the case when $hash is branch name or just 'HEAD') is worth\n> slight extra cost of \"git rev-parse --abbrev=7\"?\n\nHmm, yeah, some optimization will have to occur in that block of\ncode. Though, my reason for that extra call to rev-parse to get the\nshort hash is so I can get git to find the shortest unique SHA-1,\ninstead of just assuming that it will always be of length 7. I think\nthe cost is not too bad considering a snapshot will have to be generated\nand probably take way more time. Though, warthog9 has some caching\npatches that work, so maybe it isn't worth it. Hmm...\n\n>>  \tmy $name = $project;\n>>  \t$name =~ s,([^/])/*\\.git$,$1,;\n>>  \t$name = basename($name);\n>> @@ -5213,7 +5245,7 @@ sub git_snapshot {\n>>  \t$cmd = quote_command(\n>>  \t\tgit_cmd(), 'archive',\n>>  \t\t\"--format=$known_snapshot_formats{$format}{'format'}\",\n>> -\t\t\"--prefix=$name/\", $hash);\n>> +\t\t\"--prefix=$name/\", $full_hash);\n> \n> Why this change?\n\nSince $hash can change by becoming something like 'HEAD-43ab5f2c' due to\nthe process of creating the better name we need to pass something to\n`archive' that will be valid, and $full_hash will be valid.\n\n>> +test_description='gitweb as standalone script (parsing script output).\n>> +\n>> +This test runs gitweb (git web interface) as a CGI script from the\n>> +commandline, and checks that it produces the correct output, either\n>> +in the HTTP header or the actual script output.'\n> \n> Currently all tests here are about 'snapshot' action.  They are quite\n> specific, and they do require some knowledge about chosen archive format.\n> I think it would be better to put snapshot test into separate test,\n> i.e. in 't/t9502-gitweb-standalone-snapshot.sh'.\n> \n\nOk.\n\n>> +test_commit \\\n>> +\t'SnapshotFileTests' \\\n>> +\t'i can has snapshot?'\n> \n> Errr... with filename [cutely] called 'i can has snapshot?' you would\n> have, I guess, problems with tests on MS Windows, where IIRC '?' is\n> forbidden in filenames.\n\nI was able to confirm `?' as a forbidden file name character in Windows 7,\nso I will have to change that...\n\n> In the test below you use \"git rev-parse --verify HEAD\" and\n> \"git rev-parse --short HEAD\" over and over.  I think it would be better\n> to calculate them upfront:\n> \n>   +test_expect_success 'calculate full and short ids' '\n>   +\tFULLID= $(git rev-parse --verify  HEAD) &&\n>   +\tSHORTID=$(git rev-parse --short=7 HEAD)\n>   +'\n> \n\nOk.\n\n>> +\tID=`git rev-parse --short HEAD` &&\n>> +\tgrep \".git-$ID.tar.gz\" gitweb.output\n> \n> Here had to think a bit that gitweb.output consists both of HTTP headers,\n> and of response body, and you are grepping here in the HTTP headers part.\n> It would be better solution for gitweb_run to split gitweb.output into\n> gitweb.headers and gitweb.body (perhaps if requested by setting some\n> variable, e.g. GITWEB_SPLIT_OUTPUT).\n> \n> It can be done using the following lines:\n> \n> \tsed    -e '/^\\r$/'      <gitweb.output >gitweb.headers\n> \tsed -n -e '0,/^\\r$/!p'  <gitweb.output >gitweb.body\n> \n> \t# gitweb.headers is used to parse http headers\n> \t# gitweb.body is response without http headers\n> \n> But the second one uses GNU sed extension; I don't know how to write\n> it in more portable way.\n\nI like this and will try to find a way of setting this up without using\nGNU extensions.\n\n> Note that this would mean that t/t9501-gitweb-standalone-http-status.sh\n> should also be updated to use gitweb.headers and gitweb.body\n\nYeah, now I have a few things mentioned by either you or Junio that I\nshould probably fix in the test cases I have submitted. I will clean\nthem up in a separate patch once I finish with this patch.\n\n>> +\tgitweb_run \"p=.git;a=snapshot;h=SnapshotFileTests;sf=tgz\" &&\n>> +\tID=`git rev-parse --short SnapshotFileTests` &&\n>> +\tgrep \".git-SnapshotFileTests-$ID.tar.gz\" gitweb.output\n>> +'\n>> +test_debug 'cat gitweb.output'\n> \n> Note that to avoid ambiguities currently gitweb uses refs/heads/master\n> and refs/tags/SnapshotFileTests... but dealing with this issue should be\n> left, I think, for separate commit.\n> \n\nI do not understand what ambiguity exists, can you please explain this?\n\n\n-- \nMark Rada (ferrous26)\nmarada@uwaterloo.ca\n"},{"id":"124914","messageId":"200910140146.42285.jnareb@gmail.com","threadId":"21068","inReplyTo":"4AD34C93.20605@mailservices.uwaterloo.ca","subject":"Re: [PATCH v5 2/2] gitweb: append short hash ids to snapshot files","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-10-13T23:46:40Z","receivedAt":"2009-10-13T23:46:40Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 12 Oct 2009, Mark Rada wrote:\n> On 09-10-05 6:06 AM, Jakub Narebski wrote:\n> \n>>>  \tmy $o_git_dir = $git_dir;\n>>>  \tmy $retval = undef;\n>>>  \t$git_dir = \"$projectroot/$project\";\n>>> -\tif (open my $fd, \"-|\", git_cmd(), \"rev-parse\", \"--verify\", \"HEAD\") {\n>>> -\t\tmy $head = <$fd>;\n>>> +\tif (open my $fd, '-|', git_cmd(), 'rev-parse', '--verify', $hash) {\n>>> +\t\t$hash = <$fd>;\n>>>  \t\tclose $fd;\n>>> -\t\tif (defined $head && $head =~ /^([0-9a-fA-F]{40})$/) {\n>>> +\t\tif (defined $hash && $hash =~ /^([0-9a-fA-F]{40})$/) {\n>>> +\t\t\t$retval = $1;\n>>> +\t\t}\n>> \n>> I guess that you use \"$retval = $1;\" instead of just \"$retval = $hash;\"\n>> because of similarities with git_get_short_hash, isn't it?  Or it is just\n>> following earlier code?\n> \n> Yeah, it is following earlier code, I did not change it, \n\nAh, that's O.K.\n\nAlthough if you plan refactoring this, you might fix this bit of\ninefficiency (no need for capturing).\n\n> though the diff seems to think I added it, perhaps this is a bug with\n> diff? \n\nNo, that is just diff being ambiguous, as there is more than one way\nto generate diff of changes.  Perhaps patience diff would produce\nbetter results, perhaps not.  It might mean that refactoring common\ncode is needed ;-)))))\n\n>>> +\t}\n>>> +\tif (defined $o_git_dir) {\n>>> +\t\t$git_dir = $o_git_dir;\n>>> +\t}\n>>> +\treturn $retval;\n>>> +}\n>>> +\n>>> +# try and get a shorter hash id\n>>> +sub git_get_short_hash {\n>>> +\tmy $project = shift;\n>>> +\tmy $hash = shift;\n>>> +\tmy $o_git_dir = $git_dir;\n>>> +\tmy $retval = undef;\n>>> +\t$git_dir = \"$projectroot/$project\";\n>>> +\tif (open my $fd, '-|', git_cmd(), 'rev-parse', '--short=7', $hash) {\n>>> +\t\t$hash = <$fd>;\n>>> +\t\tclose $fd;\n>>> +\t\tif (defined $hash && $hash =~ /^([0-9a-fA-F]{7,})$/) {\n>>>  \t\t\t$retval = $1;\n>>>  \t\t}\n>>>  \t}\n>> \n>> Note that git_get_full_hash (which additionally does verification) and\n>> git_get_short_hash share much of code.  Perhaps it might be worth to\n>> avoid code duplication somehow?  On the other hand it might be not worth\n>> to complicate code by trying to extract common parts here...\n> \n> Hmm, I think it might be a good idea to just write a generic routine\n> that takes a hash length as an extra parameter. Then the short and full\n> hash fetching routines can just acts as wrappers.\n\nWell, git_get_full_hash uses --verify, git_get_short_hash uses --short=7\n(but perhaps it should also use --verify).\n\nBTW. I think that checking that output of git-rev-parse is (shortened)\nSHA-1 predates usage of --verify; with --verify is, I think, not\nnecessary:\n\n --verify\n     The parameter given must be usable as a single, valid object name.\n     Otherwise barf and abort.\n\n>>> @@ -5203,6 +5228,13 @@ sub git_snapshot {\n>>>  \t\tdie_error(400, 'Object is not a tree-ish');\n>>>  \t}\n>>>  \n>>> +\n>>> +\tmy $full_hash = git_get_full_hash($project, $hash);\n>>> +\tif ($full_hash =~ /^$hash/) {\n\nBTW, we can use  \n\n        if (index($full_hash, $hash) == 0) {\n\ninstead.  BTW, $hash could contain regexp metacharacters like '.'\n('dead.beef' is a valid branch name), so it should be\n\n\tif ($full_hash =~ /^\\Q$hash/) {\n\nif you want to use regexp (it might be easier to read).  \n\nOr you can encapsulate this into is_substring() subroutine, but that\nmight be (well, almost surely is) overkill...\n\n>>> +\t\t$hash = git_get_short_hash($project, $hash);\n>>> +\t} else {\n>>> +\t\t$hash .= '-' . git_get_short_hash($project, $hash);\n>>> +\t}\n>> \n>> I think we might want to avoid calling git_get_full_hash (and extra call\n>> to \"git rev-parse\" command, which is extra fork) if we know in advance\n>> that  $full_hash =~ /^$hash/  can't be true, i.e. if $hash doesn't match\n>> /^[0-9a-fA-F]+$/.  That would require that we continue to use $hash\n>> and not $full_hash, see comment for the chunk below.\n>> \n>> BTW do you think that having better name (nicer name in the case\n>> when $hash is full SHA-1, or name which describes exact version as \n>> in the case when $hash is branch name or just 'HEAD') is worth\n>> slight extra cost of \"git rev-parse --abbrev=7\"?\n> \n> Hmm, yeah, some optimization will have to occur in that block of\n> code. Though, my reason for that extra call to rev-parse to get the\n> short hash is so I can get git to find the shortest unique SHA-1,\n> instead of just assuming that it will always be of length 7. I think\n> the cost is not too bad considering a snapshot will have to be generated\n> and probably take way more time. Though, warthog9 has some caching\n> patches that work, so maybe it isn't worth it. Hmm...\n\nWhat I meant here that unless $hash =~ /^[0-9a-fA-F]{7,}$/ then we \nalways use git_get_short_hash, as $full_hash wouldn't match /^$hash/\n($hash wouldn't be a prefix of $full_hash).  We don't need to\ncalculate git_get_full_hash which wouldn't be used (see also comment\nbelow, though).\n\n> \n>>>  \tmy $name = $project;\n>>>  \t$name =~ s,([^/])/*\\.git$,$1,;\n>>>  \t$name = basename($name);\n>>> @@ -5213,7 +5245,7 @@ sub git_snapshot {\n>>>  \t$cmd = quote_command(\n>>>  \t\tgit_cmd(), 'archive',\n>>>  \t\t\"--format=$known_snapshot_formats{$format}{'format'}\",\n>>> -\t\t\"--prefix=$name/\", $hash);\n>>> +\t\t\"--prefix=$name/\", $full_hash);\n>> \n>> Why this change?\n> \n> Since $hash can change by becoming something like 'HEAD-43ab5f2c' due to\n> the process of creating the better name we need to pass something to\n> `archive' that will be valid, and $full_hash will be valid.\n\nErrr... why it is called _$hash_ then, if it can be not hash?  Wouldn't\nit be better to manipulate $name here?\n\nI think this fragment should be extracted into snapshot_name() subroutine,\nwhich result would be used both as proposed snapshot name, and as prefix\nto be used.\n\n> \n>>> +test_description='gitweb as standalone script (parsing script output).\n>>> +\n>>> +This test runs gitweb (git web interface) as a CGI script from the\n>>> +commandline, and checks that it produces the correct output, either\n>>> +in the HTTP header or the actual script output.'\n>> \n>> Currently all tests here are about 'snapshot' action.  They are quite\n>> specific, and they do require some knowledge about chosen archive format.\n\nThat is not true, as I haven't noticed at this point that you are \nexamining only HTTP headers... but not the HTTP status but other headers.\n\n[...]\n>>> +\tgrep \".git-$ID.tar.gz\" gitweb.output\n>> \n>> Here had to think a bit that gitweb.output consists both of HTTP headers,\n>> and of response body, and you are grepping here in the HTTP headers part.\n>> It would be better solution for gitweb_run to split gitweb.output into\n>> gitweb.headers and gitweb.body (perhaps if requested by setting some\n>> variable, e.g. GITWEB_SPLIT_OUTPUT).\n>> \n>> It can be done using the following lines:\n>> \n>> \tsed    -e '/^\\r$/'      <gitweb.output>gitweb.headers\n\nThat was meant to be\n\n>> \tsed    -e '/^\\r$/q'     <gitweb.output >gitweb.headers\n\nwhich means print (the default action) until single empty CRLF terminated\nline, which ends HTTP headers.\n\n>> \tsed -n -e '0,/^\\r$/!p'  <gitweb.output>gitweb.body\n>> \n>> \t# gitweb.headers is used to parse http headers\n>> \t# gitweb.body is response without http headers\n>> \n>> But the second one uses GNU sed extension; I don't know how to write\n>> it in more portable way.\n> \n> I like this and will try to find a way of setting this up without using\n> GNU extensions.\n\nWell, we do know that there always would be at least one header, so we\ncan use:\n\n \tsed -n -e '1,/^\\r$/!p'  <gitweb.output >gitweb.body\n\nBut I'd prefer that somebody better versed in sed would come up with\nsolution to extract everything up to first empty CRLF terminated line,\nand everything from such line till the end of file.\n\n>> Note that to avoid ambiguities currently gitweb uses refs/heads/master\n>> and refs/tags/SnapshotFileTests... but dealing with this issue should be\n>> left, I think, for separate commit.\n>> \n> \n> I do not understand what ambiguity exists, can you please explain this?\n\nThe problem I was thinking about is the following.\n\nIn commit bf901f8 (gitweb: disambiguate heads and tags withs the same\nname, 2007-12-15) started to use refs/heads/<branch> and refs/tags/<tag>\ninstead of <branch> and <tag> because there was problem when there were\ntag and branch with the same name.\n\nThe problem is that we can't use '/' in proposed snapshot file name,\nand we shouldn't use '/' in git-archive prefix.  So we can't simply\nuse (as you proposed) \n\n  $hash . '-' . git_get_short_hash($project, $hash);\n\nas a snapshot basename suffix, because $hash can be 'refs/heads/master',\nor it can be 'mr/gitweb-snapshot'.  What to do, what to do...\n\n\nAlso if $hash is refs/tags/v1.6.0, we don't really need shortened SHA-1\nsuffix.  \n\nAlternative to checking for refs/tags/ prefix would be to use\ngit-describe output... perhaps.\n\n-- \nJakub Narebski\nPoland\n"}]}