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

[Jakub Narebski <jnareb@gmail.com>]

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.

>>>  * 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.
 
>>> +     # 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.
 
>> 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.
 
>>> @@ -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-)

Cleaner is good.
 
> The double limit worked properly, btw.

Nice to know. But should we rely on corner cases handling?

-- 
Jakub Narebski
Poland