On Sat, Dec 6, 2008 at 13:34, Giuseppe Bilotta wrote:
> On Sat, Dec 6, 2008 at 1:34 AM, Jakub Narebski <jnareb@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
