From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mon, 06 Sep 2010 09:27:17 GMT
Subject: Re: [PATCH v3 3/3] New send-email option smtpserveroption.
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:
> 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.

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