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

[Jakub Narebski <jnareb@gmail.com>]

"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.
 
> 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?

> 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.

> @@ -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()?

> @@ -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");
>  	}

[...]

> @@ -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