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

From: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date: Dec 18, 2008, 16:23 UTC
Message-ID: <cb7bb73a0812180823y5249abcakd419d4fee9942d84@mail.gmail.com>
In-Reply-To: <200812181033.57360.jnareb@gmail.com>

On Thu, Dec 18, 2008 at 10:33 AM, Jakub Narebski <jnareb@gmail.com> wrote:

Show 31 quoted lines

> 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