[PATCH] Don't force imap.host to be set when imap.tunnel is set

[PATCH] Don't force imap.host to be set when imap.tunnel is set

[Andy Parkins]

The documentation for git-imap-send suggests a tunnel setting such as

  Tunnel = "ssh -q user@server.com /usr/bin/imapd ./Maildir 2> /dev/null"

which works wonderfully and doesn't require a username, password or port
setting.

However, git-imap-send currently requires that the imap.host variable be
set in the config even when it was unused.  This led me to have to put
the following in my .gitconfig.

 [imap]
   host = dummy

This patch changes imap-send to only require that the imap.host setting
is set if imap.tunnel is _not_ set.

Signed-off-by: Andy Parkins <andyparkins@gmail.com>
---
 imap-send.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 04afbc4..e15df1e 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1302,7 +1302,7 @@ main(int argc, char **argv)
 		fprintf( stderr, "no imap store specified\n" );
 		return 1;
 	}
-	if (!server.host) {
+	if (!server.host && !server.tunnel) {
 		fprintf( stderr, "no imap host specified\n" );
 		return 1;
 	}
-- 
1.5.5.1.57.g5909c

Re: [PATCH] Don't force imap.host to be set when imap.tunnel is set

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> The documentation for git-imap-send suggests a tunnel setting such as
>
>   Tunnel = "ssh -q user@server.com /usr/bin/imapd ./Maildir 2> /dev/null"
>
> which works wonderfully and doesn't require a username, password or port
> setting.
>
> However, git-imap-send currently requires that the imap.host variable be
> set in the config even when it was unused.  This led me to have to put
> the following in my .gitconfig.
>
>  [imap]
>    host = dummy
>
> This patch changes imap-send to only require that the imap.host setting
> is set if imap.tunnel is _not_ set.
>
> Signed-off-by: Andy Parkins <andyparkins@gmail.com>

I am not an imap-send user myself, but is it the case that the use of
imap.tunnel always makes imap.host useless/unnecessary and safe to be left
as NULL?

My quick scan of imap-send.c suggests that

 * imap_open_store() does not look at host/port when tunnel is defined
   while connecting at the socket level;

 * however, when not preauth, "host" is used to issue error message when
   user is not set, and in prompt when pass needs to be asked.  I suspect
   you do not want to leave "host" NULL in this case.

Driving imapd standalone like the "tunnel" example you quoted above would
trigger preauth behaviour, so that should be safe, but I suspect there are
other ways to use tunnel to just relay the connection over the firewall,
while still requiring the client to authenticate the same way as usual.

Re: [PATCH] Don't force imap.host to be set when imap.tunnel is set

[Andy Parkins]

Junio C Hamano wrote:

> I am not an imap-send user myself, but is it the case that the use of
> imap.tunnel always makes imap.host useless/unnecessary and safe to be left
> as NULL?

You're right that it isn't guaranteed to be unnecessary, but equally it's
not guaranteed to be necessary - which is just the situation that an
optional configuration setting describes.

> Driving imapd standalone like the "tunnel" example you quoted above would
> trigger preauth behaviour, so that should be safe, but I suspect there are
> other ways to use tunnel to just relay the connection over the firewall,
> while still requiring the client to authenticate the same way as usual.

I'm sure you are correct, but as I say - it's not guaranteed.  Since
git-imap-send can't know what this particular tunnel requires it shouldn't
force the creation of a dummy option.  If the tunnel does require a
hostname then there is a place to put it, and the person writing the tunnel
line can decide that.



Andy

-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

Re: [PATCH] Don't force imap.host to be set when imap.tunnel is set

[Jeff King]

On Tue, Apr 22, 2008 at 10:11:59AM +0100, Andy Parkins wrote:

> I'm sure you are correct, but as I say - it's not guaranteed.  Since
> git-imap-send can't know what this particular tunnel requires it shouldn't
> force the creation of a dummy option.  If the tunnel does require a
> hostname then there is a place to put it, and the person writing the tunnel
> line can decide that.

I think Junio's point is that it's easy to start dereferencing NULL,
because later parts of the code assume that "host" is always set, even
if only to use it for informational purposes. So those callsites either
need to be fixed to handle a NULL host, or perhaps something like this
instead (totally untested):

diff --git a/imap-send.c b/imap-send.c
index 04afbc4..db65597 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1303,8 +1303,11 @@ main(int argc, char **argv)
 		return 1;
 	}
 	if (!server.host) {
-		fprintf( stderr, "no imap host specified\n" );
-		return 1;
+		if (!server.tunnel) {
+			fprintf( stderr, "no imap host specified\n" );
+			return 1;
+		}
+		server.host = "tunnel";
 	}
 
 	/* read the messages */

-Peff

Re: [PATCH] Don't force imap.host to be set when imap.tunnel is set

[Andy Parkins]

Jeff King wrote:

> I think Junio's point is that it's easy to start dereferencing NULL,
> because later parts of the code assume that "host" is always set, even
> if only to use it for informational purposes. So those callsites either
> need to be fixed to handle a NULL host, or perhaps something like this
> instead (totally untested):

Agreed.  Yours is a much better solution.


Andy
-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com