Re: [PATCH] gitweb: Use File::Find::find in git_get_projects_list

From: Junio C Hamano <junkio@cox.net>
Date: Sep 14, 2006, 07:37 UTC
Message-ID: <7v8xkm6gr6.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <200609140839.56181.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

> +		sub wanted {
> +			# skip dot files (hidden files), check only directories
> +			#return if (/^\./);

Leftover comment?

Show 5 quoted lines

> +			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;

We might want to do an extra cheap check to make what we found is sane, to prevent us getting confused by a random file whose name happens to be HEAD.

For example, it is a regular file whose contents is a single line and begins with "ref: refs/heads/" (16 bytes) or it is a symlink and readlink result begins with "refs/heads/" (11 bytes).

If you feel opening and reading the file is an added overhead, checking for $project/$subdir/{objects,refs}/ directories might be a good alternative.

> +		File::Find::find({
> +			no_chdir => 1, # do not change directory
> +			follow_fast => 1, # follow symbolic links

What is the reason behind choosing follow_fast? By saying follow_anything, you choose to care about cases where there are symlinks under projectroot to point at various projects. If that is the case, don't you want to make sure you include the same project only once?

> +			#follow_skip => 2, # ignore duplicated files and directories

Leftover comment?

About these two leftover comments, if you decided you did not want them, please do not leave them behind.

If on the other hand you wanted to hint others that you are not sure about your decision, it would probably be better to say that honestly in the comment, perhaps mark the message as RFC, and describe what the issues are, like so:

	sub wanted {
		# We might want to also ignore dot files, by
                # saying "return if /^\./;" here, but there is
                # no inherent reason for us to forbid a repository
                # name from starting with a dot.

                # We check only if a directory looks like a git
                # repo and do not care about non directories.
                # Note that this cannot be done with "-d _"
                # because we are using follow_fast and the last
                # stat was done with lstat(); we want to catch a
                # symlink that points at a directory.
                return unless -d $_;
                ...