gitweb: add project_filter to limit project list to a subdirectory

PATCH In review Bernhard R. Link 4 participants 40 messages Jun 15, 2016

Share & tools

Bernhard R. Link

author

2016-06-15 22:52 UTC #0 lore raw

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

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 <[email protected]>

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

gitweb/gitweb.perl +22 -7

index abb5a79..a114bd4 100755
@@ -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");
 	}

Signature · 1 line

1.7.8.3

Bernhard R. Link

author

2016-06-15 22:52 UTC #1 lore raw

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

Signed-off-by Bernhard R. Link <[email protected]>

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

gitweb/gitweb.perl +22 -1

index a114bd4..ddce27d 100755
@@ -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)) . " / ";
+		}
 	}
 }

Signature · 1 line

1.7.8.3

Jakub Narebski

2016-06-15 22:52 UTC #2 lore raw

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

"Bernhard R. Link" <[email protected]> writes:

Description?

Quoted 7 lines

Signed-off-by Bernhard R. Link <[email protected]>
---
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.

Quoted 17 lines

 gitweb/gitweb.perl |   23 ++++++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)

gitweb/gitweb.perl +5 -1

index a114bd4..ddce27d 100755
@@ -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;

gitweb/gitweb.perl sub print_nav_breadcrumbs { · line 3858

Quoted 10 lines

gitweb/gitweb.perl +7 -0

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

Quoted 6 lines

gitweb/gitweb.perl +2 -0

@@ -3854,6 +3865,16 @@ sub print_nav_breadcrumbs {
 			print " / $opts{-action_extra}";
 		}
 		print "\n";
+	} elsif (defined $project_filter) {
+		my @dirname = split '/', $project_filter;

gitweb/gitweb.perl sub print_nav_breadcrumbs { · line 3879

Quoted 10 lines

gitweb/gitweb.perl +8 -0

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

Signature · 1 line

Jakub Narebski

Jakub Narebski

2016-06-15 22:52 UTC #3 lore raw

In reply to Bernhard R. Link

Quoted 4 lines · 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.

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

Quoted 4 lines

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.

Quoted 5 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?

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

gitweb/gitweb.perl sub git_get_project_url_list { · line 2840

Quoted 7 lines

gitweb/gitweb.perl +1 -0

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

Quoted 9 lines

gitweb/gitweb.perl +1 -1

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

[...]

gitweb/gitweb.perl sub git_footer_html { · line 3985

Quoted 13 lines

gitweb/gitweb.perl +4 -2

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

Signature · 1 line

Jakub Narebski

Bernhard R. Link

author

2016-06-15 22:52 UTC #4 lore raw

PATCH v3 1/1 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).

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 <[email protected]>

---

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

Quoted 10 lines · Jakub Narebski

Quoted 5 lines · Bernhard R. Link
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.

Quoted 6 lines

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.

Quoted 4 lines

A few nitpicks with respect to patch itself.
Quoted 2 lines
-2827,6 +2835,7 @@ sub git_get_project_url_list {

Quoted 8 lines

Review
Quoted 5 lines
sub git_get_projects_list {
(quoted diff) +1 -0
 	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.

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

 gitweb/gitweb.perl |   52 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 44 insertions(+), 8 deletions(-)

gitweb/gitweb.perl +44 -8

index abb5a79..089d45d 100755
@@ -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");
 	}

Signature · 1 line

1.7.8.3

Jakub Narebski

2016-06-15 22:52 UTC #5 lore raw

Quoted 8 lines · 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').

Nice and succinct.

Quoted 3 lines

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

O.K.

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

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

Quoted 4 lines

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.

Quoted 27 lines

Signed-off-by Bernhard R. Link <[email protected]>
---
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
Quoted 10 lines · Jakub Narebski
Quoted 5 lines · Bernhard R. Link
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.
Quoted 6 lines
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"

Quoted 8 lines

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.

Quoted 2 lines

Is the new description better.

Yes it is.

Quoted 13 lines

Quoted 4 lines
A few nitpicks with respect to patch itself.
Quoted 2 lines
-2827,6 +2835,7 @@ sub git_get_project_url_list {
Quoted 8 lines
gitweb/gitweb.perl sub git_get_project_url_list { · line 2839
Quoted 5 lines
sub git_get_projects_list {
gitweb/gitweb.perl +1 -0
 	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.

Quoted 2 lines

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

Quoted 7 lines

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

Quoted 2 lines

 gitweb/gitweb.perl |   52 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 44 insertions(+), 8 deletions(-)

Not that large for a new feature...

Signature · 2 lines

Jakub Narebski
Poland

Bernhard R. Link

author

2016-06-15 22:52 UTC #6 lore raw

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

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.

Signed-off-by Bernhard R. Link <[email protected]>

---

Quoted 8 lines · Jakub Narebski

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.

Quoted 4 lines

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

gitweb/gitweb.perl +36 -7

index abb5a79..f0e03d8 100755
@@ -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");
 	}

Signature · 1 line

1.7.8.3

Bernhard R. Link

author

2016-06-15 22:52 UTC #7 lore raw

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 <[email protected]>

---
 gitweb/gitweb.perl |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

gitweb/gitweb.perl +4 -1

index f0e03d8..e2a9146 100755
@@ -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}) {

Signature · 1 line

1.7.8.3

Jakub Narebski

Acked-by on v4 2/2

2016-06-15 22:52 UTC #8 lore raw

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

Quoted 3 lines · 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.

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.

Quoted 1 line

Signed-off-by Bernhard R. Link <[email protected]>

Acked-by Jakub Narebski <[email protected]>

Quoted 11 lines

---
 gitweb/gitweb.perl |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

gitweb/gitweb.perl +0 -0

index f0e03d8..e2a9146 100755
@@ -3853,7 +3853,10 @@ sub print_nav_breadcrumbs {

gitweb/gitweb.perl sub print_nav_breadcrumbs { · line 3862

Quoted 10 lines

gitweb/gitweb.perl +4 -1

 	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!

Signature · 2 lines

Jakub Narebski
Poland

Jakub Narebski

Acked-by on v4 1/2

2016-06-15 22:52 UTC #9 lore raw

In reply to Bernhard R. Link

Quoted 11 lines · 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.

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

I am still unsure if it is really necessary, but nevermind...

Quoted 6 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 <[email protected]>

Acked-by Jakub Narebski <[email protected]>

Quoted 1 line

---

[...]

Quoted 20 lines

gitweb/gitweb.perl +12 -0

@@ -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 = @_;

gitweb/gitweb.perl sub print_nav_breadcrumbs { · line 3873

Quoted 8 lines

gitweb/gitweb.perl +2 -0

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

Signature · 2 lines

Jakub Narebski
Poland

Junio C Hamano

2016-06-15 22:52 UTC #10 lore raw

In reply to Bernhard R. Link

Quoted 10 lines · 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.

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.

Quoted 4 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"?

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.

Quoted 10 lines

gitweb/gitweb.perl +1 -1

@@ -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!/+$!!;
 		}

gitweb/gitweb.perl sub git_get_projects_list { · line 2879

Quoted 7 lines

gitweb/gitweb.perl +4 -0

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

Quoted 16 lines

gitweb/gitweb.perl +12 -0

+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 = @_;

gitweb/gitweb.perl sub print_nav_breadcrumbs { · line 3872

Quoted 7 lines

gitweb/gitweb.perl +2 -0

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

2016-06-15 22:52 UTC #11 lore raw

Quoted 10 lines · Junio C Hamano

Quoted 8 lines · 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').

Quoted 7 lines

Quoted 2 lines
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.

Quoted 11 lines

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

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

Quoted 12 lines

gitweb/gitweb.perl +1 -1

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

Quoted 9 lines

gitweb/gitweb.perl sub git_get_projects_list { · line 2879
Quoted 5 lines
gitweb/gitweb.perl +4 -0
 				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.

Quoted 26 lines

Quoted 16 lines

gitweb/gitweb.perl +12 -0

+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 = @_;

gitweb/gitweb.perl sub print_nav_breadcrumbs { · line 3872

Quoted 7 lines

gitweb/gitweb.perl +2 -0

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

Quoted 8 lines

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.

Signature · 2 lines

Jakub Narebski
Poland

Bernhard R. Link

author

2016-06-15 22:52 UTC #12 lore raw

In reply to Junio C Hamano

Quoted 12 lines · Junio C Hamano

gitweb/gitweb.perl sub git_get_projects_list { · line 2879
Quoted 8 lines
gitweb/gitweb.perl +4 -0
@@ -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)).

Quoted 1 line

Perhaps that is the topic of your second patch. I dunno.

Yes, that is what the second patch does.

Bernhard R. Link

author

2016-06-15 22:52 UTC #13 lore raw

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 <[email protected]>

---

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

gitweb/gitweb.perl +8 -5

index 9cf7e71..acf1bae 100755
@@ -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);

Signature · 1 line

1.7.8.3

Jakub Narebski

2016-06-15 22:52 UTC #14 lore raw

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

gitweb/gitweb.perl sub git_summary { · line 6075

Quoted 10 lines · Bernhard R. Link

gitweb/gitweb.perl +1 -1

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

Signature · 2 lines

Jakub Narebski
Poland

Bernhard R. Link

author

2016-06-15 22:52 UTC #15 lore raw

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

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.

Signed-off-by Bernhard R. Link <[email protected]>

---

Changes since v5:
- don't you use s/.../.../r

 gitweb/gitweb.perl |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

gitweb/gitweb.perl +8 -5

index 9cf7e71..19daabc 100755
@@ -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);

Signature · 1 line

1.7.8.3

Jakub Narebski

2016-06-15 22:52 UTC #16 lore raw

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

Quoted 8 lines · 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.

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.

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

Quoted 6 lines

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

gitweb/gitweb.perl sub git_get_projects_list { · line 2843

Quoted 6 lines

gitweb/gitweb.perl +1 -1

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

Signature · 2 lines

Jakub Narebski
Poland

Bernhard R. Link

author

2016-06-15 22:52 UTC #17 lore raw

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

Quoted 11 lines · Jakub Narebski

Perhaps there are other cases...
gitweb/gitweb.perl sub git_get_projects_list { · line 2843
Quoted 6 lines
gitweb/gitweb.perl +1 -1
@@ -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

author

2016-06-15 22:52 UTC #18 lore raw

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

In reply to Bernhard R. Link

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

Signed-off-by Bernhard R. Link <[email protected]>

---
 gitweb/gitweb.perl |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

gitweb/gitweb.perl +12 -4

index acf1bae..36efc10 100755
@@ -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");
 	}

Signature · 1 line

1.7.8.3

Jakub Narebski

2016-06-15 22:52 UTC #19 lore raw

Quoted 4 lines · 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.

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

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

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

gitweb/gitweb.perl sub evaluate_and_validate_params { · line 1004

Quoted 11 lines

gitweb/gitweb.perl +7 -0

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

Review sub git_project_list { · line 5998

Quoted 11 lines

(quoted diff) +3 -3

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

Signature · 2 lines

Jakub Narebski
Poland

Bernhard R. Link

2016-06-15 22:52 UTC #20 lore raw

Quoted 11 lines · Jakub Narebski

Quoted 4 lines · 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.
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.

Quoted 7 lines

Quoted 4 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 [email protected] if no new issues are found?

Bernhard R. Link

author

2016-06-15 22:52 UTC #21 lore raw

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 <[email protected]>

---

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

gitweb/gitweb.perl +2 -4

index 9cf7e71..0ee3290 100755
@@ -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);

Signature · 1 line

1.7.8.3

Bernhard R. Link

author

2016-06-15 22:52 UTC #22 lore raw

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

In reply to 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.

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 <[email protected]>

---

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

gitweb/gitweb.perl +6 -1

index 0ee3290..9a296e2 100755
@@ -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 };

Signature · 1 line

1.7.8.3

Bernhard R. Link

author

2016-06-15 22:52 UTC #23 lore raw

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

In reply to Bernhard R. Link

Signed-off-by Bernhard R. Link <[email protected]>

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

gitweb/gitweb.perl +25 -6

index 9a296e2..b895f4c 100755
@@ -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">

Signature · 1 line

1.7.8.3

Bernhard R. Link

author

2016-06-15 22:52 UTC #24 lore raw

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

In reply to 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 <[email protected]>

---

Changes since v5:
add additional description paragraph from Jakub Narebski

---
 gitweb/gitweb.perl |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

gitweb/gitweb.perl +4 -2

index b895f4c..9299504 100755
@@ -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"

Signature · 1 line

1.7.8.3

Bernhard R. Link

author

2016-06-15 22:52 UTC #25 lore raw

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

In reply to 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 <[email protected]>

---
Changes since v5:
- improve description, better?
- equalize whitespace

---
 gitweb/gitweb.perl |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

gitweb/gitweb.perl +14 -0

index 9299504..27db84e 100755
@@ -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);
 	}
 }

Signature · 1 line

1.7.8.3

Bernhard R. Link

author

2016-06-15 22:52 UTC #26 lore raw

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

In reply to 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 <[email protected]>

Acked-by Jakub Narebski <[email protected]>

---

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

gitweb/gitweb.perl +4 -1

index 27db84e..c45e0e7 100755
@@ -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}) {

Signature · 1 line

1.7.8.3

Junio C Hamano

2016-06-15 22:52 UTC #27 lore raw

In reply to Bernhard R. Link

Quoted 1 line · Bernhard R. Link

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.

gitweb/gitweb.perl +4 -2

index b764d51..f215eaa 100755
@@ -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);

Signature · 1 line

1.7.9.154.g413bff

Bernhard R. Link

author

2016-06-15 22:52 UTC #28 lore raw

Quoted 6 lines · Junio C Hamano

"Bernhard R. Link" <[email protected]> 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 <[email protected]> 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

Jakub Narebski

2016-06-15 22:52 UTC #29 lore raw

In reply to Junio C Hamano

Quoted 6 lines · Junio C Hamano

Quoted 1 line · Bernhard R. Link
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.

Quoted 13 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.
gitweb/gitweb.perl +0 -0
index b764d51..f215eaa 100755

gitweb/gitweb.perl sub git_forks { · line 6006

Quoted 5 lines

gitweb/gitweb.perl +1 -1

@@ -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$//;

+ my $filter = $project;
+ $filter =~ s/\.git$//;

Signature · 2 lines

Jakub Narebski
Poland

Junio C Hamano

2016-06-15 22:52 UTC #30 lore raw

Quoted 9 lines · Jakub Narebski

Quoted 6 lines · Junio C Hamano
Quoted 1 line · Bernhard R. Link
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).

Junio C Hamano

2016-06-15 22:52 UTC #31 lore raw

In reply to Jakub Narebski

Quoted 10 lines · Jakub Narebski

gitweb/gitweb.perl sub git_forks { · line 6006
Quoted 2 lines
gitweb/gitweb.perl +1 -1
-	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.

Bernhard R. Link

author

2016-06-15 22:52 UTC #32 lore raw

In reply to Junio C Hamano

Quoted 10 lines · Junio C Hamano

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

2016-06-15 22:52 UTC #33 lore raw

Quoted 2 lines · Bernhard R. Link

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

author

2016-06-15 22:52 UTC #34 lore raw

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

In reply to 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 <[email protected]>

---
 gitweb/gitweb.perl |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

gitweb/gitweb.perl +4 -2

index 36efc10..e022e11 100755
@@ -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"

Signature · 1 line

1.7.8.3

Jakub Narebski

2016-06-15 22:52 UTC #35 lore raw

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

Quoted 2 lines · Bernhard R. Link

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.

Quoted 12 lines

Signed-off-by Bernhard R. Link <[email protected]>

---
 gitweb/gitweb.perl |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

gitweb/gitweb.perl +0 -0

index 36efc10..e022e11 100755
@@ -3976,9 +3976,11 @@ sub git_footer_html {
 		}

gitweb/gitweb.perl sub git_footer_html { · line 3985

Quoted 10 lines

gitweb/gitweb.perl +4 -2

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

Signature · 2 lines

Jakub Narebski
Poland

Bernhard R. Link

author

2016-06-15 22:52 UTC #36 lore raw

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

In reply to 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 <[email protected]>

---
 gitweb/gitweb.perl |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

gitweb/gitweb.perl +14 -0

index e022e11..dfc79df 100755
@@ -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);
 	}
 }

Signature · 1 line

1.7.8.3

Jakub Narebski

2016-06-15 22:52 UTC #37 lore raw

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

Quoted 2 lines · 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.

O.K. (though I'd prefer written it less concise and more clear).

Quoted 10 lines

Signed-off-by Bernhard R. Link <[email protected]>
---
 gitweb/gitweb.perl |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)
gitweb/gitweb.perl +0 -0
index e022e11..dfc79df 100755

gitweb/gitweb.perl sub print_header_links { · line 3841

Quoted 6 lines

gitweb/gitweb.perl +3 -0

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

gitweb/gitweb.perl sub print_header_links { · line 3846

Quoted 5 lines

gitweb/gitweb.perl +5 -0

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

Quoted 8 lines

gitweb/gitweb.perl +4 -0

+			      esc_html($part)) . " / ";
+	}
+}
+
 sub print_nav_breadcrumbs {
 	my %opts = @_;

gitweb/gitweb.perl sub print_nav_breadcrumbs { · line 3872

Quoted 7 lines

gitweb/gitweb.perl +2 -0

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

Signature · 2 lines

Jakub Narebski
Poland

Bernhard R. Link

author

2016-06-15 22:52 UTC #38 lore raw

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

In reply to Bernhard R. Link

Signed-off-by Bernhard R. Link <[email protected]>

---
 gitweb/gitweb.perl |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

gitweb/gitweb.perl +4 -1

index dfc79df..b54ddb9 100755
@@ -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}) {

Signature · 1 line

1.7.8.3

Jakub Narebski

2016-06-15 22:52 UTC #39 lore raw

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

Quoted 5 lines · 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).

Nice idea, nice description.

Quoted 12 lines

Signed-off-by Bernhard R. Link <[email protected]>

---
 gitweb/gitweb.perl |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

gitweb/gitweb.perl +0 -0

index dfc79df..b54ddb9 100755
@@ -3853,7 +3853,10 @@ sub print_nav_breadcrumbs {

gitweb/gitweb.perl sub print_nav_breadcrumbs { · line 3862

Quoted 10 lines

gitweb/gitweb.perl +4 -1

 	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.

Signature · 2 lines

Jakub Narebski
Poland