{"thread":{"id":"5558","subject":"[PATCH] gitweb: Use File::Find::find in git_get_projects_list","startedAt":"2006-09-14T06:39:55Z","lastAt":"2006-09-14T21:50:24Z","messageCount":19,"participants":["Jakub Narebski","Junio C Hamano","Randal L. Schwartz"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"26872","messageId":"200609140839.56181.jnareb@gmail.com","threadId":"5558","inReplyTo":null,"subject":"[PATCH] gitweb: Use File::Find::find in git_get_projects_list","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-09-14T06:39:55Z","receivedAt":"2006-09-14T06:39:55Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Earlier code to get list of projects when $projects_list is a\ndirectory (e.g. when it is equal to $projectroot) had a hardcoded flat\n(one level) list of directories. Allow for projects to be in\nsubdirectories also for $projects_list being a directory by using\nFile::Find.\n\nSigned-off-by: Jakub Narebski \n---\nThis doesn't add much overhead to \"project_list\" view,\ncompared to previous version; times are the same within margin\nof error.\n\n gitweb/gitweb.perl | 29 +++++++++++++++++++++--------\n 1 files changed, 21 insertions(+), 8 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex c3544dd..470bff2 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -715,16 +715,29 @@ sub git_get_projects_list {\n \tif (-d $projects_list) {\n \t\t# search in directory\n \t\tmy $dir = $projects_list;\n-\t\topendir my ($dh), $dir or return undef;\n-\t\twhile (my $dir = readdir($dh)) {\n-\t\t\tif (-e \"$projectroot/$dir/HEAD\") {\n-\t\t\t\tmy $pr = {\n-\t\t\t\t\tpath => $dir,\n-\t\t\t\t};\n-\t\t\t\tpush @list, $pr\n+\t\tmy $pfxlen = length(\"$dir\");\n+\n+\t\tsub wanted {\n+\t\t\t# skip dot files (hidden files), check only directories\n+\t\t\t#return if (/^\\./);\n+\t\t\treturn unless (-d $_);\n+\n+\t\t\tmy $subdir = substr($File::Find::name, $pfxlen + 1);\n+\t\t\t# we check related file in $projectroot\n+\t\t\tif (-e \"$projectroot/$subdir/HEAD\") {\n+\t\t\t\tpush @list, { path => $subdir };\n+\t\t\t\t$File::Find::prune = 1;\n \t\t\t}\n \t\t}\n-\t\tclosedir($dh);\n+\n+\t\tFile::Find::find({\n+\t\t\tno_chdir => 1, # do not change directory\n+\t\t\tfollow_fast => 1, # follow symbolic links\n+\t\t\t#follow_skip => 2, # ignore duplicated files and directories\n+\t\t\tdangling_symlinks => 0, # ignore dangling symlinks, silently\n+\t\t\twanted => \\&wanted,\n+\t\t}, \"$dir\");\n+\n \t} elsif (-f $projects_list) {\n \t\t# read from file(url-encoded):\n \t\t# 'git%2Fgit.git Linus+Torvalds'\n-- \n1.4.2\n"},{"id":"26875","messageId":"7v8xkm6gr6.fsf@assigned-by-dhcp.cox.net","threadId":"5558","inReplyTo":"200609140839.56181.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Use File::Find::find in git_get_projects_list","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-14T07:37:17Z","receivedAt":"2006-09-14T07:37:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> +\t\tsub wanted {\n> +\t\t\t# skip dot files (hidden files), check only directories\n> +\t\t\t#return if (/^\\./);\n\nLeftover comment?\n\n> +\t\t\tmy $subdir = substr($File::Find::name, $pfxlen + 1);\n> +\t\t\t# we check related file in $projectroot\n> +\t\t\tif (-e \"$projectroot/$subdir/HEAD\") {\n> +\t\t\t\tpush @list, { path => $subdir };\n> +\t\t\t\t$File::Find::prune = 1;\n\nWe might want to do an extra cheap check to make what we found\nis sane, to prevent us getting confused by a random file whose\nname happens to be HEAD.\n\nFor example, it is a regular file whose contents is a single\nline and begins with \"ref: refs/heads/\" (16 bytes) or it is a\nsymlink and readlink result begins with \"refs/heads/\" (11\nbytes).\n\nIf you feel opening and reading the file is an added overhead,\nchecking for $project/$subdir/{objects,refs}/ directories might\nbe a good alternative.\n\n> +\t\tFile::Find::find({\n> +\t\t\tno_chdir => 1, # do not change directory\n> +\t\t\tfollow_fast => 1, # follow symbolic links\n\nWhat is the reason behind choosing follow_fast? By saying\nfollow_anything, you choose to care about cases where there are\nsymlinks under projectroot to point at various projects. If\nthat is the case, don't you want to make sure you include the\nsame project only once?\n\n> +\t\t\t#follow_skip => 2, # ignore duplicated files and directories\n\nLeftover comment?\n\nAbout these two leftover comments, if you decided you did not\nwant them, please do not leave them behind.\n\nIf on the other hand you wanted to hint others that you are not\nsure about your decision, it would probably be better to say\nthat honestly in the comment, perhaps mark the message as RFC,\nand describe what the issues are, like so:\n\n\tsub wanted {\n\t\t# We might want to also ignore dot files, by\n # saying \"return if /^\\./;\" here, but there is\n # no inherent reason for us to forbid a repository\n # name from starting with a dot.\n\n # We check only if a directory looks like a git\n # repo and do not care about non directories.\n # Note that this cannot be done with \"-d _\"\n # because we are using follow_fast and the last\n # stat was done with lstat(); we want to catch a\n # symlink that points at a directory.\n return unless -d $_;\n ...\n"},{"id":"26876","messageId":"200609140959.04061.jnareb@gmail.com","threadId":"5558","inReplyTo":"7v8xkm6gr6.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] gitweb: Use File::Find::find in git_get_projects_list","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-09-14T07:59:03Z","receivedAt":"2006-09-14T07:59:03Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Dnia czwartek 14. września 2006 09:37, napisałeś:\n> Jakub Narebski writes:\n> \n> > +\t\tsub wanted {\n> > +\t\t\t# skip dot files (hidden files), check only directories\n> > +\t\t\t#return if (/^\\./);\n> \n> Leftover comment?\n\nLeftover comment, from copying anonymous 'wanted' subroutine\nfrom git_get_refs_list. But I have realized that for gitweb\nfor only one project one could have \".git\" as a project name,\ne.g. by putting $projectroot to be live git repository (working\ndirectory of git repository).\n\n> > +\t\t\tmy $subdir = substr($File::Find::name, $pfxlen + 1);\n> > +\t\t\t# we check related file in $projectroot\n> > +\t\t\tif (-e \"$projectroot/$subdir/HEAD\") {\n> > +\t\t\t\tpush @list, { path => $subdir };\n> > +\t\t\t\t$File::Find::prune = 1;\n> \n> We might want to do an extra cheap check to make what we found\n> is sane, to prevent us getting confused by a random file whose\n> name happens to be HEAD.\n\nThat is what we did before. Simplest check, also to avoid now to \nclaim top directory as git repository, and to know when to cut-off \n(prune) finding.\n\nIt was intended I think to avoid adding '.' and '..' as git \nrepositories, not stray directories. Well, perhaps index file\nif it was used.\n\n> For example, it is a regular file whose contents is a single\n> line and begins with \"ref: refs/heads/\" (16 bytes) or it is a\n> symlink and readlink result begins with \"refs/heads/\" (11\n> bytes).\n\nWe can do that, but I think it is unnecessary. Let's assume that\n$projectroot contains _only_ git repositories, perhaps in subdirs \n(directory hierarchy), and perhaps some stray files like not used\nnow index file.\n\n> If you feel opening and reading the file is an added overhead,\n> checking for $project/$subdir/{objects,refs}/ directories might\n> be a good alternative.\n\nProbably overkill.\n\n> > +\t\tFile::Find::find({\n> > +\t\t\tno_chdir => 1, # do not change directory\n> > +\t\t\tfollow_fast => 1, # follow symbolic links\n> \n> What is the reason behind choosing follow_fast? By saying\n> follow_anything, you choose to care about cases where there are\n> symlinks under projectroot to point at various projects. If\n> that is the case, don't you want to make sure you include the\n> same project only once?\n\nFirst, it is faster. Second, for testing if it works I used copy\nof a one \"live\" git repository I have (git.git repository), by making\nsecond symlink to it.\n\n> > +\t\t\t#follow_skip => 2, # ignore duplicated files and directories\n> \n> Leftover comment?\n\nLeftover from benchmarking what set of options is faster.\n\nBy the way, if we choose to use 'follow' instead of 'follow_fast' we \nmight want to uncomment it, to not spew errors in the log.\n\n> About these two leftover comments, if you decided you did not\n> want them, please do not leave them behind.\n\nO.K.\n\n> If on the other hand you wanted to hint others that you are not\n> sure about your decision, it would probably be better to say\n> that honestly in the comment, perhaps mark the message as RFC,\n> and describe what the issues are, like so:\n> \n> \tsub wanted {\n> \t\t# We might want to also ignore dot files, by\n> # saying \"return if /^\\./;\" here, but there is\n> # no inherent reason for us to forbid a repository\n> # name from starting with a dot.\n\nTrue.\n\n> # We check only if a directory looks like a git\n> # repo and do not care about non directories.\n> # Note that this cannot be done with \"-d _\"\n> # because we are using follow_fast and the last\n> # stat was done with lstat(); we want to catch a\n> # symlink that points at a directory.\n> return unless -d $_;\n> ...\n\nNot true. Link to directory is both -d $_ and -l $_, so\n\n\treturn unless (-d $_ || (-l $_ && -d readlink($_)));\n\nis not needed.\n-- \nJakub Narebski\nPoland\n"},{"id":"26878","messageId":"7v1wqe6buv.fsf@assigned-by-dhcp.cox.net","threadId":"5558","inReplyTo":"200609140959.04061.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Use File::Find::find in git_get_projects_list","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-14T09:23:04Z","receivedAt":"2006-09-14T09:23:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Not true. Link to directory is both -d $_ and -l $_, so\n>\n> \treturn unless (-d $_ || (-l $_ && -d readlink($_)));\n>\n> is not needed.\n\nI think you mis-read what I said. I first wondered why you did\nnot say \"return unless -d _\" and wrote (seemingly more\ninefficient) \"return unless -d $_\". The comment is to clarify\nwhy '$' is needed.\n\nIn other words, after this setup:\n\n $ rm -fr d dl\n $ mkdir d\n $ ln -s d dl\n\nyou do not see an output from this:\n\n $ perl -e 'lstat \"dl\"; print \"is-dir\\n\" if -d _;'\n\nbut you do from this:\n\n $ perl -e 'lstat \"dl\"; print \"is-dir\\n\" if -d \"dl\";'\n"},{"id":"26879","messageId":"7vvenq4wgp.fsf@assigned-by-dhcp.cox.net","threadId":"5558","inReplyTo":"200609140959.04061.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Use File::Find::find in git_get_projects_list","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-14T09:40:54Z","receivedAt":"2006-09-14T09:40:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> ... But I have realized that for gitweb\n> for only one project one could have \".git\" as a project name,\n> e.g. by putting $projectroot to be live git repository (working\n> directory of git repository).\n\nI do not understand this comment. I have /git/{git,linux-2.6}\nand run test gitweb with projectroot set to /git to see it can\ndisplay them both. Both are with working tree, so there are\n/git/git/.git and /git/linux-2.6/.git directories.\n\nDo you mean to say \"project name\" is always taken from the last\ncomponent of the directory name and the above setup would result\nin ambiguity? If that is the case that sounds awful, but I\nhaven't noticed it.\n\n> It was intended I think to avoid adding '.' and '..' as git \n> repositories, not stray directories. Well, perhaps index file\n> if it was used.\n\nHaving and not having index are both valid, so there is no value\nin checking the index, even if we wanted to be more paranoid.\nExistence of HEAD, refs/heads, and/or objects/ would be a more\nmeaningful alternative, but again only if we wanted to be more\nparanoid.\n\n>> > +\t\tFile::Find::find({\n>> > +\t\t\tno_chdir => 1, # do not change directory\n>> > +\t\t\tfollow_fast => 1, # follow symbolic links\n>> \n>> What is the reason behind choosing follow_fast? By saying\n>> follow_anything, you choose to care about cases where there are\n>> symlinks under projectroot to point at various projects. If\n>> that is the case, don't you want to make sure you include the\n>> same project only once?\n>\n> First, it is faster. Second, for testing if it works I used copy\n> of a one \"live\" git repository I have (git.git repository), by making\n> second symlink to it.\n\nThat was not what I wanted to ask; slower and correct is always\npreferred over fast and incorrect. I did not see anything that\ncompensates the duplicates follow_fast might give you in the\ncode, so I wondered there were some other trick you used to avoid\nit.\n\nIn other words, \"because I have such and such check to avoid\nduplicates, so I can safely use 'follow_fast', without using\nslower 'follow'\" was the answer I was after.\n"},{"id":"26880","messageId":"200609141201.41711.jnareb@gmail.com","threadId":"5558","inReplyTo":"7vvenq4wgp.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] gitweb: Use File::Find::find in git_get_projects_list","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-09-14T10:01:41Z","receivedAt":"2006-09-14T10:01:41Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Dnia czwartek 14. września 2006 11:40, napisałeś:\n> Jakub Narebski writes:\n> \n> > ... But I have realized that for gitweb\n> > for only one project one could have \".git\" as a project name,\n> > e.g. by putting $projectroot to be live git repository (working\n> > directory of git repository).\n> \n> I do not understand this comment. I have /git/{git,linux-2.6}\n> and run test gitweb with projectroot set to /git to see it can\n> display them both. Both are with working tree, so there are\n> /git/git/.git and /git/linux-2.6/.git directories.\n> \n> Do you mean to say \"project name\" is always taken from the last\n> component of the directory name and the above setup would result\n> in ambiguity? If that is the case that sounds awful, but I\n> haven't noticed it.\n\nSorry, this comment was leftover from before, when no_chdir was not \nused. Then $_ was the last part of directory, and 'return if (/^\\./);'\nwould not take into consideration git/.git nor linux-2.6/.git as\nvalid project names ($projectroot / $projects_list is stripped from\nproject name).\n\nNow the test should read \"if (m!^.*/\\.!)\" if we want to skip\ndot files and dot directories.\n\n> > It was intended I think to avoid adding '.' and '..' as git \n> > repositories, not stray directories. Well, perhaps index file\n> > if it was used.\n> \n> Having and not having index are both valid, so there is no value\n> in checking the index, even if we wanted to be more paranoid.\n> Existence of HEAD, refs/heads, and/or objects/ would be a more\n> meaningful alternative, but again only if we wanted to be more\n> paranoid.\n\nSorry for the confusion. By index I meant alternate way of specyfying \nprojects, and up till now the only way to specify hierarchical \nrepository structure, namely $projects_list being file with encoded\ndirectories and encoded owners (it was named index.aux or \nindex/index.aux by default).\n\n> >> > +\t\tFile::Find::find({\n> >> > +\t\t\tno_chdir => 1, # do not change directory\n> >> > +\t\t\tfollow_fast => 1, # follow symbolic links\n> >> \n> >> What is the reason behind choosing follow_fast? By saying\n> >> follow_anything, you choose to care about cases where there are\n> >> symlinks under projectroot to point at various projects. If\n> >> that is the case, don't you want to make sure you include the\n> >> same project only once?\n> >\n> > First, it is faster. Second, for testing if it works I used copy\n> > of a one \"live\" git repository I have (git.git repository), by\n> > making second symlink to it.\n> \n> That was not what I wanted to ask; slower and correct is always\n> preferred over fast and incorrect. I did not see anything that\n> compensates the duplicates follow_fast might give you in the\n> code, so I wondered there were some other trick you used to avoid\n> it.\n> \n> In other words, \"because I have such and such check to avoid\n> duplicates, so I can safely use 'follow_fast', without using\n> slower 'follow'\" was the answer I was after.\n\nFirst of all, why duplicated repositories are considered error?\nIt is perhaps error/mistake (besides testing of course) in doing the \nlayout of repositories (projectroot), but it is not that something bad \nwould happen if there are duplicated repositories.\n\nSecond, follow_fast is faster, especially that in gitweb we would \nhave to set \"follow_skip => 2\" to not spew errors to web server log when \nthere are duplicated repositories. \n\n-- \nJakub Narebski\nPoland\n"},{"id":"26895","messageId":"7vmz924cxy.fsf@assigned-by-dhcp.cox.net","threadId":"5558","inReplyTo":"200609141201.41711.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Use File::Find::find in git_get_projects_list","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-14T16:42:33Z","receivedAt":"2006-09-14T16:42:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Sorry, this comment was leftover from before, when no_chdir was not \n> used. Then $_ was the last part of directory,...\n\nAh, thanks. I missed that difference. Did you choose to use\nno_chdir for performance reasons or coding convenience (somehow\nI had an impression that no_chdir would be slower)?\n\n> Sorry for the confusion. By index I meant alternate way of specyfying \n> projects,...\n\nAgain, thanks for clarification. I forgot about the handcrafted\nproject index file gitweb uses.\n\n> First of all, why duplicated repositories are considered\n> error? ... it is not that something bad would happen if there\n> are duplicated repositories.\n\nFair enough, and I agree that that is a sane reasoning. Thanks.\n"},{"id":"26898","messageId":"eec1tp$9c9$1@sea.gmane.org","threadId":"5558","inReplyTo":"7vmz924cxy.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] gitweb: Use File::Find::find in git_get_projects_list","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-09-14T17:02:09Z","receivedAt":"2006-09-14T17:02:09Z","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 writes:\n> \n>> Sorry, this comment was leftover from before, when no_chdir was not \n>> used. Then $_ was the last part of directory,...\n> \n> Ah, thanks. I missed that difference. Did you choose to use\n> no_chdir for performance reasons or coding convenience (somehow\n> I had an impression that no_chdir would be slower)?\n\nFirst benchmarks showed that no_chdir was some faster. I have rechecked, \nand they are the same within the margin of error, perhaps without\nno_chdir is slightly faster. 447 +/- 11 ms vs. 450 +/- 10 ms according\nto ApacheBench (ab -n 10).\n\nSo it is really the matter of convenience. The default (without no_chdir) is\nI think more convenient.\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"26900","messageId":"86mz921if6.fsf@blue.stonehenge.com","threadId":"5558","inReplyTo":"eec1tp$9c9$1@sea.gmane.org","subject":"Re: [PATCH] gitweb: Use File::Find::find in git_get_projects_list","fromName":"Randal L. Schwartz","fromEmail":"merlyn@stonehenge.com","sentAt":"2006-09-14T17:12:29Z","receivedAt":"2006-09-14T17:12:29Z","isPatch":true,"sender":{"key":"merlyn@stonehenge.com","avatar":"https://gravatar.com/avatar/dc528d210743ff0333e6213f9ee7b33b23f1b7bc1f3c5a8c2d819074ecd7ab19?d=mp&s=160"},"body":">>>>> \"Jakub\" == Jakub Narebski writes:\n\nJakub> First benchmarks showed that no_chdir was some faster. I have rechecked, \nJakub> and they are the same within the margin of error, perhaps without\nJakub> no_chdir is slightly faster. 447 +/- 11 ms vs. 450 +/- 10 ms according\nJakub> to ApacheBench (ab -n 10).\n\nAny benchmarks will certainly depend on the O/S as well. The advantage to\nchdir is that the name-to-inode lookups don't need to keep retraversing the\nupper directories. And keep in mind that caching will affect the benchmarks\non that.\n\nAs for that whole -d thing...\n\nIf you use _, you have to keep in mind whether the previous call was a stat()\nor an lstat(). Both lstat() (explictly) and -l use lstat(). Everything else\nis a stat(). So, \"-l and not -d\" uses an lstat to determine that we're\nlooking at a symlink, but a stat to determine that the item pointed at is not\na directory. NEVER use readlink(), as it very likely will require a lot of\nflattening for you to simulate what the OS does. (See my article on that at\n.)\n\n-- \nRandal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095\n \nPerl/Unix/security consulting, Technical writing, Comedy, etc. etc.\nSee PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!\n"},{"id":"26904","messageId":"200609141939.39406.jnareb@gmail.com","threadId":"5558","inReplyTo":"200609140839.56181.jnareb@gmail.com","subject":"[PATCH (amend)] gitweb: Use File::Find::find in git_get_projects_list","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-09-14T17:39:39Z","receivedAt":"2006-09-14T17:39:39Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Earlier code to get list of projects when $projects_list is a\ndirectory (e.g. when it is equal to $projectroot) had a hardcoded flat\n(one level) list of directories. Allow for projects to be in\nsubdirectories also for $projects_list being a directory by using\nFile::Find.\n\nSigned-off-by: Jakub Narebski \n---\n gitweb/gitweb.perl | 26 ++++++++++++++++++--------\n 1 files changed, 18 insertions(+), 8 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex c3544dd..27641a6 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -715,16 +715,26 @@ sub git_get_projects_list {\n \tif (-d $projects_list) {\n \t\t# search in directory\n \t\tmy $dir = $projects_list;\n-\t\topendir my ($dh), $dir or return undef;\n-\t\twhile (my $dir = readdir($dh)) {\n-\t\t\tif (-e \"$projectroot/$dir/HEAD\") {\n-\t\t\t\tmy $pr = {\n-\t\t\t\t\tpath => $dir,\n-\t\t\t\t};\n-\t\t\t\tpush @list, $pr\n+\t\tmy $pfxlen = length(\"$dir\");\n+\n+\t\tsub wanted {\n+\t\t\t# only directories can be git repositories\n+\t\t\treturn unless (-d $_);\n+\n+\t\t\tmy $subdir = substr($File::Find::name, $pfxlen + 1);\n+\t\t\t# we check related file in $projectroot\n+\t\t\tif (-e \"$projectroot/$subdir/HEAD\") {\n+\t\t\t\tpush @list, { path => $subdir };\n+\t\t\t\t$File::Find::prune = 1;\n \t\t\t}\n \t\t}\n-\t\tclosedir($dh);\n+\n+\t\tFile::Find::find({\n+\t\t\tfollow_fast => 1, # follow symbolic links\n+\t\t\tdangling_symlinks => 0, # ignore dangling symlinks, silently\n+\t\t\twanted => \\&wanted,\n+\t\t}, \"$dir\");\n+\n \t} elsif (-f $projects_list) {\n \t\t# read from file(url-encoded):\n \t\t# 'git%2Fgit.git Linus+Torvalds'\n-- \n1.4.2\n"},{"id":"26908","messageId":"7vbqpi47vi.fsf@assigned-by-dhcp.cox.net","threadId":"5558","inReplyTo":"7v1wqe6buv.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] gitweb: Use File::Find::find in git_get_projects_list","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-14T18:32:01Z","receivedAt":"2006-09-14T18:32:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jakub Narebski writes:\n>\n>> Not true. Link to directory is both -d $_ and -l $_, so\n>>\n>> \treturn unless (-d $_ || (-l $_ && -d readlink($_)));\n>>\n>> is not needed.\n>\n> I think you mis-read what I said. I first wondered why you did\n> not say \"return unless -d _\" and wrote (seemingly more\n> inefficient) \"return unless -d $_\". The comment is to clarify\n> why '$' is needed.\n>\n> In other words, after this setup:\n>\n> $ rm -fr d dl\n> $ mkdir d\n> $ ln -s d dl\n>\n> you do not see an output from this:\n>\n> $ perl -e 'lstat \"dl\"; print \"is-dir\\n\" if -d _;'\n>\n> but you do from this:\n>\n> $ perl -e 'lstat \"dl\"; print \"is-dir\\n\" if -d \"dl\";'\n\nSide note:\n\nWhile\n\n\treturn unless -d $_\n\nthere is definitely more correct than \"return unless -d _\" which\nis not, it is not the most efficient. Because you use fast_xxx,\nyou know the last stat was lstat so \"-d _\" would be true if the\nthing you are looking at is a real directory and will be a\nzero-cost operation. The only case you want to be careful is a\nsymlink pointing at a directory, so\n\n\treturn unless ((-d _) || (-l _ && -d $_))\n\nwould be more efficient.\n\nI have a strange suspicion that Merlyn will soon join us with\nmore expertise if we keep talking about Perl ;-)\n"},{"id":"26909","messageId":"eec7rp$vjd$1@sea.gmane.org","threadId":"5558","inReplyTo":"7vbqpi47vi.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] gitweb: Use File::Find::find in git_get_projects_list","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-09-14T18:43:29Z","receivedAt":"2006-09-14T18:43:29Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n\n> Side note:\n> \n> While\n> \n> return unless -d $_\n> \n> there is definitely more correct than \"return unless -d _\" which\n> is not, it is not the most efficient. Because you use fast_xxx,\n> you know the last stat was lstat so \"-d _\" would be true if the\n> thing you are looking at is a real directory and will be a\n> zero-cost operation. The only case you want to be careful is a\n> symlink pointing at a directory, so\n> \n> return unless ((-d _) || (-l _ && -d $_))\n> \n> would be more efficient.\n> \n> I have a strange suspicion that Merlyn will soon join us with\n> more expertise if we keep talking about Perl ;-)\n\nTruth to be told, I didn't know about '-d _', and File::Find(3pm) \ndoes talk only about $_.\n\nBesides, I guess that the possible speedup is negligible, and of \ncourse depend if for example the whole $projectroot aka. $projects_list\nis for example populated by symlinks. Then it would be slower.\n\nThis is the place where more readable should win.\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"26911","messageId":"eec9lt$7ll$1@sea.gmane.org","threadId":"5558","inReplyTo":"200609141939.39406.jnareb@gmail.com","subject":"Re: [PATCH (amend)] gitweb: Use File::Find::find in git_get_projects_list","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-09-14T19:14:29Z","receivedAt":"2006-09-14T19:14:29Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Gaah, the code spews quite a lot of warnings as of now.\n\nVariable \"$pfxlen\" will not stay shared at gitweb.perl line 724.\nVariable \"@list\" will not stay shared at gitweb.perl line 727.\n...\n-- \nJakub Narebski\n"},{"id":"26914","messageId":"200609142134.33725.jnareb@gmail.com","threadId":"5558","inReplyTo":"200609140839.56181.jnareb@gmail.com","subject":"[PATCH (take 3)] gitweb: Use File::Find::find in git_get_projects_list","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-09-14T19:34:33Z","receivedAt":"2006-09-14T19:34:33Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Earlier code to get list of projects when $projects_list is a\ndirectory (e.g. when it is equal to $projectroot) had a hardcoded flat\n(one level) list of directories. Allow for projects to be in\nsubdirectories also for $projects_list being a directory by using\nFile::Find.\n\nSigned-off-by: Jakub Narebski \n---\nUse anonymous subroutine to avoid \n Variable \"@list\" will not stay shared at gitweb.perl line 727.\nwarning. Check for the current directory to avoid substr outside \nstring warning.\n\n gitweb/gitweb.perl | 30 ++++++++++++++++++++----------\n 1 files changed, 20 insertions(+), 10 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex c3544dd..bea75d3 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -715,16 +715,26 @@ sub git_get_projects_list {\n \tif (-d $projects_list) {\n \t\t# search in directory\n \t\tmy $dir = $projects_list;\n-\t\topendir my ($dh), $dir or return undef;\n-\t\twhile (my $dir = readdir($dh)) {\n-\t\t\tif (-e \"$projectroot/$dir/HEAD\") {\n-\t\t\t\tmy $pr = {\n-\t\t\t\t\tpath => $dir,\n-\t\t\t\t};\n-\t\t\t\tpush @list, $pr\n-\t\t\t}\n-\t\t}\n-\t\tclosedir($dh);\n+\t\tmy $pfxlen = length(\"$dir\");\n+\n+\t\tFile::Find::find({\n+\t\t\tfollow_fast => 1, # follow symbolic links\n+\t\t\tdangling_symlinks => 0, # ignore dangling symlinks, silently\n+\t\t\twanted => sub {\n+\t\t\t\t# skip current directory\n+\t\t\t\treturn if (m!^/|.|..$!);\n+\t\t\t\t# only directories can be git repositories\n+\t\t\t\treturn unless (-d $_);\n+\n+\t\t\t\tmy $subdir = substr($File::Find::name, $pfxlen + 1);\n+\t\t\t\t# we check related file in $projectroot\n+\t\t\t\tif (-e \"$projectroot/$subdir/HEAD\") {\n+\t\t\t\t\tpush @list, { path => $subdir };\n+\t\t\t\t\t$File::Find::prune = 1;\n+\t\t\t\t}\n+\t\t\t},\n+\t\t}, \"$dir\");\n+\n \t} elsif (-f $projects_list) {\n \t\t# read from file(url-encoded):\n \t\t# 'git%2Fgit.git Linus+Torvalds'\n-- \n1.4.2\n"},{"id":"26918","messageId":"7vodti2pmj.fsf@assigned-by-dhcp.cox.net","threadId":"5558","inReplyTo":"eec9lt$7ll$1@sea.gmane.org","subject":"Re: [PATCH (amend)] gitweb: Use File::Find::find in git_get_projects_list","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-14T19:51:32Z","receivedAt":"2006-09-14T19:51:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Gaah, the code spews quite a lot of warnings as of now.\n>\n> Variable \"$pfxlen\" will not stay shared at gitweb.perl line 724.\n> Variable \"@list\" will not stay shared at gitweb.perl line 727.\n> ...\n\nThanks for an early warning. Will not apply that one and\ninstead will wait for updates.\n"},{"id":"26921","messageId":"7vejue2omq.fsf@assigned-by-dhcp.cox.net","threadId":"5558","inReplyTo":"200609142134.33725.jnareb@gmail.com","subject":"Re: [PATCH (take 3)] gitweb: Use File::Find::find in git_get_projects_list","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-14T20:13:01Z","receivedAt":"2006-09-14T20:13:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> +\t\t\twanted => sub {\n> +\t\t\t\t# skip current directory\n> +\t\t\t\treturn if (m!^/|.|..$!);\n\nHuh?\n\n (1) Did you mean to say \"\\.\" (not any single character but\n literally dot)?\n\n (2) how does the alternatives within m{} construct bind (iow,\n please be gentle to the readers)? Do you mean\n\n\treturn if (/^\\/(?:\\.|\\.\\.)$/)\n\n in other words,\n\n \treturn if (/^\\/\\.$/ || /^\\/\\.\\.$/)\n\n in other words,\n\n return if (/^\\/\\.{1,2}/)\n\n ???\n"},{"id":"26922","messageId":"200609142218.59428.jnareb@gmail.com","threadId":"5558","inReplyTo":"7vejue2omq.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH (take 4)] gitweb: Use File::Find::find in git_get_projects_list","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-09-14T20:18:59Z","receivedAt":"2006-09-14T20:18:59Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Earlier code to get list of projects when $projects_list is a\ndirectory (e.g. when it is equal to $projectroot) had a hardcoded flat\n(one level) list of directories. Allow for projects to be in\nsubdirectories also for $projects_list being a directory by using\nFile::Find.\n\nSigned-off-by: Jakub Narebski \n---\n gitweb/gitweb.perl | 30 ++++++++++++++++++++----------\n 1 files changed, 20 insertions(+), 10 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex c3544dd..25383bc 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -715,16 +715,26 @@ sub git_get_projects_list {\n \tif (-d $projects_list) {\n \t\t# search in directory\n \t\tmy $dir = $projects_list;\n-\t\topendir my ($dh), $dir or return undef;\n-\t\twhile (my $dir = readdir($dh)) {\n-\t\t\tif (-e \"$projectroot/$dir/HEAD\") {\n-\t\t\t\tmy $pr = {\n-\t\t\t\t\tpath => $dir,\n-\t\t\t\t};\n-\t\t\t\tpush @list, $pr\n-\t\t\t}\n-\t\t}\n-\t\tclosedir($dh);\n+\t\tmy $pfxlen = length(\"$dir\");\n+\n+\t\tFile::Find::find({\n+\t\t\tfollow_fast => 1, # follow symbolic links\n+\t\t\tdangling_symlinks => 0, # ignore dangling symlinks, silently\n+\t\t\twanted => sub {\n+\t\t\t\t# skip current directory\n+\t\t\t\treturn if (m!^[/.]$!);\n+\t\t\t\t# only directories can be git repositories\n+\t\t\t\treturn unless (-d $_);\n+\n+\t\t\t\tmy $subdir = substr($File::Find::name, $pfxlen + 1);\n+\t\t\t\t# we check related file in $projectroot\n+\t\t\t\tif (-e \"$projectroot/$subdir/HEAD\") {\n+\t\t\t\t\tpush @list, { path => $subdir };\n+\t\t\t\t\t$File::Find::prune = 1;\n+\t\t\t\t}\n+\t\t\t},\n+\t\t}, \"$dir\");\n+\n \t} elsif (-f $projects_list) {\n \t\t# read from file(url-encoded):\n \t\t# 'git%2Fgit.git Linus+Torvalds'\n-- \n1.4.2\n"},{"id":"26924","messageId":"eecdoe$n4k$1@sea.gmane.org","threadId":"5558","inReplyTo":"200609142218.59428.jnareb@gmail.com","subject":"Re: [PATCH (take 4)] gitweb: Use File::Find::find in git_get_projects_list","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-09-14T20:24:07Z","receivedAt":"2006-09-14T20:24:07Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jakub Narebski wrote:\n\n> +                               # skip current directory\n\nPerhaps it would be better to say \"skip $projects_list (top) directory\".\nI don't think it is important enough to resend a patch.\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"26930","messageId":"86bqpi15jz.fsf@blue.stonehenge.com","threadId":"5558","inReplyTo":"200609141939.39406.jnareb@gmail.com","subject":"Re: [PATCH (amend)] gitweb: Use File::Find::find in git_get_projects_list","fromName":"Randal L. Schwartz","fromEmail":"merlyn@stonehenge.com","sentAt":"2006-09-14T21:50:24Z","receivedAt":"2006-09-14T21:50:24Z","isPatch":true,"sender":{"key":"merlyn@stonehenge.com","avatar":"https://gravatar.com/avatar/dc528d210743ff0333e6213f9ee7b33b23f1b7bc1f3c5a8c2d819074ecd7ab19?d=mp&s=160"},"body":">>>>> \"Jakub\" == Jakub Narebski writes:\n\nJakub> +\t\tsub wanted {\nJakub> +\t\t\t# only directories can be git repositories\nJakub> +\t\t\treturn unless (-d $_);\nJakub> +\nJakub> +\t\t\tmy $subdir = substr($File::Find::name, $pfxlen + 1);\nJakub> +\t\t\t# we check related file in $projectroot\nJakub> +\t\t\tif (-e \"$projectroot/$subdir/HEAD\") {\nJakub> +\t\t\t\tpush @list, { path => $subdir };\nJakub> +\t\t\t\t$File::Find::prune = 1;\nJakub> \t\t\t}\nJakub> \t\t}\nJakub> -\t\tclosedir($dh);\nJakub> +\nJakub> +\t\tFile::Find::find({\nJakub> +\t\t\tfollow_fast => 1, # follow symbolic links\nJakub> +\t\t\tdangling_symlinks => 0, # ignore dangling symlinks, silently\nJakub> +\t\t\twanted => \\&wanted,\nJakub> +\t\t}, \"$dir\");\n\nDon't define a sub inside a sub. That's the \"won't stay shared\"\nproblem.\n\nMove that like this:\n\nfind( {\n follow_fast => 1,\n dangling_symlinks => 0,\n wanted => sub {\n\n ... everything in &wanted above ...\n\n }, $dir);\n\n-- \nRandal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095\n \nPerl/Unix/security consulting, Technical writing, Comedy, etc. etc.\nSee PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!\n"}]}