On 2015-07-22, Tony Finch wrote: > Jakub Narębski <jnareb@xxxxxxxxx> wrote: > > Thanks for the review! > >>> * tf/gitweb-project-listing (2015-03-19) 5 commits >>> - gitweb: make category headings into links when they are directories >>> - gitweb: optionally set project category from its pathname >>> - gitweb: add a link under the search box to clear a project filter >>> - gitweb: if the PATH_INFO is incomplete, use it as a project_filter >>> >>> Update gitweb to make it more pleasant to deal with a hierarchical >>> forest of repositories. > > By the way, you can see this patch series in action at > https://git.csx.cam.ac.uk/x/ucs/ Thanks. I don't have my computer set up completely yet (after reinstall). >> Second one, "gitweb: if the PATH_INFO is incomplete, use it as a >> project_filter" looks interesting and quite useful. Though it doesn't >> do much: it allows for handcrafted URL, and provides mechanism to >> create breadcrumbs. It doesn't use this feature in its output... >> Well, I think it doesn't: I cannot check it at this moment. > > Hmm, I think this means I need a better commit message. > > This patch fixes the ugly query-parameter URLs in the breadcrumbs that > you get even in path-info mode. Have a look at the breadcrumbs on the > following pages: > > https://git.csx.cam.ac.uk/g/ucs/git/git.git (unpatched) > https://git.csx.cam.ac.uk/x/ucs/git/git.git (patched) > > If you click on the antepenultimate /git/ in the breadcumbs you get query > parameters without the patch and path_info with the patch. With the patch > the breadcrumbs match the URL. Ah. Yes, the patch itself looks all right, but it definitely needs a better (or at least enhanced) commit message if it is about *adding* path info counterpart to existing query parameter project_filter - - it is (also) about uniquifying URLs used in breadcrumbs when gitweb uses path info links. Current version is (if I have it correctly): gitweb: if the PATH_INFO is incomplete, use it as a project_filter Previously gitweb would ignore partial PATH_INFO. For example, it would produce a project list for the top URL https://www.example.org/projects/ and a project summary for https://www.example.org/projects/git/git.git but if you tried to list just the git-related projects with https://www.example.org/projects/git/ you would get a list of all projects, same as the top URL. As well as fixing that omission, this change also makes gitweb generate PATH_INFO-style URLs for project filter links, such as in the breadcrumbs. A question about implementation: why emptying $path_info in evaluate_path_info()? >> What is missing is a support for query parameters path, and not only >> path info. > > Query parameter support is already present, in the form of project > filters. > >> Thought some thought is needed for generating (or not) breadcrumbs >> if path_info is turned off. > > That already works in unpatched gitweb. Right. >> The third, "gitweb: add a link under the search box to clear a project >> filter" notices a problem... then solves it in strange way. IMVHO >> a better solution would be to add "List all projects" URL together >> with " / " (or other separator) conditionally, if $project_filter >> is set. Or have "List all projects" and add "List projects$limit" >> if $project_filter is set. > > Yes, that is exactly what the patch does. I used a suffix "if" to align > the print statements and markup: > + if $project_filter; > > Compare and contrast the search box on these pages: > > https://git.csx.cam.ac.uk/g/ucs/?a=project_list;pf=u/fanf2 > https://git.csx.cam.ac.uk/x/ucs/u/fanf2/ > > Perhaps you would prefer the following? > > --- a/gitweb/gitweb.perl > +++ b/gitweb/gitweb.perl > @@ -5549,10 +5549,14 @@ sub git_project_search_form { > "</span>\n" . > $cgi->submit(-name => 'btnS', -value => 'Search') . > $cgi->end_form() . "\n" . > - $cgi->a({-href => href(project => undef, searchtext => undef, > - project_filter => $project_filter)}, > - esc_html("List all projects$limit")) . "<br />\n"; > - print "</div>\n"; > + $cgi->a({-href => $my_uri}, esc_html("List all projects")); > + if ($project_filter) { > + print " / " . > + $cgi->a({-href => href(project => undef, action => "project_list", > + project_filter => $project_filter)}, > + esc_html("List projects$limit")); > + } > + print "<br />\n</div>\n"; > } > > # entry for given @keys needs filling if at least one of keys in list Yes, it is eminently more readable. Postfix controls are discouraged, especially with multi-line constructs c.f. Perl::Critic::Policy::ControlStructures::ProhibitPostfixControls >> The last two, which form the crux of this patch series, looks like >> a good idea, though not without a few caveats. I am talking here >> only about conceptual level, not about how it is coded (which has >> few issues as well): >> >> - I think that non-bare repositories "repo/.git" should be >> treated as one directory entry, i.e. gitweb should not create >> a separate category for "repo/". This is admittedly a corner >> case, but useful for git-instaweb > > Yes, that's a bug, thanks for spotting it! Well, more like a corner case. With "repo/.git" there wouldn't be other repositories in "repo/"... well, except for old-style submodules for git-instaweb. >> - I think that people would want to be able to configure how >> many levels of directory hierarchy gets turned into categories. >> Perhaps only top level should be turned into category? Deep >> hierarchies means deep categories (usually with very few >> repositories) with current implementation. > > Good question. I was assuming flat-ish directory hierarchies, but that's > clearly not very true, e.g. https://git.kernel.org/cgit/ > > I think it would be right to make this a %feature since categories already > nearly fit the %feature per-project override style. On the other hand $projects_list_group_categories is a global gitweb configuration variable, and $projects_list_directory_is_category was patterned after it. Note that cgit, when using first part of path (first directory) as project category, it strips it from project name, but indents project list... though I am not sure if it would work if more than first directory is used for category (as in this case there can be repos mixed with categories: "sub/repo.git", "sub/foo/bar.git", "sub/foo/baz.git") > I will send a new version of the series shortly. A few thoughts about implementation: - the comment above $projects_list_directory_is_category does not mention that it needs $projects_list_group_categories to function - $project_list_default_category is moved to inside of git_get_project_category(), which is not mentioned in commit message (and might be good independent cleanup) - with more complicated rules it would be worth moving the core of work into newly created git_get_category_from_path(), or something like that - can we turn category header into link even if the category didn't came from $projects_list_directory_is_category? - even if $projects_list_directory_is_category is true, the category could came from 'category' file, or otherwise manually set category, though I wonder how we can easily detect this... Best regards -- Jakub Narębski -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html