Re: [PATCH v3 3/3] New send-email option smtpserveroption.
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Sep 6, 2010, 10:23 UTC
- 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:
Show 10 quoted lines
>>> --- >>> 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
Show 29 quoted lines
>>> 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'.
Show 7 quoted lines
> 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.
Show 5 quoted lines
> > > 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.
Show 22 quoted lines
> 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