git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCHv5 12/12] gitweb: gather more remote data

From
Jakub Narebski <jnareb@gmail.com>
Date
Sep 27, 2010, 15:47 UTC
Message-ID
<201009271747.39201.jnareb@gmail.com>
In-Reply-To
<1285344167-8518-13-git-send-email-giuseppe.bilotta@gmail.com>
On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:

Isn't the most important and visible information the fact that both standalone 'remotes' view and 'remotes' section in the 'summary' page is now grouped by remotes? This should be explicitely mentioned in the commit message. It is not very visible IMVHO in what is written below.

Show 9 quoted lines
> Collect remote information by gathering the list of remotes, and then
> the URL(s) and heads in each remote. In summary view, limit the number
> of remotes for which we collect data, as well as the maximum number of
> heads per remote that we display.
> 
> If the number of remotes is higher than the prescribed limit, do not
> collect any heads information and just show the remotes names and the
> links to the corresponding fetch and push URLs. Otherwise, create a
> group for each remote and display all the information there.

You mean here that in 'summary' view, the 'remotes' section consist only of list of remote names (is it truncated to 15 remotes) with corresponding fetch and push URLs if number of remotes is higher than the prescribed limit. In other case 'remotes' section consist of separate subsection for each remote (is number of remotes limited also in this case), and in each subsection there are displayed up to prescribed limit remote-tracking branches associated with given remote.

Isn't it?
Show 17 quoted lines
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |  171 ++++++++++++++++++++++++++++++++++++++++++++++------
>  1 files changed, 153 insertions(+), 18 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 93017a4..1e671ff 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2758,6 +2758,57 @@ sub git_get_last_activity {
>  	return (undef, undef);
>  }
>  
> +# Return an array with: a hash of remote names mapping to the corresponding
> +# remote heads, the value of the $limit parameter, and a boolean indicating
> +# whether we managed to get all the remotes or not.

Perhaps it would be better to provide an example, rather than trying to describe the output.

Show 5 quoted lines
> +# If $limit is specified and the number of heads found is higher, the head
> +# list is empy. Additional filtering on the number of heads can be done when
> +# displaying the remotes.
> +sub git_get_remotes_data {
> +	my ($limit, $wanted) = @_;

I'm not sure if it wouldn't be better to not worry git_get_remotes_data about $limit, but do limiting on output. The amount of work gitweb needs to do in both situation is, I guess, nearly the same.

Show 5 quoted lines
> +	my %remotes;
> +	open my $fd, '-|' , git_cmd(), 'remote', '-v';
> +	return unless $fd;
> +	my $more = 1;
> +	while (my $remote = <$fd> and $more) {

Why not use 'last' instead of using $more variable (though I have not checked if it would really make code simpler and more readable).

Show 5 quoted lines
> +		chomp $remote;
> +		$remote =~ s!\t(.*?)\s+\((\w+)\)$!!;
> +		next if $wanted and not $remote eq $wanted;
> +		my $url = $1;
> +		my $key = $2;
Minor nitpick: why not
  +		my ($url, $key) = ($1, $2);
Show 20 quoted lines
> +
> +		# a remote may appear more than once because of multiple URLs,
> +		# so if this is a remote we know already, be sure to continue,
> +		# lest we end up with a remote for which we get the fetch URL
> +		# bot not the push URL, for example
> +		$more = exists $remotes{$remote};
> +		$more ||= defined $limit ? (keys(%remotes) < $limit) : 1;
> +		if ($more) {
> +			$remotes{$remote} ||= { 'heads' => () };
> +			$remotes{$remote}{$key} = $url;
> +		}
> +	}
> +	close $fd or return;
> +
> +	# if the while finished with $more being true, it means we ran
> +	# out of remotes before we hit $limit; paradoxically, it being true out
> +	# of the loop means there are 'no more' remotes.
> +	# Rather than waste time renaming the variable, we just read it to
> +	# answer the question: "did we get all remotes before we hit
> +	# the limit?"
Ah, I see that the fact that we exited early is important.
Show 11 quoted lines
> +	if ($more) {
> +		my @heads = map { "remotes/$_" } keys %remotes;
> +		my @remoteheads = git_get_heads_list(undef, @heads);
> +		foreach (keys %remotes) {
> +			my $remote = $_;
> +			$remotes{$remote}{'heads'} = [ grep {
> +				$_->{'name'} =~ s!^$remote/!!
> +			} @remoteheads ];
> +		}
> +	}
> +	return (\%remotes, $limit, $more);

Hmmmm... as it can be seen making this function do more work results in this ugly API. If git_get_remotes_data didn't worry about limiting, we could return simply %remotes hash (or \%remotes hash reference).

Show 13 quoted lines
> +}
> +
>  sub git_get_references {
>  	my $type = shift || "";
>  	my %refs;
> @@ -5018,6 +5069,93 @@ sub git_heads_body {
>  	print "</table>\n";
>  }
>  
> +# Display a single remote block
> +sub git_remote_body {
> +	my ($remote, $rdata, $limit, $head, $single) = @_;
> +	my %rdata = %{$rdata};
Why do you need this, instead of simply using $rdata->{'heads'} etc.?
> +	my $heads = $rdata{'heads'};
Why not
  +	my @heads = @{$rdata{'heads'}};
Or
  +	my @heads = @{$rdata->{'heads'}};
without this strange '%rdata = %{$rdata};'
Show 18 quoted lines
> +
> +	my $fetch = $rdata{'fetch'};
> +	my $push = $rdata{'push'};
> +
> +	my $urls = "<table class=\"projects_list\">\n" ;
> +
> +	if (defined $fetch) {
> +		if ($fetch eq $push) {
> +			$urls .= git_repo_url("URL", $fetch);
> +		} else {
> +			$urls .= git_repo_url("Fetch URL", $fetch);
> +			$urls .= git_repo_url("Push URL", $push) if defined $push;
> +		}
> +	} elsif (defined $push) {
> +		$urls .= git_repo_url("Push URL", $push);
> +	} else {
> +		$urls .= git_repo_url("", "No remote URL");
> +	}

Perhaps reverse order of conditions would result in less nested conditional... but perhaps not.

Show 6 quoted lines
> +
> +	$urls .= "</table>\n";
> +
> +	my ($maxheads, $dots);
> +	if (defined $limit) {
> +		$maxheads = $limit - 1;
If git_get_remotes_data didn't do limiting, you could use
  +		$maxheads = scalar @heads;
> +		if ($#{$heads} > $maxheads) {
> +			$dots = $cgi->a({-href => href(action=>"remotes", hash=>$remote)}, "...");
> +		}
> +	}
We can always check if we got more remotes than limit.
Show 7 quoted lines
> +
> +	my $content = sub {
> +		print $urls;
> +		git_heads_body($heads, $head, 0, $maxheads, $dots);
> +	};
> +
> +	if (defined $single and $single) {

'defined $single and $single' is the same as '$single', because undefined value is false-ish.

> +		$content->();
> +	} else {
> +		git_group("remotes", $remote, "remotes", $remote, $remote, $content);
> +	}

Hmmm... should git_remote_body be responsible for wrapping in subsection? Wouldn't it be better to have caller use

  git_group("remotes", $remote, "remotes", $remote, $remote, 
            sub { git_remote_body($remote, $rdata, $limit, $head) });
Sidenote: strange (but perhaps unavoidable) repetition we have here...
Show 5 quoted lines
> +}
> +
> +# Display remote heads grouped by remote, unless there are too many
> +# remotes ($have_all is false), in which case we only display the remote
> +# names

Wouldn't it be better to put displaying only remote names in a separate subroutine, which would make git_remotes_body extremely simple?

Show 27 quoted lines
> +sub git_remotes_body {
> +	my ($remotelist, $limit, $have_all, $head) = @_;
> +	my %remotes = %$remotelist;
> +	if ($have_all) {
> +		while (my ($remote, $rdata) = each %remotes) {
> +			git_remote_body($remote, $rdata, $limit, $head);
> +		}
> +	} else {
> +		print "<table class=\"heads\">\n";
> +		my $alternate = 1;
> +		while (my ($remote, $rdata) = each (%$remotelist)) {
> +			my $fetch = $rdata->{'fetch'};
> +			my $push = $rdata->{'push'};
> +			if ($alternate) {
> +				print "<tr class=\"dark\">\n";
> +			} else {
> +				print "<tr class=\"light\">\n";
> +			}
> +			$alternate ^= 1;
> +			print "<td>" .
> +			      $cgi->a({-href=> href(action=>'remotes', hash=>$remote),
> +			               -class=> "list name"},esc_html($remote)) . "</td>";
> +			print "<td class=\"link\">" .
> +			      (defined $fetch ? $cgi->a({-href=> $fetch}, "fetch") : "fetch") .
> +			      " | " .
> +			      (defined $push ? $cgi->a({-href=> $push}, "push") : "push") .
> +			      "</td>";

I see that you don't worry here if $fetch == $push, only if they are defined. But I think it might be all right... though the exact output might need some discussion.

Show 20 quoted lines
> +
> +			print "</tr>\n";
> +		}
> +		print "<tr>\n" .
> +		      "<td colspan=\"3\">" .
> +		      $cgi->a({-href => href(action=>"remotes")}, "...") .
> +		      "</td>\n" . "</tr>\n";
> +		print "</table>";
> +	}
> +}
> +
>  sub git_search_grep_body {
>  	my ($commitlist, $from, $to, $extra) = @_;
>  	$from = 0 unless defined $from;
> @@ -5164,7 +5302,7 @@ sub git_summary {
>  	# there are more ...
>  	my @taglist  = git_get_tags_list(16);
>  	my @headlist = git_get_heads_list(16, 'heads');
> -	my @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();
> +	my @remotelist = $remote_heads ? git_get_remotes_data(16) : ();
That's not @remotelist any longer, isn't it?
Show 11 quoted lines
>  	my @forklist;
>  	my $check_forks = gitweb_check_feature('forks');
>  
> @@ -5244,9 +5382,7 @@ sub git_summary {
>  
>  	if (@remotelist) {
>  		git_print_header_div('remotes');
> -		git_heads_body(\@remotelist, $head, 0, 15,
> -		               $#remotelist <= 15 ? undef :
> -		               $cgi->a({-href => href(action=>"remotes")}, "..."));
> +		git_remotes_body(@remotelist, $head);

Hmmm... (current API, with @remotelist (!) containing list of arguments to a subroutine).

Show 9 quoted lines
>  	}
>  
>  	if (@forklist) {
> @@ -5570,26 +5706,25 @@ sub git_remotes {
>  	my $head = git_get_head_hash($project);
>  	my $remote = $input_params{'hash'};
>  
> +	my @remotelist = git_get_remotes_data(undef, $remote);
> +	die_error(500, "Unable to get remote information") unless @remotelist;
@remotelist is not list of remotes.
What if there are no remotes, and no remote-tracking branches?
Show 6 quoted lines
> +
> +	if (keys(%{$remotelist[0]}) == 0) {
> +		die_error(404, defined $remote ?
> +			"Remote $remote not found" :
> +			"No remotes found");
> +	}

Ah, I see that it is handled here. Sidenote: with proposed changed API we can distinguish error case from no-remotes case by returning undefined value versus returning empty hash / empty hash reference.

Show 32 quoted lines
> +
>  	git_header_html(undef, undef, 'header_extra' => $remote);
>  	git_print_page_nav('', '',  $head, undef, $head,
>  		format_ref_views($remote ? '' : 'remotes'));
> -	git_print_header_div('summary', $project);
>  
>  	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");
> -		if (@headslist) {
> -			git_heads_body(\@headslist, $head);
> -		}
> +		git_print_header_div('remotes', "$remote remote for $project");
> +		git_remote_body($remote, $remotelist[0]->{$remote}, undef, $head, 1);
>  	} else {
> -		my @remotelist = git_get_heads_list(undef, 'remotes');
> -		if (@remotelist) {
> -			git_heads_body(\@remotelist, $head);
> -		}
> +		git_print_header_div('summary', "$project remotes");
> +		git_remotes_body(@remotelist, $head);
>  	}
>  	git_footer_html();
>  }
> -- 
> 1.7.3.68.g6ec8
> 
> 
-- 
Jakub Narebski
Poland
Previous: Giuseppe BilottaNext: Giuseppe Bilotta
Message 39 of 41 in “[PATCHv5 00/12] gitweb: remote_heads feature”
  1. Giuseppe BilottaSep 24, 2010
  2. 01/12 gitweb: introduce remote_heads featureGiuseppe Bilotta, Sep 24, 2010
  3. Jakub NarebskiSep 26, 2010
  4. Ævar Arnfjörð BjarmasonSep 26, 2010
  5. David RiptonSep 26, 2010
  6. Giuseppe BilottaSep 27, 2010
  7. 02/12 gitweb: git_get_heads_list accepts an optional list of refs.Giuseppe Bilotta, Sep 24, 2010
  8. Jakub NarebskiSep 26, 2010
  9. 03/12 gitweb: separate heads and remotes listsGiuseppe Bilotta, Sep 24, 2010
  10. Jakub NarebskiSep 26, 2010
  11. 04/12 gitweb: nagivation menu for tags, heads and remotesGiuseppe Bilotta, Sep 24, 2010
  12. Jakub NarebskiSep 26, 2010
  13. Giuseppe BilottaSep 27, 2010
  14. Jakub NarebskiSep 27, 2010
  15. 05/12 gitweb: use fullname as hash_base in heads linkGiuseppe Bilotta, Sep 24, 2010
  16. Jakub NarebskiSep 26, 2010
  17. 06/12 gitweb: allow extra text after action in page headerGiuseppe Bilotta, Sep 24, 2010
  18. Jakub NarebskiSep 26, 2010
  19. Giuseppe BilottaSep 27, 2010
  20. Jakub NarebskiSep 27, 2010
  21. 07/12 gitweb: remotes view for a single remoteGiuseppe Bilotta, Sep 24, 2010
  22. Jakub NarebskiSep 26, 2010
  23. Giuseppe BilottaSep 27, 2010
  24. Jakub NarebskiSep 27, 2010
  25. 08/12 gitweb: auxiliary function to group dataGiuseppe Bilotta, Sep 24, 2010
  26. Jakub NarebskiSep 26, 2010
  27. Giuseppe BilottaSep 27, 2010
  28. Jakub NarebskiSep 27, 2010
  29. Giuseppe BilottaSep 27, 2010
  30. 09/12 gitweb: group stylingGiuseppe Bilotta, Sep 24, 2010
  31. Jakub NarebskiSep 26, 2010
  32. Giuseppe BilottaSep 27, 2010
  33. 10/12 gitweb: git_repo_url() routineGiuseppe Bilotta, Sep 24, 2010
  34. Jakub NarebskiSep 26, 2010
  35. Giuseppe BilottaSep 27, 2010
  36. 11/12 gitweb: use git_repo_url() in summaryGiuseppe Bilotta, Sep 24, 2010
  37. Jakub NarebskiSep 26, 2010
  38. 12/12 gitweb: gather more remote dataGiuseppe Bilotta, Sep 24, 2010
  39. Jakub NarebskiSep 27, 2010
  40. Giuseppe BilottaOct 23, 2010
  41. Jakub NarebskiSep 26, 2010

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.