Re: [RFCv4 3/3] gitweb: link to patch(es) view from commit and log views
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Dec 16, 2008, 01:03 UTC
- Message-ID
- <200812160203.29425.jnareb@gmail.com>
- In-Reply-To
- <1228575755-13432-4-git-send-email-giuseppe.bilotta@gmail.com>
On Sat, 6 Dec 2008, Giuseppe Bilotta wrote:
Show 7 quoted lines
> 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>
Show 13 quoted lines
> ---
> 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...
Show 5 quoted lines
> + if ($patch_max) {
> + if ($patch_max < 0 || @commitlist <= $patch_max) {
> + $paging_nav .= " ⋅ " .
> + $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).
Show 15 quoted lines
> + }
> + }
> +
> 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.
Show 9 quoted lines
>
> 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?...
Show 14 quoted lines
> 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'
Show 15 quoted lines
>
> 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 .= " ⋅ " .
> + $cgi->a({-href => href(action=>"patches", -replay=>1)},
> + @commitlist > 1 ? "patchset" : "patch");
> + }
> + }Same comment as for git_log...
Show 7 quoted lines
>
> git_header_html();
> git_print_page_nav('shortlog','', $hash,$hash,$hash, $paging_nav);
> --
> 1.5.6.5
>
> -- Jakub Narebski Poland