Re: [RFCv3 1/2] gitweb: add patch view

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

On Sat, Dec 6, 2008 at 1:34 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:
>
>> The manually-built email format in commitdiff_plain output is not
>> appropriate for feeding git-am, because of two limitations:
>>  * when a range of commits is specified, commitdiff_plain publishes a
>>    single patch with the message from the first commit, instead of a
>>    patchset,
>
> This is because 'commitdiff_plain' wasn't _meant_ as patch series view,
> to be fed to git-am. Actually it is a bit cross between "git show"
> result with '--pretty=email' format, and "git diff" between two commits,
> to be fed to git-apply or GNU patch.
>
> Nevertheless the above reasoning doesn't need to be put in a commit
> message. But it explains why new 'patch' / 'patchset' view is needed:
> because there was no equivalent.

I'll remove it.

>>  * in either case, the patch summary is replicated both as email subject
>>    and as first line of the email itself, resulting in a doubled summary
>>    if the output is fed to git-am.
>
> This is independent issue which is I think worth correcting anyway,
> unless we decide to scrap 'commitdiff_plain' view altogether.
> But I think we would want some text/plain patch view to be applied
> by GNU patch (for example in RPM .spec file).

I don't think we should scrap commitdiff either, but the subject
replication is not really an issue if the view is not fed to git am.

>> +     # The maximum number of patches in a patchset generated in patch
>> +     # view. Set this to 0 or undef to disable patch view, or to a
>> +     # negative number to remove any limit.
>> +     'patches' => {
>> +             'override' => 1,
>> +             'default' => [16]},
>>  );
>
> You need to set "'sub' => \&feature_patches" for feature to be
> override-able at all.  Also features are usually not overridable
> by default, which reduces load a tiny bit (by _possibly_ not reading
> config, although that shouldn't matter much now with reading whole
> commit using single call to git-config, and not one call per variable).

I think I'll make the feature non-overridable. I'll also make it
default to disabled, although I'm not particularly happy with the
choice.

> And I think the default might be set larger: 'log' view generates
> as big if not bigger load, and it is split into 100 commits long
> pages.

Hm, I would say the load of patch view is much higher than the load of
log view, both in terms of bandwidth and in terms of load on the
server, because of the diffs.

>>  sub git_commitdiff {
>>       my $format = shift || 'html';
>> +
>> +     my $patch_max = gitweb_check_feature('patches');
>
> Wouldn't it be better to name this variable $max_patchset_size, or
> something like that? I'm not saying that this name is bad, but I'm
> wondering if it could be better...

max_patchset_size sounds much worse than patch_max to me, which is why
I went for the latter 8-)

>>       if ($format eq 'html') {
>>               $formats_nav =
>>                       $cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
>> @@ -5483,7 +5498,12 @@ sub git_commitdiff {
>>               open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
>>                       '-p', $hash_parent_param, $hash, "--"
>>                       or die_error(500, "Open git-diff-tree failed");
>> -
>> +     } elsif ($format eq 'patch') {
>> +             open $fd, "-|", git_cmd(), "format-patch", '--encoding=utf8',
>> +                     '--stdout', $patch_max > 0 ? "-$patch_max" : (),
>
>> +                     $hash_parent ? ('-n', "$hash_parent..$hash") :
>> +                     ('--root', '-1', $hash)
>
> This bit makes 'patch' view behave bit differently than git-format-patch,
> which I think is a good idea: namely it shows single patch if there is
> no cutoff.  This should be mentioned in commit message, and perhaps
> even in a comment in code.
>
> Beside, if you show only single patch because $hash_parent is not set,
> you don't need to examine $patch_max nor set limit, because you use '-1'.
> Currently if $patch_max > 0 and !$hash_parent, you pass limit for the
> number of patches twice.  This I think is harmless but...

I've reworked the code a bit, making the commit spec an array computed
before passing it to the command line. The code is cleaner but
obviously longer 8-)

The double limit worked properly, btw.

>> +             # TODO add X-Git-Tag/X-Git-Url headers in a sensible way
>
> Sensible way would mean modifying git-format-patch to be able to add
> extra headers via command option, just like it is now possible via
> config variable format.headers, I think.  Because there are no surefire
> markers of where one patch ends and another begins: commit message is
> free text, and can contain diff... although if it contains diff
> separator '---' then git-am would have problem with patch; or at least
> even assuming sane commit messages it is not easy.
>
> Also I think that only X-Git-Url makes sense, and it is per whole
> patchset (whole 'patch' view output) and not for each individual
> patch.

I've stripped this commet for the time being. I'm not sure even
X-Git-Url makes sense, and the fact that it should only attached to
the first email makes it an oddball.

-- 
Giuseppe "Oblomov" Bilotta