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

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

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 [off-list ref] [120128 23:45]:

quoted

"Bernhard R. Link" [off-list ref] writes:

quoted

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.

quoted

A few nitpicks with respect to patch itself.

quoted

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

quoted

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