From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 06 Sep 2010 06:38:01 GMT
Subject: Re: [PATCH v3 3/3] New send-email option smtpserveroption.
Message-ID: <7vpqwrv0l2.fsf@alter.siamese.dyndns.org>
In-Reply-To: <m3lj7fn9oy.fsf@localhost.localdomain>

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?

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