Bernhard Reiter <ockham@xxxxxxxxx> writes:
> Resending this once more, as indicated by <xmqqbnp4hu8g.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
> 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.
> diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
> index 7d991d9..1c24e1f 100644
> --- a/Documentation/git-imap-send.txt
> +++ b/Documentation/git-imap-send.txt
> @@ -9,7 +9,7 @@ git-imap-send - Send a collection of patches from stdin to an IMAP folder
> SYNOPSIS
> --------
> [verse]
> -'git imap-send'
> +'git imap-send' [-v] [-q] --[no-]curl
> DESCRIPTION
> @@ ...
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?
> diff --git a/Makefile b/Makefile
> index f34a2d4..69b2fbf 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -992,6 +992,9 @@ ifdef HAVE_ALLOCA_H
> BASIC_CFLAGS += -DHAVE_ALLOCA_H
> endif
> +IMAP_SEND_BUILDDEPS =
> +IMAP_SEND_LDFLAGS = $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
> +
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.
> diff --git a/imap-send.c b/imap-send.c
> index 70bcc7a..9cc80ae 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -26,11 +26,31 @@
> #include "credential.h"
> #include "exec_cmd.h"
> #include "run-command.h"
> +#include "parse-options.h"
> #ifdef NO_OPENSSL
> typedef void *SSL;
> #endif
> +#ifdef USE_CURL_FOR_IMAP_SEND
> +#include "http.h"
> +#endif
> -static const char imap_send_usage[] = "git imap-send < <mbox>";
> +static int Verbose, Quiet;
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html