Re: [PATCH/RFC] gitweb: Restructure projects list generation

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Feb 27, 2011, 10:08 UTC
Message-ID: <20110227100838.GA24919@elie>
In-Reply-To: <20110226215230.5333.92839.stgit@localhost.localdomain>

Hi,

Jakub Narebski wrote:

> Extract filtering out forks (which is done if 'forks' feature is
> enabled) into filter_forks_from_projects_list subroutine

Context (to remind myself): the git_get_projects_list function decides which repositories to show on the homepage. git_project_list_body takes care of actually showing them in a table; it displays a "+" sign in front of projects with forks and generally the project list it is passed omits the forks themselves.

git_get_projects_list needs to be reasonably fast, since it is used to validate the "project" parameter in every request.

[...]

> Both are now run _before_ displaying projects, and not while printing;
> this allow to know upfront if there were any found projects.  Gitweb
> now can and do print 'No such projects found' if user searches for
> phrase which does not correspond to any project (any repository).

Aren't forks filtered out by git-get-projects-list already?

[...]

> Filtering out forks and marking repository (project) as having forks
> is now consolidated into one subroutine (special case of handling
> forks in git_get_projects_list only for $projects_list being file is
> now removed).

Ah, no, they aren't.  That's a good change for consistency already.

> Forks handling is also cleaned up and simplified, which
> might mean some changes in output.

(Not having read the code yet) this part is not obvious to me. Maybe an example could help.

[...]

> The interaction between forks, content tags and searching is now made
> more explicit: searching whether by tag, or via search form turns off
> fork filtering (gitweb searches also forks, and will show all
> results).

Hm, or maybe this is it.

> IMVHO the code is
> cleaner.

So some general code tidying is intermixed.

> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2646,18 +2646,20 @@ sub git_get_projects_list {

[...]

Show 8 quoted lines

> -		my $dir = $projects_list . ($filter ? "/$filter" : '');
> +		my $dir = $projects_list;
>  		# remove the trailing "/"
>  		$dir =~ s!/+$!!;
> -		my $pfxlen = length("$dir");
> -		my $pfxdepth = ($dir =~ tr!/!!);
> +		my $pfxlen = length("$projects_list");
> +		my $pfxdepth = ($projects_list =~ tr!/!!);

[...]

$dir is the same as before except that trailing slashes are removed, and meanwhile the stripped names of directories returned from find() will include $filter. Should be almost a no-op (i.e., a nice simplification).

Show 6 quoted lines

> @@ -2672,14 +2674,14 @@ sub git_get_projects_list {
>  				# only directories can be git repositories
>  				return unless (-d $_);
>  				# don't traverse too deep (Find is super slow on os x)
> +				# $project_maxdepth excludes depth of $projectroot
>  				if (($File::Find::name =~ tr!/!!) - $pfxdepth > $project_maxdepth) {

A bugfix. $project_maxdepth is advertised to be relative to the projectroot but it was relative to $projects_list/$filter.

[...]

Show 14 quoted lines

> @@ -2703,32 +2704,9 @@ sub git_get_projects_list {
>  			if (!defined $path) {
>  				next;
>  			}
> -			if ($filter ne '') {
> -				# looking for forks;
> -				my $pfx = substr($path, 0, length($filter));
> -				if ($pfx ne $filter) {
> -					next PROJECT;
> -				}
> -				my $sfx = substr($path, length($filter));
> -				if ($sfx !~ /^\/.*\.git$/) {
> -					next PROJECT;
> -				}

[...]

> +			# if $filter is rpovided, check if $path begins with $filter

s/rpovided/provided/ :)

> +			if ($filter && $path !~ m!^\Q$filter\E/!) {
> +				next;

Why did the code check for \Q.git\E$ before? If I'm not missing something, I'm happy to see the condition go.

What if $filter is 0?  Silly question, I guess.

[...]

Show 12 quoted lines

> @@ -2745,6 +2721,67 @@ sub git_get_projects_list {
>  	return @list;
>  }
>  
> +# this assumes that forks follow immediately forked projects:
> +# [ ..., 'repo.git', 'repo/fork.git', ...]; if not, set second
> +# parameter to true value to sort projects by path first.
> +sub filter_forks_from_projects_list {
> +	my ($projects, $need_sorting) = @_;
> +
> +	@$projects = sort { $a->{'path'} cmp $b->{'path'} } @$projects
> +		if ($need_sorting);

What happens in this scenario?

	git.git
	git.related.project.git
	git/platforms.git

[...]

Show 10 quoted lines

> @@ -4747,23 +4784,36 @@ sub fill_project_list_info {
>  		if (!defined $pr->{'owner'}) {
>  			$pr->{'owner'} = git_get_project_owner("$pr->{'path'}") || "";
>  		}
> -		if ($check_forks) {
> -			my $pname = $pr->{'path'};
> -			if (($pname =~ s/\.git$//) &&
> -			    ($pname !~ /\/$/) &&
> -			    (-d "$projectroot/$pname")) {
> -				$pr->{'forks'} = "-d $projectroot/$pname";

So currently we treat project.git as having forks when the corresponding project/ directory exists, even when the latter is empty. The proposed new rule is that project.git has forks if and only if there is at least one project/foo.git repository. Sensible.

Thanks for a pleasant read.

Hope that helps, Jonathan