threads / patch / 29461

v2, 2 partsgitweb: add project_filter to limit project list to a subdirectory

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

## tl;dr

40 messages between Jan 28, 2012 and Feb 1, 2012. Diffs are folded; open one to read it.

replies: 39people: 4as markdown or json

Bernhard R. Link· Jan 28, 2012, 16:56 UTC · lore

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.

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

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.

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.

Signed-off-by: Bernhard R. Link <brlink@debian.org>
---
 Changed since version 1:
   - improve description
   - simplify as suggested by Jakub Narebski
   - support the strict_exports + no projects_list case
 gitweb/gitweb.perl |   29 ++++++++++++++++++++++-------
 1 files changed, 22 insertions(+), 7 deletions(-)
Show changes to gitweb/gitweb.perl +22 −7
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index abb5a79..a114bd4 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -760,6 +760,7 @@ our @cgi_param_mapping = (
 	search_use_regexp => "sr",
 	ctag => "by_tag",
 	diff_style => "ds",
+	project_filter => "pf",
 	# this must be last entry (for manipulation from JavaScript)
 	javascript => "js"
 );
@@ -976,7 +977,7 @@ sub evaluate_path_info {
 
 our ($action, $project, $file_name, $file_parent, $hash, $hash_parent, $hash_base,
      $hash_parent_base, @extra_options, $page, $searchtype, $search_use_regexp,
-     $searchtext, $search_regexp);
+     $searchtext, $search_regexp, $project_filter);
 sub evaluate_and_validate_params {
 	our $action = $input_params{'action'};
 	if (defined $action) {
@@ -994,6 +995,13 @@ sub evaluate_and_validate_params {
 		}
 	}
 
+	our $project_filter = $input_params{'project_filter'};
+	if (defined $project_filter) {
+		if (!validate_pathname($project_filter)) {
+			die_error(404, "Invalid project_filter parameter");
+		}
+	}
+
 	our $file_name = $input_params{'file_name'};
 	if (defined $file_name) {
 		if (!validate_pathname($file_name)) {
@@ -2827,6 +2835,7 @@ sub git_get_project_url_list {
 
 sub git_get_projects_list {
 	my $filter = shift || '';
+	my $paranoid = shift || 0;
 	my @list;
 
 	$filter =~ s/\.git$//;
@@ -2839,7 +2848,7 @@ sub git_get_projects_list {
 		my $pfxlen = length("$dir");
 		my $pfxdepth = ($dir =~ tr!/!!);
 		# when filtering, search only given subdirectory
-		if ($filter) {
+		if ($filter and not $paranoid) {
 			$dir .= "/$filter";
 			$dir =~ s!/+$!!;
 		}
@@ -2864,6 +2873,10 @@ sub git_get_projects_list {
 				}
 
 				my $path = substr($File::Find::name, $pfxlen + 1);
+				# paranoidly only filter here
+				if ($paranoid && $filter && $path !~ m!^\Q$filter\E/!) {
+					next;
+				}
 				# we check related file in $projectroot
 				if (check_export_ok("$projectroot/$path")) {
 					push @list, { path => $path };
@@ -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";
 	}
 	print "</div>\n"; # class="page_footer"
@@ -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");
 	}
@@ -6018,7 +6033,7 @@ sub git_forks {
 }
 
 sub git_project_index {
-	my @projects = git_get_projects_list();
+	my @projects = git_get_projects_list($project_filter, $strict_export);
 	if (!@projects) {
 		die_error(404, "No projects found");
 	}
@@ -7855,7 +7870,7 @@ sub git_atom {
 }
 
 sub git_opml {
-	my @list = git_get_projects_list();
+	my @list = git_get_projects_list($project_filter, $strict_export);
 	if (!@list) {
 		die_error(404, "No projects found");
 	}
-- 
1.7.8.3
Bernhard R. Link· Jan 28, 2012, 16:57 UTC · re: Bernhard R. Link · lore

[PATCH v2 2/2] gitweb: place links to parent directories in page header

Signed-off-by: Bernhard R. Link <brlink@debian.org>

--- This patch was not yet part of v1.

I'm not sure this if having this as seperate patch or merged into 1/2 makes more sense.

 gitweb/gitweb.perl |   23 ++++++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)
Show changes to gitweb/gitweb.perl +22 −1
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a114bd4..ddce27d 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3841,7 +3841,18 @@ sub print_nav_breadcrumbs {
 
 	print $cgi->a({-href => esc_url($home_link)}, $home_link_str) . " / ";
 	if (defined $project) {
-		print $cgi->a({-href => href(action=>"summary")}, esc_html($project));
+		my @dirname = split '/', $project;
+		my $projectbasename = pop @dirname;
+		my $dirprefix = undef;
+		while (my $part = shift @dirname) {
+			$dirprefix .= "/" if defined $dirprefix;
+			$dirprefix .= $part;
+			print $cgi->a({-href => href(project => undef,
+			                             project_filter => $dirprefix,
+			                             action=>"project_list")},
+			              esc_html($part)) . " / ";
+		}
+		print $cgi->a({-href => href(action=>"summary")}, esc_html($projectbasename));
 		if (defined $action) {
 			my $action_print = $action ;
 			if (defined $opts{-action_extra}) {
@@ -3854,6 +3865,16 @@ sub print_nav_breadcrumbs {
 			print " / $opts{-action_extra}";
 		}
 		print "\n";
+	} elsif (defined $project_filter) {
+		my @dirname = split '/', $project_filter;
+		my $dirprefix = undef;
+		while (my $part = shift @dirname) {
+			$dirprefix .= "/" if defined $dirprefix;
+			$dirprefix .= $part;
+			print $cgi->a({-href => href(project_filter => $dirprefix,
+			                             action=>"project_list")},
+			              esc_html($part)) . " / ";
+		}
 	}
 }
 
-- 
1.7.8.3
Jakub Narebski· Jan 28, 2012, 22:54 UTC · re: Bernhard R. Link · lore

Re: [PATCH v2 2/2] gitweb: place links to parent directories in page header

"Bernhard R. Link" <brl+git@mail.brlink.eu> writes:
Description?
Show 7 quoted lines
> Signed-off-by: Bernhard R. Link <brlink@debian.org>
> 
> ---
> This patch was not yet part of v1.
> 
> I'm not sure this if having this as seperate patch or merged into 1/2
> makes more sense.
While adding links that lead to gitweb URLs with project_filter
parameter set, i.e. linking new feature in, could be postponed to a
later commit, I think some way of notifying client that project list
is filtered would be better to have in 1/2.
 
Show 27 quoted lines
>  gitweb/gitweb.perl |   23 ++++++++++++++++++++++-
>  1 files changed, 22 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index a114bd4..ddce27d 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3841,7 +3841,18 @@ sub print_nav_breadcrumbs {
>  
>  	print $cgi->a({-href => esc_url($home_link)}, $home_link_str) . " / ";
>  	if (defined $project) {
> -		print $cgi->a({-href => href(action=>"summary")}, esc_html($project));
> +		my @dirname = split '/', $project;
> +		my $projectbasename = pop @dirname;
> +		my $dirprefix = undef;
> +		while (my $part = shift @dirname) {
> +			$dirprefix .= "/" if defined $dirprefix;
> +			$dirprefix .= $part;
> +			print $cgi->a({-href => href(project => undef,
> +			                             project_filter => $dirprefix,
> +			                             action=>"project_list")},
> +			              esc_html($part)) . " / ";
> +		}
> +		print $cgi->a({-href => href(action=>"summary")}, esc_html($projectbasename));
>  		if (defined $action) {
>  			my $action_print = $action ;
>  			if (defined $opts{-action_extra}) {
Nice solution.
Show 16 quoted lines
> @@ -3854,6 +3865,16 @@ sub print_nav_breadcrumbs {
>  			print " / $opts{-action_extra}";
>  		}
>  		print "\n";
> +	} elsif (defined $project_filter) {
> +		my @dirname = split '/', $project_filter;
> +		my $dirprefix = undef;
> +		while (my $part = shift @dirname) {
> +			$dirprefix .= "/" if defined $dirprefix;
> +			$dirprefix .= $part;
> +			print $cgi->a({-href => href(project_filter => $dirprefix,
> +			                             action=>"project_list")},
> +			              esc_html($part)) . " / ";
> +		}
>  	}
>  }

Hmmm... I'd have to check how it looks like, but seems like a good idea... even if there is a little bit of code duplication.

-- 
Jakub Narebski
Jakub Narebski· Jan 28, 2012, 22:45 UTC · re: Bernhard R. Link · lore

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

"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
Bernhard R. Link· Jan 29, 2012, 01:22 UTC · re: Jakub Narebski · lore

[PATCH v3] gitweb: add project_filter to limit project list to a subdirectory

This commit changes the project listing views (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.

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

If there is a GITWEB_LIST file, the contents are just filtered like with the forks action.

Without a GITWEB_LIST file only the given subdirectory is searched for projects (like with forks) unless GITWEB_STRICT_EXPORT is enabled. In the later case GITWEB_PROJECTROOT is traversed normally (unlike with forks) and projects not in the directory ignored. (As there is no check if the filter_path would have been found in the usual search as the project path is checked with forks).

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.

Additionally change html page headers to not only link the project root and the currently selected project but also the directories in between using project_filter.

Signed-off-by: Bernhard R. Link <brlink@debian.org>
---
changes since v2:
        improve description
        remove || 0 for boolean argument
        merge with patch using this feature
        use user-visible configuration names instead of internal ones
* Jakub Narebski <jnareb@gmail.com> [120128 23:45]:
Show 16 quoted lines
> "Bernhard R. Link" <brl+git@mail.brlink.eu> writes:
> > 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"

No. If project_list is set, i.e. a file, then this is always used. If it is a directory (because it is not set thus set to projectroot), then with forks it still traverses that directory (as that was checked before to be a reachable project with a previous call to git_get_projects_list). In the case of project_filter only the directory is traversed without strict_export and the whole projectroot is traversed with strict_export.

Is the new description better.
Show 12 quoted lines
> 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.

I thought it make it clearer that the argument might not be set and what the default is. But that is personal taste.

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

That would change the action=forks behaviour to traverse the whole projectroot two times. This way paranoia is only activated if strict_mode is set _and_ the argument was not yet checked to be reachable.

 gitweb/gitweb.perl |   52 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 44 insertions(+), 8 deletions(-)
Show changes to gitweb/gitweb.perl +44 −8
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index abb5a79..089d45d 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -760,6 +760,7 @@ our @cgi_param_mapping = (
 	search_use_regexp => "sr",
 	ctag => "by_tag",
 	diff_style => "ds",
+	project_filter => "pf",
 	# this must be last entry (for manipulation from JavaScript)
 	javascript => "js"
 );
@@ -976,7 +977,7 @@ sub evaluate_path_info {
 
 our ($action, $project, $file_name, $file_parent, $hash, $hash_parent, $hash_base,
      $hash_parent_base, @extra_options, $page, $searchtype, $search_use_regexp,
-     $searchtext, $search_regexp);
+     $searchtext, $search_regexp, $project_filter);
 sub evaluate_and_validate_params {
 	our $action = $input_params{'action'};
 	if (defined $action) {
@@ -994,6 +995,13 @@ sub evaluate_and_validate_params {
 		}
 	}
 
+	our $project_filter = $input_params{'project_filter'};
+	if (defined $project_filter) {
+		if (!validate_pathname($project_filter)) {
+			die_error(404, "Invalid project_filter parameter");
+		}
+	}
+
 	our $file_name = $input_params{'file_name'};
 	if (defined $file_name) {
 		if (!validate_pathname($file_name)) {
@@ -2827,6 +2835,7 @@ sub git_get_project_url_list {
 
 sub git_get_projects_list {
 	my $filter = shift || '';
+	my $paranoid = shift;
 	my @list;
 
 	$filter =~ s/\.git$//;
@@ -2839,7 +2848,7 @@ sub git_get_projects_list {
 		my $pfxlen = length("$dir");
 		my $pfxdepth = ($dir =~ tr!/!!);
 		# when filtering, search only given subdirectory
-		if ($filter) {
+		if ($filter and not $paranoid) {
 			$dir .= "/$filter";
 			$dir =~ s!/+$!!;
 		}
@@ -2864,6 +2873,10 @@ sub git_get_projects_list {
 				}
 
 				my $path = substr($File::Find::name, $pfxlen + 1);
+				# paranoidly only filter here
+				if ($paranoid && $filter && $path !~ m!^\Q$filter\E/!) {
+					next;
+				}
 				# we check related file in $projectroot
 				if (check_export_ok("$projectroot/$path")) {
 					push @list, { path => $path };
@@ -3828,7 +3841,18 @@ sub print_nav_breadcrumbs {
 
 	print $cgi->a({-href => esc_url($home_link)}, $home_link_str) . " / ";
 	if (defined $project) {
-		print $cgi->a({-href => href(action=>"summary")}, esc_html($project));
+		my @dirname = split '/', $project;
+		my $projectbasename = pop @dirname;
+		my $dirprefix = undef;
+		while (my $part = shift @dirname) {
+			$dirprefix .= "/" if defined $dirprefix;
+			$dirprefix .= $part;
+			print $cgi->a({-href => href(project => undef,
+			                             project_filter => $dirprefix,
+			                             action=>"project_list")},
+			              esc_html($part)) . " / ";
+		}
+		print $cgi->a({-href => href(action=>"summary")}, esc_html($projectbasename));
 		if (defined $action) {
 			my $action_print = $action ;
 			if (defined $opts{-action_extra}) {
@@ -3841,6 +3865,16 @@ sub print_nav_breadcrumbs {
 			print " / $opts{-action_extra}";
 		}
 		print "\n";
+	} elsif (defined $project_filter) {
+		my @dirname = split '/', $project_filter;
+		my $dirprefix = undef;
+		while (my $part = shift @dirname) {
+			$dirprefix .= "/" if defined $dirprefix;
+			$dirprefix .= $part;
+			print $cgi->a({-href => href(project_filter => $dirprefix,
+			                             action=>"project_list")},
+			              esc_html($part)) . " / ";
+		}
 	}
 }
 
@@ -3963,9 +3997,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";
 	}
 	print "</div>\n"; # class="page_footer"
@@ -5979,7 +6015,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");
 	}
@@ -6018,7 +6054,7 @@ sub git_forks {
 }
 
 sub git_project_index {
-	my @projects = git_get_projects_list();
+	my @projects = git_get_projects_list($project_filter, $strict_export);
 	if (!@projects) {
 		die_error(404, "No projects found");
 	}
@@ -7855,7 +7891,7 @@ sub git_atom {
 }
 
 sub git_opml {
-	my @list = git_get_projects_list();
+	my @list = git_get_projects_list($project_filter, $strict_export);
 	if (!@list) {
 		die_error(404, "No projects found");
 	}
-- 
1.7.8.3
Jakub Narebski· Jan 29, 2012, 12:54 UTC · re: Bernhard R. Link · lore

Re: [PATCH v3] gitweb: add project_filter to limit project list to a subdirectory

On Sun, 29 Jan 2012, Bernhard R. Link wrote:
Show 8 quoted lines
> This commit changes the project listing views (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.
> 
> 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 and succinct.
 
> If there is a GITWEB_LIST file, the contents are just filtered like
> with the forks action.
> 
O.K.
Show 7 quoted lines
> Without a GITWEB_LIST file only the given subdirectory is searched
> for projects (like with forks) unless GITWEB_STRICT_EXPORT is enabled.
> In the later case GITWEB_PROJECTROOT is traversed normally (unlike
> with forks) and projects not in the directory ignored.
> (As there is no check if the filter_path would have been found in
> the usual search as the project path is checked with forks).
> 
Now I understand how project_filter interacts with strict_export.

Though I am not sure if this "paranoid mode" is really necessary. I don't see how you could get in situation where scanning from $project_list and filtering with $project_filter prefix, and scanning from $project_list/$project_filter would give different results.

I think you are overly paranoid here, but perhaps it is better to be overly strict, and then relax it if it turns out to be not necessary.

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.
> 
Sidenote: support for actionless PATH_INFO URLs would make it even more
complicated...
> Additionally change html page headers to not only link the project
> root and the currently selected project but also the directories in
> between using project_filter.
> 

Excuse me changing my mind, but I think that as far as this patch series is applied as whole, it would be better for maintability to keep those two patches split; though put the above as a [part of] commit message in 2/2 patch.

Show 35 quoted lines
> Signed-off-by: Bernhard R. Link <brlink@debian.org>
> ---
> 
> changes since v2:
>         improve description
>         remove || 0 for boolean argument
>         merge with patch using this feature
>         use user-visible configuration names instead of internal ones
> 
> * Jakub Narebski <jnareb@gmail.com> [120128 23:45]:
> > "Bernhard R. Link" <brl+git@mail.brlink.eu> writes:
> > > 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"
> 
> No. If project_list is set, i.e. a file, then this is always used.
> If it is a directory (because it is not set thus set to projectroot),
> then with forks it still traverses that directory (as that was checked
> before to be a reachable project with a previous call to
> git_get_projects_list). In the case of project_filter only the directory
> is traversed without strict_export and the whole projectroot is
> traversed with strict_export.
> 
O.K., now I understand it.
> Is the new description better.
> 
Yes it is.
Show 15 quoted lines
> > 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.
> 
> I thought it make it clearer that the argument might not be set and
> what the default is. But that is personal taste.
First, optional parameter defaults to false in the 'my $foo = shift;'
or equivalent form is (I think) idiomatic Perl.  Second, this way of
writing it is used through gitweb code (CodingGuidelines: imitate existing
coding practices). 
 
Show 7 quoted lines
> > Second, why not use global variable $strict_export instead of adding
> > another parameter to git_get_projects_list()?
> 
> That would change the action=forks behaviour to traverse the whole
> projectroot two times. This way paranoia is only activated if
> strict_mode is set _and_ the argument was not yet checked to be
> reachable.
Thanks for explanation.
 
>  gitweb/gitweb.perl |   52 ++++++++++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 44 insertions(+), 8 deletions(-)
Not that large for a new feature...
-- 
Jakub Narebski
Poland
Bernhard R. Link· Jan 29, 2012, 16:06 UTC · re: Jakub Narebski · lore

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

This commit changes the project listing views (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.

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

If there is a GITWEB_LIST file, the contents are just filtered like with the forks action.

Without a GITWEB_LIST file only the given subdirectory is searched for projects (like with forks) unless GITWEB_STRICT_EXPORT is enabled. In the later case GITWEB_PROJECTROOT is traversed normally (unlike with forks) and projects not in the directory ignored. (As there is no check if the filter_path would have been found in the usual search as the project path is checked with forks).

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.

Signed-off-by: Bernhard R. Link <brlink@debian.org>
---
* Jakub Narebski <jnareb@gmail.com> [120129 13:54]:
Show 8 quoted lines
> On Sun, 29 Jan 2012, Bernhard R. Link wrote:
> Though I am not sure if this "paranoid mode" is really necessary.  I don't
> see how you could get in situation where scanning from $project_list and
> filtering with $project_filter prefix, and scanning from
> $project_list/$project_filter would give different results.
>
> I think you are overly paranoid here, but perhaps it is better to be
> overly strict, and then relax it if it turns out to be not necessary.

As far as I do understand it, this is the only (hopefully unecessary) effect strict_export without a project_list has in gitweb, so I did not want to remove that with this change.

> Excuse me changing my mind, but I think that as far as this patch series
> is applied as whole, it would be better for maintability to keep those
> two patches split; though put the above as a [part of] commit message
> in 2/2 patch.

Split again, though this time only the change for existing pages in the second commit and the code duplication you spoke against removed.

 gitweb/gitweb.perl |   43 ++++++++++++++++++++++++++++++++++++-------
 1 files changed, 36 insertions(+), 7 deletions(-)
Show changes to gitweb/gitweb.perl +36 −7
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index abb5a79..f0e03d8 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -760,6 +760,7 @@ our @cgi_param_mapping = (
 	search_use_regexp => "sr",
 	ctag => "by_tag",
 	diff_style => "ds",
+	project_filter => "pf",
 	# this must be last entry (for manipulation from JavaScript)
 	javascript => "js"
 );
@@ -976,7 +977,7 @@ sub evaluate_path_info {
 
 our ($action, $project, $file_name, $file_parent, $hash, $hash_parent, $hash_base,
      $hash_parent_base, @extra_options, $page, $searchtype, $search_use_regexp,
-     $searchtext, $search_regexp);
+     $searchtext, $search_regexp, $project_filter);
 sub evaluate_and_validate_params {
 	our $action = $input_params{'action'};
 	if (defined $action) {
@@ -994,6 +995,13 @@ sub evaluate_and_validate_params {
 		}
 	}
 
+	our $project_filter = $input_params{'project_filter'};
+	if (defined $project_filter) {
+		if (!validate_pathname($project_filter)) {
+			die_error(404, "Invalid project_filter parameter");
+		}
+	}
+
 	our $file_name = $input_params{'file_name'};
 	if (defined $file_name) {
 		if (!validate_pathname($file_name)) {
@@ -2827,6 +2835,7 @@ sub git_get_project_url_list {
 
 sub git_get_projects_list {
 	my $filter = shift || '';
+	my $paranoid = shift;
 	my @list;
 
 	$filter =~ s/\.git$//;
@@ -2839,7 +2848,7 @@ sub git_get_projects_list {
 		my $pfxlen = length("$dir");
 		my $pfxdepth = ($dir =~ tr!/!!);
 		# when filtering, search only given subdirectory
-		if ($filter) {
+		if ($filter and not $paranoid) {
 			$dir .= "/$filter";
 			$dir =~ s!/+$!!;
 		}
@@ -2864,6 +2873,10 @@ sub git_get_projects_list {
 				}
 
 				my $path = substr($File::Find::name, $pfxlen + 1);
+				# paranoidly only filter here
+				if ($paranoid && $filter && $path !~ m!^\Q$filter\E/!) {
+					next;
+				}
 				# we check related file in $projectroot
 				if (check_export_ok("$projectroot/$path")) {
 					push @list, { path => $path };
@@ -3823,6 +3836,18 @@ sub print_header_links {
 	}
 }
 
+sub print_nav_breadcrumbs_path {
+	my $dirprefix = undef;
+	while (my $part = shift) {
+		$dirprefix .= "/" if defined $dirprefix;
+		$dirprefix .= $part;
+		print $cgi->a({-href => href(project => undef,
+		                             project_filter => $dirprefix,
+					     action=>"project_list")},
+			      esc_html($part)) . " / ";
+	}
+}
+
 sub print_nav_breadcrumbs {
 	my %opts = @_;
 
@@ -3841,6 +3866,8 @@ sub print_nav_breadcrumbs {
 			print " / $opts{-action_extra}";
 		}
 		print "\n";
+	} elsif (defined $project_filter) {
+		print_nav_breadcrumbs_path(split '/', $project_filter);
 	}
 }
 
@@ -3963,9 +3990,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";
 	}
 	print "</div>\n"; # class="page_footer"
@@ -5979,7 +6008,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");
 	}
@@ -6018,7 +6047,7 @@ sub git_forks {
 }
 
 sub git_project_index {
-	my @projects = git_get_projects_list();
+	my @projects = git_get_projects_list($project_filter, $strict_export);
 	if (!@projects) {
 		die_error(404, "No projects found");
 	}
@@ -7855,7 +7884,7 @@ sub git_atom {
 }
 
 sub git_opml {
-	my @list = git_get_projects_list();
+	my @list = git_get_projects_list($project_filter, $strict_export);
 	if (!@list) {
 		die_error(404, "No projects found");
 	}
-- 
1.7.8.3
Bernhard R. Link· Jan 29, 2012, 16:13 UTC · re: Bernhard R. Link · lore

[PATCH v4 2/2] gitweb: place links to parent directories in page header

Change html page headers to not only link the project root and the currently selected project but also the directories in between using project_filter.

Signed-off-by: Bernhard R. Link <brlink@debian.org>
---
 gitweb/gitweb.perl |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
Show changes to gitweb/gitweb.perl +4 −1
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index f0e03d8..e2a9146 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3853,7 +3853,10 @@ sub print_nav_breadcrumbs {
 
 	print $cgi->a({-href => esc_url($home_link)}, $home_link_str) . " / ";
 	if (defined $project) {
-		print $cgi->a({-href => href(action=>"summary")}, esc_html($project));
+		my @dirname = split '/', $project;
+		my $projectbasename = pop @dirname;
+		print_nav_breadcrumbs_path(@dirname);
+		print $cgi->a({-href => href(action=>"summary")}, esc_html($projectbasename));
 		if (defined $action) {
 			my $action_print = $action ;
 			if (defined $opts{-action_extra}) {
-- 
1.7.8.3
Jakub Narebski· Jan 29, 2012, 16:46 UTC · re: Bernhard R. Link · lore

Re: [PATCH v4 2/2] gitweb: place links to parent directories in page header

On Sun, 29 Jan 2012, Bernhard R. Link wrote:
> Change html page headers to not only link the project root and the
> currently selected project but also the directories in between using
> project_filter.
Nice interface to the new feature... though it doesn't really address
the problem that gitweb homepage is slow to generate with large number
of projects.  Still, it is IMVHO a good improvement.
 
> Signed-off-by: Bernhard R. Link <brlink@debian.org>
Acked-by: Jakub Narebski <jnareb@gmail.com>
Show 21 quoted lines
> ---
>  gitweb/gitweb.perl |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index f0e03d8..e2a9146 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3853,7 +3853,10 @@ sub print_nav_breadcrumbs {
>  
>  	print $cgi->a({-href => esc_url($home_link)}, $home_link_str) . " / ";
>  	if (defined $project) {
> -		print $cgi->a({-href => href(action=>"summary")}, esc_html($project));
> +		my @dirname = split '/', $project;
> +		my $projectbasename = pop @dirname;
> +		print_nav_breadcrumbs_path(@dirname);
> +		print $cgi->a({-href => href(action=>"summary")}, esc_html($projectbasename));
>  		if (defined $action) {
>  			my $action_print = $action ;
>  			if (defined $opts{-action_extra}) {
> -- 
Nicely short with refactoring of print_nav_breadcrumbs_path() in 1/2!
-- 
Jakub Narebski
Poland
Jakub Narebski· Jan 29, 2012, 16:41 UTC · re: Bernhard R. Link · lore

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

On Sun, 29 Jan 2012, Bernhard R. Link wrote:
Show 17 quoted lines
> This commit changes the project listing views (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.
> 
> 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').
> 
> If there is a GITWEB_LIST file, the contents are just filtered like
> with the forks action.
> 
> Without a GITWEB_LIST file only the given subdirectory is searched
> for projects (like with forks) unless GITWEB_STRICT_EXPORT is enabled.
> In the later case GITWEB_PROJECTROOT is traversed normally (unlike
> with forks) and projects not in the directory ignored.
> (As there is no check if the filter_path would have been found in
> the usual search as the project path is checked with forks).
I am still unsure if it is really necessary, but nevermind...
 
Show 6 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.
> 
> Signed-off-by: Bernhard R. Link <brlink@debian.org>
Acked-by: Jakub Narebski <jnareb@gmail.com>
> ---
[...]
Show 28 quoted lines
> @@ -3823,6 +3836,18 @@ sub print_header_links {
>  	}
>  }
>  
> +sub print_nav_breadcrumbs_path {
> +	my $dirprefix = undef;
> +	while (my $part = shift) {
> +		$dirprefix .= "/" if defined $dirprefix;
> +		$dirprefix .= $part;
> +		print $cgi->a({-href => href(project => undef,
> +		                             project_filter => $dirprefix,
> +					     action=>"project_list")},
> +			      esc_html($part)) . " / ";
> +	}
> +}
> +
>  sub print_nav_breadcrumbs {
>  	my %opts = @_;
>  
> @@ -3841,6 +3866,8 @@ sub print_nav_breadcrumbs {
>  			print " / $opts{-action_extra}";
>  		}
>  		print "\n";
> +	} elsif (defined $project_filter) {
> +		print_nav_breadcrumbs_path(split '/', $project_filter);
>  	}
>  }
>  

This could have been split into a separate 2/3 commit, but nevermind; it can be squashed here.

-- 
Jakub Narebski
Poland
Junio C Hamano· Jan 29, 2012, 21:06 UTC · re: Bernhard R. Link · lore

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

"Bernhard R. Link" <brl+git@mail.brlink.eu> writes:
Show 10 quoted lines
> This commit changes the project listing views (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.
>
> 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').
>
> If there is a GITWEB_LIST file, the contents are just filtered like
> with the forks action.

Meaning, a directory is shown if it is listed on GITWEB_LIST and is a subdirectory of the directory specified with project_filter? If so, spelling it out instead of saying "just filtered like with the forks action" may be clearer without making the description excessively longer.

> Without a GITWEB_LIST file only the given subdirectory is searched
> for projects (like with forks) unless GITWEB_STRICT_EXPORT is enabled.
> In the later case GITWEB_PROJECTROOT is traversed normally (unlike
> with forks) and projects not in the directory ignored.

It is unclear to me what "In the later case" refers to, even assuming that it is a typo of "the latter case".

Do you mean "When there is no GITWEB_LIST but GITWEB_STRICT_EXPORT is set, project_filter that specifies anything outside GITWEB_PROJECTROOT is ignored"?

A more fundamental issue I have with this patch is how an end user starts using this. Once project_filter is set, the breadcrumbs would let the user click and navigate around, but in my superficial glance at the patch it is not apparent how the initial setting of project_filter can happen without the user manually adding pf= to the URL, which is a less than ideal end user experience.

Show 17 quoted lines
> @@ -2839,7 +2848,7 @@ sub git_get_projects_list {
>  		my $pfxlen = length("$dir");
>  		my $pfxdepth = ($dir =~ tr!/!!);
>  		# when filtering, search only given subdirectory
> -		if ($filter) {
> +		if ($filter and not $paranoid) {
>  			$dir .= "/$filter";
>  			$dir =~ s!/+$!!;
>  		}
> @@ -2864,6 +2873,10 @@ sub git_get_projects_list {
>  				}
>  
>  				my $path = substr($File::Find::name, $pfxlen + 1);
> +				# paranoidly only filter here
> +				if ($paranoid && $filter && $path !~ m!^\Q$filter\E/!) {
> +					next;
> +				}

When you find "foo" directory and a project_filter tells you to match "foo", because $path does not match "^foo/", it will not match (even though its subdirectory "foo/bar" would)?

Show 23 quoted lines
> +sub print_nav_breadcrumbs_path {
> +	my $dirprefix = undef;
> +	while (my $part = shift) {
> +		$dirprefix .= "/" if defined $dirprefix;
> +		$dirprefix .= $part;
> +		print $cgi->a({-href => href(project => undef,
> +		                             project_filter => $dirprefix,
> +					     action=>"project_list")},
> +			      esc_html($part)) . " / ";
> +	}
> +}
> +
>  sub print_nav_breadcrumbs {
>  	my %opts = @_;
>  
> @@ -3841,6 +3866,8 @@ sub print_nav_breadcrumbs {
>  			print " / $opts{-action_extra}";
>  		}
>  		print "\n";
> +	} elsif (defined $project_filter) {
> +		print_nav_breadcrumbs_path(split '/', $project_filter);
>  	}
>  }
Hmm.

While this may not be wrong, I wonder if this is limiting a useful feature too narrowly. When I visit "/pub/scm /linux/kernel/git/torvals/linux.git" at git.kernel.org, for example, there currently are two links, "/pub/scm" to the toplevel and "/linux/kernel/git/torvals/linux.git" to itself. I often wish to see uplinks to intermediate levels like "/linux/kernel/git" and "/linux/kernel/git/torvalds".

Perhaps that is the topic of your second patch. I dunno.
Jakub Narebski· Jan 29, 2012, 23:06 UTC · re: Junio C Hamano · lore

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

On Sun, 29 Jan 2012, Junio C Hamano wrote:
Show 17 quoted lines
> "Bernhard R. Link" <brl+git@mail.brlink.eu> writes:
> 
> > This commit changes the project listing views (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.
> >
> > 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').
> >
> > If there is a GITWEB_LIST file, the contents are just filtered like
> > with the forks action.
> 
> Meaning, a directory is shown if it is listed on GITWEB_LIST and is a
> subdirectory of the directory specified with project_filter?  If so,
> spelling it out instead of saying "just filtered like with the forks
> action" may be clearer without making the description excessively longer.
This means the following:
  If $projects_list point to file with a list of projects, gitweb will
  show only those project on the list which name matches $project_filter
  prefix.
Show 11 quoted lines
> > Without a GITWEB_LIST file only the given subdirectory is searched
> > for projects (like with forks) unless GITWEB_STRICT_EXPORT is enabled.
> > In the later case GITWEB_PROJECTROOT is traversed normally (unlike
> > with forks) and projects not in the directory ignored.
> 
> It is unclear to me what "In the later case" refers to, even assuming that
> it is a typo of "the latter case".
> 
> Do you mean "When there is no GITWEB_LIST but GITWEB_STRICT_EXPORT is set,
> project_filter that specifies anything outside GITWEB_PROJECTROOT is
> ignored"?
  If $projects_list points to a directory, but $strict_export is true, then
  $project_list is scanned recursively for git repositories, as without
  this feature, but only those projects that begin with $project_filter
  are shown.
  Otherwise ($projects_list points to directory and $strict_export not true)
  then $project_filter subdirectory of $projects_list is scanned recursively
  for git repositories (i.e. starting from "projects_list/$project_filter").
 
I am not sure if this paranoia mode is really needed for $strict_export,
though.
Show 6 quoted lines
> A more fundamental issue I have with this patch is how an end user starts
> using this. Once project_filter is set, the breadcrumbs would let the user
> click and navigate around, but in my superficial glance at the patch it is
> not apparent how the initial setting of project_filter can happen without
> the user manually adding pf= to the URL, which is a less than ideal end
> user experience.

The second patch in series adds breadcrumbs allowing to filter projects in any per-project view.

This feature was originally intended for giving handcrafter URL with 'pf=....' to people...

Show 21 quoted lines
> > @@ -2839,7 +2848,7 @@ sub git_get_projects_list {
> >  		my $pfxlen = length("$dir");
> >  		my $pfxdepth = ($dir =~ tr!/!!);
> >  		# when filtering, search only given subdirectory
> > -		if ($filter) {
> > +		if ($filter and not $paranoid) {
> >  			$dir .= "/$filter";
> >  			$dir =~ s!/+$!!;
> >  		}
> > @@ -2864,6 +2873,10 @@ sub git_get_projects_list {
> >  				}
> >  
> >  				my $path = substr($File::Find::name, $pfxlen + 1);
> > +				# paranoidly only filter here
> > +				if ($paranoid && $filter && $path !~ m!^\Q$filter\E/!) {
> > +					next;
> > +				}
> 
> When you find "foo" directory and a project_filter tells you to match
> "foo", because $path does not match "^foo/", it will not match (even
> though its subdirectory "foo/bar" would)?
Strictly speaking the match is on dirname of a project path; the basename
of a project does not matter.  It is intended, but perhaps should be made
more clear in the commit message.
 
Show 34 quoted lines
> > +sub print_nav_breadcrumbs_path {
> > +	my $dirprefix = undef;
> > +	while (my $part = shift) {
> > +		$dirprefix .= "/" if defined $dirprefix;
> > +		$dirprefix .= $part;
> > +		print $cgi->a({-href => href(project => undef,
> > +		                             project_filter => $dirprefix,
> > +					     action=>"project_list")},
> > +			      esc_html($part)) . " / ";
> > +	}
> > +}
> > +
> >  sub print_nav_breadcrumbs {
> >  	my %opts = @_;
> >  
> > @@ -3841,6 +3866,8 @@ sub print_nav_breadcrumbs {
> >  			print " / $opts{-action_extra}";
> >  		}
> >  		print "\n";
> > +	} elsif (defined $project_filter) {
> > +		print_nav_breadcrumbs_path(split '/', $project_filter);
> >  	}
> >  }
> 
> Hmm.
> 
> While this may not be wrong, I wonder if this is limiting a useful feature
> too narrowly. When I visit "/pub/scm /linux/kernel/git/torvals/linux.git"
> at git.kernel.org, for example, there currently are two links, "/pub/scm"
> to the toplevel and "/linux/kernel/git/torvals/linux.git" to itself. I
> often wish to see uplinks to intermediate levels like "/linux/kernel/git"
> and "/linux/kernel/git/torvalds".
> 
> Perhaps that is the topic of your second patch. I dunno.
Yes, it is.
-- 
Jakub Narebski
Poland
Bernhard R. Link· Jan 30, 2012, 09:52 UTC · re: Junio C Hamano · lore

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

* Junio C Hamano <gitster@pobox.com> [120129 22:06]:
Show 12 quoted lines
> > @@ -2864,6 +2873,10 @@ sub git_get_projects_list {
> >  				}
> >  
> >  				my $path = substr($File::Find::name, $pfxlen + 1);
> > +				# paranoidly only filter here
> > +				if ($paranoid && $filter && $path !~ m!^\Q$filter\E/!) {
> > +					next;
> > +				}
>
> When you find "foo" directory and a project_filter tells you to match
> "foo", because $path does not match "^foo/", it will not match (even
> though its subdirectory "foo/bar" would)?

Yes, for consistency with what would be shown with a project list file. (And that it would only show projects which would have a link to this directory in their breadcrumbs (with 2/2)).

> Perhaps that is the topic of your second patch. I dunno.
Yes, that is what the second patch does.
        Bernhard R. Link
Bernhard R. Link· Jan 30, 2012, 11:44 UTC · re: Bernhard R. Link · lore

[PATCH v5 1/5] gitweb: prepare git_get_projects_list for use outside 'forks'.

Use of the filter option of git_get_projects_list is currently limited to forks. It hard codes removal of ".git" suffixes from the filter and assumes the project belonging to the filter directory was already validated to be visible in the project list.

To make it more generic move the .git suffix removal to the callers and add an optional argument to denote visibility verification is still needed.

If there is a projects list file (GITWEB_LIST) only projects from this list are returned anyway, so no more checks needed.

If there is no projects list file and the caller requests strict checking (GITWEB_STRICT_EXPORT), do not jump directly to the given directory but instead do a normal search and filter the results instead.

The only (hopefully non-existing) effect of GITWEB_STRICT_EXPORT without GITWEB_LIST is to make sure no project can be viewed without also be found starting from project root. git_get_projects_list without this patch does not enforce this but all callers only call it with a filter already checked this way. With this parameter a caller can request this check if the filter cannot be checked this way.

Signed-off-by: Bernhard R. Link <brlink@debian.org>
---
Changes to v4:
	- split patch in smaller parts
	- move ".git" suffix removal from filters to forks specific code
          (if you want this as patch on top of the previous series, let me know)
	- improve the descriptions of all patches
---
 gitweb/gitweb.perl |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)
Show changes to gitweb/gitweb.perl +8 −5
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 9cf7e71..acf1bae 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2829,10 +2829,9 @@ sub git_get_project_url_list {
 
 sub git_get_projects_list {
 	my $filter = shift || '';
+	my $paranoid = shift;
 	my @list;
 
-	$filter =~ s/\.git$//;
-
 	if (-d $projects_list) {
 		# search in directory
 		my $dir = $projects_list;
@@ -2841,7 +2840,7 @@ sub git_get_projects_list {
 		my $pfxlen = length("$dir");
 		my $pfxdepth = ($dir =~ tr!/!!);
 		# when filtering, search only given subdirectory
-		if ($filter) {
+		if ($filter and not $paranoid) {
 			$dir .= "/$filter";
 			$dir =~ s!/+$!!;
 		}
@@ -2866,6 +2865,10 @@ sub git_get_projects_list {
 				}
 
 				my $path = substr($File::Find::name, $pfxlen + 1);
+				# paranoidly only filter here
+				if ($paranoid && $filter && $path !~ m!^\Q$filter\E/!) {
+					next;
+				}
 				# we check related file in $projectroot
 				if (check_export_ok("$projectroot/$path")) {
 					push @list, { path => $path };
@@ -6007,7 +6010,7 @@ sub git_forks {
 		die_error(400, "Unknown order parameter");
 	}
 
-	my @list = git_get_projects_list($project);
+	my @list = git_get_projects_list($project =~ s/\.git$//r);
 	if (!@list) {
 		die_error(404, "No forks found");
 	}
@@ -6066,7 +6069,7 @@ sub git_summary {
 
 	if ($check_forks) {
 		# find forks of a project
-		@forklist = git_get_projects_list($project);
+		@forklist = git_get_projects_list($project =~ s/\.git$//r);
 		# filter out forks of forks
 		@forklist = filter_forks_from_projects_list(\@forklist)
 			if (@forklist);
-- 
1.7.8.3
Jakub Narebski· Jan 30, 2012, 13:42 UTC · re: Bernhard R. Link · lore

Re: [PATCH v5 1/5] gitweb: prepare git_get_projects_list for use outside 'forks'.

Bernhard R. Link wrote:
Show 10 quoted lines
> @@ -6066,7 +6069,7 @@ sub git_summary {
>  
>  	if ($check_forks) {
>  		# find forks of a project
> -		@forklist = git_get_projects_list($project);
> +		@forklist = git_get_projects_list($project =~ s/\.git$//r);
>  		# filter out forks of forks
>  		@forklist = filter_forks_from_projects_list(\@forklist)
>  			if (@forklist);
> -- 

The '/r' non-destructive modifier for regexp replacement is quite new invention and requires Perl 5.14, while gitweb requires Perl 5.8.x something. Please don't use it.

You can use this instead.
  (my $filter = $project) =~ s/\.git$//
-- 
Jakub Narebski
Poland
Bernhard R. Link· Jan 30, 2012, 14:55 UTC · re: Jakub Narebski · lore

[PATCH v5.5 1/5] gitweb: prepare git_get_projects_list for use outside 'forks'.

Use of the filter option of git_get_projects_list is currently limited to forks. It hard codes removal of ".git" suffixes from the filter and assumes the project belonging to the filter directory was already validated to be visible in the project list.

To make it more generic move the .git suffix removal to the callers and add an optional argument to denote visibility verification is still needed.

If there is a projects list file (GITWEB_LIST) only projects from this list are returned anyway, so no more checks needed.

If there is no projects list file and the caller requests strict checking (GITWEB_STRICT_EXPORT), do not jump directly to the given directory but instead do a normal search and filter the results instead.

The only (hopefully non-existing) effect of GITWEB_STRICT_EXPORT without GITWEB_LIST is to make sure no project can be viewed without also be found starting from project root. git_get_projects_list without this patch does not enforce this but all callers only call it with a filter already checked this way. With this parameter a caller can request this check if the filter cannot be checked this way.

Signed-off-by: Bernhard R. Link <brlink@debian.org>
---
Changes since v5:
	- don't you use s/.../.../r
 gitweb/gitweb.perl |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)
Show changes to gitweb/gitweb.perl +8 −5
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 9cf7e71..19daabc 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2829,10 +2829,9 @@ sub git_get_project_url_list {
 
 sub git_get_projects_list {
 	my $filter = shift || '';
+	my $paranoid = shift;
 	my @list;
 
-	$filter =~ s/\.git$//;
-
 	if (-d $projects_list) {
 		# search in directory
 		my $dir = $projects_list;
@@ -2841,7 +2840,7 @@ sub git_get_projects_list {
 		my $pfxlen = length("$dir");
 		my $pfxdepth = ($dir =~ tr!/!!);
 		# when filtering, search only given subdirectory
-		if ($filter) {
+		if ($filter and not $paranoid) {
 			$dir .= "/$filter";
 			$dir =~ s!/+$!!;
 		}
@@ -2866,6 +2865,10 @@ sub git_get_projects_list {
 				}
 
 				my $path = substr($File::Find::name, $pfxlen + 1);
+				# paranoidly only filter here
+				if ($paranoid && $filter && $path !~ m!^\Q$filter\E/!) {
+					next;
+				}
 				# we check related file in $projectroot
 				if (check_export_ok("$projectroot/$path")) {
 					push @list, { path => $path };
@@ -6007,7 +6010,7 @@ sub git_forks {
 		die_error(400, "Unknown order parameter");
 	}
 
-	my @list = git_get_projects_list($project);
+	my @list = git_get_projects_list((my $filter = $project) =~ s/\.git$//);
 	if (!@list) {
 		die_error(404, "No forks found");
 	}
@@ -6066,7 +6069,7 @@ sub git_summary {
 
 	if ($check_forks) {
 		# find forks of a project
-		@forklist = git_get_projects_list($project);
+		@forklist = git_get_projects_list((my $filter = $project) =~ s/\.git$//);
 		# filter out forks of forks
 		@forklist = filter_forks_from_projects_list(\@forklist)
 			if (@forklist);
-- 
1.7.8.3
Jakub Narebski· Jan 30, 2012, 15:40 UTC · re: Bernhard R. Link · lore

Re: [PATCH v5.5 1/5] gitweb: prepare git_get_projects_list for use outside 'forks'.

On Mon, 30 Jul 2012, Bernhard R. Link wrote:
Show 8 quoted lines
> Use of the filter option of git_get_projects_list is currently
> limited to forks. It hard codes removal of ".git" suffixes from
> the filter and assumes the project belonging to the filter directory
> was already validated to be visible in the project list.
> 
> To make it more generic move the .git suffix removal to the callers
> and add an optional argument to denote visibility verification is
> still needed.
Even better for patch readability would be to split this patch further,
with the first part just moving removal of ".git" suffix from said
function to callers.
 
Show 14 quoted lines
> If there is a projects list file (GITWEB_LIST) only projects from
> this list are returned anyway, so no more checks needed.
> 
> If there is no projects list file and the caller requests strict
> checking (GITWEB_STRICT_EXPORT), do not jump directly to the
> given directory but instead do a normal search and filter the
> results instead.
> 
> The only (hopefully non-existing) effect of GITWEB_STRICT_EXPORT
> without GITWEB_LIST is to make sure no project can be viewed without
> also be found starting from project root. git_get_projects_list without
> this patch does not enforce this but all callers only call it with
> a filter already checked this way. With this parameter a caller
> can request this check if the filter cannot be checked this way.

O.K. now I see where the "paranoid mode" might make difference: if one of intermediate directories in $project_filter subdirectory has search/access permission ('x' bit) but is not readable ('r' bit), then gitweb would show nothing in $strict_export mode, but scan from "$projects_list/$project_filter" in non-strict mode.

Perhaps there are other cases...
 
Show 6 quoted lines
> @@ -2841,7 +2840,7 @@ sub git_get_projects_list {
>  		my $pfxlen = length("$dir");
>  		my $pfxdepth = ($dir =~ tr!/!!);
>  		# when filtering, search only given subdirectory
> -		if ($filter) {
> +		if ($filter and not $paranoid) {

Hmmmm... ($filter and !$paranoid) or ($filter && !$paranoid)? Which would be more Perl-ish and fit current code style better...

-- 
Jakub Narebski
Poland
Bernhard R. Link· Jan 30, 2012, 16:29 UTC · re: Jakub Narebski · lore

Re: [PATCH v5.5 1/5] gitweb: prepare git_get_projects_list for use outside 'forks'.

* Jakub Narebski <jnareb@gmail.com> [120130 16:40]:
Show 11 quoted lines
> Perhaps there are other cases...
>  
> > @@ -2841,7 +2840,7 @@ sub git_get_projects_list {
> >  		my $pfxlen = length("$dir");
> >  		my $pfxdepth = ($dir =~ tr!/!!);
> >  		# when filtering, search only given subdirectory
> > -		if ($filter) {
> > +		if ($filter and not $paranoid) {
> 
> Hmmmm... ($filter and !$paranoid) or ($filter && !$paranoid)?
> Which would be more Perl-ish and fit current code style better...

I cannot say what is more perlish, but gitweb.perl seems to contain only the combinations "and not" (1 time) and "&& !" (8 times).

	Bernhard R. Link
Bernhard R. Link· Jan 30, 2012, 11:45 UTC · re: Bernhard R. Link · lore

[PATCH v5 2/5] gitweb: add project_filter to limit project list to a subdirectory

This commit changes the project listing views (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.

The implementation of the filter reuses the implementation used for the 'forks' action (i.e. listing all projects within that directory from the projects list file (GITWEB_LIST) or only projects in the given subdirectory of the project root directory without a projects list file).

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.

Signed-off-by: Bernhard R. Link <brlink@debian.org>
---
 gitweb/gitweb.perl |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)
Show changes to gitweb/gitweb.perl +12 −4
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index acf1bae..36efc10 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -760,6 +760,7 @@ our @cgi_param_mapping = (
 	search_use_regexp => "sr",
 	ctag => "by_tag",
 	diff_style => "ds",
+	project_filter => "pf",
 	# this must be last entry (for manipulation from JavaScript)
 	javascript => "js"
 );
@@ -976,7 +977,7 @@ sub evaluate_path_info {
 
 our ($action, $project, $file_name, $file_parent, $hash, $hash_parent, $hash_base,
      $hash_parent_base, @extra_options, $page, $searchtype, $search_use_regexp,
-     $searchtext, $search_regexp);
+     $searchtext, $search_regexp, $project_filter);
 sub evaluate_and_validate_params {
 	our $action = $input_params{'action'};
 	if (defined $action) {
@@ -994,6 +995,13 @@ sub evaluate_and_validate_params {
 		}
 	}
 
+	our $project_filter = $input_params{'project_filter'};
+	if (defined $project_filter) {
+		if (!validate_pathname($project_filter)) {
+			die_error(404, "Invalid project_filter parameter");
+		}
+	}
+
 	our $file_name = $input_params{'file_name'};
 	if (defined $file_name) {
 		if (!validate_pathname($file_name)) {
@@ -5984,7 +5992,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");
 	}
@@ -6023,7 +6031,7 @@ sub git_forks {
 }
 
 sub git_project_index {
-	my @projects = git_get_projects_list();
+	my @projects = git_get_projects_list($project_filter, $strict_export);
 	if (!@projects) {
 		die_error(404, "No projects found");
 	}
@@ -7860,7 +7868,7 @@ sub git_atom {
 }
 
 sub git_opml {
-	my @list = git_get_projects_list();
+	my @list = git_get_projects_list($project_filter, $strict_export);
 	if (!@list) {
 		die_error(404, "No projects found");
 	}
-- 
1.7.8.3
Jakub Narebski· Jan 30, 2012, 15:57 UTC · re: Bernhard R. Link · lore

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

On Mon, 30 Jan 2012, Bernhard R. Link wrote:
> This commit changes the project listing views (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.

It would be nice to have in this or in a separate commit an update to get_page_title() for HTML output, and to git_opml() updating <title> element in OPML output, so that it mentions that project list is limitied to $project_filter subdirectory.

For plain text output of git_project_index() nothing really can be
done -- there is no title.  Well, we could fiddle with 'filename'
part of Content-Disposition HTTP header...
 
Show 5 quoted lines
> The implementation of the filter reuses the implementation used for
> the 'forks' action (i.e. listing all projects within that directory
> from the projects list file (GITWEB_LIST) or only projects in the
> given subdirectory of the project root directory without a projects
> list file).

O.K., more detailed description of $strict_export interaction is in that other commit.

> 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.
Nb. I wonder if we should make it invalid to have both 'project' and
'project_filter' parameters...
 
Show 11 quoted lines
> @@ -994,6 +995,13 @@ sub evaluate_and_validate_params {
>  		}
>  	}
>  
> +	our $project_filter = $input_params{'project_filter'};
> +	if (defined $project_filter) {
> +		if (!validate_pathname($project_filter)) {
> +			die_error(404, "Invalid project_filter parameter");
> +		}
> +	}
> +

That accidentally makes "pf=foo/" (with trailing slash) invalid. On the other hand being able to assume that $project_filter doesn't end in '/' simplifies code a bit.

> @@ -5984,7 +5992,7 @@ sub git_project_list {
> -	my @list = git_get_projects_list();
> +	my @list = git_get_projects_list($project_filter, $strict_export);
>  sub git_project_index {
> -	my @projects = git_get_projects_list();
> +	my @projects = git_get_projects_list($project_filter, $strict_export);
>  sub git_opml {
> -	my @list = git_get_projects_list();
> +	my @list = git_get_projects_list($project_filter, $strict_export);
Nice!
-- 
Jakub Narebski
Poland
Bernhard R. Link· Jan 30, 2012, 20:03 UTC · re: Jakub Narebski · lore

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

* Jakub Narebski <jnareb@gmail.com> [120130 16:56]:
Show 11 quoted lines
> On Mon, 30 Jan 2012, Bernhard R. Link wrote:
> 
> > This commit changes the project listing views (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.
> 
> It would be nice to have in this or in a separate commit an update
> to get_page_title() for HTML output, and to git_opml() updating
> <title> element in OPML output, so that it mentions that project
> list is limitied to $project_filter subdirectory.
Indeed. I overlooked that.
Show 7 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.
>
> Nb. I wonder if we should make it invalid to have both 'project' and
> 'project_filter' parameters...

$project_filter should be ignored when $project is defined which is enforced in all but those three actions.

action=project_list gets confused (shows wrong breadcrumbs) if $project is defined, but that is unrelated to this changes, so one might to fix that independently.

I'll resend the series as replies to this mail. What to do next? Wait foranother explitit Acked-By of those? Or resend it to gitster@pobox.com if no new issues are found?

        Bernhard R. Link
Bernhard R. Link· Jan 30, 2012, 20:05 UTC · re: Bernhard R. Link · lore

[PATCH 1/6] gitweb: move hard coded .git suffix out of git_get_projects_list

Use of the filter option of git_get_projects_list is currently limited to forks. It hard codes removal of ".git" suffixes from the filter.

To make it more generic move the .git suffix removal to the callers.
Signed-off-by: Bernhard R. Link <brlink@debian.org>
---
Changes to v5.5:
	- split first patch in two as suggested by Jakub Narebski
---
 gitweb/gitweb.perl |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)
Show changes to gitweb/gitweb.perl +2 −4
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 9cf7e71..0ee3290 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2831,8 +2831,6 @@ sub git_get_projects_list {
 	my $filter = shift || '';
 	my @list;
 
-	$filter =~ s/\.git$//;
-
 	if (-d $projects_list) {
 		# search in directory
 		my $dir = $projects_list;
@@ -6007,7 +6005,7 @@ sub git_forks {
 		die_error(400, "Unknown order parameter");
 	}
 
-	my @list = git_get_projects_list($project);
+	my @list = git_get_projects_list((my $filter = $project) =~ s/\.git$//);
 	if (!@list) {
 		die_error(404, "No forks found");
 	}
@@ -6066,7 +6064,7 @@ sub git_summary {
 
 	if ($check_forks) {
 		# find forks of a project
-		@forklist = git_get_projects_list($project);
+		@forklist = git_get_projects_list((my $filter = $project) =~ s/\.git$//);
 		# filter out forks of forks
 		@forklist = filter_forks_from_projects_list(\@forklist)
 			if (@forklist);
-- 
1.7.8.3
Bernhard R. Link· Jan 30, 2012, 20:06 UTC · re: Bernhard R. Link · lore

[PATCH v6 2/6] gitweb: prepare git_get_projects_list for use outside 'forks'.

Use of the filter option of git_get_projects_list is currently limited to forks. It currently assumes the project belonging to the filter directory was already validated to be visible in the project list.

To make it more generic add an optional argument to denote visibility verification is still needed.

If there is a projects list file (GITWEB_LIST) only projects from this list are returned anyway, so no more checks needed.

If there is no projects list file and the caller requests strict checking (GITWEB_STRICT_EXPORT), do not jump directly to the given directory but instead do a normal search and filter the results instead.

The only effect of GITWEB_STRICT_EXPORT without GITWEB_LIST is to make sure no project can be viewed without also be found starting from project root. git_get_projects_list without this patch does not enforce this but all callers only call it with a filter already checked this way. With this parameter a caller can request this check if the filter cannot be checked this way.

Signed-off-by: Bernhard R. Link <brlink@debian.org>
---
Changes to v5:
	- split first patch in two as suggested by Jakub Narebski
	- replace "and not" with the more common "&& !"
---
 gitweb/gitweb.perl |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)
Show changes to gitweb/gitweb.perl +6 −1
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 0ee3290..9a296e2 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2829,6 +2829,7 @@ sub git_get_project_url_list {
 
 sub git_get_projects_list {
 	my $filter = shift || '';
+	my $paranoid = shift;
 	my @list;
 
 	if (-d $projects_list) {
@@ -2839,7 +2840,7 @@ sub git_get_projects_list {
 		my $pfxlen = length("$dir");
 		my $pfxdepth = ($dir =~ tr!/!!);
 		# when filtering, search only given subdirectory
-		if ($filter) {
+		if ($filter && !$paranoid) {
 			$dir .= "/$filter";
 			$dir =~ s!/+$!!;
 		}
@@ -2864,6 +2865,10 @@ sub git_get_projects_list {
 				}
 
 				my $path = substr($File::Find::name, $pfxlen + 1);
+				# paranoidly only filter here
+				if ($paranoid && $filter && $path !~ m!^\Q$filter\E/!) {
+					next;
+				}
 				# we check related file in $projectroot
 				if (check_export_ok("$projectroot/$path")) {
 					push @list, { path => $path };
-- 
1.7.8.3
Bernhard R. Link· Jan 30, 2012, 20:07 UTC · re: Bernhard R. Link · lore

[PATCH v6 3/6] gitweb: add project_filter to limit project list to a subdirectory

This commit changes the project listing views (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.

The implementation of the filter reuses the implementation used for the 'forks' action (i.e. listing all projects within that directory from the projects list file (GITWEB_LIST) or only projects in the given subdirectory of the project root directory without a projects list file).

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.

Signed-off-by: Bernhard R. Link <brlink@debian.org>
---
changed since v5.5:
	- change page titles to show what directory it is limited to
---
 gitweb/gitweb.perl |   31 +++++++++++++++++++++++++------
 1 files changed, 25 insertions(+), 6 deletions(-)
Show changes to gitweb/gitweb.perl +25 −6
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 9a296e2..b895f4c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -760,6 +760,7 @@ our @cgi_param_mapping = (
 	search_use_regexp => "sr",
 	ctag => "by_tag",
 	diff_style => "ds",
+	project_filter => "pf",
 	# this must be last entry (for manipulation from JavaScript)
 	javascript => "js"
 );
@@ -976,7 +977,7 @@ sub evaluate_path_info {
 
 our ($action, $project, $file_name, $file_parent, $hash, $hash_parent, $hash_base,
      $hash_parent_base, @extra_options, $page, $searchtype, $search_use_regexp,
-     $searchtext, $search_regexp);
+     $searchtext, $search_regexp, $project_filter);
 sub evaluate_and_validate_params {
 	our $action = $input_params{'action'};
 	if (defined $action) {
@@ -994,6 +995,13 @@ sub evaluate_and_validate_params {
 		}
 	}
 
+	our $project_filter = $input_params{'project_filter'};
+	if (defined $project_filter) {
+		if (!validate_pathname($project_filter)) {
+			die_error(404, "Invalid project_filter parameter");
+		}
+	}
+
 	our $file_name = $input_params{'file_name'};
 	if (defined $file_name) {
 		if (!validate_pathname($file_name)) {
@@ -3734,7 +3742,12 @@ sub run_highlighter {
 sub get_page_title {
 	my $title = to_utf8($site_name);
 
-	return $title unless (defined $project);
+	unless (defined $project) {
+		if (defined $project_filter) {
+			$title .= " - " . to_utf8($project_filter);
+		}
+		return $title;
+	}
 	$title .= " - " . to_utf8($project);
 
 	return $title unless (defined $action);
@@ -5984,7 +5997,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");
 	}
@@ -6023,7 +6036,7 @@ sub git_forks {
 }
 
 sub git_project_index {
-	my @projects = git_get_projects_list();
+	my @projects = git_get_projects_list($project_filter, $strict_export);
 	if (!@projects) {
 		die_error(404, "No projects found");
 	}
@@ -7860,7 +7873,7 @@ sub git_atom {
 }
 
 sub git_opml {
-	my @list = git_get_projects_list();
+	my @list = git_get_projects_list($project_filter, $strict_export);
 	if (!@list) {
 		die_error(404, "No projects found");
 	}
@@ -7871,11 +7884,17 @@ sub git_opml {
 		-content_disposition => 'inline; filename="opml.xml"');
 
 	my $title = esc_html($site_name);
+	my $filter = " within subdirectory ";
+	if (defined $project_filter) {
+		$filter .= esc_html($project_filter);
+	} else {
+		$filter = "";
+	}
 	print <<XML;
 <?xml version="1.0" encoding="utf-8"?>
 <opml version="1.0">
 <head>
-  <title>$title OPML Export</title>
+  <title>$title OPML Export$filter</title>
 </head>
 <body>
 <outline text="git RSS feeds">
-- 
1.7.8.3
Bernhard R. Link· Jan 30, 2012, 20:09 UTC · re: Bernhard R. Link · lore

[PATCH v6 4/6] gitweb: limit links to alternate forms of project_list to active project_filter

If project_list action is given a project_filter argument, pass that to TXT and OPML formats.

This way [OPML] and [TXT] links provide the same list of projects as the projects_list page they are linked from.

Signed-off-by: Bernhard R. Link <brlink@debian.org>
---
Changes since v5:
	add additional description paragraph from Jakub Narebski
---
 gitweb/gitweb.perl |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
Show changes to gitweb/gitweb.perl +4 −2
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index b895f4c..9299504 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3981,9 +3981,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";
 	}
 	print "</div>\n"; # class="page_footer"
-- 
1.7.8.3
Bernhard R. Link· Jan 30, 2012, 20:09 UTC · re: Bernhard R. Link · lore

[PATCH v6 5/6] gitweb: show active project_filter in project_list page header

In the page header of a project_list view with a project_filter given show breadcrumbs in the page headers showing which directory it is currently limited to and also containing links to the parent directories.

Signed-off-by: Bernhard R. Link <brlink@debian.org>
---
Changes since v5:
	- improve description, better?
	- equalize whitespace
---
 gitweb/gitweb.perl |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)
Show changes to gitweb/gitweb.perl +14 −0
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 9299504..27db84e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3841,6 +3841,18 @@ sub print_header_links {
 	}
 }
 
+sub print_nav_breadcrumbs_path {
+	my $dirprefix = undef;
+	while (my $part = shift) {
+		$dirprefix .= "/" if defined $dirprefix;
+		$dirprefix .= $part;
+		print $cgi->a({-href => href(project => undef,
+		                             project_filter => $dirprefix,
+		                             action => "project_list")},
+			      esc_html($part)) . " / ";
+	}
+}
+
 sub print_nav_breadcrumbs {
 	my %opts = @_;
 
@@ -3859,6 +3871,8 @@ sub print_nav_breadcrumbs {
 			print " / $opts{-action_extra}";
 		}
 		print "\n";
+	} elsif (defined $project_filter) {
+		print_nav_breadcrumbs_path(split '/', $project_filter);
 	}
 }
 
-- 
1.7.8.3
Bernhard R. Link· Jan 30, 2012, 20:10 UTC · re: Bernhard R. Link · lore

[PATCH v6 6/6] gitweb: place links to parent directories in page header

Change html page headers to not only link the project root and the currently selected project but also the directories in between using project_filter. (Allowing to jump to a list of all projects within that intermediate directory directly and making the project_filter feature visible to users).

Signed-off-by: Bernhard R. Link <brlink@debian.org>
Acked-by: Jakub Narebski <jnareb@gmail.com>
---
What are the rules for copying Acked-by? This change and it's
description are unchanged since v4 which got a Acked-by. Do
I keep that Acked-by if only the other patches change or do I reset
it?
---
 gitweb/gitweb.perl |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
Show changes to gitweb/gitweb.perl +4 −1
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 27db84e..c45e0e7 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3858,7 +3858,10 @@ sub print_nav_breadcrumbs {
 
 	print $cgi->a({-href => esc_url($home_link)}, $home_link_str) . " / ";
 	if (defined $project) {
-		print $cgi->a({-href => href(action=>"summary")}, esc_html($project));
+		my @dirname = split '/', $project;
+		my $projectbasename = pop @dirname;
+		print_nav_breadcrumbs_path(@dirname);
+		print $cgi->a({-href => href(action=>"summary")}, esc_html($projectbasename));
 		if (defined $action) {
 			my $action_print = $action ;
 			if (defined $opts{-action_extra}) {
-- 
1.7.8.3
Junio C Hamano· Jan 30, 2012, 20:34 UTC · re: Bernhard R. Link · lore

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

"Bernhard R. Link" <brl@mail.brlink.eu> writes:
> I'll resend the series as replies to this mail.

Thanks; I'll queue them in 'pu' for now (if Jakub wants to Ack the pieces, I'll amend them).

Regarding the first patch in the series, while it may be a valid perl to introduce a new variable, assign to it and then munge its contents with s///, all inside a parameter list of a function call, it is doing a bit too much and makes it hard to see if the variable may or may not later be used in the same scope (in this case, it is not).

I am tempted to squash the following in.
Show changes to gitweb/gitweb.perl +4 −2
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index b764d51..f215eaa 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -6003,7 +6003,8 @@ sub git_forks {
 		die_error(400, "Unknown order parameter");
 	}
 
-	my @list = git_get_projects_list((my $filter = $project) =~ s/\.git$//);
+	my ($filter = $project) =~ s/\.git$//;
+	my @list = git_get_projects_list($filter);
 	if (!@list) {
 		die_error(404, "No forks found");
 	}
@@ -6062,7 +6063,8 @@ sub git_summary {
 
 	if ($check_forks) {
 		# find forks of a project
-		@forklist = git_get_projects_list((my $filter = $project) =~ s/\.git$//);
+		my ($filter = $project) =~ s/\.git$//;
+		@forklist = git_get_projects_list($filter);
 		# filter out forks of forks
 		@forklist = filter_forks_from_projects_list(\@forklist)
 			if (@forklist);
-- 
1.7.9.154.g413bff
Jakub Narebski· Jan 30, 2012, 20:48 UTC · re: Junio C Hamano · lore

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

On Mon, 30 Jan 2012, Junio C Hamano wrote:
Show 6 quoted lines
> "Bernhard R. Link" <brl@mail.brlink.eu> writes:
> 
> > I'll resend the series as replies to this mail.
> 
> Thanks; I'll queue them in 'pu' for now (if Jakub wants to Ack the pieces,
> I'll amend them).
You can add Ack from me for the whole series.
Show 18 quoted lines
> Regarding the first patch in the series, while it may be a valid perl to
> introduce a new variable, assign to it and then munge its contents with
> s///, all inside a parameter list of a function call, it is doing a bit
> too much and makes it hard to see if the variable may or may not later be
> used in the same scope (in this case, it is not).
> 
> I am tempted to squash the following in.
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index b764d51..f215eaa 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -6003,7 +6003,8 @@ sub git_forks {
>  		die_error(400, "Unknown order parameter");
>  	}
>  
> -	my @list = git_get_projects_list((my $filter = $project) =~ s/\.git$//);
> +	my ($filter = $project) =~ s/\.git$//;
This doesn't work: it is syntax error:
  Can't declare scalar assignment in "my"
  
It has to be either
 +	(my $filter = $project) =~ s/\.git$//;
or
 +	my $filter = $project;
 +	$filter =~ s/\.git$//;
-- 
Jakub Narebski
Poland
Junio C Hamano· Jan 30, 2012, 21:05 UTC · re: Jakub Narebski · lore

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

Jakub Narebski <jnareb@gmail.com> writes:
Show 10 quoted lines
>> -	my @list = git_get_projects_list((my $filter = $project) =~ s/\.git$//);
>> +	my ($filter = $project) =~ s/\.git$//;
>
> This doesn't work: it is syntax error:
>
>   Can't declare scalar assignment in "my"
>   
> It has to be either
>
>  +	(my $filter = $project) =~ s/\.git$//;
Sorry, that is what I meant.
Junio C Hamano· Jan 30, 2012, 21:08 UTC · re: Jakub Narebski · lore

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

Jakub Narebski <jnareb@gmail.com> writes:
Show 9 quoted lines
> On Mon, 30 Jan 2012, Junio C Hamano wrote:
>> "Bernhard R. Link" <brl@mail.brlink.eu> writes:
>> 
>> > I'll resend the series as replies to this mail.
>> 
>> Thanks; I'll queue them in 'pu' for now (if Jakub wants to Ack the pieces,
>> I'll amend them).
>
> You can add Ack from me for the whole series.
Ok, amended and queued (but not pushed out yet).
Bernhard R. Link· Jan 30, 2012, 20:48 UTC · re: Junio C Hamano · lore

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

* Junio C Hamano <gitster@pobox.com> [120130 21:34]:
Show 6 quoted lines
> "Bernhard R. Link" <brl@mail.brlink.eu> writes:
> Regarding the first patch in the series, while it may be a valid perl to
> introduce a new variable, assign to it and then munge its contents with
> s///, all inside a parameter list of a function call, it is doing a bit
> too much and makes it hard to see if the variable may or may not later be
> used in the same scope (in this case, it is not).

I'm fine either way. I had interpreted <201201301442.06707.jnareb@gmail.com> to be meant this way, but rereading it I am not sure it was meant this way at all. I thought this was to express that those variables are not used outside this scope.

        Bernhard R. Link
Bernhard R. Link· Feb 1, 2012, 16:59 UTC · re: Junio C Hamano · lore

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

* Junio C Hamano <gitster@pobox.com> [120130 21:34]:
Show 10 quoted lines
> Thanks; I'll queue them in 'pu' for now (if Jakub wants to Ack the pieces,
> I'll amend them).
>
> Regarding the first patch in the series, while it may be a valid perl to
> introduce a new variable, assign to it and then munge its contents with
> s///, all inside a parameter list of a function call, it is doing a bit
> too much and makes it hard to see if the variable may or may not later be
> used in the same scope (in this case, it is not).
>
> I am tempted to squash the following in.
Look liks a change like that is actually needed. I made the mistake of
assuming
  (my $filter = $project) =~ s/\.git$//;
was the same like
  $project =~ s/\.git$//r;
but the latter returns the changed string, the former returns the number
of arguments. (So it looks for forks in a directory named '1').
(Should have tested it again after this last change)...

Can you squash it in (with the correction of Jakub Narebski), or do you prefer a new patch?

        Bernhard R. Link
Junio C Hamano· Feb 1, 2012, 20:55 UTC · re: Bernhard R. Link · lore

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

"Bernhard R. Link" <brl+git@mail.brlink.eu> writes:
> Look liks a change like that is actually needed...
> ... So it looks for forks in a directory named '1'

Yeah, that was exactly what was causing failures in 9502. Fixed locally so no further action is required.

Thanks.
Bernhard R. Link· Jan 30, 2012, 11:47 UTC · re: Bernhard R. Link · lore

[PATCH 3/5] gitweb: limit links to alternate forms of project_list to active project_filter

If project_list action is given a project_filter argument, pass that to TXT and OPML formats.

Signed-off-by: Bernhard R. Link <brlink@debian.org>
---
 gitweb/gitweb.perl |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
Show changes to gitweb/gitweb.perl +4 −2
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 36efc10..e022e11 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3976,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";
 	}
 	print "</div>\n"; # class="page_footer"
-- 
1.7.8.3
Jakub Narebski· Jan 30, 2012, 16:09 UTC · re: Bernhard R. Link · lore

Re: [PATCH 3/5] gitweb: limit links to alternate forms of project_list to active project_filter

On Mon, 30 Jan 2012, Bernhard R. Link wrote:
> If project_list action is given a project_filter argument, pass that to
> TXT and OPML formats.
Nice.

This way [OPML] and [TXT] links provide the same list of projects as the projects_list page they are linked from.

Show 22 quoted lines
> Signed-off-by: Bernhard R. Link <brlink@debian.org>
> ---
>  gitweb/gitweb.perl |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 36efc10..e022e11 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3976,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";
>  	}
Nicely short.
-- 
Jakub Narebski
Poland
Bernhard R. Link· Jan 30, 2012, 11:48 UTC · re: Bernhard R. Link · lore

[PATCH v5 4/5] gitweb: show active project_filter in project_list page header

In a project_list view show breadcrumbs with the currently active project_filter (and those of parent directories) in the page header.

Signed-off-by: Bernhard R. Link <brlink@debian.org>
---
 gitweb/gitweb.perl |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)
Show changes to gitweb/gitweb.perl +14 −0
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e022e11..dfc79df 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3836,6 +3836,18 @@ sub print_header_links {
 	}
 }
 
+sub print_nav_breadcrumbs_path {
+	my $dirprefix = undef;
+	while (my $part = shift) {
+		$dirprefix .= "/" if defined $dirprefix;
+		$dirprefix .= $part;
+		print $cgi->a({-href => href(project => undef,
+		                             project_filter => $dirprefix,
+					     action=>"project_list")},
+			      esc_html($part)) . " / ";
+	}
+}
+
 sub print_nav_breadcrumbs {
 	my %opts = @_;
 
@@ -3854,6 +3866,8 @@ sub print_nav_breadcrumbs {
 			print " / $opts{-action_extra}";
 		}
 		print "\n";
+	} elsif (defined $project_filter) {
+		print_nav_breadcrumbs_path(split '/', $project_filter);
 	}
 }
 
-- 
1.7.8.3
Jakub Narebski· Jan 30, 2012, 16:38 UTC · re: Bernhard R. Link · lore

Re: [PATCH v5 4/5] gitweb: show active project_filter in project_list page header

On Mon, 30 Jan 2012, Bernhard R. Link wrote:
> In a project_list view show breadcrumbs with the currently active
> project_filter (and those of parent directories) in the page header.
O.K. (though I'd prefer written it less concise and more clear).
 
Show 16 quoted lines
> Signed-off-by: Bernhard R. Link <brlink@debian.org>
> ---
>  gitweb/gitweb.perl |   14 ++++++++++++++
>  1 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index e022e11..dfc79df 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3836,6 +3836,18 @@ sub print_header_links {
>  	}
>  }
>  
> +sub print_nav_breadcrumbs_path {
> +	my $dirprefix = undef;
> +	while (my $part = shift) {
Hmmm... using agument list directly, without copying it?  Well, all right.
Show 5 quoted lines
> +		$dirprefix .= "/" if defined $dirprefix;
> +		$dirprefix .= $part;
> +		print $cgi->a({-href => href(project => undef,
> +		                             project_filter => $dirprefix,
> +					     action=>"project_list")},
Minor nitpick: Let's use same whitespace rules for all key-value pairs
  +					     action => "project_list")},
Show 15 quoted lines
> +			      esc_html($part)) . " / ";
> +	}
> +}
> +
>  sub print_nav_breadcrumbs {
>  	my %opts = @_;
>  
> @@ -3854,6 +3866,8 @@ sub print_nav_breadcrumbs {
>  			print " / $opts{-action_extra}";
>  		}
>  		print "\n";
> +	} elsif (defined $project_filter) {
> +		print_nav_breadcrumbs_path(split '/', $project_filter);
>  	}
>  }
Nice!
-- 
Jakub Narebski
Poland
Bernhard R. Link· Jan 30, 2012, 11:50 UTC · re: Bernhard R. Link · lore

[PATCH v5 5/5] gitweb: place links to parent directories in page header

Change html page headers to not only link the project root and the currently selected project but also the directories in between using project_filter. (Allowing to jump to a list of all projects within that intermediate directory directly and making the project_filter feature visible to users).

Signed-off-by: Bernhard R. Link <brlink@debian.org>
---
 gitweb/gitweb.perl |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
Show changes to gitweb/gitweb.perl +4 −1
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index dfc79df..b54ddb9 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3853,7 +3853,10 @@ sub print_nav_breadcrumbs {
 
 	print $cgi->a({-href => esc_url($home_link)}, $home_link_str) . " / ";
 	if (defined $project) {
-		print $cgi->a({-href => href(action=>"summary")}, esc_html($project));
+		my @dirname = split '/', $project;
+		my $projectbasename = pop @dirname;
+		print_nav_breadcrumbs_path(@dirname);
+		print $cgi->a({-href => href(action=>"summary")}, esc_html($projectbasename));
 		if (defined $action) {
 			my $action_print = $action ;
 			if (defined $opts{-action_extra}) {
-- 
1.7.8.3
Jakub Narebski· Jan 30, 2012, 17:10 UTC · re: Bernhard R. Link · lore

Re: [PATCH v5 5/5] gitweb: place links to parent directories in page header

On Mon, 30 Jan 2012, Bernhard R. Link wrote:
Show 5 quoted lines
> Change html page headers to not only link the project root and the
> currently selected project but also the directories in between using
> project_filter. (Allowing to jump to a list of all projects within
> that intermediate directory directly and making the project_filter
> feature visible to users).
Nice idea, nice description.
 
Show 22 quoted lines
> Signed-off-by: Bernhard R. Link <brlink@debian.org>
> ---
>  gitweb/gitweb.perl |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index dfc79df..b54ddb9 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3853,7 +3853,10 @@ sub print_nav_breadcrumbs {
>  
>  	print $cgi->a({-href => esc_url($home_link)}, $home_link_str) . " / ";
>  	if (defined $project) {
> -		print $cgi->a({-href => href(action=>"summary")}, esc_html($project));
> +		my @dirname = split '/', $project;
> +		my $projectbasename = pop @dirname;
> +		print_nav_breadcrumbs_path(@dirname);
> +		print $cgi->a({-href => href(action=>"summary")}, esc_html($projectbasename));
>  		if (defined $action) {
>  			my $action_print = $action ;
>  			if (defined $opts{-action_extra}) {
> -- 
Nice code.
-- 
Jakub Narebski
Poland

← back to recent threads