On Thu, Dec 18, 2008 at 10:33 AM, Jakub Narebski <jnareb@xxxxxxxxx> wrote:
> On Thu, 18 Dec 2008, Junio C Hamano wrote:
>> Giuseppe Bilotta <giuseppe.bilotta@xxxxxxxxx> 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
--
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
