From: "Jakub Narębski" <jnareb@gmail.com>
To: Tony Finch <dot@dotat.at>
Cc: Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org
Subject: Re: What's cooking in git.git (Jul 2015, #01; Wed, 1)
Date: Wed, 22 Jul 2015 13:16:44 +0200 [thread overview]
Message-ID: <55AF7B9C.4000108@gmail.com> (raw)
In-Reply-To: <alpine.LSU.2.00.1507220957350.16350@hermes-1.csi.cam.ac.uk>
On 2015-07-22, Tony Finch wrote:
> Jakub Narębski <jnareb@gmail.com> 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
next prev parent reply other threads:[~2015-07-22 11:17 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-01 22:37 What's cooking in git.git (Jul 2015, #01; Wed, 1) Junio C Hamano
2015-07-02 9:33 ` [PUB]What's " Matthieu Moy
2015-07-03 17:57 ` Junio C Hamano
2015-07-21 18:09 ` What's " Jakub Narębski
2015-07-22 10:05 ` Tony Finch
2015-07-22 11:16 ` Jakub Narębski [this message]
2015-07-22 13:19 ` Tony Finch
2015-07-22 19:32 ` Jakub Narębski
2015-07-22 21:14 ` Tony Finch
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=55AF7B9C.4000108@gmail.com \
--to=jnareb@gmail.com \
--cc=dot@dotat.at \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/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).