Re: [RFCv3 2/2] gitweb: links to patch action in commitdiff and shortlog view

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

On Sat, Dec 6, 2008 at 1:53 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:
>
>> In shortlog view, a link to the patchset is only offered when the number
>> of commits shown is less than the allowed maximum number of patches.
>>
>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

>> +     if (gitweb_check_feature('patches')) {
>> +             $formats_nav .= " | " .
>> +                     $cgi->a({-href => href(action=>"patch", -replay=>1)},
>> +                             "patch");
>> +     }
>>
>>       if (!defined $parent) {
>>               $parent = "--root";
>> @@ -5415,6 +5420,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");
>> +             }
>>
>>               if (defined $hash_parent &&
>>                   $hash_parent ne '-c' && $hash_parent ne '--cc') {
>
> In the above two hunks 'patch' view functions as "git show --pretty=email"
> text/plain equivalent, but this duplicates a bit 'commitdiff_plain'
> functionality.  Well, 'commitdiff_plain' has currently some errors,
> but...

All things considered, for single commit view there is (modulo bugs)
no factual difference between plain diff and patch view.

Although we could merge them, I'm thinking that the plain diff view
has somewhat too much information in it. For backwards compatibility
it's probably not wise to change it, but we should consider it for the
next major version, honestly.

>> @@ -5949,6 +5959,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=>"patch", -replay=>1)},
>> +                                     @commitlist > 1 ? "patchset" : "patch");
>> +             }
>> +     }
>
> Here 'patch' view functions as "git format-patch", able to be downloaded
> and fed to git-am.  Perhaps the action should also be named 'patches'
> here?; it could lead to the same function.

I had half an idea to do so. 'patches' or 'patchset'?

> By the way, there is subtle bug in above link. If shortlog view is less
> than $patch_max commits long, but it is because the history for a given
> branch (or starting from given commit) is so short, and not because
> there is cutoff $hash_parent set, the 'patchset' view wouldn't display
> plain text equivalent view, but only patch for top commit.

Ah, good point.

Hm, not easy to solve. One way could be to add the hash_parent
manually. Or we could make the 'patches' view different from the
'patch' view in the way it handles refspecs without ranges. I'm
leaning towards the latter. What's your opinion?

> I assume that the link is only for 'shortlog' view, and not also for
> 'log' and 'history' views because 'shortlog' is the only log-like view
> which support $hash_parent?

The actual reason is that I never use log nor history view, but since
they don't support hash_parent because of this (I was the one who sent
the patch to support hash_parent in shortlog view) you could
paralogistically say that's the reason ;-)

I'm not sure about history view, but for log view I'm considering
addiong also a 'patch' link next to each commit. I'll think about it.

-- 
Giuseppe "Oblomov" Bilotta