{"thread":{"id":"5565","subject":"[PATCH] gitweb: Do not parse refs by hand, use git-peek-remote instead","startedAt":"2006-09-14T21:27:22Z","lastAt":"2006-09-15T10:49:35Z","messageCount":9,"participants":["Jakub Narebski","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"26926","messageId":"200609142327.23059.jnareb@gmail.com","threadId":"5565","inReplyTo":null,"subject":"[PATCH] gitweb: Do not parse refs by hand, use git-peek-remote instead","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-09-14T21:27:22Z","receivedAt":"2006-09-14T21:27:22Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"This is response to Linus work on packed refs. Additionally it\nmakes gitweb work with symrefs, too.\n\nDo not parse refs by hand, using File::Find and reading individual\nheads to get hash of reference, but use git-peek-remote output\ninstead. Assume that the hash for deref (with ^{}) always follows hash\nfor ref, and that we hav derefs only for tags objects; this removes\ncall to git_get_type (and git-cat-file -t invocation) for tags, which\nspeeds \"summary\" and \"tags\" views generation, but might slow\ngeneration of \"heads\" view a bit. As of now we do not save and use the\nderef hash.\n\nRemove git_get_hash_by_ref while at it, as git_get_refs_list was the\nonly place it was used.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nBy the way, the previous patch\n  \"gitweb: Use File::Find::find in git_get_projects_list\"\nwas added during working on this feature.\n\n gitweb/gitweb.perl |   39 +++++++++++++++++----------------------\n 1 files changed, 17 insertions(+), 22 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 25383bc..4e58a6b 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -676,19 +676,6 @@ sub git_get_hash_by_path {\n ## ......................................................................\n ## git utility functions, directly accessing git repository\n \n-# assumes that PATH is not symref\n-sub git_get_hash_by_ref {\n-\tmy $path = shift;\n-\n-\topen my $fd, \"$projectroot/$path\" or return undef;\n-\tmy $head = <$fd>;\n-\tclose $fd;\n-\tchomp $head;\n-\tif ($head =~ m/^[0-9a-fA-F]{40}$/) {\n-\t\treturn $head;\n-\t}\n-}\n-\n sub git_get_project_description {\n \tmy $path = shift;\n \n@@ -1098,17 +1085,25 @@ sub git_get_refs_list {\n \tmy @reflist;\n \n \tmy @refs;\n-\tmy $pfxlen = length(\"$projectroot/$project/$ref_dir\");\n-\tFile::Find::find(sub {\n-\t\treturn if (/^\\./);\n-\t\tif (-f $_) {\n-\t\t\tpush @refs, substr($File::Find::name, $pfxlen + 1);\n+\topen my $fd, \"-|\", $GIT, \"peek-remote\", \"$projectroot/$project/\"\n+\t\tor return;\n+\twhile (my $line = <$fd>) {\n+\t\tchomp $line;\n+\t\tif ($line =~ m/^([0-9a-fA-F]{40})\\t$ref_dir\\/?([^\\^]+)$/) {\n+\t\t\tpush @refs, { hash => $1, name => $2 };\n+\t\t} elsif ($line =~ m/^[0-9a-fA-F]{40}\\t$ref_dir\\/?.*\\^\\{\\}$/) {\n+\t\t\t# assume that \"peeled\" ref is always after ref,\n+\t\t\t# and that you \"peel\" (deref) tags only\n+\t\t\t$refs[$#refs]->{'type'} = \"tag\";\n \t\t}\n-\t}, \"$projectroot/$project/$ref_dir\");\n+\t}\n+\tclose $fd;\n+\n+\tforeach my $ref (@refs) {\n+\t\tmy $ref_file = $ref->{'name'};\n+\t\tmy $ref_id   = $ref->{'hash'};\n \n-\tforeach my $ref_file (@refs) {\n-\t\tmy $ref_id = git_get_hash_by_ref(\"$project/$ref_dir/$ref_file\");\n-\t\tmy $type = git_get_type($ref_id) || next;\n+\t\tmy $type = $ref->{'type'} || git_get_type($ref_id) || next;\n \t\tmy %ref_item = parse_ref($ref_file, $ref_id, $type);\n \n \t\tpush @reflist, \\%ref_item;\n-- \n1.4.2\n"},{"id":"26935","messageId":"7v8xkm2gfs.fsf@assigned-by-dhcp.cox.net","threadId":"5565","inReplyTo":"200609142327.23059.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Do not parse refs by hand, use git-peek-remote instead","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-14T23:09:59Z","receivedAt":"2006-09-14T23:09:59Z","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> Remove git_get_hash_by_ref while at it, as git_get_refs_list was the\n> only place it was used.\n\nThat's a very good news.\n\n> +\topen my $fd, \"-|\", $GIT, \"peek-remote\", \"$projectroot/$project/\"\n> +\t\tor return;\n> +\twhile (my $line = <$fd>) {\n> +\t\tchomp $line;\n> +\t\tif ($line =~ m/^([0-9a-fA-F]{40})\\t$ref_dir\\/?([^\\^]+)$/) {\n> +\t\t\tpush @refs, { hash => $1, name => $2 };\n> +\t\t} elsif ($line =~ m/^[0-9a-fA-F]{40}\\t$ref_dir\\/?.*\\^\\{\\}$/) {\n> +\t\t\t# assume that \"peeled\" ref is always after ref,\n> +\t\t\t# and that you \"peel\" (deref) tags only\n> +\t\t\t$refs[$#refs]->{'type'} = \"tag\";\n> \t\t}\n\nThe assumption is good; we never show a ref^{} before showing\nref itself.  But you probably would want to safeguard yourself\nfrom future changes by not depending on \"immediately after\".\nAt least you can check $refs[-1]{'name'} is the same as the\nunpeeled ref you are looking at, like this:\n\n\tif ($line =~ m/^([0-9a-fA-F]{40})\\t$ref_dir\\/?([^\\^]+)$/) {\n\t\tpush @refs, { hash => $1, name => $2 };\n\t} elsif ($line =~ m/^[0-9a-fA-F]{40}\\t$ref_dir\\/?(.*)\\^\\{\\}$/ &&\n                 $1 eq $refs[-1]{'name'}) {\n\t\t# most likely a tag is followed by its peeled\n\t\t# one, and when that happens we know the\n\t\t# previous one was of type 'tag'.\n\t\t$refs[$#refs]->{'type'} = \"tag\";\n\t} ...\n\n> +\tforeach my $ref (@refs) {\n> +\t\tmy $ref_file = $ref->{'name'};\n> +\t\tmy $ref_id   = $ref->{'hash'};\n> -\t\tmy $type = git_get_type($ref_id) || next;\n> +\t\tmy $type = $ref->{'type'} || git_get_type($ref_id) || next;\n\nAnd this is a good incremental change to reduce number of\nexternal calls.\n"},{"id":"26938","messageId":"200609150343.28334.jnareb@gmail.com","threadId":"5565","inReplyTo":"7v8xkm2gfs.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH (take 2)] gitweb: Do not parse refs by hand, use git-peek-remote instead","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-09-15T01:43:28Z","receivedAt":"2006-09-15T01:43:28Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"This is in response to Linus work on packed refs. Additionally it\nmakes gitweb work with symrefs, too.\n\nDo not parse refs by hand, using File::Find and reading individual\nheads to get hash of reference, but use git-peek-remote output\ninstead. Assume that the hash for deref (with ^{}) always follows hash\nfor ref, and that we hav derefs only for tags objects; this removes\ncall to git_get_type (and git-cat-file -t invocation) for tags, which\nspeeds \"summary\" and \"tags\" views generation, but might slow\ngeneration of \"heads\" view a bit. As of now we do not save and use the\nderef hash.\n\nRemove git_get_hash_by_ref while at it, as git_get_refs_list was the\nonly place it was used.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\n gitweb/gitweb.perl |   41 +++++++++++++++++++----------------------\n 1 files changed, 19 insertions(+), 22 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 25383bc..b4a890b 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -676,19 +676,6 @@ sub git_get_hash_by_path {\n ## ......................................................................\n ## git utility functions, directly accessing git repository\n \n-# assumes that PATH is not symref\n-sub git_get_hash_by_ref {\n-\tmy $path = shift;\n-\n-\topen my $fd, \"$projectroot/$path\" or return undef;\n-\tmy $head = <$fd>;\n-\tclose $fd;\n-\tchomp $head;\n-\tif ($head =~ m/^[0-9a-fA-F]{40}$/) {\n-\t\treturn $head;\n-\t}\n-}\n-\n sub git_get_project_description {\n \tmy $path = shift;\n \n@@ -1098,17 +1085,27 @@ sub git_get_refs_list {\n \tmy @reflist;\n \n \tmy @refs;\n-\tmy $pfxlen = length(\"$projectroot/$project/$ref_dir\");\n-\tFile::Find::find(sub {\n-\t\treturn if (/^\\./);\n-\t\tif (-f $_) {\n-\t\t\tpush @refs, substr($File::Find::name, $pfxlen + 1);\n+\topen my $fd, \"-|\", $GIT, \"peek-remote\", \"$projectroot/$project/\"\n+\t\tor return;\n+\twhile (my $line = <$fd>) {\n+\t\tchomp $line;\n+\t\tif ($line =~ m/^([0-9a-fA-F]{40})\\t$ref_dir\\/?([^\\^]+)$/) {\n+\t\t\tpush @refs, { hash => $1, name => $2 };\n+\t\t} elsif ($line =~ m/^[0-9a-fA-F]{40}\\t$ref_dir\\/?(.*)\\^\\{\\}$/ &&\n+\t\t         $1 eq $refs[-1]{'name'}) {\n+\t\t\t# most likely a tag is followed by its peeled\n+\t\t\t# (deref) one, and when that happens we know the\n+\t\t\t# previous one was of type 'tag'.\n+\t\t\t$refs[-1]{'type'} = \"tag\";\n \t\t}\n-\t}, \"$projectroot/$project/$ref_dir\");\n+\t}\n+\tclose $fd;\n+\n+\tforeach my $ref (@refs) {\n+\t\tmy $ref_file = $ref->{'name'};\n+\t\tmy $ref_id   = $ref->{'hash'};\n \n-\tforeach my $ref_file (@refs) {\n-\t\tmy $ref_id = git_get_hash_by_ref(\"$project/$ref_dir/$ref_file\");\n-\t\tmy $type = git_get_type($ref_id) || next;\n+\t\tmy $type = $ref->{'type'} || git_get_type($ref_id) || next;\n \t\tmy %ref_item = parse_ref($ref_file, $ref_id, $type);\n \n \t\tpush @reflist, \\%ref_item;\n-- \n1.4.2\n"},{"id":"26945","messageId":"7v4pv91wqk.fsf@assigned-by-dhcp.cox.net","threadId":"5565","inReplyTo":"200609150343.28334.jnareb@gmail.com","subject":"Re: [PATCH (take 2)] gitweb: Do not parse refs by hand, use git-peek-remote instead","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-15T06:15:31Z","receivedAt":"2006-09-15T06:15:31Z","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> This is in response to Linus work on packed refs. Additionally it\n> makes gitweb work with symrefs, too.\n>\n> Do not parse refs by hand, using File::Find and reading individual\n> heads to get hash of reference, but use git-peek-remote output\n> instead.\n\nLooks nicer.  Will apply.\n\nNow, once we start doing this, it may make sense to rethink how\nthis function and git_get_references functions are used.  I\nthink\n\n\tgit grep -n -e '^sub ' \\\n        \t-e git_get_references \\\n                -e git_get_refs_list gitweb/gitweb.perl\n\nwould be instructive how wasteful the current code is.\n\nget_refs_list is called _TWICE_ in git_summary and worse yet\nvery late in the function, after calling git_get_references that\ncould already have done what the function does (by the way,\ngit_get_references already knows how to use peek-remote output\nbut for some reason it uses ls-remote -- I think you can safely\nrewrite it to use peek-remote).  So you end up doing peek-remote\nthree times to draw the summary page.\n\ngit_get_references are called from almost everywhere that shows\nthe list of commits, which is understandable because we would\nwant to see those markers in the list.\n\nI very much suspect that you can use git_get_refs_list to return\na hash and a sorted list at the same time from the same input\nand make git_summary to do with just a single call to it, and\nget rid of git_get_references with a little bit of restructuring\nof the caller.\n\nHmm?\n"},{"id":"26947","messageId":"eedjrj$tja$1@sea.gmane.org","threadId":"5565","inReplyTo":"7v4pv91wqk.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH (take 2)] gitweb: Do not parse refs by hand, use git-peek-remote instead","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-09-15T07:14:23Z","receivedAt":"2006-09-15T07:14:23Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n>> This is in response to Linus work on packed refs. Additionally it\n>> makes gitweb work with symrefs, too.\n>>\n>> Do not parse refs by hand, using File::Find and reading individual\n>> heads to get hash of reference, but use git-peek-remote output\n>> instead.\n> \n> Looks nicer.  Will apply.\n> \n> Now, once we start doing this, it may make sense to rethink how\n> this function and git_get_references functions are used.\n[...]\n> I very much suspect that you can use git_get_refs_list to return\n> a hash and a sorted list at the same time from the same input\n> and make git_summary to do with just a single call to it, and\n> get rid of git_get_references with a little bit of restructuring\n> of the caller.\n\nIt can be done. Well, we could also collapse git_get_references and\ngit_get_refs_list into one subroutine, but it would be slightly slower\nthan git_get_references.\n\nBut, if we change git_get_refs_list to do also work of git_get_references,\nwe should also change git_get_references to not use info/refs file at all\n(it can, and usually for unknown reasons is stale) but always use\ngit-peek-remote, for consistency.\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"26950","messageId":"7vvenpzi1h.fsf@assigned-by-dhcp.cox.net","threadId":"5565","inReplyTo":"eedjrj$tja$1@sea.gmane.org","subject":"Re: [PATCH (take 2)] gitweb: Do not parse refs by hand, use git-peek-remote instead","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-15T07:48:58Z","receivedAt":"2006-09-15T07:48:58Z","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> But, if we change git_get_refs_list to do also work of git_get_references,\n> we should also change git_get_references to not use info/refs file at all\n> (it can, and usually for unknown reasons is stale) but always use\n> git-peek-remote, for consistency.\n\nYes that would make sense.  A repository served by gitweb does\nnot necessarily has to serve objects over http transport, so it\nis nicer not to require info/refs to even exist or up to date.\n"},{"id":"26952","messageId":"200609151043.20681.jnareb@gmail.com","threadId":"5565","inReplyTo":"7vvenpzi1h.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH (take 2)] gitweb: Do not parse refs by hand, use git-peek-remote instead","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-09-15T08:43:20Z","receivedAt":"2006-09-15T08:43:20Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n> > But, if we change git_get_refs_list to do also work of git_get_references,\n> > we should also change git_get_references to not use info/refs file at all\n> > (it can, and usually for unknown reasons is stale) but always use\n> > git-peek-remote, for consistency.\n> \n> Yes that would make sense.  A repository served by gitweb does\n> not necessarily has to serve objects over http transport, so it\n> is nicer not to require info/refs to even exist or up to date.\n\nWe do not require info/refs, as currently git_get_references falls\nback to git-ls-remotes (should be git-peek-remote as it is faster)\nif info/refs does not exist. But info/refs is usually stale; I guess\nit is updated on pull/fetch/push, but not on commit.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"26955","messageId":"7vr6ydzehk.fsf@assigned-by-dhcp.cox.net","threadId":"5565","inReplyTo":"200609151043.20681.jnareb@gmail.com","subject":"Re: [PATCH (take 2)] gitweb: Do not parse refs by hand, use git-peek-remote instead","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-15T09:05:43Z","receivedAt":"2006-09-15T09:05:43Z","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> Junio C Hamano wrote:\n>> Jakub Narebski <jnareb@gmail.com> writes:\n>> \n>> > But, if we change git_get_refs_list to do also work of git_get_references,\n>> > we should also change git_get_references to not use info/refs file at all\n>> > (it can, and usually for unknown reasons is stale) but always use\n>> > git-peek-remote, for consistency.\n>> \n>> Yes that would make sense.  A repository served by gitweb does\n>> not necessarily has to serve objects over http transport, so it\n>> is nicer not to require info/refs to even exist or up to date.\n>\n> We do not require info/refs, as currently git_get_references falls\n> back to git-ls-remotes (should be git-peek-remote as it is faster)\n> if info/refs does not exist. But info/refs is usually stale; I guess\n> it is updated on pull/fetch/push, but not on commit.\n\nYes, we are in agreement and are saying the same thing.\n"},{"id":"26958","messageId":"200609151249.35872.jnareb@gmail.com","threadId":"5565","inReplyTo":"7v4pv91wqk.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH (take 2)] gitweb: Do not parse refs by hand, use git-peek-remote instead","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-09-15T10:49:35Z","receivedAt":"2006-09-15T10:49:35Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Now, once we start doing this, it may make sense to rethink how\n> this function and git_get_references functions are used.  I\n> think\n> \n> \tgit grep -n -e '^sub ' \\\n>         \t-e git_get_references \\\n>                 -e git_get_refs_list gitweb/gitweb.perl\n> \n> would be instructive how wasteful the current code is.\n> \n> get_refs_list is called _TWICE_ in git_summary and worse yet\n> very late in the function, after calling git_get_references that\n> could already have done what the function does (by the way,\n> git_get_references already knows how to use peek-remote output\n> but for some reason it uses ls-remote -- I think you can safely\n> rewrite it to use peek-remote).  So you end up doing peek-remote\n> three times to draw the summary page.\n> \n> git_get_references are called from almost everywhere that shows\n> the list of commits, which is understandable because we would\n> want to see those markers in the list.\n> \n> I very much suspect that you can use git_get_refs_list to return\n> a hash and a sorted list at the same time from the same input\n> and make git_summary to do with just a single call to it, and\n> get rid of git_get_references with a little bit of restructuring\n> of the caller.\n\nWe can easily collapse two calls for git_get_refs_list in gi_summary, \none for tags and one for heads into one call plus some filtering. \nChanging git_get_refs_list to do also the job of git_get_references \nmeans that in git_tags and git_heads we do extra the job of \ngit_get_references. Neither git_tags, nor git_heads use references and \nrefs marker; using the heads references in git_heads and tags \nreferences in git_tags is repeating the same information twice, \ncluttering the output. Unless we want to add yet another subroutine...\n\nBut as call to git-peek-remote is what takes most time, we can waste \nsome time processing references which we would not use for the sake of \nclarity. Well, we can get rid of git_get_references too, if we don't \nmind slight decrease in performance.\n\n\nWe would then use:\ngit_summary:\n  my ($refs, $reflist) = git_get_refs_list();\n  my @taglist = map { s!^tags/!! } grep { m!^tags/! } @$reflist;\n  my @headlist = map { s!^heads/!! } grep { m!^heads/! } @$reflist;\n\ngit_heads, git_tags:\n  my (undef, $taglist)  = git_get_refs_list(\"tags\");\n  my (undef, $headlist) = git_get_refs_list(\"heads\");\n\neverywhere else\n  my ($refs, undef) = git_get_refs_list($type);\n\n-- \nJakub Narebski\nPoland\n"}]}