Re: [PATCH v3 3/3] New send-email option smtpserveroption.
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Sep 6, 2010, 09:27 UTC
- Message-ID
- <AANLkTi=gm6wZ_0Q5-0r+ZSFLMtCzgi0Ah7WFLBWfJer6@mail.gmail.com>
- In-Reply-To
- <7vpqwrv0l2.fsf@alter.siamese.dyndns.org>
On Mon, Sep 6, 2010 at 06:38, Junio C Hamano <gitster@pobox.com> wrote:
Show 67 quoted lines
> Jakub Narebski <jnareb@gmail.com> writes: > >> Pascal Obry <pascal@obry.net> writes: >> >>> The new command line parameter --smtp-server-option or default >>> configuration sendemail.smtpserveroption can be used to pass >>> specific options to the SMTP server. Update the documentation >>> accordingly. >> >> Sign-off? (See Documentation/SubmittingPatches). > > Thanks. > >>> --- >>> 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). > >>> 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 > > 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. > > 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. > > I dunno. Have we solved a similar issue with other parts of the system, > and how?
I can tell you that I *didn't* solve a similar problem with t/harness:
? (test_args => [ split /\s+/, $ENV{GIT_TEST_OPTS} ])That'll fail if the GIT_TEST_OPTS contains a space, e.g. GIT_TEST_OPTS='--root="/tmp/a space"'. But I just ignored it at the time.
It would be useful for both of these if we had a str_to_options() utility function in e.g. Git.pm. If nothing else it's probably more user friendly to specify them in one smtpserveroption string, since you can copy/paste an existing invocation then.
Maybe we can convince a POSIX shell (which we require anyway) to split these up for us? Shelling out from Perl to split these up would be a good solution if it can be done.
Show 19 quoted lines
>>> @@ -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?It's not just you. The customary style in Perl is to avoid parentheses.