Re: [PATCHv5 07/12] gitweb: remotes view for a single remote

[Giuseppe Bilotta <giuseppe.bilotta@gmail.com>]

2010/9/26 Jakub Narebski <jnareb@gmail.com>:
> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:
>
>> If the hash parameter is passed to gitweb, remotes will interpret it as
>> the name of a remote and limit the view the the heads of that remote.
>
> Errr... I think this commit message needs rewriting to be more clear.
> Perhaps:
>
>  When 'remotes' view is passed 'hash' parameter, it would interprete it
>  as the name of a remote ...

Can do.

>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
>> ---
>>  gitweb/gitweb.perl |   25 ++++++++++++++++++++-----
>>  1 files changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 76cf806..7c62701 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -5547,13 +5547,28 @@ sub git_remotes {
>>               or die_error(403, "Remote heads view is disabled");
>>
>>       my $head = git_get_head_hash($project);
>> -     git_header_html();
>> -     git_print_page_nav('','', $head,undef,$head,format_ref_views('remotes'));
>> +     my $remote = $input_params{'hash'};
>
> I am not sure about using 'hash' parameter for that.
>
> On one hand it is a hack that allow us to not worry about adding extra
> code to evaluate_path_info() subroutine, so that natural path_info URL
> of http://git.example.com/repo.git/remotes/<remote> would use <remote>
> as name of remote to limit to.

It also spares us the introduction of a new parameter, since I'm not
aware of any of the current parameters that could take this role.

> On the other hand it is abusing semantic of 'hash' parameter.  Remote
> name is not revision name or object id.

True. But I cannot think of a use for the hash parameter in remotes
view, and since hash is also used for _named_ refs, it  "kind of"
makes sense to use for a name that is not actually a hash or ref. The
only other option (aside from the use of a new parameter) would be to
use 'hash_base', by reading it as 'base for the ref names' in the
sense that remote refs are 'refs/remote/<base>/name' where the base is
the remote name. But I doubt that's actually more sensible than using
'hash'.

> What makes this issue stronger is the fact that URL is part of API,
> and if we make mistake here, we would have to maintain backward
> compatibility (at least if it appears in a released version).

We _could_ go on the safe side and use a new parameter for this.

>> +
>> +     git_header_html(undef, undef, 'header_extra' => $remote);
>
> I don't quite like the name of this parameter, and I am not sure
> if I like the API either.
>
>> +     git_print_page_nav('', '',  $head, undef, $head,
>> +             format_ref_views($remote ? '' : 'remotes'));
>
> Why this change?

As I mentioned in my replies to the other respective patches, I think
it makes sense to make "all remotes" view easily accessible from the
"single remote" view, and there are two ways I can think of: one is
the "extra header text" way, by making the action name before it point
to "all remotes". The other is to enable 'remotes' in the page nav
submenu when we are in single remotes view (which is why I had the
$current in format_ref_views instead of $action, and which is what is
done by this change).  IMO it makes sense to have both ways available,
but I'm open to suggestions about different approaches.

>>       git_print_header_div('summary', $project);
>>
>> -     my @remotelist = git_get_heads_list(undef, 'remotes');
>> -     if (@remotelist) {
>> -             git_heads_body(\@remotelist, $head);
>> +     if (defined $remote) {
>> +             # only display the heads in a given remote
>> +             my @headslist = map {
>> +                     my $ref = $_ ;
>> +                     $ref->{'name'} =~ s!^$remote/!!;
>> +                     $ref
>> +             } git_get_heads_list(undef, "remotes/$remote");
>
> Hmmm... do we need this temporary variable?  Does it make anything
> more clear?
>
>> +             if (@headslist) {
>> +                     git_heads_body(\@headslist, $head);
>> +             }
>
> This part is the same (modulo name of variable) in both branches of this
> conditional.
>
>> +     } else {
>> +             my @remotelist = git_get_heads_list(undef, 'remotes');
>> +             if (@remotelist) {
>> +                     git_heads_body(\@remotelist, $head);
>> +             }
>>       }
>>       git_footer_html();

Yup, I can make the logic here cleaner and reuse variables (and code)
across the conditionals.

-- 
Giuseppe "Oblomov" Bilotta