Re: [RFCv3 2/2] gitweb: links to patch action in commitdiff and shortlog view
- From
Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
- Date
- Dec 6, 2008, 13:25 UTC
- Message-ID
- <cb7bb73a0812060525k65a7f549l2cce5f0dae9fc76c@mail.gmail.com>
- In-Reply-To
- <200812060153.52947.jnareb@gmail.com>
On Sat, Dec 6, 2008 at 1:53 AM, Jakub Narebski <jnareb@gmail.com> wrote:
Show 6 quoted lines
> 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>
Show 25 quoted lines
>> + 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.
Show 16 quoted lines
>> @@ -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 .= " ⋅ " .
>> + $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'?
Show 5 quoted lines
> 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