Re: [PATCH] git-imap-send: use libcurl for implementation

Bernhard Reiter [off-list ref] writes:

Resending this once more, as indicated by [ref]
Hope my formatting and posting style is now conformant. Sorry for the noise.

Thanks.

The patch does not apply for me (please send a trial message to
yourself and then try to apply it out of your mailbox to avoid such
an incident in the future), unfortunately.  I'll comment on the
patch from just code inspection without applying it, though.

The hunk header says that the original and the updated both starts
at line #9 and they both should have 7 lines, but I count only 5
lines.  Where did you lose two lines of the post-context?

Here the patch looks funny as well.  I guess "IMAP_SEND_BUILDDEPS ="
is an added line prefixed with "+", but where does the SP before the
plus sign come from?

I won't point out other patch breakages in the remainder of this
message.

Yikes.  I know this is not a new problem, but please do not name
variables Capitalized.  Also what would it mean to set both Verbose
and Quiet at the same time?

I think we would want to use OPT__VERBOSITY with a single variable
"verbosity" (see hits from "git grep OPT__VERBOSITY" for examples).
Such a change should probably come before this change to use libcurl
as a separate preparatory clean-up patch.

+#ifdef USE_CURL_FOR_IMAP_SEND
+static int use_curl = 1; // on by default; set later by parse_options
+#else
+static int use_curl = 0; // not available
+#endif

Please don't use // comments.  Besides, I think this should be
initialized to -1 to mean "unspecified by the user" in both cases
(i.e. no need for #ifdef/#endif).

+static struct option imap_send_options[] = {
+#ifdef USE_CURL_FOR_IMAP_SEND
+	OPT_BOOL(0, "curl", &use_curl, "use libcurl to communicate with the IMAP server"),
+#endif

And lose the ifdef here (i.e. take "--use-curl" even on a build that
does not support it).

+int main(int argc, char **argv)
+{
+	struct strbuf all_msgs = STRBUF_INIT;
+	int total;
 	int nongit_ok;
  	git_extract_argv0_path(argv[0]);
 -	git_setup_gettext();
+	argc = parse_options(argc, (const char **)argv, "", imap_send_options, imap_send_usage, 0);
 -	if (argc != 1)
-		usage(imap_send_usage);
+	git_setup_gettext();
  	setup_git_directory_gently(&nongit_ok);
 	git_imap_config();

Usually we read config and then parse options, so that people can
set configuration variables for their usual use pattern and override
what is configured from the command line option as needed.

For example, you may want to add

	[imap]
        	useCurl = true

in the configuration file and run "git imap-send" without "--curl"
option on the command line almost always, but in some specific
occasion, "git imap-send --no-curl" to countermand the
configuration.

Was there a specific reason why you had to add parse_options()
before git_imap_config()?

With "use_curl" initialized to "-1 (unspecified)", the final code
structure may look more like this:

		...
                git_imap_config();
                parse_options();

        #ifndef USE_CURL_FOR_IMAP_SEND
                if (use_curl < 0)
                        use_curl = 0;
                if (use_curl) {
                        warning("--use-curl not supported in this build");
                        use_curl = 0;
                }
        #else
                if (use_curl < 0) /* default to enable libcurl */
                        use_curl = 1;
        #endif        

although I think it is also OK to make this feature strictly optional
by defaulting to "use_curl = 0" in the #else part above.  Initializing
the static variable to 0 would make the result even shorter if we
were to go that route, i.e.

	static int use_curl; /* strictly opt in */
        ...
	main()
        {
		...
		git_imap_config();
	        parse_options();
	#ifndef USE_CURL_FOR_IMAP_SEND
        	if (use_curl) {
			warning(...);
			use_curl = 0;
		}
	#endif