threads / patch / 13197

patchDon't force imap.host to be set when imap.tunnel is set

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

## tl;dr

5 messages between Apr 21, 2008 and Apr 22, 2008. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Andy Parkins· Apr 21, 2008, 13:59 UTC · lore

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(-)

Show changes to imap-send.c +1 −1

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

Junio C Hamano· Apr 22, 2008, 06:47 UTC · re: Andy Parkins · lore

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

Andy Parkins <andyparkins@gmail.com> writes:

Show 18 quoted lines

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

Andy Parkins· Apr 22, 2008, 09:11 UTC · re: Junio C Hamano · lore

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

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

Jeff King· Apr 22, 2008, 10:41 UTC · re: Andy Parkins · lore

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

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

Show 5 quoted lines

> 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):

Show changes to imap-send.c +5 −3

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

Andy Parkins· Apr 22, 2008, 15:07 UTC · re: Jeff King · lore

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

Jeff King wrote:

Show 5 quoted lines

> 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