Re: [PATCH v2 1/2] gitweb: add project_filter to limit project list to a subdirectory

From: Jakub Narebski <jnareb@gmail.com>
Date: Jan 28, 2012, 22:45 UTC
Message-ID: <m3wr8bcuon.fsf@localhost.localdomain>
In-Reply-To: <20120128165606.GA6770@server.brlink.eu>

"Bernhard R. Link" <brl+git@mail.brlink.eu> writes:

> This commit changes the project listings (project_list, project_index
> and opml) to limit the output to only projects in a subdirectory if the
> new optional parameter ?pf=directory name is used.
>

"project listings" to "projects listing views", isn't it?

> The change is quite minimal as git_get_projects_list already can limit
> itself to a subdirectory (though that was previously only used for
> 'forks').
>

Nice description, and more clear than before.

Show 5 quoted lines

> If strict_export is enabled and there is no projects_list, it still
> traverses the full tree and only filters afterwards to avoid anything
> getting visible by this. Otherwise only the subtree needs to be
> traversed, significantly reducing load times.
>

I still don't understand interaction between project_filter ('pf'), $strict_export and $projects_list being either directory or a file with a list of projects.

Does it mean, that when $projects_list is a file with a list of projects, and we use project_filter, then:

* if $strict_export is false, then $project_list is ignored, and the
  filtered list of projects is created by scanning
  "$projectroot/$project_filter"

* if $strict_export is true, then $project_list file is read in full,
  and then filtered to project with $project_filter as prefix
 
Is it correct?  Is it sane, stated this way?

Show 5 quoted lines

> Reusing $project instead of adding a new parameter would have been
> nicer from a UI point-of-view (including PATH_INFO support) but
> would complicate the $project validating code that is currently being
> used to ensure nothing is exported that should not be viewable.
>

O.K.

Anyway PATH_INFO support can be added in the future, by special casing situation where project list action is stated using PATH_INFO... I think.

A few nitpicks with respect to patch itself.

Show 7 quoted lines

> @@ -2827,6 +2835,7 @@ sub git_get_project_url_list {
>  
>  sub git_get_projects_list {
>  	my $filter = shift || '';
> +	my $paranoid = shift || 0;
>  	my @list;
>

First, undefined value is false in Perl, so there is no need for " || 0" in setting $paranoid variable.

Second, why not use global variable $strict_export instead of adding another parameter to git_get_projects_list()?

Show 9 quoted lines

> @@ -5979,7 +5994,7 @@ sub git_project_list {
>  		die_error(400, "Unknown order parameter");
>  	}
>  
> -	my @list = git_get_projects_list();
> +	my @list = git_get_projects_list($project_filter, $strict_export);
>  	if (!@list) {
>  		die_error(404, "No projects found");
>  	}

[...]

Show 13 quoted lines

> @@ -3963,9 +3976,11 @@ sub git_footer_html {
>  		}
>  
>  	} else {
> -		print $cgi->a({-href => href(project=>undef, action=>"opml"),
> +		print $cgi->a({-href => href(project=>undef, action=>"opml",
> +		                             project_filter => $project_filter),
>  		              -class => $feed_class}, "OPML") . " ";
> -		print $cgi->a({-href => href(project=>undef, action=>"project_index"),
> +		print $cgi->a({-href => href(project=>undef, action=>"project_index",
> +		                             project_filter => $project_filter),
>  		              -class => $feed_class}, "TXT") . "\n";
>  	}

O.K.

-- 
Jakub Narebski