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

From: Jakub Narebski <jnareb@gmail.com>
Date: Dec 18, 2008, 09:33 UTC
Message-ID: <200812181033.57360.jnareb@gmail.com>
In-Reply-To: <7v4p12hv5q.fsf@gitster.siamese.dyndns.org>

On Thu, 18 Dec 2008, Junio C Hamano wrote:

Show 24 quoted lines

> 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.

-- 
Jakub Narebski
Poland