imap.preformattedHTML and imap.sslverify

I hate to bring up a topic that is almost a year old, but has either of
these configuration variables ever worked?

The code does this while reading its configuration file:

        static int git_imap_config(const char *key, const char *val, void *cb)
        {
                char imap_key[] = "imap.";

                if (strncmp(key, imap_key, sizeof imap_key - 1))
                        return 0;

                if (!val)
                        return config_error_nonbool(key);
                ...
                else if (!strcmp("sslverify", key))
                        server.ssl_verify = git_config_bool(key, val);
                else if (!strcmp("preformattedHTML", key))
                        server.use_html = git_config_bool(key, val);

Two issues:

 - The body of the function is protected by "nonbool" written back in the
   days when there was no boolean variables in imap.* namespace.  Hence,
   a user cannot write

           [imap]
                sslverify

   and turn it on.  The user needs to write


           [imap]
                sslverify = True

   which is against the parsing rules for boolean variables.

 - The config parser downcases the key before calling the parse callback
   function, so !strcmp("preformattedHTML", key) will never trigger.

The fix is obvious (see below), but I am far more disturbed by the
apparent lack of testing.  Especially, preformattedHTML one would have
never worked as setting the configuration is the only way to trigger this.

Could peole _test_ this patch and report, as I don't use this program at
all.

Thanks.

 imap-send.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index de8114b..ea769a9 100644
--- a/imap-send.c
+++ b/imap-send.c

@@ -1335,11 +1335,16 @@ static int git_imap_config(const char *key, const char *val, void *cb)
 	if (strncmp(key, imap_key, sizeof imap_key - 1))
 		return 0;
 
-	if (!val)
-		return config_error_nonbool(key);
-
 	key += sizeof imap_key - 1;
 
+	/* check booleans first, and barf on others */
+	if (!strcmp("sslverify", key))
+		server.ssl_verify = git_config_bool(key, val);
+	else if (!strcmp("preformattedhtml", key))
+		server.use_html = git_config_bool(key, val);
+	else if (!val)
+		return config_error_nonbool(key);
+
 	if (!strcmp("folder", key)) {
 		imap_folder = xstrdup(val);
 	} else if (!strcmp("host", key)) {

@@ -1360,10 +1365,6 @@ static int git_imap_config(const char *key, const char *val, void *cb)
 		server.port = git_config_int(key, val);
 	else if (!strcmp("tunnel", key))
 		server.tunnel = xstrdup(val);
-	else if (!strcmp("sslverify", key))
-		server.ssl_verify = git_config_bool(key, val);
-	else if (!strcmp("preformattedHTML", key))
-		server.use_html = git_config_bool(key, val);
 	return 0;
 }
 

Hi Junio,

           [imap]
                sslverify = True

*blush* - I never looked at the parsing rules; = True, or = 1 would have seemed fine to me.
I blame Rob.  He led me astray.  That's my story, and I'm sticking to it <grin>.

 - The config parser downcases the key before calling the parse callback
   function, so !strcmp("preformattedHTML", key) will never trigger.

Um...the patch I submitted used the (admittedly badly named) key 'html':
  http://marc.info/?l=git&m=123445427011604&w=2

I'm guessing that you changed it to preformattedHTML prior to committing;
that was something we discussed:
  http://marc.info/?l=git&m=123453315529656&w=2

So I think I can claim 'not guilty!' on that one (but only that one :-/).

Could peole _test_ this patch and report, as I don't use this program at
all.

I did confirm that your patch does work.

However, I assumed that my original patch was rejected.  
I never realized that it had been applied.

That clearly means that no one has ever used this option.

I'll remain unhurt if you revert it (c64d84f1452ec56fd1586493a0b0707bf7442c42), but
let me know if you choose to apply your patch instead; I'll make a point
to use it.

Cheers,

Jeremy