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

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

On Sat, Dec 6, 2008 at 2:09 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Sat, Dec 6, 2008 at 13:34, Giuseppe Bilotta wrote:
>> 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.
>
> Errr... sorry, I haven't made myself clear. I meant here that *my*
> comment should not be added; your explanation about adding 'patch'
> view should IMHO stay, perhaps reworked a bit: commitdiff is not
> about generating patch series.

Oops, ok, I'll just rewrite it better 8-)

>>>>  * 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.
>
> Well, there is it.

Considering the comments on the second patch too, I've been thinking
that it might worth it to merge commitdiff_plain and patch view for
single commits. Some changes for multi-commits commitdiff_plain can be
considered too, but as I mentioned this is a major changes and should
be dealt with on its own.

>>>> +     # 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.
>
> I think that having 'patches' feature to be able to override is a good
> idea, as limit on number of patches might depend on _repository_, for
> example being lower for repository with large commits (large in sense
> of diff size).
>
> Default with override unset seems to be precedence... as to having it
> disabled by default: having feature which require configuration enabled
> by default serves as an example of configuration; on the other hand the
> example might be in comments, like for 'actions' %feature.

I've added the feature_patches sub. Also, considering that there are
now basically 3 votes against 1 for enabling the feature by default,
I'll keep it enabled.

>>> 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.
>
> Ah, I forgot about that. Limit to 16-25 seems to be reasonable, then.

I'll go with 16 for the time being, I think it's large enough to
accomodate most patchsets.

-- 
Giuseppe "Oblomov" Bilotta