From: Jakub Narebski <jnareb@gmail.com>
Date: Thu, 18 Dec 2008 19:44:31 GMT
Subject: Re: [PATCHv5 2/3] gitweb: add patches view
Message-ID: <200812182044.32154.jnareb@gmail.com>
In-Reply-To: <7viqphguqu.fsf@gitster.siamese.dyndns.org>

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>> 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_;
> 
> My reading of the first line of the sub seems to contradict with your
> statement.  "If the first argument is empty, we use 'html' instead" sounds
> pretty optional to me.
 
Well, for me it is more of required argument, but with default value.
But now we argue semantics... :-)

-- 
Jakub Narebski
Poland