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

[Jakub Narebski <jnareb@gmail.com>]

Dnia sobota 6. grudnia 2008 14:25, Giuseppe Bilotta napisał:
> 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.

I'm just wondering if we should add 'patch' link to 'commit' and
'commitdiff' views (as alternate view) at all...
 
>>> @@ -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'?

Hmmm... I think 'patches'.

>> 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 think simplest solution would be to add $hash_parent if it is not
set from the last commit, i.e. $commitlist[-1]{'id'}

>> 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.

Well, you can add it only for 'shortlog' view, and when the code for
all log-like views would get consolidated, you will get link to 'patches'
view automatically :-)

-- 
Jakub Narebski
Poland