{"thread":{"id":"13738","subject":"[PATCH] gitweb: use Git.pm, and use its parse_rev method for git_get_head_hash","startedAt":"2008-05-30T23:00:12Z","lastAt":"2008-06-03T00:20:57Z","messageCount":23,"participants":["Lea Wiemann","Jakub Narebski","Petr Baudis","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"78194","messageId":"1212188412-20479-1-git-send-email-LeWiemann@gmail.com","threadId":"13738","inReplyTo":null,"subject":"[PATCH] gitweb: use Git.pm, and use its parse_rev method for git_get_head_hash","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-05-30T23:00:12Z","receivedAt":"2008-05-30T23:00:12Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"This simplifies git_get_head_hash a lot; the method might eventually\neven go away.\n\nI haven't checked whether this causes an IO performance regression by\ninstantiating a new Git repository instance, but in the end\nGit->repository will be as fast as possible and do no eager disk\naccesses.  No benchmarking yet at this stage.\n\nMinor change: Moved the parameter shift in git_blob_plain to the top\nfor readability.\n\nSigned-off-by: Lea Wiemann <LeWiemann@gmail.com>\n---\n\nI have tested this by running the smoke-test.pl script on my small\ntest repository -- this covers calls to git_get_head_hash -- and then\nrecursively diffing the two resulting wget output directories before\nand after the change.  The trees were essentially identical (save a\nfew timestamps inside the snapshot files).\n\nFor brevity, I'll refer to this test procedure as something like\n\"smoke-test.pl showed no differences\" in future patches. ;-)\n\n gitweb/gitweb.perl |   24 ++++++++----------------\n 1 files changed, 8 insertions(+), 16 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 57a1905..0efd2f7 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -16,6 +16,7 @@ use Encode;\n use Fcntl ':mode';\n use File::Find qw();\n use File::Basename qw(basename);\n+use Git;\n binmode STDOUT, ':utf8';\n \n BEGIN {\n@@ -1508,20 +1509,11 @@ sub git_cmd_str {\n # get HEAD ref of given project as hash\n sub git_get_head_hash {\n \tmy $project = 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-\t\tclose $fd;\n-\t\tif (defined $head && $head =~ /^([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+\tmy $directory = \"$projectroot/$project\";\n+\t# Legacy side effect on $git_dir.  This will eventually go\n+\t# away as the global $git_dir is eliminated.\n+\t$git_dir = $directory if (!defined $git_dir);\n+\tGit->repository(Directory => $directory)->parse_rev(\"HEAD\");\n }\n \n # get type of given object\n@@ -4377,8 +4369,9 @@ sub git_heads {\n }\n \n sub git_blob_plain {\n-\tmy $expires;\n+\tmy $type = shift;\n \n+\tmy $expires;\n \tif (!defined $hash) {\n \t\tif (defined $file_name) {\n \t\t\tmy $base = $hash_base || git_get_head_hash($project);\n@@ -4392,7 +4385,6 @@ sub git_blob_plain {\n \t\t$expires = \"+1d\";\n \t}\n \n-\tmy $type = shift;\n \topen my $fd, \"-|\", git_cmd(), \"cat-file\", \"blob\", $hash\n \t\tor die_error(undef, \"Couldn't cat $file_name, $hash\");\n \n-- \n1.5.5.GIT\n"},{"id":"78195","messageId":"484087A7.2030107@gmail.com","threadId":"13738","inReplyTo":"1212188412-20479-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH] gitweb: use Git.pm, and use its parse_rev method for git_get_head_hash","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-05-30T23:03:03Z","receivedAt":"2008-05-30T23:03:03Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Lea Wiemann wrote:\n> Subject: [PATCH] gitweb: use Git.pm, and use its parse_rev method for git_get_head_hash\n\nFor clarification, this is my first patch for refactoring Gitweb to use \nGit.pm's API.\n\nIn the end, I'm hoping that all (or at least most) of Gitweb's accesses \nto the repositories will go through this API, which allows us to add \ncaching to the Git.pm API (rather than Gitweb) pretty easily and cleanly.\n\n-- Lea\n"},{"id":"78209","messageId":"m3lk1q24nb.fsf@localhost.localdomain","threadId":"13738","inReplyTo":"484087A7.2030107@gmail.com","subject":"Re: [PATCH] gitweb: use Git.pm, and use its parse_rev method for git_get_head_hash","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-05-31T09:40:13Z","receivedAt":"2008-05-31T09:40:13Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Lea Wiemann <lewiemann@gmail.com> writes:\n\n> Lea Wiemann wrote:\n> > Subject: [PATCH] gitweb: use Git.pm, and use its parse_rev method\n> >          for git_get_head_hash\n> \n> For clarification, this is my first patch for refactoring Gitweb to\n> use Git.pm's API.\n\nGood.\n\nI'm not sure however how much of gitweb engine should be moved to\nGit.pm.  I think calling git commands could be moved from git_cmd() to\ncommand_output_pipe() or command_oneline(), replacing global variable\n$git_dir by global variable $repo.  Perhaps gitweb's config file\nparsing should be added as an option to Git.pm.  But I'm not sure for\nexample about parsing subroutines.\n\n> In the end, I'm hoping that all (or at least most) of Gitweb's\n> accesses to the repositories will go through this API, which allows us\n> to add caching to the Git.pm API (rather than Gitweb) pretty easily\n> and cleanly.\n\nIMVHO caching command output is bad idea.  I'd rather have gitweb\ncache _parsed_ data (i.e. Perl structures).\n\nFor example projects list (but also summary page for a project) is\ncomposed as result of parsing output of _many_ git commands, in the\ncase of projects list coming from many different repositories.\nCaching git command output (something like IPC::Cmd::Cached?)\nwould only repeat bad I/O patterns of git commands.\n\nAlso I don't think it is a good idea to pollute Git.pm by caching\ncommand output.  Perhaps in Git::Cached, but as a last resort...\n\nP.S. Here it is what could go to Git.pm:\n * Eager config parsing, using e.g. $repo->parse_config() to prepare\n   %config hash, and have $repo->config(VAR) etc. use %config hash.\n   This means one git command and not one git command per variable,\n   but it also means that we have to convert to bool or int, or color\n   in Perl.\n * unquote() (which is not repository instance merhod, but utility\n   subroutine) to unquote and unescape filenames.\n * generating rfc2822 and iso-8601 dates from timestamp plus timezone,\n   i.e. from author/committer/tagger date; it would be useful for\n   example in git-send-email.\n\nI'm not sure about parse_* subroutines (some of which are not generic\nenough, I guess).\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"78221","messageId":"4841471E.2070302@gmail.com","threadId":"13738","inReplyTo":"m3lk1q24nb.fsf@localhost.localdomain","subject":"Re: [PATCH] gitweb: use Git.pm, and use its parse_rev method for git_get_head_hash","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-05-31T12:39:58Z","receivedAt":"2008-05-31T12:39:58Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Jakub Narebski wrote:\n> But I'm not sure for example [that] parsing subroutines [that parse the\n> output of the git commands should be in Git.pm].\n> \n> [...] IMVHO caching command output is bad idea.  I'd rather have gitweb\n> cache _parsed_ data (i.e. Perl structures).\n\nYes, I agree with you that parsed data should be cached, but for that \nreason I think that the subroutine that parse git's output should be in \nGit.pm, not in Gitweb:\n\nIf you want to cache parsed data, you need the caching layer in between \nthe application (= Gitweb) and the repository access layer (= command \noutput parsing), or you would have to insert cache code at every call to \nthe repository access layer.  So you end up with these layers:\n\n(Layer 0: Front-end [HTML] caching.)\nLayer 1: Application (Gitweb)\nLayer 2: Back-end caching\nLayer 3: Repository access (command parsing)\nLayer 4: Calls to the git binary\n\nLayer 3 and 4 are application-independent (i.e. not Gitweb specific), \nand since they form a usable API, they might as well be written as a \nseparate API rather than lumped together with Gitweb.  Git.pm is a start \nof such an API (it does layer 4 and a little bit of layer 3), so it \nseems natural for me to extend it.\n\nLayer 2 is application-independent as well, so it can become an extra \nclass in Git.pm or a separate module.  (It should stay independent of \nlayers 3 and 4).\n\nWe may need to have an explicitly implemented layer 0 (front-end \ncaching) in Gitweb for at least a subset of pages (like project pages), \nbut we'll see if that's necessary.\n\n> Caching git command output (something like IPC::Cmd::Cached?)\n> would only repeat bad I/O patterns of git commands.\n\nI hope you're not assuming that the back-end cache will reside on disk \n-- the problem is IO throughput, so having a cache on disk can really \nonly work for a front-end cache.  For the back-end cache, we *will* have \nto use a memory cache (or no cache at all).\n\n> P.S. Here it is what could go [from Gitweb] to Git.pm: [snip]\n\nGood points, I agree those should eventually be moved to Git.pm.  (They \nare not my priority at the moment, since I really want to implement \ncaching.  So unless they move naturally as part of my refactoring, I'll \nleave them in Gitweb for now.)\n\n-- Lea\n"},{"id":"78224","messageId":"20080531130450.GI18781@machine.or.cz","threadId":"13738","inReplyTo":"1212188412-20479-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH] gitweb: use Git.pm, and use its parse_rev method for git_get_head_hash","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-05-31T13:04:51Z","receivedAt":"2008-05-31T13:04:51Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Sat, May 31, 2008 at 01:00:12AM +0200, Lea Wiemann wrote:\n> This simplifies git_get_head_hash a lot; the method might eventually\n> even go away.\n> \n> I haven't checked whether this causes an IO performance regression by\n> instantiating a new Git repository instance, but in the end\n> Git->repository will be as fast as possible and do no eager disk\n> accesses.  No benchmarking yet at this stage.\n> \n> Minor change: Moved the parameter shift in git_blob_plain to the top\n> for readability.\n\nPlease split this to a separate patch.\n\n> Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>\n\nOther than that,\n\nAcked-by: Petr Baudis <pasky@suse.cz>\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nWhatever you can do, or dream you can, begin it.\nBoldness has genius, power, and magic in it.\t-- J. W. von Goethe\n"},{"id":"78227","messageId":"1212243564-30109-1-git-send-email-LeWiemann@gmail.com","threadId":"13738","inReplyTo":"20080531130450.GI18781@machine.or.cz","subject":"[PATCH v2] gitweb: use Git.pm, and use its parse_rev method for git_get_head_hash","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-05-31T14:19:24Z","receivedAt":"2008-05-31T14:19:24Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"This simplifies git_get_head_hash a lot; the method might eventually\neven go away.\n\nI haven't checked whether this causes an IO performance regression by\ninstantiating a new Git repository instance, but in the end\nGit->repository will be as fast as possible and do no eager disk\naccesses.  No benchmarking yet at this stage.\n\nSigned-off-by: Lea Wiemann <LeWiemann@gmail.com>\n---\n\nPer your request without the cleanup.  I won't submit the cleanup\npatch separately, but I assume it will get cleaned up eventually when\nsomeone touches that function.\n\n gitweb/gitweb.perl |   20 ++++++--------------\n 1 files changed, 6 insertions(+), 14 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 57a1905..0ed3d6e 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -16,6 +16,7 @@ use Encode;\n use Fcntl ':mode';\n use File::Find qw();\n use File::Basename qw(basename);\n+use Git;\n binmode STDOUT, ':utf8';\n \n BEGIN {\n@@ -1508,20 +1509,11 @@ sub git_cmd_str {\n # get HEAD ref of given project as hash\n sub git_get_head_hash {\n \tmy $project = 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-\t\tclose $fd;\n-\t\tif (defined $head && $head =~ /^([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+\tmy $directory = \"$projectroot/$project\";\n+\t# Legacy side effect on $git_dir.  This will eventually go\n+\t# away as the global $git_dir is eliminated.\n+\t$git_dir = $directory if (!defined $git_dir);\n+\tGit->repository(Directory => $directory)->parse_rev(\"HEAD\");\n }\n \n # get type of given object\n-- \n1.5.5.GIT\n"},{"id":"78231","messageId":"48416211.1030807@gmail.com","threadId":"13738","inReplyTo":"1212243564-30109-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH v2] gitweb: use Git.pm, and use its parse_rev method for git_get_head_hash","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-05-31T14:34:57Z","receivedAt":"2008-05-31T14:34:57Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Lea Wiemann wrote:\n> Subject: [PATCH v2] gitweb: use Git.pm, and use its parse_rev method for git_get_head_hash\n\nJust to clarify, this needs \"[PATCH v2] perl/Git.pm: add parse_rev \nmethod\" to work, here:\nhttp://mid.gmane.org/1212241932-28605-1-git-send-email-LeWiemann@gmail.com\n\n(Apologies I keep replying to my own patches.)\n\n-- Lea\n"},{"id":"78278","messageId":"7vzlq5bm32.fsf@gitster.siamese.dyndns.org","threadId":"13738","inReplyTo":"1212243564-30109-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH v2] gitweb: use Git.pm, and use its parse_rev method for git_get_head_hash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-01T08:23:13Z","receivedAt":"2008-06-01T08:23:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lea Wiemann <lewiemann@gmail.com> writes:\n\n> This simplifies git_get_head_hash a lot; the method might eventually\n> even go away.\n>\n> I haven't checked whether this causes an IO performance regression by\n> instantiating a new Git repository instance, but in the end\n> Git->repository will be as fast as possible and do no eager disk\n> accesses.  No benchmarking yet at this stage.\n>\n> Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>\n\nWith this on top of your parse_rev patch (I used v2 but I do not think v3\nchanges the situation in any way), you seem to have broken t9500.\n\ngitweb.log left in the trash directory says that it cannot find Git.pm in\n@INC.  I suspect that you are not using your own Git in the build tree in\nyour test, but an already installed one.\n\nPlease make sure that you are testing with Git.pm that you are shipping.\nA good way to do so would be to add a deliberate error in perl/Git.pm to\ncause loading of the module to fail, and make sure test barfs upon \"use\nGit\".\n\nSomething like this is needed on top of your patch, I think.\n\n---\n\n t/t9500-gitweb-standalone-no-errors.sh |    3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh\nindex ae7082b..a62231f 100755\n--- a/t/t9500-gitweb-standalone-no-errors.sh\n+++ b/t/t9500-gitweb-standalone-no-errors.sh\n@@ -47,8 +47,9 @@ gitweb_run () {\n \tPATH_INFO=\"\"$2\"\"\n \texport GATEWAY_INTERFACE HTTP_ACCEPT REQUEST_METHOD QUERY_STRING PATH_INFO\n \n+\tPERL5LIB=$(pwd)/../../perl/blib/lib\n \tGITWEB_CONFIG=$(pwd)/gitweb_config.perl\n-\texport GITWEB_CONFIG\n+\texport GITWEB_CONFIG PERL5LIB\n \n \t# some of git commands write to STDERR on error, but this is not\n \t# written to web server logs, so we are not interested in that:\n"},{"id":"78293","messageId":"4842C3F1.5000001@gmail.com","threadId":"13738","inReplyTo":"7vzlq5bm32.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] gitweb: use Git.pm, and use its parse_rev method for git_get_head_hash","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-01T15:44:49Z","receivedAt":"2008-06-01T15:44:49Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Junio C Hamano wrote:\n> With this on top of your parse_rev patch (I used v2 but I do not think v3\n> changes the situation in any way), you seem to have broken t9500.\n> \n> [...] I suspect that you are not using your own Git in the build tree in\n> your test, but an already installed one.\n\nThat was indeed the case, thanks for pointing it out!\n\nHowever, after applying my two patches and your patch on a pristine \ncurrent git.git clone, I still don't get an error, even though the \nGitweb test uses the new Git.pm (which I tested it does).  Care to send \nme your error message so I can track it down, or even upload your \ncomplete tree somewhere?  Feel free to reply off-list or ping me on IRC.\n\n> +++ b/t/t9500-gitweb-standalone-no-errors.sh\n>  \n> +\tPERL5LIB=$(pwd)/../../perl/blib/lib\n\nHow about putting this into test-lib.sh?  There are more tests (like my \nnew Git.pm test suite) that will need it, so setting it up in a central \nplace would probably more convenient and prevent future problems of this \nsort.\n\nIf PERL5LIB already contains paths, can we just discard them, or should \nwe preserve them?\n\nSince perl/Makefile only copies Git.pm to blib/lib/Git.pm, we could also \nset the path to ../../perl, which would prevent us from accidentally \nrunning tests against an old version of Git.pm (because we haven't run \ncd perl; make before).  And perhaps add a comment to perl/Makefile about \nthis, in case someone wants to change the build process in the future. \nOr is there some reason why this would be a bad idea?\n\n-- Lea\n"},{"id":"78310","messageId":"7vy75oalh7.fsf@gitster.siamese.dyndns.org","threadId":"13738","inReplyTo":"4842C3F1.5000001@gmail.com","subject":"Re: [PATCH v2] gitweb: use Git.pm, and use its parse_rev method for git_get_head_hash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-01T21:33:56Z","receivedAt":"2008-06-01T21:33:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lea Wiemann <lewiemann@gmail.com> writes:\n\n> Junio C Hamano wrote:\n>> With this on top of your parse_rev patch (I used v2 but I do not think v3\n>> changes the situation in any way), you seem to have broken t9500.\n>>\n>> [...] I suspect that you are not using your own Git in the build tree in\n>> your test, but an already installed one.\n>\n> That was indeed the case, thanks for pointing it out!\n>\n> However, after applying my two patches and your patch on a pristine\n> current git.git clone, I still don't get an error...\n\nIf you applied the \"this on top of yours\" fix, you shouldn't have seen any\nbreakage.\n\nThe breakage was not about what you did in Git.pm, but about adding \"use\nGit\" in gitweb.perl but without arranging it to find Git.pm (I do not have\nany git installed in usual places on my machine, and I strip the directory\nI keep my git installation out of my $PATH when running tests, to make\nsure \"make test\" can bootstrap itself without first installing).\n\n> How about putting this into test-lib.sh?  There are more tests (like\n> my new Git.pm test suite) that will need it, so setting it up in a\n> central place would probably more convenient and prevent future\n> problems of this sort.\n\nSure, I think that would be sensible.\n\n> If PERL5LIB already contains paths, can we just discard them, or\n> should we preserve them?\n\nIt would be best to keep the existing one but make ours the first thing to\nbe found, so that we will be sure that we test the one we just built.\n\n> Since perl/Makefile only copies Git.pm to blib/lib/Git.pm, we could\n> also set the path to ../../perl, which would prevent us from\n> accidentally running tests against an old version of Git.pm (because\n> we haven't run cd perl; make before).  And perhaps add a comment to\n> perl/Makefile about this, in case someone wants to change the build\n> process in the future. Or is there some reason why this would be a bad\n> idea?\n\nI think it is a bad idea.  In principle we should be testing what we just\nbuilt and will install; even if currently the \"building procedure\" for\nblib/lib/Git.pm may happen to be bit-for-bit copy of the original, that is\njust by accident and not something we would want to rely on.\n"},{"id":"78314","messageId":"1212358587-15948-1-git-send-email-LeWiemann@gmail.com","threadId":"13738","inReplyTo":"7vy75oalh7.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] test-lib.sh: set PERL5LIB instead of GITPERLLIB","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-01T22:16:27Z","receivedAt":"2008-06-01T22:16:27Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"By setting PERL5LIB for the tests, we enable Perl test scripts to just\nsay \"use Git;\" without adding the GITPERLLIB paths to @INC beforehand.\n\nSigned-off-by: Lea Wiemann <LeWiemann@gmail.com>\n---\nJunio C Hamano wrote:\n> Lea Wiemann <lewiemann@gmail.com> writes:\n>> How about [setting PERL5LIB in] test-lib.sh?\n> \n> Sure, I think that would be sensible.\n\nActually, GITPERLLIB becomes redundant in that case, so I've simply\nreplaced it with PERL5LIB in test-lib.sh.\n\nI've tested the patch: all scripts pick up the right Git.pm, and all\ntests still pass.  It also preserves existing PERL5LIB variables\ncorrectly.\n\nBtw, I'm assuming that it's OK to post small patches (like this one)\nas replies.  If you or anyone else would prefer independent patches to\nalways start a new thread, please let me know.\n\n-- Lea\n\n t/test-lib.sh |    5 +++--\n 1 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 99b63da..2f86831 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -454,8 +454,9 @@ GIT_CONFIG_NOSYSTEM=1\n GIT_CONFIG_NOGLOBAL=1\n export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_CONFIG_NOGLOBAL\n \n-GITPERLLIB=$(pwd)/../perl/blib/lib:$(pwd)/../perl/blib/arch/auto/Git\n-export GITPERLLIB\n+test -n \"$PERL5LIB\" && PERL5LIB=\":$PERL5LIB\"\n+PERL5LIB=\"$(pwd)/../perl/blib/lib:$(pwd)/../perl/blib/arch/auto/Git$PERL5LIB\"\n+export PERL5LIB\n test -d ../templates/blt || {\n \terror \"You haven't built things yet, have you?\"\n }\n-- \n1.5.5.GIT\n"},{"id":"78315","messageId":"200806020019.23858.jnareb@gmail.com","threadId":"13738","inReplyTo":"4841471E.2070302@gmail.com","subject":"Re: [PATCH] gitweb: use Git.pm, and use its parse_rev method for git_get_head_hash","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-01T22:19:23Z","receivedAt":"2008-06-01T22:19:23Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 31 May 2008, Lea Wiemann wrote:\n> Jakub Narebski wrote:\n>>\n>> But I'm not sure for example [that] parsing subroutines [that parse the\n>> output of the git commands should be in Git.pm].\n>> \n>> [...] IMVHO caching command output is bad idea.  I'd rather have gitweb\n>> cache _parsed_ data (i.e. Perl structures).\n> \n> Yes, I agree with you that parsed data should be cached, but for that \n> reason I think that the subroutine that parse git's output should be in \n> Git.pm, not in Gitweb:\n\nThis not necessarily follows.  First, I'm not sure if gitweb's parse_*\nsubroutines are generic enough; please take into account that Git.pm \nis used also by other git commands coded in Perl: git-cvsexportcommit\n(but not git-cvsserver?), git-send-email, git-svn and (internal)\ngit-add--interactive.  Second, caching of _parsed_ data can be\nimplemented in gitweb, or in Gitweb::Cache / Gitweb::Caching.\n\n> If you want to cache parsed data, you need the caching layer in between \n> the application (= Gitweb) and the repository access layer (= command \n> output parsing), or you would have to insert cache code at every call to \n> the repository access layer.  \n\nNot every.  Please don't assume that we would want to cache _all_ the\ndata; herein madness lies.\n\nBut there is another reason to have caching as a layer, namely having\ncaching optional (i.e. enabled or disable), although this can be also\nachieved by choosing 'Null'/'None' caching engine.\n\n> So you end up with these layers: \n> \n> (Layer 0: Front-end [HTML] caching.)\n> Layer 1: Application (Gitweb)\n> Layer 2: Back-end caching\n> Layer 3: Repository access (command parsing)\n> Layer 4: Calls to the git binary\n> \n> Layer 3 and 4 are application-independent (i.e. not Gitweb specific), \n> and since they form a usable API, they might as well be written as a \n> separate API rather than lumped together with Gitweb.  Git.pm is a start \n> of such an API (it does layer 4 and a little bit of layer 3), so it \n> seems natural for me to extend it.\n\nThis assumes that command parsing used by gitweb are generic enough\nto put them in Git.pm.  But some IMVHO are very gitweb-specific, for\nexample the part in parse_commit_text() beginning with \n  # remove leading stuff of merges to make the interesting part visible\nand the 'age_string*' keys there, parse_difftree_raw_line() which\ncurrently does not support '-z' output, parse_from_to_diffinfo() which\nis _very_ gitweb specific, git_get_heads_list() which is not generic\nenough (it gets info which gitweb needs, but no more), etc.\n\n> Layer 2 is application-independent as well, so it can become an extra \n> class in Git.pm or a separate module.  (It should stay independent of \n> layers 3 and 4).\n\nI think it would be better as separate module.  Would it be Git::Cache\n(or Git::Caching), Gitweb::Cache, or part of gitweb, that would have\nto be decided.  Besides, I'm not sure if it is really application-\n-independent as you say: I think we would get better result if we\ncollate data first, which is application dependent.  Also I think\nthere is no sense to cache everything: what to cache is again\napplication dependent.\n\n> We may need to have an explicitly implemented layer 0 (front-end \n> caching) in Gitweb for at least a subset of pages (like project pages), \n> but we'll see if that's necessary.\n\nI think that front-end caching (HTML/RSS/Atom output caching) has sense\nfor large web pages (large size and large number of items), such as\nprojects list page if it is unpaginated (and perhaps even if it is\ndivided into pages, although I'm sure not for project search page),\ncommonly requested snapshots (if they are enabled), large trees like\nSPECS directory with all the package *.spec files for distribution\nrepository, perhaps summary/feed for most popular projects.  If (when)\nsyntax highlighting would got implemented, probably also caching\nblob view (as CPU can become issue).\n\nFront-end (HTML output) caching has the advantages of easy to calculate\nstrong ETag, and web server support for If-Match: and If-None-Match:\nHTTP/1.1 headers.  You can easy support Range: request, needed for\nresuming download (e.g. for downloading snapshots, if this feature is\nenabled in gitweb).  You can even compress the output, and serve it to\nclients which support proper transparent compression (Content-Encoding).\n\nAnd of course it has the advantage of actually been written and tested\nin work, in the case of kernel.org gitweb.  Although caching parsed\ndata was implemented in repo.or.cz gitweb, it was done only for\nprojects list view, and it is quite new and not so thoroughly tested\n  http://article.gmane.org/gmane.comp.version-control.git/77469\n\n\nIt would be nice for front-end caching to have an option to use absolute\ntime for all time/dates, and to (optionally) not use adaptive\nContent-Type...\n\n>> Caching git command output (something like IPC::Cmd::Cached?)\n>> would only repeat bad I/O patterns of git commands.\n> \n> I hope you're not assuming that the back-end cache will reside on disk \n> -- the problem is IO throughput, so having a cache on disk can really \n> only work for a front-end cache.  For the back-end cache, we *will* have \n> to use a memory cache (or no cache at all).\n\nDon't assume, check (in this case: benchmark[1]).  \n\nBut I reallu don't know enough about caching to say if it is one way\nor the other...\n\n[1] http://cpan.robm.fastmail.fm/cache_perf.html (a bit old)\n-- \nJakub Narebski\nPoland\n"},{"id":"78319","messageId":"48432E45.9070200@gmail.com","threadId":"13738","inReplyTo":"1212243564-30109-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH v2] gitweb: use Git.pm, and use its parse_rev method for git_get_head_hash","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-01T23:18:29Z","receivedAt":"2008-06-01T23:18:29Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Lea Wiemann wrote:\n> Re: [PATCH v2] gitweb: use Git.pm, and use its parse_rev method for git_get_head_hash\n\nSince I just renamed the method from parse_rev to get_hash, I would \ntechnically have to post a v3 of my patch, but I'm afraid that at some \npoint I'll have a v4 and v5, with a lot of time wasted. :)\n\nHence, to minimize effort on all sides, I'd rather suggest that I won't \npost any Gitweb patches (other than as RFCs, or perhaps bug fixes to \nexisting code) for now, so that we don't introduce an unnecessary Git.pm \ndependency while the refactoring happens.  Also, since I'm posting \n(too?) many patches around Git.pm and Gitweb already, this will reduce \nthe load on reviewers.\n\n-- Lea\n"},{"id":"78331","messageId":"7vfxrwa008.fsf@gitster.siamese.dyndns.org","threadId":"13738","inReplyTo":"1212358587-15948-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH] test-lib.sh: set PERL5LIB instead of GITPERLLIB","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-02T05:17:43Z","receivedAt":"2008-06-02T05:17:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lea Wiemann <lewiemann@gmail.com> writes:\n\n>>> How about [setting PERL5LIB in] test-lib.sh?\n>> \n>> Sure, I think that would be sensible.\n>\n> Actually, GITPERLLIB becomes redundant in that case, so I've simply\n> replaced it with PERL5LIB in test-lib.sh.\n\nSorry, but now you mention it, it comes back to me why we have GITPERLLIB,\nwhy the setting of GITPERLLIB in t/test-lib.sh we already have did not\nwork, and why my \"this on top of yours\" was a mere workaround and not a\nreal fix.\n\nThe fact is that Git.pm can be installed in a private path outside of\nwhere people usually install site-perl modules, and the primary Makefile\nhas this rule to munge our perl scripts:\n\n        $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl\n                $(QUIET_GEN)$(RM) $@ $@+ && \\\n                INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \\\n                sed -e '1{' \\\n                    -e '\ts|#!.*perl|#!$(PERL_PATH_SQ)|' \\\n                    -e '\th' \\\n                    -e '\ts=.*=use lib (split(/:/, $$ENV{GITPERLLIB} || \"@@INSTLIBDIR@@\"));=' \\\n                    -e '\tH' \\\n                    -e '\tx' \\\n                    -e '}' \\\n                    -e 's|@@INSTLIBDIR@@|'\"$$INSTLIBDIR\"'|g' \\\n                    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \\\n                    $@.perl >$@+ && \\\n                chmod +x $@+ && \\\n                mv $@+ $@\n\nso that people do not have to have that path on PERL5LIB in their\nenvironment when they use git (namely, git-send-email and other scripts\nthat do \"use Git\").\n\nThe real fix to the issue, which is consistent with what 6fcca93 (Use\n$GITPERLLIB instead of $RUNNING_GIT_TESTS and centralize @INC munging,\n2006-07-03) did, is not touch any of the t/* files (neither my \"this on\ntop of yours\", nor the patch I am responding to), but to fix the build\nprocedure of gitweb/gitweb.perl so that the above script rewriting is also\napplied to it.\n"},{"id":"78332","messageId":"7vbq2k9z65.fsf@gitster.siamese.dyndns.org","threadId":"13738","inReplyTo":"200806020019.23858.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: use Git.pm, and use its parse_rev method for git_get_head_hash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-02T05:35:46Z","receivedAt":"2008-06-02T05:35:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> On Sat, 31 May 2008, Lea Wiemann wrote:\n> ...\n>> So you end up with these layers: \n>> \n>> (Layer 0: Front-end [HTML] caching.)\n>> Layer 1: Application (Gitweb)\n>> Layer 2: Back-end caching\n>> Layer 3: Repository access (command parsing)\n>> Layer 4: Calls to the git binary\n>> \n>> Layer 3 and 4 are application-independent (i.e. not Gitweb specific), \n>> and since they form a usable API, they might as well be written as a \n>> separate API rather than lumped together with Gitweb.  Git.pm is a start \n>> of such an API (it does layer 4 and a little bit of layer 3), so it \n>> seems natural for me to extend it.\n>\n> This assumes that command parsing used by gitweb are generic enough\n> to put them in Git.pm.  But some IMVHO are very gitweb-specific, for\n> example the part in parse_commit_text() beginning with \n>   # remove leading stuff of merges to make the interesting part visible\n> and the 'age_string*' keys there, parse_difftree_raw_line() which\n> currently does not support '-z' output, parse_from_to_diffinfo() which\n> is _very_ gitweb specific, git_get_heads_list() which is not generic\n> enough (it gets info which gitweb needs, but no more), etc.\n>\n>> Layer 2 is application-independent as well, so it can become an extra \n>> class in Git.pm or a separate module.  (It should stay independent of \n>> layers 3 and 4).\n>\n> I think it would be better as separate module.  Would it be Git::Cache\n> (or Git::Caching), Gitweb::Cache, or part of gitweb, that would have\n> to be decided.  Besides, I'm not sure if it is really application-\n> -independent as you say: I think we would get better result if we\n> collate data first, which is application dependent.  Also I think\n> there is no sense to cache everything: what to cache is again\n> application dependent.\n\nEven though I (for some unknown reason) rarely agree with Jakub on this\nlist, I agree 100% with the above paragraph.  In fact I yesterday started\nto write exactly the same thing but I could not word it well enough, and I\nam glad Jakub said what I wanted to say in a form that is much clearer\nthan I would have ;-).\n"},{"id":"78345","messageId":"20080602092926.GJ18781@machine.or.cz","threadId":"13738","inReplyTo":"200806020019.23858.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: use Git.pm, and use its parse_rev method for git_get_head_hash","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-06-02T09:29:26Z","receivedAt":"2008-06-02T09:29:26Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Mon, Jun 02, 2008 at 12:19:23AM +0200, Jakub Narebski wrote:\n> On Sat, 31 May 2008, Lea Wiemann wrote:\n> > Layer 2 is application-independent as well, so it can become an extra \n> > class in Git.pm or a separate module.  (It should stay independent of \n> > layers 3 and 4).\n> \n> I think it would be better as separate module.  Would it be Git::Cache\n> (or Git::Caching), Gitweb::Cache, or part of gitweb, that would have\n> to be decided.  Besides, I'm not sure if it is really application-\n> -independent as you say: I think we would get better result if we\n> collate data first, which is application dependent.  Also I think\n> there is no sense to cache everything: what to cache is again\n> application dependent.\n\nI'm not very comfortable with putting caching directly to Git either.\n\nEven if you _would_ decide that you want to add caching directly to Git\ninterface, it would be better to use extra module Git::Cached and\nauto-wrap all Git functions through AUTOLOAD. But that still has the\nproblems Jakub desrcibed.\n\n> > We may need to have an explicitly implemented layer 0 (front-end \n> > caching) in Gitweb for at least a subset of pages (like project pages), \n> > but we'll see if that's necessary.\n> \n> I think that front-end caching (HTML/RSS/Atom output caching) has sense\n> for large web pages (large size and large number of items), such as\n> projects list page if it is unpaginated (and perhaps even if it is\n> divided into pages, although I'm sure not for project search page),\n> commonly requested snapshots (if they are enabled), large trees like\n> SPECS directory with all the package *.spec files for distribution\n> repository, perhaps summary/feed for most popular projects. If (when)\n> syntax highlighting would got implemented, probably also caching\n> blob view (as CPU can become issue).\n> \n> Front-end (HTML output) caching has the advantages of easy to calculate\n> strong ETag, and web server support for If-Match: and If-None-Match:\n> HTTP/1.1 headers.  You can easy support Range: request, needed for\n> resuming download (e.g. for downloading snapshots, if this feature is\n> enabled in gitweb).\n\nCaching snapshots would definitely make sense, sure.\n\n> You can even compress the output, and serve it to clients which\n> support proper transparent compression (Content-Encoding).\n\nWhat does this have to do with caching?\n\n> And of course it has the advantage of actually been written and tested\n> in work, in the case of kernel.org gitweb.  Although caching parsed\n> data was implemented in repo.or.cz gitweb, it was done only for\n> projects list view, and it is quite new and not so thoroughly tested\n>   http://article.gmane.org/gmane.comp.version-control.git/77469\n\nThis argument does have some value, but I don't think it matters too\nmuch, since as far as I understood, it is going to get largely\nreimplemented anyway.\n\n> It would be nice for front-end caching to have an option to use absolute\n> time for all time/dates, and to (optionally) not use adaptive\n> Content-Type...\n\nI'd hate to have to do unnecessary compromises in order to get sensible\ncaching.\n\nEven in your excellent series on Gitweb caching series, I didn't spot\nany arguments that would put frontend caching in front of the\nintermediate data caching option; yes, it is the simplest solution\nimplementation-wise, but also the least flexible one. My gut feeling is\nstill to go with data caching instead of HTML caching.\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nWhatever you can do, or dream you can, begin it.\nBoldness has genius, power, and magic in it.\t-- J. W. von Goethe\n"},{"id":"78365","messageId":"4843FEE2.1070708@gmail.com","threadId":"13738","inReplyTo":"7vfxrwa008.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] test-lib.sh: set PERL5LIB instead of GITPERLLIB","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-02T14:08:34Z","receivedAt":"2008-06-02T14:08:34Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Junio C Hamano wrote:\n> the primary Makefile has this rule to munge our perl scripts:\n> \n>                     -e '\ts=.*=use lib (split(/:/, $$ENV{GITPERLLIB} || \"@@INSTLIBDIR@@\"));=' \\\n> \n> The real fix to the issue [is] to fix the build procedure of\n> gitweb/gitweb.perl so that the above script rewriting is also applied to it.\n\nI see -- in that case, gitweb.perl should indeed be fixed.\n\nStill however, for the Perl tests (t/t9700/test.pl in my branch) we \ndon't have a Makefile, so the only way for them to pick up the right \nGit.pm module is modifying PERL5LIB in test-lib.sh. [1]  I just realized \nthat we not only don't need to set GITPERLLIB in this case, but we \nshould actually unset it so that user-set paths don't cause a wrong \nversion of Git.pm to be loaded.  I'll send a new patch.\n\n-- Lea\n\n[1] Or alternatively adding some (lengthy and cwd-dependent) \"use lib \n...\" statement at the top of every Perl test file, but that doesn't seem \npreferable given that we can solve this centrally in test-lib.sh.\n"},{"id":"78366","messageId":"1212415996-32130-1-git-send-email-LeWiemann@gmail.com","threadId":"13738","inReplyTo":"4843FEE2.1070708@gmail.com","subject":"[PATCH v2] test-lib.sh: set PERL5LIB instead of GITPERLLIB","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-02T14:13:16Z","receivedAt":"2008-06-02T14:13:16Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"From: Lea Wiemann <lewiemann@gmail.com>\n\nBy setting PERL5LIB for the tests, we enable Perl test scripts to just\nsay \"use Git;\" without adding the GITPERLLIB paths to @INC beforehand.\n\nAlso, unset GITPERLLIB so that user-set paths in GETPERLLIB don't\ncause the wrong module to be loaded.\n\nSigned-off-by: Lea Wiemann <LeWiemann@gmail.com>\n---\nAdded since v1: Unset GITPERLLIB.\n\n t/test-lib.sh |    6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 99b63da..8ea0511 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -454,8 +454,10 @@ GIT_CONFIG_NOSYSTEM=1\n GIT_CONFIG_NOGLOBAL=1\n export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_CONFIG_NOGLOBAL\n \n-GITPERLLIB=$(pwd)/../perl/blib/lib:$(pwd)/../perl/blib/arch/auto/Git\n-export GITPERLLIB\n+unset GITPERLLIB\n+test -n \"$PERL5LIB\" && PERL5LIB=\":$PERL5LIB\"\n+PERL5LIB=\"$(pwd)/../perl/blib/lib:$(pwd)/../perl/blib/arch/auto/Git$PERL5LIB\"\n+export PERL5LIB\n test -d ../templates/blt || {\n \terror \"You haven't built things yet, have you?\"\n }\n-- \n1.5.5.GIT\n"},{"id":"78402","messageId":"7v4p8b7gij.fsf@gitster.siamese.dyndns.org","threadId":"13738","inReplyTo":"1212415996-32130-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH v2] test-lib.sh: set PERL5LIB instead of GITPERLLIB","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-02T20:01:40Z","receivedAt":"2008-06-02T20:01:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lea Wiemann <lewiemann@gmail.com> writes:\n\n> From: Lea Wiemann <lewiemann@gmail.com>\n>\n> By setting PERL5LIB for the tests, we enable Perl test scripts to just\n> say \"use Git;\" without adding the GITPERLLIB paths to @INC beforehand.\n>\n> Also, unset GITPERLLIB so that user-set paths in GETPERLLIB don't\n> cause the wrong module to be loaded.\n>\n> Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>\n> ---\n> Added since v1: Unset GITPERLLIB.\n\nThis goes even further in the opposite direction from the original.  It\nlooks cleaner, and I'd prefer the approach _if_ it worked.\n\nBut I do not think it does.\n\nI think the reason we did not do this long time ago in 6fcca93 (Use\n$GITPERLLIB instead of $RUNNING_GIT_TESTS and centralize @INC munging,\n2006-07-03) was because of the precedence between \"use lib @these_paths\"\nand $PERL5LIB does not work the way you seem to think it does.\n\nIf you say in your script:\n\n\tuse lib '/usr/local/git/perl'\n        use Git;\n\n(and we do want to say so in our script to make sure that people can\ninstall Git.pm outside of system install paths), running the script with\nPERL5LIB set to elsewhere that has Git.pm would not help.\n\nI just tried:\n\n\t$ pwd\n        /net/knick/home/junio/junk/\n\t$ cat j.perl\n        #!/usr/bin/perl -w\n        use lib '/net/knick/home/junio/junk/1dir';\n        use G;\n        print $G::it;\n        $ cat 1dir/G.pm\n        package G;\n        our $it = 1;\n        1;\n        $ cat 2dir/G.pm\n        package G;\n        our $it = 2;\n        1;\n        $ perl -w j.perl\n        1\n        $ PERL5LIB=/net/knick/home/junio/junk/2dir perl -w j.perl\n        1\n\nBecause \"t/t9700/<many>.perl\" will _not_ be installed anyway, I do not\nthink they need to be \"built\" like the scripted Porcelain commands.  If\nthey unconditionally do \"use lib $ENV{GITPERLLIB};\" upfront, wouldn't that\nbe enough to guarantee that you would use the freshly built one during the\ntest, not the instsalled one?\n"},{"id":"78418","messageId":"200806022332.29853.jnareb@gmail.com","threadId":"13738","inReplyTo":"20080602092926.GJ18781@machine.or.cz","subject":"Re: [PATCH] gitweb: use Git.pm, and use its parse_rev method for git_get_head_hash","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-02T21:32:27Z","receivedAt":"2008-06-02T21:32:27Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 2 June 2008, Petr Baudis wrote:\n> On Mon, Jun 02, 2008 at 12:19:23AM +0200, Jakub Narebski wrote:\n> > On Sat, 31 May 2008, Lea Wiemann wrote:\n> > >\n> > > We may need to have an explicitly implemented layer 0 (front-end \n> > > caching) in Gitweb for at least a subset of pages (like project pages), \n> > > but we'll see if that's necessary.\n> > \n> > I think that front-end caching (HTML/RSS/Atom output caching) has sense\n> > for large web pages (large size and large number of items), such as\n> > projects list page if it is unpaginated (and perhaps even if it is\n> > divided into pages, although I'm sure not for project search page),\n> > commonly requested snapshots (if they are enabled), large trees like\n> > SPECS directory with all the package *.spec files for distribution\n> > repository, perhaps summary/feed for most popular projects. If (when)\n> > syntax highlighting would got implemented, probably also caching\n> > blob view (as CPU can become issue).\n> > \n> > Front-end (HTML output) caching has the advantages of easy to calculate\n> > strong ETag, and web server support for If-Match: and If-None-Match:\n> > HTTP/1.1 headers.  You can easy support Range: request, needed for\n> > resuming download (e.g. for downloading snapshots, if this feature is\n> > enabled in gitweb).\n> \n> Caching snapshots would definitely make sense, sure.\n\nThat reminds me to finally implement nicer (git-describe like)\n[proposed] snapshot filenames.  For example for snapshot of state\ngiven by some tag (snapshot of tagged release [1]), don't use\ngeneric\n  <project basename>-<40-chars sha1>.<suffix>\n  git-b2a42f55bc419352b848751b0763b0a2d1198479.tar.gz\nbut\n  <project basename>-<tag name>.<suffix>\n  git-v1.5.5.3.tar.gz\n(well, currently tags don't have 'snapshot' link, but this is easily\nfixed).  What do you think about (ab)using 'fp' (file_parent)\nparameter to pass proposed snapshot file name?\n\n[1] Would it be good feature to add support for limiting snapshots\nto snapshots only of tagged releases (which would be I guess more\nimportant when gitweb caching gets implemented).\n \n> > You can even compress the output, and serve it to clients which\n> > support proper transparent compression (Content-Encoding).\n> \n> What does this have to do with caching?\n\nWell, only in a sense that with front-end caching (to choose if CPU\nmatters most) this can be done \"for free\", without incurring extra\nCPU, at the cost of little more disk space.\n\nOf course, if most clients understand (accept) Content-Encoding\n(transfer encoding), you can store compressed output, with a little\nCPU cost to decompress for non-conformant clients; this way frontend\ncaching can have cache size comparable to [parsed] data caching.\n\n> > And of course it has the advantage of actually been written and tested\n> > in work, in the case of kernel.org gitweb.  Although caching parsed\n> > data was implemented in repo.or.cz gitweb, it was done only for\n> > projects list view, and it is quite new and not so thoroughly tested\n> >   http://article.gmane.org/gmane.comp.version-control.git/77469\n> \n> This argument does have some value, but I don't think it matters too\n> much, since as far as I understood, it is going to get largely\n> reimplemented anyway.\n\nWhat I'd like to see in a bit of time is some estimate how much time\nwould take implementing data caching almost from scratch (a bit of\ncode in repo's gitweb), compared to merging in kernel.org's gitweb\ncaching code...\n\n> > It would be nice for front-end caching to have an option to use absolute\n> > time for all time/dates, and to (optionally) not use adaptive\n> > Content-Type...\n> \n> I'd hate to have to do unnecessary compromises in order to get sensible\n> caching.\n> \n> Even in your excellent series on Gitweb caching series, I didn't spot\n> any arguments that would put frontend caching in front of the\n> intermediate data caching option; yes, it is the simplest solution\n> implementation-wise, but also the least flexible one. My gut feeling is\n> still to go with data caching instead of HTML caching.\n\nAs Lars Hjemli wrote in \"[RFC/PATCH] gitweb: Paginate project list\"\nthread (unfortunately not all articles got to git mailing list)\n\n  http://thread.gmane.org/gmane.comp.version-control.git/81838/focus=81875\n\n  <quote>\n  In cgit I've chosen \"projectlist in a single file\" and \"cache html\n  output\". This makes it cheap (in terms of cpu and io) to both generate\n  and serve the cached page (and the cache works for all pages).\n  </quote>\n\n  <quote>\n  While I agree that caching search result output almost never makes\n  sense, I think it's more important that cache hits requires minimal\n  processing. This is why I've chosen to cache the final result instead\n  of an intermediate state, but both solutions obviously got some pros\n  and cons.\n  </quote>\n\ncaching final output is important if you want to minimize processing\n(CPU time).  I'd say also if you want to implement Range: for resumable\ndownloads (snapshots), because otherwise I think it would be quote hard\nto do reasonably (with caching only data).\n\nSo I guess best solution would be mixed one: use output cache for large\nor CPU intensive pages, use data caching to limit cache size and for\nmaximum flexibility (relative dates, sorting by columns: athough that\nwould be best solved using some DHTML/JavaScript, paginated output,\nprojects search etc.)\n\n\nBy the way, we have your (Petr 'Pasky' Baudis, based on repo.or.cz)\nand John 'Warthog9' Hawley (based on kernel.org) statements that gitweb\nperformance is I/O bound, but I don't remember any hard data.\n\nI have said wrongly that one can use 'fio' tool to check I/O\nperformance; it is not true, this tool can be used to test _filesystem_\nby generating specified pattern of I/O load.  I don't know of any tool\nwhich allow to measure if I/O is bottleneck for given application;\n'iogrind' measures cold cache start, and it requires I think compiled\nprogram, as it uses Valgrind.  You can measure CPU load, time to\nresponse and memory usage using 'time' (running gitweb as script),\nApacheBench and top; you can measure latency using LatencyTOP.  \n\nIs there some iotop tool, and can it be used to measure performance\nbottlenecks of web scripts (web applications)?\n\n-- \nJakub Narebski\nPoland\n"},{"id":"78425","messageId":"484471E1.4040509@gmail.com","threadId":"13738","inReplyTo":"7v4p8b7gij.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] test-lib.sh: set PERL5LIB instead of GITPERLLIB","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-02T22:19:13Z","receivedAt":"2008-06-02T22:19:13Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Junio C Hamano wrote:\n> the precedence between \"use lib @these_paths\"\n\nApologies, I somehow missed the or-expression in $$ENV{GITPERLLIB} || \n\"@@INSTLIBDIR@@\" and thought we didn't set any library paths if we unset \nGITPERLLIB. :(  So yes, of course my patch is rubbish -- sorry for \nwasting your time, I'll check more carefully next time!\n\n> If [t/t9700/<many>.perl] unconditionally do \"use lib $ENV{GITPERLLIB};\"\n> upfront, wouldn't that be enough to guarantee that you would use the\n> freshly built one during the test, not the instsalled one?\n\nYup, that's fine.  I'll add it to the test file.\n\n-- Lea\n"},{"id":"78427","messageId":"484474D7.6070306@gmail.com","threadId":"13738","inReplyTo":"200806022332.29853.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: use Git.pm, and use its parse_rev method for git_get_head_hash","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-02T22:31:51Z","receivedAt":"2008-06-02T22:31:51Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Jakub Narebski wrote:\n> Is there some iotop tool, and can it be used to measure performance\n> bottlenecks of web scripts (web applications)?\n\nThere is iotop and sysstat's iostat, but the more interesting measure \nwould probably be comparing top's 'wait' vs user+sys CPU time under \nrealistic load ('wait' indicates that the CPU is idling because it is \nwaiting for IO).\n\n-- Lea\n"},{"id":"78441","messageId":"48448E69.30203@gmail.com","threadId":"13738","inReplyTo":"7vfxrwa008.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] test-lib.sh: set PERL5LIB instead of GITPERLLIB","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-03T00:20:57Z","receivedAt":"2008-06-03T00:20:57Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Junio C Hamano wrote:\n> the primary Makefile has this rule to munge our perl scripts:\n>\n>                 [snip]\n>                 sed -e '1{' \\\n>                     -e '\ts|#!.*perl|#!$(PERL_PATH_SQ)|' \\\n>                     -e '\th' \\\n>                     -e '\ts=.*=use lib (split(/:/, $$ENV{GITPERLLIB} || \"@@INSTLIBDIR@@\"));=' \\\n> \n> so that people do not have to have that path on PERL5LIB in their\n> environment when they use [...] scripts that do \"use Git\"\n> \n> The real fix to the issue [is]  to fix the build\n> procedure of gitweb/gitweb.perl so that the above script rewriting is also\n> applied to it.\n\nI'm not good enough with sed to fix this, so if anyone wants to write a \npatch to the Makefile that adds \"use lib (split(/:/, $$ENV{GITPERLLIB} \n|| \"@@INSTLIBDIR@@\"))\" to gitweb.perl, please go ahead. ;-)\n\n-- Lea\n"}]}