Re: [PATCHv5 2/3] gitweb: add patches view

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

On Thu, Dec 18, 2008 at 10:33 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Thu, 18 Dec 2008, Junio C Hamano wrote:
>> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>>
>> >  sub git_commitdiff {
>> >     my $format = shift || 'html';
>> > +   my %params = @_;
>> > ...
>> > +                   if ($params{-single}) {
>> > +                           push @commit_spec, '-1';
>> > +                   } else {
>> > +                           if ($patch_max > 0) {
>> > ...
>> > +                   }
>> > @@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {
>> >
>> >  # format-patch-style patches
>> >  sub git_patch {
>> > +   git_commitdiff('patch', -single=> 1);
>> > +}
>>
>> Hmm, if you are changing the interface this way, wouldn't it make more
>> sense to also do this?
>>
>>       git_commitdiff(--format => 'patch', --single => 1);
>>       git_commitdiff(--format => 'html');
>
> The first argument (format) is _required_, second is _optional_;
> I'd rather use named parameters trick only for optional parameters.
> Because with more than one optional parameter function call begins
> to be cryptic; also flag (boolean) parameters are more readable
> when used as named parameters.

I have mixed feelings about this: on the one hand we have href() (say)
that takes all its params from a has, but on the other hand we have
esc_html() (say) that takes only additional options from a hash. I'm
personally more inclined towards the latter usage for git_commitdiff
(i.e. this patch) but since the other alternative is straightforward I
sent a v6 of the patchset which implements it.

-- 
Giuseppe "Oblomov" Bilotta