From: Jakub Narebski <jnareb@gmail.com>
Date: Mon, 06 Sep 2010 10:23:57 GMT
Subject: Re: [PATCH v3 3/3] New send-email option smtpserveroption.
Message-ID: <201009061223.59090.jnareb@gmail.com>
In-Reply-To: <7vpqwrv0l2.fsf@alter.siamese.dyndns.org>

On Mon, 6 Sep 2010, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>> Pascal Obry <pascal@obry.net> writes:

>>> ---
>>>  Documentation/git-send-email.txt |    8 ++++++++
>>>  git-send-email.perl              |    8 +++++++-
>>>  2 files changed, 15 insertions(+), 1 deletions(-)
>>
>> Needs update to Documentation/config.txt, adding line about
>> sendemail.smtpserveroption.
> 
> And test if it is easy to arrange (otherwise I'll take a look myself, so
> do not worry too much about it).

Actually because *all* options use the same mechanism (%config_settings
which is used in read_config, and GetOptions from Getopt::Long), it would
be better to just test the mechanism, I think.

But that is outside the scope of this patch... and probably require
moving (some of) functionality to Git.pm
 
>>> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
>>> index c283084..5af05bc 100644
>>> --- a/Documentation/git-send-email.txt
>>> +++ b/Documentation/git-send-email.txt
>>> @@ -157,6 +157,14 @@ user is prompted for a password while the input is masked for privacy.
>>>  	`/usr/lib/sendmail` if such program is available, or
>>>  	`localhost` otherwise.
>>>  
>>> +--smtp-server-option=<option>::
>>> +	If set, specifies the outgoing SMTP server option to use.
>>> +	Default value can be specified by the 'sendemail.smtpserveroption'
>>> +	configuration option.
>>> ++
>>> +The --smtp-server-option option must be repeated for each option you want
>>> +to pass to the server.
>>
>> Just a nitpick.
>>
>> How do multiple options are supported with sendemail.smtpserveroption?
>> This also needs to be described, I think.
> 
> That is a good and important point.
> 
> We could
> 
> 	[sendemail]
>         	smtpserveroption = opt1
>         	smtpserveroption = opt2
> 

We do it this way, the same as for 'sendemail.to'... though I admit
that it is much more natural for 'sendemail.to' than for 
'sendemail.smtpServerOption'.
                
> or if we choose to split at WS
> 
> 	[sendemail]
>         	smtpserveroption = "opt1 opt2"
> 
> but with the second form there always is this nagging "how would you
> specify an option with WS in it" issue, so the former might be easier.

Yes, there would be a problem with something like this:

  	[sendemail]
          	smtpserveroption = "opt1 'opt2_a opt2_b'"

We could use extract_delimited or extract_quotelike, or just a regexp
generated using gen_delimited_pat from Text::Balanced.  Or better use
shellwords from Text::ParseWords

Text::ParseWords is in Perl core since Perl 5... which is I think good
enough for Git.


If we go this route we should probably provide shellwords / shellsplit /
/ split_sq also for C.

> 
> 
> If we take the latter route, we should take a single command line option
> and split it, i.e. --smtp-server-option='opt1 opt2', using the same WS
> quoting mechanism, for consistency between command line and configuration.

Right.

> I dunno.  Have we solved a similar issue with other parts of the system,
> and how?
> 
>>> @@ -1015,6 +1019,8 @@ X-Mailer: git-send-email $gitversion
>>>  		}
>>>  	}
>>>  
>>> +	unshift (@sendmail_parameters, @smtp_server_options);
>>> +
>>
>> I guess that you are following strange style that other 'unshift'
>> invocation uses, but there should be no space between function and
>> opening parentheses beginning its arguments, e.g.
>>
>>   join("\n", @xh)
>>
>> not
>>
>>   join ("\n", @xh)
> 
> I tend to prefer shift/unshift/push/pop written without these
> parentheses.  Is it just me?

The Perl-ish way is to avoid parentheses for built-in functions with
prototypes, such as shift/unshift/join.  But consistency trumps this
rule, I think (till the cleanup :-))

-- 
Jakub Narebski
Poland