Re: [PATCH v3] gitweb: add project_filter to limit project list to a subdirectory
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Jan 29, 2012, 12:54 UTC
- Message-ID
- <201201291354.50241.jnareb@gmail.com>
- In-Reply-To
- <20120129012234.GD16079@server.brlink.eu>
On Sun, 29 Jan 2012, Bernhard R. Link wrote:
Show 8 quoted lines
> This commit changes the project listing views (project_list, > project_index and opml) to limit the output to only projects in a > subdirectory if the new optional parameter ?pf=directory name is used. > > The change is quite minimal as git_get_projects_list already can limit > itself to a subdirectory (though that was previously only used for > 'forks'). >
Nice and succinct.
> If there is a GITWEB_LIST file, the contents are just filtered like > with the forks action. >
O.K.
Show 7 quoted lines
> Without a GITWEB_LIST file only the given subdirectory is searched > for projects (like with forks) unless GITWEB_STRICT_EXPORT is enabled. > In the later case GITWEB_PROJECTROOT is traversed normally (unlike > with forks) and projects not in the directory ignored. > (As there is no check if the filter_path would have been found in > the usual search as the project path is checked with forks). >
Now I understand how project_filter interacts with strict_export.
Though I am not sure if this "paranoid mode" is really necessary. I don't see how you could get in situation where scanning from $project_list and filtering with $project_filter prefix, and scanning from $project_list/$project_filter would give different results.
I think you are overly paranoid here, but perhaps it is better to be overly strict, and then relax it if it turns out to be not necessary.
Show 5 quoted lines
> Reusing $project instead of adding a new parameter would have been > nicer from a UI point-of-view (including PATH_INFO support) but > would complicate the $project validating code that is currently being > used to ensure nothing is exported that should not be viewable. >
Sidenote: support for actionless PATH_INFO URLs would make it even more complicated...
> Additionally change html page headers to not only link the project > root and the currently selected project but also the directories in > between using project_filter. >
Excuse me changing my mind, but I think that as far as this patch series is applied as whole, it would be better for maintability to keep those two patches split; though put the above as a [part of] commit message in 2/2 patch.
Show 35 quoted lines
> Signed-off-by: Bernhard R. Link <brlink@debian.org>
> ---
>
> changes since v2:
> improve description
> remove || 0 for boolean argument
> merge with patch using this feature
> use user-visible configuration names instead of internal ones
>
> * Jakub Narebski <jnareb@gmail.com> [120128 23:45]:
> > "Bernhard R. Link" <brl+git@mail.brlink.eu> writes:
> > > If strict_export is enabled and there is no projects_list, it still
> > > traverses the full tree and only filters afterwards to avoid anything
> > > getting visible by this. Otherwise only the subtree needs to be
> > > traversed, significantly reducing load times.
> > >
> > I still don't understand interaction between project_filter ('pf'),
> > $strict_export and $projects_list being either directory or a file
> > with a list of projects.
> >
> > Does it mean, that when $projects_list is a file with a list of projects,
> > and we use project_filter, then:
> >
> > * if $strict_export is false, then $project_list is ignored, and the
> > filtered list of projects is created by scanning
> > "$projectroot/$project_filter"
>
> No. If project_list is set, i.e. a file, then this is always used.
> If it is a directory (because it is not set thus set to projectroot),
> then with forks it still traverses that directory (as that was checked
> before to be a reachable project with a previous call to
> git_get_projects_list). In the case of project_filter only the directory
> is traversed without strict_export and the whole projectroot is
> traversed with strict_export.
> O.K., now I understand it.
> Is the new description better. >
Yes it is.
Show 15 quoted lines
> > A few nitpicks with respect to patch itself.
> >
> > > -2827,6 +2835,7 @@ sub git_get_project_url_list {
> > >
> > > sub git_get_projects_list {
> > > my $filter = shift || '';
> > > + my $paranoid = shift || 0;
> > > my @list;
> > >
> >
> > First, undefined value is false in Perl, so there is no need for
> > " || 0" in setting $paranoid variable.
>
> I thought it make it clearer that the argument might not be set and
> what the default is. But that is personal taste.First, optional parameter defaults to false in the 'my $foo = shift;' or equivalent form is (I think) idiomatic Perl. Second, this way of writing it is used through gitweb code (CodingGuidelines: imitate existing coding practices).
Show 7 quoted lines
> > Second, why not use global variable $strict_export instead of adding > > another parameter to git_get_projects_list()? > > That would change the action=forks behaviour to traverse the whole > projectroot two times. This way paranoia is only activated if > strict_mode is set _and_ the argument was not yet checked to be > reachable.
Thanks for explanation.
> gitweb/gitweb.perl | 52 ++++++++++++++++++++++++++++++++++++++++++++-------- > 1 files changed, 44 insertions(+), 8 deletions(-)
Not that large for a new feature...
-- Jakub Narebski Poland