gitweb: Use File::Find::find in git_get_projects_list

Earlier code to get list of projects when $projects_list is a directory (e.g. when it is equal to $projectroot) had a hardcoded flat (one level) list of directories. Allow for projects to be in subdirectories also for $projects_list being a directory by using File::Find.

---
This doesn't add much overhead to "project_list" view, compared to previous version; times are the same within margin of error.

 gitweb/gitweb.perl |   29 +++++++++++++++++++++--------
 1 files changed, 21 insertions(+), 8 deletions(-)

index c3544dd..470bff2 100755
@@ -715,16 +715,29 @@ sub git_get_projects_list {
 	if (-d $projects_list) {
 		# search in directory
 		my $dir = $projects_list;
-		opendir my ($dh), $dir or return undef;
-		while (my $dir = readdir($dh)) {
-			if (-e "$projectroot/$dir/HEAD") {
-				my $pr = {
-					path => $dir,
-				};
-				push @list, $pr
+		my $pfxlen = length("$dir");
+
+		sub wanted {
+			# skip dot files (hidden files), check only directories
+			#return if (/^\./);
+			return unless (-d $_);
+
+			my $subdir = substr($File::Find::name, $pfxlen + 1);
+			# we check related file in $projectroot
+			if (-e "$projectroot/$subdir/HEAD") {
+				push @list, { path => $subdir };
+				$File::Find::prune = 1;
 			}
 		}
-		closedir($dh);
+
+		File::Find::find({
+			no_chdir => 1, # do not change directory
+			follow_fast => 1, # follow symbolic links
+			#follow_skip => 2, # ignore duplicated files and directories
+			dangling_symlinks => 0, # ignore dangling symlinks, silently
+			wanted => \&wanted,
+		}, "$dir");
+
 	} elsif (-f $projects_list) {
 		# read from file(url-encoded):
 		# 'git%2Fgit.git Linus+Torvalds'

1.4.2

Earlier code to get list of projects when $projects_list is a directory (e.g. when it is equal to $projectroot) had a hardcoded flat (one level) list of directories. Allow for projects to be in subdirectories also for $projects_list being a directory by using File::Find.

---
 gitweb/gitweb.perl |   26 ++++++++++++++++++--------
 1 files changed, 18 insertions(+), 8 deletions(-)

index c3544dd..27641a6 100755
@@ -715,16 +715,26 @@ sub git_get_projects_list {
 	if (-d $projects_list) {
 		# search in directory
 		my $dir = $projects_list;
-		opendir my ($dh), $dir or return undef;
-		while (my $dir = readdir($dh)) {
-			if (-e "$projectroot/$dir/HEAD") {
-				my $pr = {
-					path => $dir,
-				};
-				push @list, $pr
+		my $pfxlen = length("$dir");
+
+		sub wanted {
+			# only directories can be git repositories
+			return unless (-d $_);
+
+			my $subdir = substr($File::Find::name, $pfxlen + 1);
+			# we check related file in $projectroot
+			if (-e "$projectroot/$subdir/HEAD") {
+				push @list, { path => $subdir };
+				$File::Find::prune = 1;
 			}
 		}
-		closedir($dh);
+
+		File::Find::find({
+			follow_fast => 1, # follow symbolic links
+			dangling_symlinks => 0, # ignore dangling symlinks, silently
+			wanted => \&wanted,
+		}, "$dir");
+
 	} elsif (-f $projects_list) {
 		# read from file(url-encoded):
 		# 'git%2Fgit.git Linus+Torvalds'

1.4.2

Jakub> + sub wanted {
Jakub> + # only directories can be git repositories
Jakub> + return unless (-d $_);
Jakub> +
Jakub> + my $subdir = substr($File::Find::name, $pfxlen + 1);
Jakub> + # we check related file in $projectroot
Jakub> + if (-e "$projectroot/$subdir/HEAD") {
Jakub> + push @list, { path => $subdir };
Jakub> + $File::Find::prune = 1;
Jakub> }
Jakub> }
Jakub> - closedir($dh);
Jakub> +
Jakub> + File::Find::find({
Jakub> + follow_fast => 1, # follow symbolic links
Jakub> + dangling_symlinks => 0, # ignore dangling symlinks, silently
Jakub> + wanted => \&wanted,
Jakub> + }, "$dir");

Don't define a sub inside a sub. That's the "won't stay shared" problem.

Move that like this:

find( {
        follow_fast => 1,
        dangling_symlinks => 0,
        wanted => sub {

                ... everything in &wanted above ...

        }, $dir);

Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<[email protected]> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

Gaah, the code spews quite a lot of warnings as of now.

Variable "$pfxlen" will not stay shared at gitweb.perl line 724. Variable "@list" will not stay shared at gitweb.perl line 727.
...

Jakub Narebski

Thanks for an early warning. Will not apply that one and instead will wait for updates.

Earlier code to get list of projects when $projects_list is a directory (e.g. when it is equal to $projectroot) had a hardcoded flat (one level) list of directories. Allow for projects to be in subdirectories also for $projects_list being a directory by using File::Find.

---
Use anonymous subroutine to avoid
Variable "@list" will not stay shared at gitweb.perl line 727.
warning. Check for the current directory to avoid substr outside string warning.

 gitweb/gitweb.perl |   30 ++++++++++++++++++++----------
 1 files changed, 20 insertions(+), 10 deletions(-)

index c3544dd..bea75d3 100755
@@ -715,16 +715,26 @@ sub git_get_projects_list {
 	if (-d $projects_list) {
 		# search in directory
 		my $dir = $projects_list;
-		opendir my ($dh), $dir or return undef;
-		while (my $dir = readdir($dh)) {
-			if (-e "$projectroot/$dir/HEAD") {
-				my $pr = {
-					path => $dir,
-				};
-				push @list, $pr
-			}
-		}
-		closedir($dh);
+		my $pfxlen = length("$dir");
+
+		File::Find::find({
+			follow_fast => 1, # follow symbolic links
+			dangling_symlinks => 0, # ignore dangling symlinks, silently
+			wanted => sub {
+				# skip current directory
+				return if (m!^/|.|..$!);
+				# only directories can be git repositories
+				return unless (-d $_);
+
+				my $subdir = substr($File::Find::name, $pfxlen + 1);
+				# we check related file in $projectroot
+				if (-e "$projectroot/$subdir/HEAD") {
+					push @list, { path => $subdir };
+					$File::Find::prune = 1;
+				}
+			},
+		}, "$dir");
+
 	} elsif (-f $projects_list) {
 		# read from file(url-encoded):
 		# 'git%2Fgit.git Linus+Torvalds'

1.4.2

Earlier code to get list of projects when $projects_list is a directory (e.g. when it is equal to $projectroot) had a hardcoded flat (one level) list of directories. Allow for projects to be in subdirectories also for $projects_list being a directory by using File::Find.

---
 gitweb/gitweb.perl |   30 ++++++++++++++++++++----------
 1 files changed, 20 insertions(+), 10 deletions(-)

index c3544dd..25383bc 100755
@@ -715,16 +715,26 @@ sub git_get_projects_list {
 	if (-d $projects_list) {
 		# search in directory
 		my $dir = $projects_list;
-		opendir my ($dh), $dir or return undef;
-		while (my $dir = readdir($dh)) {
-			if (-e "$projectroot/$dir/HEAD") {
-				my $pr = {
-					path => $dir,
-				};
-				push @list, $pr
-			}
-		}
-		closedir($dh);
+		my $pfxlen = length("$dir");
+
+		File::Find::find({
+			follow_fast => 1, # follow symbolic links
+			dangling_symlinks => 0, # ignore dangling symlinks, silently
+			wanted => sub {
+				# skip current directory
+				return if (m!^[/.]$!);
+				# only directories can be git repositories
+				return unless (-d $_);
+
+				my $subdir = substr($File::Find::name, $pfxlen + 1);
+				# we check related file in $projectroot
+				if (-e "$projectroot/$subdir/HEAD") {
+					push @list, { path => $subdir };
+					$File::Find::prune = 1;
+				}
+			},
+		}, "$dir");
+
 	} elsif (-f $projects_list) {
 		# read from file(url-encoded):
 		# 'git%2Fgit.git Linus+Torvalds'

1.4.2

Perhaps it would be better to say "skip $projects_list (top) directory". I don't think it is important enough to resend a patch.

Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Dnia czwartek 14. września 2006 09:37, napisałeś:

Leftover comment, from copying anonymous 'wanted' subroutine from git_get_refs_list. But I have realized that for gitweb for only one project one could have ".git" as a project name, e.g. by putting $projectroot to be live git repository (working directory of git repository).

That is what we did before. Simplest check, also to avoid now to claim top directory as git repository, and to know when to cut-off (prune) finding.

It was intended I think to avoid adding '.' and '..' as git repositories, not stray directories. Well, perhaps index file if it was used.

We can do that, but I think it is unnecessary. Let's assume that $projectroot contains _only_ git repositories, perhaps in subdirs (directory hierarchy), and perhaps some stray files like not used now index file.

Probably overkill.

First, it is faster. Second, for testing if it works I used copy of a one "live" git repository I have (git.git repository), by making second symlink to it.

Leftover from benchmarking what set of options is faster.

By the way, if we choose to use 'follow' instead of 'follow_fast' we might want to uncomment it, to not spew errors in the log.

O.K.

True.

Not true. Link to directory is both -d $_ and -l $_, so

	return unless (-d $_ || (-l $_ && -d readlink($_)));

is not needed.

Jakub Narebski
Poland

I think you mis-read what I said. I first wondered why you did not say "return unless -d _" and wrote (seemingly more inefficient) "return unless -d $_". The comment is to clarify why '$' is needed.

In other words, after this setup:

        $ rm -fr d dl
        $ mkdir d
        $ ln -s d dl

you do not see an output from this:

        $ perl -e 'lstat "dl"; print "is-dir\n" if -d _;'

but you do from this:

        $ perl -e 'lstat "dl"; print "is-dir\n" if -d "dl";'

Side note:

While

return unless -d $_

there is definitely more correct than "return unless -d _" which is not, it is not the most efficient. Because you use fast_xxx, you know the last stat was lstat so "-d _" would be true if the thing you are looking at is a real directory and will be a zero-cost operation. The only case you want to be careful is a symlink pointing at a directory, so

	return unless ((-d _) || (-l _ && -d $_))

would be more efficient.

I have a strange suspicion that Merlyn will soon join us with more expertise if we keep talking about Perl ;-)

Truth to be told, I didn't know about '-d _', and File::Find(3pm) does talk only about $_.

Besides, I guess that the possible speedup is negligible, and of course depend if for example the whole $projectroot aka. $projects_list is for example populated by symlinks. Then it would be slower.

This is the place where more readable should win.

Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

I do not understand this comment. I have /git/{git,linux-2.6} and run test gitweb with projectroot set to /git to see it can display them both. Both are with working tree, so there are /git/git/.git and /git/linux-2.6/.git directories.

Do you mean to say "project name" is always taken from the last component of the directory name and the above setup would result in ambiguity? If that is the case that sounds awful, but I haven't noticed it.

Having and not having index are both valid, so there is no value in checking the index, even if we wanted to be more paranoid. Existence of HEAD, refs/heads, and/or objects/ would be a more meaningful alternative, but again only if we wanted to be more paranoid.

That was not what I wanted to ask; slower and correct is always preferred over fast and incorrect. I did not see anything that compensates the duplicates follow_fast might give you in the code, so I wondered there were some other trick you used to avoid it.

In other words, "because I have such and such check to avoid duplicates, so I can safely use 'follow_fast', without using slower 'follow'" was the answer I was after.

Dnia czwartek 14. września 2006 11:40, napisałeś:

Sorry, this comment was leftover from before, when no_chdir was not used. Then $_ was the last part of directory, and 'return if (/^\./);' would not take into consideration git/.git nor linux-2.6/.git as valid project names ($projectroot / $projects_list is stripped from project name).

Now the test should read "if (m!^.*/\.!)" if we want to skip dot files and dot directories.

Sorry for the confusion. By index I meant alternate way of specyfying projects, and up till now the only way to specify hierarchical repository structure, namely $projects_list being file with encoded directories and encoded owners (it was named index.aux or index/index.aux by default).

First of all, why duplicated repositories are considered error? It is perhaps error/mistake (besides testing of course) in doing the layout of repositories (projectroot), but it is not that something bad would happen if there are duplicated repositories.

Second, follow_fast is faster, especially that in gitweb we would have to set "follow_skip => 2" to not spew errors to web server log when there are duplicated repositories.

Jakub Narebski
Poland

Ah, thanks. I missed that difference. Did you choose to use no_chdir for performance reasons or coding convenience (somehow I had an impression that no_chdir would be slower)?

Again, thanks for clarification. I forgot about the handcrafted project index file gitweb uses.

Fair enough, and I agree that that is a sane reasoning. Thanks.

First benchmarks showed that no_chdir was some faster. I have rechecked, and they are the same within the margin of error, perhaps without no_chdir is slightly faster. 447 +/- 11 ms vs. 450 +/- 10 ms according to ApacheBench (ab -n 10).

So it is really the matter of convenience. The default (without no_chdir) is I think more convenient.

Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Jakub> First benchmarks showed that no_chdir was some faster. I have rechecked, Jakub> and they are the same within the margin of error, perhaps without Jakub> no_chdir is slightly faster. 447 +/- 11 ms vs. 450 +/- 10 ms according Jakub> to ApacheBench (ab -n 10).

Any benchmarks will certainly depend on the O/S as well. The advantage to chdir is that the name-to-inode lookups don't need to keep retraversing the upper directories. And keep in mind that caching will affect the benchmarks on that.

As for that whole -d thing...

If you use _, you have to keep in mind whether the previous call was a stat() or an lstat(). Both lstat() (explictly) and -l use lstat(). Everything else is a stat(). So, "-l and not -d" uses an lstat to determine that we're looking at a symlink, but a stat to determine that the item pointed at is not a directory. NEVER use readlink(), as it very likely will require a lot of flattening for you to simulate what the OS does. (See my article on that at
<http://www.stonehenge.com/merlyn/UnixReview/col27.html>.)

Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<[email protected]> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!