Re: [PATCHv5 07/12] gitweb: remotes view for a single remote
- From
Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
- Date
- Sep 27, 2010, 07:11 UTC
- Message-ID
- <AANLkTimemfDPdMycasFmqUvk0eF-eD9z7P1RFnitLD9G@mail.gmail.com>
- In-Reply-To
- <201009262255.45959.jnareb@gmail.com>
2010/9/26 Jakub Narebski <jnareb@gmail.com>:
Show 10 quoted lines
> 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.
Show 23 quoted lines
>> 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.
Show 10 quoted lines
>> +
>> + 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.
Show 30 quoted lines
>> 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