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

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

[Bernhard R. Link]

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

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

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

[Bernhard R. Link]

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

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

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

[Jakub Narebski]

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

Description?

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

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

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

[Jakub Narebski]

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

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

> The change is quite minimal as git_get_projects_list already can limit
> itself to a subdirectory (though that was previously only used for
> 'forks').
>
Nice description, and more clear than before.
 
> If strict_export is enabled and there is no projects_list, it still
> traverses the full tree and only filters afterwards to avoid anything
> getting visible by this. Otherwise only the subtree needs to be
> traversed, significantly reducing load times.
>
I still don't understand interaction between project_filter ('pf'),
$strict_export and $projects_list being either directory or a file
with a list of projects.

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

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

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

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

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


A few nitpicks with respect to patch itself.

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

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

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

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

[...]

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

O.K.

-- 
Jakub Narebski

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

[Bernhard R. Link]

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]:
> "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.

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

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

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

[Jakub Narebski]

On Sun, 29 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.
> 
> 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.

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

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

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

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

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

[Bernhard R. Link]

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]:
> 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(-)

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

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

[Bernhard R. Link]

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

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

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

[Jakub Narebski]

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>

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

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

[Jakub Narebski]

On Sun, 29 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.
> 
> 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...
 
> 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>

> ---
[...]
> @@ -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

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

[Junio C Hamano]

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

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

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

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

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

[Jakub Narebski]

On Sun, 29 Jan 2012, Junio C Hamano wrote:
> "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.

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

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

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

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

[Bernhard R. Link]

* Junio C Hamano <gitster@pobox.com> [120129 22:06]:
> > @@ -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

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

[Bernhard R. Link]

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

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

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

[Jakub Narebski]

Bernhard R. Link wrote:

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

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

[Bernhard R. Link]

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

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

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

[Jakub Narebski]

On Mon, 30 Jul 2012, Bernhard R. Link wrote:

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

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

[Bernhard R. Link]

* Jakub Narebski <jnareb@gmail.com> [120130 16:40]:
> 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

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

[Bernhard R. Link]

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

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

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

[Jakub Narebski]

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

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

[Bernhard R. Link]

* Jakub Narebski <jnareb@gmail.com> [120130 16:56]:
> 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.

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

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

[Bernhard R. Link]

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

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

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

[Bernhard R. Link]

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

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

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

[Bernhard R. Link]

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

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

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

[Bernhard R. Link]

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

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

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

[Bernhard R. Link]

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

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

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

[Bernhard R. Link]

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

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

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

[Junio C Hamano]

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

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

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

[Jakub Narebski]

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.

> 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

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

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

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

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

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

[Bernhard R. Link]

* Junio C Hamano <gitster@pobox.com> [120130 21:34]:
> "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

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

[Bernhard R. Link]

* Junio C Hamano <gitster@pobox.com> [120130 21:34]:
> 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

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

[Junio C Hamano]

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

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

[Bernhard R. Link]

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

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

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

[Jakub Narebski]

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.

> 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

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

[Bernhard R. Link]

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

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

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

[Jakub Narebski]

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

> +		$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")},


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

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

[Bernhard R. Link]

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

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

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

[Jakub Narebski]

On Mon, 30 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. (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.
 
> 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