Re: [RFCv4 3/3] gitweb: link to patch(es) view from commit and log views

[Jakub Narebski <jnareb@gmail.com>]

On Sat, 6 Dec 2008, Giuseppe Bilotta wrote:

> We link to patch view in commit and commitdiff view, and to patches view
> in log and shortlog view.
> 
> In (short)log view, the link is only offered when the number of commits
> shown is no more than the allowed maximum number of patches.
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

A few remarks, but otherwise:

Acked-by: Jakub Narebski <jnareb@gmail.com>

> ---
>  gitweb/gitweb.perl |   30 ++++++++++++++++++++++++++++--
>  1 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index dfc7128..8198875 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -5019,6 +5019,15 @@ sub git_log {
>  
>  	my $paging_nav = format_paging_nav('log', $hash, $head, $page, $#commitlist >= 100);
>  
> +	my $patch_max = gitweb_check_feature('patches');

If you change other places to use git_get_feature, then you should
change it too. I'm not sure if it is worth it...

> +	if ($patch_max) {
> +		if ($patch_max < 0 || @commitlist <= $patch_max) {
> +			$paging_nav .= " &sdot; " .
> +				$cgi->a({-href => href(action=>"patches", -replay=>1)},
> +					@commitlist > 1 ? "patchset" : "patch");

I think it would be better to always use "patches" here, as it is
series of patches by design, only by accident it is one commit long.

I wonder if it would make sense to pass

   href(..., hash_parent => $commitlist[-1]{'id'}, ...)

here. But I think having separate "patches" action, with intent being
displaying series of patches, is a better solution. This way you can
see in URL and in the page title (thus also in window title, or in
bookmark name) if it is single patch or patch series (perhaps consisting
of single patch).

> +		}
> +	}
> +
>  	git_header_html();
>  	git_print_page_nav('log','', $hash,undef,undef, $paging_nav);
>  
> @@ -5098,6 +5107,11 @@ sub git_commit {
>  			} @$parents ) .
>  			')';
>  	}
> +	if (gitweb_check_feature('patches')) {
> +		$formats_nav .= " | " .
> +			$cgi->a({-href => href(action=>"patch", -replay=>1)},
> +				"patch");
> +	}

Here using gitweb_check_feature makes perfect sense.

>  
>  	if (!defined $parent) {
>  		$parent = "--root";
> @@ -5413,9 +5427,8 @@ sub git_commitdiff {
>  	# if only a single commit is passed?
>  	my $single_patch = shift && 1;
>  
> -	my $patch_max;
> +	my $patch_max = gitweb_check_feature('patches');

gitweb_check_feature or gitweb_get_feature?...

>  	if ($format eq 'patch') {
> -		$patch_max = gitweb_check_feature('patches');
>  		die_error(403, "Patch view not allowed") unless $patch_max;
>  	}
>  
> @@ -5433,6 +5446,11 @@ sub git_commitdiff {
>  		$formats_nav =
>  			$cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
>  			        "raw");
> +		if ($patch_max) {
> +			$formats_nav .= " | " .
> +				$cgi->a({-href => href(action=>"patch", -replay=>1)},
> +					"patch");
> +		}

Nice reusing $patch_max in different way, as $have_patch if $format is
'html', and as limiter ($patch_max) if $format is 'patch'/'patches'

>  
>  		if (defined $hash_parent &&
>  		    $hash_parent ne '-c' && $hash_parent ne '--cc') {
> @@ -5989,6 +6007,14 @@ sub git_shortlog {
>  			$cgi->a({-href => href(-replay=>1, page=>$page+1),
>  			         -accesskey => "n", -title => "Alt-n"}, "next");
>  	}
> +	my $patch_max = gitweb_check_feature('patches');
> +	if ($patch_max) {
> +		if ($patch_max < 0 || @commitlist <= $patch_max) {
> +			$paging_nav .= " &sdot; " .
> +				$cgi->a({-href => href(action=>"patches", -replay=>1)},
> +					@commitlist > 1 ? "patchset" : "patch");
> +		}
> +	}

Same comment as for git_log...

>  
>  	git_header_html();
>  	git_print_page_nav('shortlog','', $hash,$hash,$hash, $paging_nav);
> -- 
> 1.5.6.5
> 
> 

-- 
Jakub Narebski
Poland