From: Jakub Narebski <jnareb@gmail.com>
To: "Bernhard R. Link" <brl+git@mail.brlink.eu>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v3] gitweb: add project_filter to limit project list to a subdirectory
Date: Sun, 29 Jan 2012 13:54:49 +0100 [thread overview]
Message-ID: <201201291354.50241.jnareb@gmail.com> (raw)
In-Reply-To: <20120129012234.GD16079@server.brlink.eu>
On Sun, 29 Jan 2012, Bernhard R. Link wrote:
> This commit changes the project listing views (project_list,
> project_index and opml) to limit the output to only projects in a
> subdirectory if the new optional parameter ?pf=directory name is used.
>
> The change is quite minimal as git_get_projects_list already can limit
> itself to a subdirectory (though that was previously only used for
> 'forks').
>
Nice and succinct.
> If there is a GITWEB_LIST file, the contents are just filtered like
> with the forks action.
>
O.K.
> Without a GITWEB_LIST file only the given subdirectory is searched
> for projects (like with forks) unless GITWEB_STRICT_EXPORT is enabled.
> In the later case GITWEB_PROJECTROOT is traversed normally (unlike
> with forks) and projects not in the directory ignored.
> (As there is no check if the filter_path would have been found in
> the usual search as the project path is checked with forks).
>
Now I understand how project_filter interacts with strict_export.
Though I am not sure if this "paranoid mode" is really necessary. I don't
see how you could get in situation where scanning from $project_list and
filtering with $project_filter prefix, and scanning from
$project_list/$project_filter would give different results.
I think you are overly paranoid here, but perhaps it is better to be
overly strict, and then relax it if it turns out to be not necessary.
> Reusing $project instead of adding a new parameter would have been
> nicer from a UI point-of-view (including PATH_INFO support) but
> would complicate the $project validating code that is currently being
> used to ensure nothing is exported that should not be viewable.
>
Sidenote: support for actionless PATH_INFO URLs would make it even more
complicated...
> Additionally change html page headers to not only link the project
> root and the currently selected project but also the directories in
> between using project_filter.
>
Excuse me changing my mind, but I think that as far as this patch series
is applied as whole, it would be better for maintability to keep those
two patches split; though put the above as a [part of] commit message
in 2/2 patch.
> Signed-off-by: Bernhard R. Link <brlink@debian.org>
> ---
>
> changes since v2:
> improve description
> remove || 0 for boolean argument
> merge with patch using this feature
> use user-visible configuration names instead of internal ones
>
> * Jakub Narebski <jnareb@gmail.com> [120128 23:45]:
> > "Bernhard R. Link" <brl+git@mail.brlink.eu> writes:
> > > If strict_export is enabled and there is no projects_list, it still
> > > traverses the full tree and only filters afterwards to avoid anything
> > > getting visible by this. Otherwise only the subtree needs to be
> > > traversed, significantly reducing load times.
> > >
> > I still don't understand interaction between project_filter ('pf'),
> > $strict_export and $projects_list being either directory or a file
> > with a list of projects.
> >
> > Does it mean, that when $projects_list is a file with a list of projects,
> > and we use project_filter, then:
> >
> > * if $strict_export is false, then $project_list is ignored, and the
> > filtered list of projects is created by scanning
> > "$projectroot/$project_filter"
>
> No. If project_list is set, i.e. a file, then this is always used.
> If it is a directory (because it is not set thus set to projectroot),
> then with forks it still traverses that directory (as that was checked
> before to be a reachable project with a previous call to
> git_get_projects_list). In the case of project_filter only the directory
> is traversed without strict_export and the whole projectroot is
> traversed with strict_export.
>
O.K., now I understand it.
> Is the new description better.
>
Yes it is.
> > A few nitpicks with respect to patch itself.
> >
> > > -2827,6 +2835,7 @@ sub git_get_project_url_list {
> > >
> > > sub git_get_projects_list {
> > > my $filter = shift || '';
> > > + my $paranoid = shift || 0;
> > > my @list;
> > >
> >
> > First, undefined value is false in Perl, so there is no need for
> > " || 0" in setting $paranoid variable.
>
> I thought it make it clearer that the argument might not be set and
> what the default is. But that is personal taste.
First, optional parameter defaults to false in the 'my $foo = shift;'
or equivalent form is (I think) idiomatic Perl. Second, this way of
writing it is used through gitweb code (CodingGuidelines: imitate existing
coding practices).
> > Second, why not use global variable $strict_export instead of adding
> > another parameter to git_get_projects_list()?
>
> That would change the action=forks behaviour to traverse the whole
> projectroot two times. This way paranoia is only activated if
> strict_mode is set _and_ the argument was not yet checked to be
> reachable.
Thanks for explanation.
> gitweb/gitweb.perl | 52 ++++++++++++++++++++++++++++++++++++++++++++--------
> 1 files changed, 44 insertions(+), 8 deletions(-)
Not that large for a new feature...
--
Jakub Narebski
Poland
next prev parent reply other threads:[~2012-01-29 12:54 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-01-28 16:56 [PATCH v2 1/2] gitweb: add project_filter to limit project list to a subdirectory Bernhard R. Link
2012-01-28 16:57 ` [PATCH v2 2/2] gitweb: place links to parent directories in page header Bernhard R. Link
2012-01-28 22:54 ` Jakub Narebski
2012-01-28 22:45 ` [PATCH v2 1/2] gitweb: add project_filter to limit project list to a subdirectory Jakub Narebski
2012-01-29 1:22 ` [PATCH v3] " Bernhard R. Link
2012-01-29 12:54 ` Jakub Narebski [this message]
2012-01-29 16:06 ` [PATCH v4 1/2] " Bernhard R. Link
2012-01-29 16:13 ` [PATCH v4 2/2] gitweb: place links to parent directories in page header Bernhard R. Link
2012-01-29 16:46 ` Jakub Narebski
2012-01-29 16:41 ` [PATCH v4 1/2] gitweb: add project_filter to limit project list to a subdirectory Jakub Narebski
2012-01-29 21:06 ` Junio C Hamano
2012-01-29 23:06 ` Jakub Narebski
2012-01-30 9:52 ` Bernhard R. Link
2012-01-30 11:44 ` [PATCH v5 1/5] gitweb: prepare git_get_projects_list for use outside 'forks' Bernhard R. Link
2012-01-30 13:42 ` Jakub Narebski
2012-01-30 14:55 ` [PATCH v5.5 " Bernhard R. Link
2012-01-30 15:40 ` Jakub Narebski
2012-01-30 16:29 ` Bernhard R. Link
2012-01-30 11:45 ` [PATCH v5 2/5] gitweb: add project_filter to limit project list to a subdirectory Bernhard R. Link
2012-01-30 15:57 ` Jakub Narebski
2012-01-30 20:03 ` Bernhard R. Link
2012-01-30 20:05 ` [PATCH 1/6] gitweb: move hard coded .git suffix out of git_get_projects_list Bernhard R. Link
2012-01-30 20:06 ` [PATCH v6 2/6] gitweb: prepare git_get_projects_list for use outside 'forks' Bernhard R. Link
2012-01-30 20:07 ` [PATCH v6 3/6] gitweb: add project_filter to limit project list to a subdirectory Bernhard R. Link
2012-01-30 20:09 ` [PATCH v6 4/6] gitweb: limit links to alternate forms of project_list to active project_filter Bernhard R. Link
2012-01-30 20:09 ` [PATCH v6 5/6] gitweb: show active project_filter in project_list page header Bernhard R. Link
2012-01-30 20:10 ` [PATCH v6 6/6] gitweb: place links to parent directories in " Bernhard R. Link
2012-01-30 20:34 ` [PATCH v5 2/5] gitweb: add project_filter to limit project list to a subdirectory Junio C Hamano
2012-01-30 20:48 ` Jakub Narebski
2012-01-30 21:05 ` Junio C Hamano
2012-01-30 21:08 ` Junio C Hamano
2012-01-30 20:48 ` Bernhard R. Link
2012-02-01 16:59 ` Bernhard R. Link
2012-02-01 20:55 ` Junio C Hamano
2012-01-30 11:47 ` [PATCH 3/5] gitweb: limit links to alternate forms of project_list to active project_filter Bernhard R. Link
2012-01-30 16:09 ` Jakub Narebski
2012-01-30 11:48 ` [PATCH v5 4/5] gitweb: show active project_filter in project_list page header Bernhard R. Link
2012-01-30 16:38 ` Jakub Narebski
2012-01-30 11:50 ` [PATCH v5 5/5] gitweb: place links to parent directories in " Bernhard R. Link
2012-01-30 17:10 ` Jakub Narebski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201201291354.50241.jnareb@gmail.com \
--to=jnareb@gmail.com \
--cc=brl+git@mail.brlink.eu \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).