Re: What's cooking in git.git (Jul 2015, #01; Wed, 1)

From: Jakub Narębski <jnareb@gmail.com>
Date: Jul 22, 2015, 11:16 UTC
Message-ID: <55AF7B9C.4000108@gmail.com>
In-Reply-To: <alpine.LSU.2.00.1507220957350.16350@hermes-1.csi.cam.ac.uk>

On 2015-07-22, Tony Finch wrote:

Show 15 quoted lines

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

Show 18 quoted lines

>> 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()?

Show 10 quoted lines

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

Show 39 quoted lines

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

Show 11 quoted lines

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

Show 11 quoted lines

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