On Sat, Dec 6, 2008 at 2:09 PM, Jakub Narebski <jnareb@xxxxxxxxx> wrote: > 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. 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 -- 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