{"thread":{"id":"59173","subject":"[PATCH 1/2] Makefile: not use mismatched curl_config to check version","startedAt":"2023-02-01T11:31:49Z","lastAt":"2023-02-17T20:50:37Z","messageCount":37,"participants":["Jiang Xin","Junio C Hamano","Ævar Arnfjörð Bjarmason","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"471224","messageId":"20230201113133.10195-1-worldhello.net@gmail.com","threadId":"59173","inReplyTo":null,"subject":"[PATCH 1/2] Makefile: not use mismatched curl_config to check version","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-02-01T11:31:32Z","receivedAt":"2023-02-01T11:31:49Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nWe may install different versions of curl, E.g.:\n\n * A system default curl, which version is below 7.34.0, is installed\n in \"/usr\", and the \"curl_config\" program is located in \"/usr/bin/\".\n\n * A higher version of curl is installed in \"/opt/git/embedded/\", and\n the \"curl_config\" program is located in \"/opt/git/embedded/bin/\".\n\nIf we add the path \"/opt/git/embedded/bin\" in search PATH, and install\ngit using command \"make && sudo make install\", the source code may be\ncompiled twice.\n\nThis is because when we run \"make\" using normal user account, make will\ncall \"/opt/git/embedded/bin/curl_config\" to check curl version, and will\nset variable USE_CURL_FOR_IMAP_SEND. But when we call \"make install\"\nusing root user's account, we call the system default version of\ncurl_config to check curl version, and will lead to a different\n\"GIT-CFLAGS\" file, and will recompile all source code again.\n\nAppend \"$(CURLDIR)/bin\" before the \"CURL_CONFIG\" variable to use the\nspecific \"curl_config\" program we want to check curl version, we will\nget the correct \"CURL_CFLAGS\" and \"CURL_LDFLAGS\" variables, and we can\nalso have a stable \"GIT-CFLAGS\" file to prevent recompile when running\n\"sudo make install\".\n\nSigned-off-by: Jiang Xin \n---\n Makefile | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/Makefile b/Makefile\nindex 45bd6ac9c3..f4eaf22523 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1597,6 +1597,7 @@ else\n \t\t# Try \"-Wl,-rpath=$(CURLDIR)/$(lib)\" in such a case.\n \t\tCURL_CFLAGS = -I$(CURLDIR)/include\n \t\tCURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib)\n+\t\tCURL_CONFIG := $(CURLDIR)/bin/$(CURL_CONFIG)\n \telse\n \t\tCURL_CFLAGS =\n \t\tCURL_LIBCURL =\n-- \n2.38.2.109.g8b8c02ffae.agit.6.7.7.dev\n\n"},{"id":"471225","messageId":"20230201113133.10195-2-worldhello.net@gmail.com","threadId":"59173","inReplyTo":"20230201113133.10195-1-worldhello.net@gmail.com","subject":"[PATCH 2/2] imap-send: not define USE_CURL_FOR_IMAP_SEND in Makefile","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-02-01T11:31:33Z","receivedAt":"2023-02-01T11:31:52Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nThe definition and using of macro \"USE_CURL_FOR_IMAP_SEND\" are at\ndifferent locations. It is defined in Makefile and is used in file\n\"imap-send.c\". Even though we have fixed the mismatched \"curl_config\"\nissue in Makefile in the previous commit, moving the definition of the\nmacro \"USE_CURL_FOR_IMAP_SEND\" to souce code \"imap-send.c\" seems more\nnature and may help us to use curl in imap-send by force in future by\nremoving \"USE_CURL_FOR_IMAP_SEND\".\n\nThe side effect of this change is that the \"git-imap-send\" program may\nbe larger than necessary if we have a lower version of libcurl\ninstalled.\n\nSigned-off-by: Jiang Xin \n---\n Makefile | 11 ++---------\n contrib/buildsystems/CMakeLists.txt | 3 ---\n imap-send.c | 29 +++++++++++++++++------------\n 3 files changed, 19 insertions(+), 24 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex f4eaf22523..83721216fc 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1621,15 +1621,8 @@ else\n \tifndef NO_EXPAT\n \t\tPROGRAM_OBJS += http-push.o\n \tendif\n-\tcurl_check := $(shell (echo 072200; $(CURL_CONFIG) --vernum | sed -e '/^70[BC]/s/^/0/') 2>/dev/null | sort -r | sed -ne 2p)\n-\tifeq \"$(curl_check)\" \"072200\"\n-\t\tUSE_CURL_FOR_IMAP_SEND = YesPlease\n-\tendif\n-\tifdef USE_CURL_FOR_IMAP_SEND\n-\t\tBASIC_CFLAGS += -DUSE_CURL_FOR_IMAP_SEND\n-\t\tIMAP_SEND_BUILDDEPS = http.o\n-\t\tIMAP_SEND_LDFLAGS += $(CURL_LIBCURL)\n-\tendif\n+\tIMAP_SEND_BUILDDEPS = http.o\n+\tIMAP_SEND_LDFLAGS += $(CURL_LIBCURL)\n \tifndef NO_EXPAT\n \t\tifdef EXPATDIR\n \t\t\tBASIC_CFLAGS += -I$(EXPATDIR)/include\ndiff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\nindex 2f6e0197ff..d508db4d29 100644\n--- a/contrib/buildsystems/CMakeLists.txt\n+++ b/contrib/buildsystems/CMakeLists.txt\n@@ -622,9 +622,6 @@ if(NOT CURL_FOUND)\n \tmessage(WARNING \"git-http-push and git-http-fetch will not be built\")\n else()\n \tlist(APPEND PROGRAMS_BUILT git-http-fetch git-http-push git-imap-send git-remote-http)\n-\tif(CURL_VERSION_STRING VERSION_GREATER_EQUAL 7.34.0)\n-\t\tadd_compile_definitions(USE_CURL_FOR_IMAP_SEND)\n-\tendif()\n endif()\n \n if(NOT EXPAT_FOUND)\ndiff --git a/imap-send.c b/imap-send.c\nindex a50af56b82..c0a2c2b4e6 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -30,20 +30,25 @@\n #if defined(NO_OPENSSL) && !defined(HAVE_OPENSSL_CSPRNG)\n typedef void *SSL;\n #endif\n-#ifdef USE_CURL_FOR_IMAP_SEND\n+#ifdef NO_CURL\n+#define USE_CURL_FOR_IMAP_SEND 0\n+#else\n #include \"http.h\"\n-#endif\n-\n-#if defined(USE_CURL_FOR_IMAP_SEND)\n-/* Always default to curl if it's available. */\n-#define USE_CURL_DEFAULT 1\n+/*\n+ * Since version 7.30.0, libcurl's API has been able to communicate with\n+ * IMAP servers, and curl's CURLOPT_LOGIN_OPTIONS (enabling IMAP\n+ * authentication) parameter is available if curl's version is >= 7.34.0,\n+ * Always use curl if there is a matching libcurl.\n+ */\n+#if LIBCURL_VERSION_NUM >= 0x072200\n+#define USE_CURL_FOR_IMAP_SEND 1\n #else\n-/* We don't have curl, so continue to use the historical implementation */\n-#define USE_CURL_DEFAULT 0\n+#define USE_CURL_FOR_IMAP_SEND 0\n+#endif\n #endif\n \n static int verbosity;\n-static int use_curl = USE_CURL_DEFAULT;\n+static int use_curl = USE_CURL_FOR_IMAP_SEND;\n \n static const char * const imap_send_usage[] = { \"git imap-send [-v] [-q] [--[no-]curl] < \", NULL };\n \n@@ -1396,7 +1401,7 @@ static int append_msgs_to_imap(struct imap_server_conf *server,\n \treturn 0;\n }\n \n-#ifdef USE_CURL_FOR_IMAP_SEND\n+#if USE_CURL_FOR_IMAP_SEND\n static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)\n {\n \tCURL *curl;\n@@ -1531,7 +1536,7 @@ int cmd_main(int argc, const char **argv)\n \tif (argc)\n \t\tusage_with_options(imap_send_usage, imap_send_options);\n \n-#ifndef USE_CURL_FOR_IMAP_SEND\n+#if !USE_CURL_FOR_IMAP_SEND\n \tif (use_curl) {\n \t\twarning(\"--curl not supported in this build\");\n \t\tuse_curl = 0;\n@@ -1580,7 +1585,7 @@ int cmd_main(int argc, const char **argv)\n \tif (server.tunnel)\n \t\treturn append_msgs_to_imap(&server, &all_msgs, total);\n \n-#ifdef USE_CURL_FOR_IMAP_SEND\n+#if USE_CURL_FOR_IMAP_SEND\n \tif (use_curl)\n \t\treturn curl_append_msgs_to_imap(&server, &all_msgs, total);\n #endif\n-- \n2.38.2.109.g8b8c02ffae.agit.6.7.7.dev\n\n"},{"id":"471256","messageId":"xmqqa61x1cr2.fsf@gitster.g","threadId":"59173","inReplyTo":"20230201113133.10195-1-worldhello.net@gmail.com","subject":"Re: [PATCH 1/2] Makefile: not use mismatched curl_config to check version","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-01T18:06:41Z","receivedAt":"2023-02-01T18:06:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jiang Xin writes:\n\n> From: Jiang Xin \n>\n> We may install different versions of curl, E.g.:\n>\n> * A system default curl, which version is below 7.34.0, is installed\n> in \"/usr\", and the \"curl_config\" program is located in \"/usr/bin/\".\n>\n> * A higher version of curl is installed in \"/opt/git/embedded/\", and\n> the \"curl_config\" program is located in \"/opt/git/embedded/bin/\".\n> ...\n> diff --git a/Makefile b/Makefile\n> index 45bd6ac9c3..f4eaf22523 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1597,6 +1597,7 @@ else\n> \t\t# Try \"-Wl,-rpath=$(CURLDIR)/$(lib)\" in such a case.\n> \t\tCURL_CFLAGS = -I$(CURLDIR)/include\n> \t\tCURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib)\n> +\t\tCURL_CONFIG := $(CURLDIR)/bin/$(CURL_CONFIG)\n> \telse\n> \t\tCURL_CFLAGS =\n> \t\tCURL_LIBCURL =\n\nThe above is inside \"ifdef CURLDIR/else/endif\". Is the assumption\nhere that any and all installation of cURL that needs CURLDIR\nspecified should have CURL_CONFIG binary under $(CURLDIR)/bin?\n\nWhat does this patch do to folks who know the exact location of the\ncurl-config binary and have been using CURL_CONFIG from the command\nline or in config.mak to point at it? Doesn't the above break their\nworking set-up?\n\nFor that matter, if you do want to use a specific curl-config binary,\ncan't you use the existing mechanism to set CURL_CONFIG to the path,\nwhich was invented for this exact purpose?\n\nI am not opposed to make it more convenient but I am worried about\nbreaking people's working set-up with this change.\n\n"},{"id":"471279","messageId":"patch-1.1-3bea1312322-20230201T225915Z-avarab@gmail.com","threadId":"59173","inReplyTo":"20230201113133.10195-2-worldhello.net@gmail.com","subject":"[PATCH] imap-send: replace auto-probe libcurl with hard dependency","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-01T23:04:24Z","receivedAt":"2023-02-01T23:04:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the \"imap-send\" command to have a hard dependency on libcurl,\nbefore this it had an optional dependency on both libcurl and OpenSSL,\nnow only the OpenSSL dependency is optional.\n\nThis simplifies our dependency matrix my getting rid of yet another\nspecial-case. Given the prevalence of libcurl and portability of\nlibcurl it seems reasonable to say that \"git imap-send\" cannot be used\nwithout libcurl, almost everyone building git needs to be able to push\nor pull over http(s), so they'll be building with libcurl already.\n\nSo let's remove the previous \"USE_CURL_FOR_IMAP_SEND\" knob. Whether we\nbuild git-imap-send or not is now controlled by the \"NO_CURL\"\nknob. Let's also hide the old --curl and --no-curl options, and die if\n\"--no-curl\" is provided.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nOn Wed, Feb 01 2023, Jiang Xin wrote:\n\n> From: Jiang Xin \n> [...]\n\nI don't have any issue per-se with your proposed change, but perhaps\nwe can go a step further here. I've had this as part of my local build\nfor about a year, but never got around to submitting it.\n\nAs argued above I think we should just make curl a hard dependency for\n\"git-imap-send\", and that furthermore it's OK to require a newer\nversion of curl for that utility than we do in general (as the http(s)\ntransports are more widely required, git-imap-send is more obscure).\n\n Documentation/git-imap-send.txt | 10 ---------\n INSTALL | 8 ++++----\n Makefile | 18 +++++------------\n imap-send.c | 36 +++++----------------------------\n 4 files changed, 14 insertions(+), 58 deletions(-)\n\ndiff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt\nindex f7b18515141..dcd29f011ce 100644\n--- a/Documentation/git-imap-send.txt\n+++ b/Documentation/git-imap-send.txt\n@@ -37,16 +37,6 @@ OPTIONS\n --quiet::\n \tBe quiet.\n \n---curl::\n-\tUse libcurl to communicate with the IMAP server, unless tunneling\n-\tinto it. Ignored if Git was built without the USE_CURL_FOR_IMAP_SEND\n-\toption set.\n-\n---no-curl::\n-\tTalk to the IMAP server using git's own IMAP routines instead of\n-\tusing libcurl. Ignored if Git was built with the NO_OPENSSL option\n-\tset.\n-\n \n CONFIGURATION\n -------------\ndiff --git a/INSTALL b/INSTALL\nindex d5694f8c470..d9538bbcb45 100644\n--- a/INSTALL\n+++ b/INSTALL\n@@ -129,13 +129,13 @@ Issues of note:\n \t itself, e.g. Digest::MD5, File::Spec, File::Temp, Net::Domain,\n \t Net::SMTP, and Time::HiRes.\n \n-\t- git-imap-send needs the OpenSSL library to talk IMAP over SSL if\n-\t you are using libcurl older than 7.34.0. Otherwise you can use\n-\t NO_OPENSSL without losing git-imap-send.\n+\t- git-imap-send needs libcurl 7.34.0 or newer, in addition\n+\t OpenSSL is needed if using the \"imap.tunnel\" open to tunnel\n+\t over SSL. Define NO_OPENSSL to omit the OpenSSL prerequisite.\n \n \t- \"libcurl\" library is used for fetching and pushing\n \t repositories over http:// or https://, as well as by\n-\t git-imap-send if the curl version is >= 7.34.0. If you do\n+\t git-imap-send. If you do\n \t not need that functionality, use NO_CURL to build without\n \t it.\n \ndiff --git a/Makefile b/Makefile\nindex 45bd6ac9c3e..b08a855198c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -773,7 +773,9 @@ PROGRAMS += $(EXTRA_PROGRAMS)\n \n PROGRAM_OBJS += daemon.o\n PROGRAM_OBJS += http-backend.o\n+ifndef NO_CURL\n PROGRAM_OBJS += imap-send.o\n+endif\n PROGRAM_OBJS += sh-i18n--envsubst.o\n PROGRAM_OBJS += shell.o\n .PHONY: program-objs\n@@ -1583,7 +1585,6 @@ ifdef HAVE_ALLOCA_H\n \tBASIC_CFLAGS += -DHAVE_ALLOCA_H\n endif\n \n-IMAP_SEND_BUILDDEPS =\n IMAP_SEND_LDFLAGS =\n \n ifdef NO_CURL\n@@ -1592,6 +1593,7 @@ ifdef NO_CURL\n \tREMOTE_CURL_ALIASES =\n \tREMOTE_CURL_NAMES =\n \tEXCLUDED_PROGRAMS += git-http-fetch git-http-push\n+\tEXCLUDED_PROGRAMS += git-imap-send\n else\n \tifdef CURLDIR\n \t\t# Try \"-Wl,-rpath=$(CURLDIR)/$(lib)\" in such a case.\n@@ -1617,19 +1619,9 @@ else\n \tREMOTE_CURL_NAMES = $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_ALIASES)\n \tPROGRAM_OBJS += http-fetch.o\n \tPROGRAMS += $(REMOTE_CURL_NAMES)\n+\tIMAP_SEND_LDFLAGS += $(CURL_LIBCURL)\n \tifndef NO_EXPAT\n \t\tPROGRAM_OBJS += http-push.o\n-\tendif\n-\tcurl_check := $(shell (echo 072200; $(CURL_CONFIG) --vernum | sed -e '/^70[BC]/s/^/0/') 2>/dev/null | sort -r | sed -ne 2p)\n-\tifeq \"$(curl_check)\" \"072200\"\n-\t\tUSE_CURL_FOR_IMAP_SEND = YesPlease\n-\tendif\n-\tifdef USE_CURL_FOR_IMAP_SEND\n-\t\tBASIC_CFLAGS += -DUSE_CURL_FOR_IMAP_SEND\n-\t\tIMAP_SEND_BUILDDEPS = http.o\n-\t\tIMAP_SEND_LDFLAGS += $(CURL_LIBCURL)\n-\tendif\n-\tifndef NO_EXPAT\n \t\tifdef EXPATDIR\n \t\t\tBASIC_CFLAGS += -I$(EXPATDIR)/include\n \t\t\tEXPAT_LIBEXPAT = -L$(EXPATDIR)/$(lib) $(CC_LD_DYNPATH)$(EXPATDIR)/$(lib) -lexpat\n@@ -2786,7 +2778,7 @@ endif\n git-%$X: %.o GIT-LDFLAGS $(GITLIBS)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)\n \n-git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS)\n+git-imap-send$X: imap-send.o http.o GIT-LDFLAGS $(GITLIBS)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n \t\t$(IMAP_SEND_LDFLAGS) $(LIBS)\n \ndiff --git a/imap-send.c b/imap-send.c\nindex a50af56b827..0e36ed4f854 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -30,26 +30,16 @@\n #if defined(NO_OPENSSL) && !defined(HAVE_OPENSSL_CSPRNG)\n typedef void *SSL;\n #endif\n-#ifdef USE_CURL_FOR_IMAP_SEND\n #include \"http.h\"\n-#endif\n-\n-#if defined(USE_CURL_FOR_IMAP_SEND)\n-/* Always default to curl if it's available. */\n-#define USE_CURL_DEFAULT 1\n-#else\n-/* We don't have curl, so continue to use the historical implementation */\n-#define USE_CURL_DEFAULT 0\n-#endif\n \n static int verbosity;\n-static int use_curl = USE_CURL_DEFAULT;\n+static int use_curl = 1;\n \n static const char * const imap_send_usage[] = { \"git imap-send [-v] [-q] [--[no-]curl] < \", NULL };\n \n static struct option imap_send_options[] = {\n \tOPT__VERBOSITY(&verbosity),\n-\tOPT_BOOL(0, \"curl\", &use_curl, \"use libcurl to communicate with the IMAP server\"),\n+\tOPT_HIDDEN_BOOL(0, \"curl\", &use_curl, \"use libcurl to communicate with the IMAP server\"),\n \tOPT_END()\n };\n \n@@ -1396,7 +1386,6 @@ static int append_msgs_to_imap(struct imap_server_conf *server,\n \treturn 0;\n }\n \n-#ifdef USE_CURL_FOR_IMAP_SEND\n static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)\n {\n \tCURL *curl;\n@@ -1515,7 +1504,6 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,\n \n \treturn res != CURLE_OK;\n }\n-#endif\n \n int cmd_main(int argc, const char **argv)\n {\n@@ -1531,17 +1519,8 @@ int cmd_main(int argc, const char **argv)\n \tif (argc)\n \t\tusage_with_options(imap_send_usage, imap_send_options);\n \n-#ifndef USE_CURL_FOR_IMAP_SEND\n-\tif (use_curl) {\n-\t\twarning(\"--curl not supported in this build\");\n-\t\tuse_curl = 0;\n-\t}\n-#elif defined(NO_OPENSSL)\n-\tif (!use_curl) {\n-\t\twarning(\"--no-curl not supported in this build\");\n-\t\tuse_curl = 1;\n-\t}\n-#endif\n+\tif (!use_curl)\n+\t\tdie(_(\"the --no-curl option to imap-send has been deprecated\"));\n \n \tif (!server.port)\n \t\tserver.port = server.use_ssl ? 993 : 143;\n@@ -1580,10 +1559,5 @@ int cmd_main(int argc, const char **argv)\n \tif (server.tunnel)\n \t\treturn append_msgs_to_imap(&server, &all_msgs, total);\n \n-#ifdef USE_CURL_FOR_IMAP_SEND\n-\tif (use_curl)\n-\t\treturn curl_append_msgs_to_imap(&server, &all_msgs, total);\n-#endif\n-\n-\treturn append_msgs_to_imap(&server, &all_msgs, total);\n+\treturn curl_append_msgs_to_imap(&server, &all_msgs, total);\n }\n-- \n2.39.1.1301.gffb37c08dee\n\n"},{"id":"471282","messageId":"xmqqlelhx973.fsf@gitster.g","threadId":"59173","inReplyTo":"patch-1.1-3bea1312322-20230201T225915Z-avarab@gmail.com","subject":"Re: [PATCH] imap-send: replace auto-probe libcurl with hard dependency","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-01T23:22:24Z","receivedAt":"2023-02-01T23:22:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change the \"imap-send\" command to have a hard dependency on libcurl,\n> before this it had an optional dependency on both libcurl and OpenSSL,\n> now only the OpenSSL dependency is optional.\n>\n> This simplifies our dependency matrix my getting rid of yet another\n\n\"my\" -> \"by\", I think.\n\n> special-case. Given the prevalence of libcurl and portability of\n> libcurl it seems reasonable to say that \"git imap-send\" cannot be used\n> without libcurl, almost everyone building git needs to be able to push\n> or pull over http(s), so they'll be building with libcurl already.\n\nOK.\n\n> So let's remove the previous \"USE_CURL_FOR_IMAP_SEND\" knob. Whether we\n> build git-imap-send or not is now controlled by the \"NO_CURL\"\n> knob.\n\nOK.\n\n> Let's also hide the old --curl and --no-curl options, and die if\n> \"--no-curl\" is provided.\n\nIn other words, if we are building imap-send, we sure know cURL is\nthere, and there is no need to tell a running imap-send not to use\ncURL to talk to the IMAP service? I am not sure the linkage of this\nchange with the rest of the patch. Isn't that a totally orthogonal\nissue? Your imap-send might be cURL enabled, but unless we stop to\nship with our own IMAP routines compiled into imap-send, --no-curl\ndoes have a purpose.\n\nOr did you just forget to document that we stop to ship with our own\nIMAP routines in the above? If so, as long as it is made a bit more\nprominent in the proposed log message in a reroll, I would be happy\nwith such a change rolled into the same patch.\n"},{"id":"471290","messageId":"Y9r84bezJ0scapwC@coredump.intra.peff.net","threadId":"59173","inReplyTo":"xmqqlelhx973.fsf@gitster.g","subject":"Re: [PATCH] imap-send: replace auto-probe libcurl with hard dependency","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-01T23:59:29Z","receivedAt":"2023-02-01T23:59:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 01, 2023 at 03:22:24PM -0800, Junio C Hamano wrote:\n\n> > Let's also hide the old --curl and --no-curl options, and die if\n> > \"--no-curl\" is provided.\n> \n> In other words, if we are building imap-send, we sure know cURL is\n> there, and there is no need to tell a running imap-send not to use\n> cURL to talk to the IMAP service? I am not sure the linkage of this\n> change with the rest of the patch. Isn't that a totally orthogonal\n> issue? Your imap-send might be cURL enabled, but unless we stop to\n> ship with our own IMAP routines compiled into imap-send, --no-curl\n> does have a purpose.\n> \n> Or did you just forget to document that we stop to ship with our own\n> IMAP routines in the above? If so, as long as it is made a bit more\n> prominent in the proposed log message in a reroll, I would be happy\n> with such a change rolled into the same patch.\n\nFWIW, I had the same urge as Ævar, to drop the non-curl support\ncompletely, and was puzzled that his patch did not have a big code\ndeletion. ;)\n\nThe problem is that the tunnel mode still relies on the non-curl code.\nThere was a series to address that a while ago:\n\n https://lore.kernel.org/git/ab866314-608b-eaca-b335-12cffe165526@morey-chaisemartin.com/\n\nbut it ran into the problem that curl did not support PREAUTH\nconnections (which is one of the main points of tunneling). It looks\nlike that got added to curl via their befaa7b14f, which is in curl\n7.56.0 from 2017. That's not old enough for us to require for http, but\nmight be OK for a marginal component like the tunneling mode of\nimap-send.\n\nI think there was also some question of how you even get the tunnel\ngoing. Curl really wants to have a single socket descriptor, not two\npipe descriptors, so there may have to be some trickery with\nsocketpair(). There's more discussion in the linked thread.\n\nSo I think there's a path forward here for getting rid of the legacy\ncode (and I'd be really happy to see it gone; it's imported code that\ndoes not seem super well maintained by us). But until we do that,\ndisabling --no-curl doesn't seem like that big a win, if that code can\nall still be triggered for tunnel mode.\n\n-Peff\n"},{"id":"471292","messageId":"230202.86zg9xormj.gmgdl@evledraar.gmail.com","threadId":"59173","inReplyTo":"xmqqlelhx973.fsf@gitster.g","subject":"Re: [PATCH] imap-send: replace auto-probe libcurl with hard dependency","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-01T23:56:24Z","receivedAt":"2023-02-02T00:09:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 01 2023, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Change the \"imap-send\" command to have a hard dependency on libcurl,\n>> before this it had an optional dependency on both libcurl and OpenSSL,\n>> now only the OpenSSL dependency is optional.\n>>\n>> This simplifies our dependency matrix my getting rid of yet another\n>\n> \"my\" -> \"by\", I think.\n\nThanks, I'll fix that in a re-roll, pending the below...\n\n>> special-case. Given the prevalence of libcurl and portability of\n>> libcurl it seems reasonable to say that \"git imap-send\" cannot be used\n>> without libcurl, almost everyone building git needs to be able to push\n>> or pull over http(s), so they'll be building with libcurl already.\n>\n> OK.\n>\n>> So let's remove the previous \"USE_CURL_FOR_IMAP_SEND\" knob. Whether we\n>> build git-imap-send or not is now controlled by the \"NO_CURL\"\n>> knob.\n>\n> OK.\n>\n>> Let's also hide the old --curl and --no-curl options, and die if\n>> \"--no-curl\" is provided.\n>\n> In other words, if we are building imap-send, we sure know cURL is\n> there, and there is no need to tell a running imap-send not to use\n> cURL to talk to the IMAP service? I am not sure the linkage of this\n> change with the rest of the patch. Isn't that a totally orthogonal\n> issue? Your imap-send might be cURL enabled, but unless we stop to\n> ship with our own IMAP routines compiled into imap-send, --no-curl\n> does have a purpose.\n\nThe equivalent of USE_CURL_FOR_IMAP_SEND is now always true, and that's\nwhat \"--curl\" would enable.\n\nThe \"--no-curl\" option would then have us use the OpenSSL codepath, but\nthat'll no longer be supported, we'll always use curl.\n\nThe link to the rest of the patch is then that \"USE_CURL_FOR_IMAP_SEND\"\nand \"curl_check\" etc. was needed to check if we had curl with imap-send,\nnow we declare that we'll always need it.\n\nAnd the link to the thread-at-large is that Jiang Xin's upthread version\nmoves those checks from the Makefile into the code itself, I agree that\nwolud be an improvement, but if we're happy to just make it a hard\ndependency we won't need it there either...\n\n> Or did you just forget to document that we stop to ship with our own\n> IMAP routines in the above? If so, as long as it is made a bit more\n> prominent in the proposed log message in a reroll, I would be happy\n> with such a change rolled into the same patch.\n\nI'm not sure what you mean here, we still ship with the same routines,\nwe just always take the \"curl\" codepath for the non-tunnel codepath now.\n\nIs this perhaps confusion because while we do make curl mandatory, we're\nnot dropping the OpenSSL code? That's because we're dropping its use for\nthe non-tunnel codepath, but unfortunately for the tunnel case we'll\nstill need it.\n\nSo we only make curl mandatory, but OpenSSL remains optional.\n"},{"id":"471297","messageId":"230202.86v8kkq53u.gmgdl@evledraar.gmail.com","threadId":"59173","inReplyTo":"Y9r84bezJ0scapwC@coredump.intra.peff.net","subject":"Re: [PATCH] imap-send: replace auto-probe libcurl with hard dependency","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-02T00:20:31Z","receivedAt":"2023-02-02T00:32:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 01 2023, Jeff King wrote:\n\n> On Wed, Feb 01, 2023 at 03:22:24PM -0800, Junio C Hamano wrote:\n>\n>> > Let's also hide the old --curl and --no-curl options, and die if\n>> > \"--no-curl\" is provided.\n>> \n>> In other words, if we are building imap-send, we sure know cURL is\n>> there, and there is no need to tell a running imap-send not to use\n>> cURL to talk to the IMAP service? I am not sure the linkage of this\n>> change with the rest of the patch. Isn't that a totally orthogonal\n>> issue? Your imap-send might be cURL enabled, but unless we stop to\n>> ship with our own IMAP routines compiled into imap-send, --no-curl\n>> does have a purpose.\n>> \n>> Or did you just forget to document that we stop to ship with our own\n>> IMAP routines in the above? If so, as long as it is made a bit more\n>> prominent in the proposed log message in a reroll, I would be happy\n>> with such a change rolled into the same patch.\n>\n> FWIW, I had the same urge as Ævar, to drop the non-curl support\n> completely, and was puzzled that his patch did not have a big code\n> deletion. ;)\n\nFWIW I arrived at this from looking at the mandatory $(shell)-outs in\nthe Makefile, and wasn't looking to drop the OpenSSL code.\n\nThen in looking at that, I found that we could probably make the curl\ndependency mandatory.\n\n> The problem is that the tunnel mode still relies on the non-curl code.\n> There was a series to address that a while ago:\n>\n> https://lore.kernel.org/git/ab866314-608b-eaca-b335-12cffe165526@morey-chaisemartin.com/\n>\n> but it ran into the problem that curl did not support PREAUTH\n> connections (which is one of the main points of tunneling). It looks\n> like that got added to curl via their befaa7b14f, which is in curl\n> 7.56.0 from 2017. That's not old enough for us to require for http, but\n> might be OK for a marginal component like the tunneling mode of\n> imap-send.\n>\n> I think there was also some question of how you even get the tunnel\n> going. Curl really wants to have a single socket descriptor, not two\n> pipe descriptors, so there may have to be some trickery with\n> socketpair(). There's more discussion in the linked thread.\n\nThat's neat, I didn't know about that attempt.\n\n> So I think there's a path forward here for getting rid of the legacy\n> code (and I'd be really happy to see it gone; it's imported code that\n> does not seem super well maintained by us). But until we do that,\n> disabling --no-curl doesn't seem like that big a win, if that code can\n> all still be triggered for tunnel mode.\n\nI think the biggest win is that we're dropping the dual curl/OpenSSL\ncodepath for everything except the \"tunnel\" mode, which is really\nobscure compared to the already-obscure functionality of the main\n\"imap-send\" tool.\n\nIt would also get us part of the way to e.g. depending on 7.56.0, as a\nhard dependency on curl (and a newer version than we usually depend on)\nwould reveal if anyone's got an issue with that stepping stone.\n"},{"id":"471300","messageId":"xmqqpmasx35s.fsf@gitster.g","threadId":"59173","inReplyTo":"230202.86zg9xormj.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] imap-send: replace auto-probe libcurl with hard dependency","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-02T01:32:47Z","receivedAt":"2023-02-02T01:32:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> The equivalent of USE_CURL_FOR_IMAP_SEND is now always true, and that's\n> what \"--curl\" would enable.\n>\n> The \"--no-curl\" option would then have us use the OpenSSL codepath, but\n> that'll no longer be supported, we'll always use curl.\n> ...\n>> Or did you just forget to document that we stop to ship with our own\n>> IMAP routines in the above? If so, as long as it is made a bit more\n>> prominent in the proposed log message in a reroll, I would be happy\n>> with such a change rolled into the same patch.\n>\n> I'm not sure what you mean here, we still ship with the same routines,\n> we just always take the \"curl\" codepath for the non-tunnel codepath now.\n\nI am referring to this part of the documentation:\n\n --no-curl::\n Talk to the IMAP server using git's own IMAP routines instead of\n using libcurl. Ignored if Git was built with the NO_OPENSSL option\n set.\n\nSo when built with openssl and libcURL, we used to have a feature\nthat allowed to bypass cURL by passing --no-curl for whatever reason\nthe user chooses to avoid cURL. This patch discards that option,\ndoesn't it?\n\nMaybe such an optional feature may not be very useful, but it should\nbe explained and defended in the proposed log message, and it also\nsounds like an orthogonal change to always require libcURL.\n\n"},{"id":"471318","messageId":"patch-v2-1.6-3187a643035-20230202T093706Z-avarab@gmail.com","threadId":"59173","inReplyTo":"cover-v2-0.6-00000000000-20230202T093706Z-avarab@gmail.com","subject":"[PATCH v2 1/6] imap-send: note \"auth_method\", not \"host\" on auth method failure","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-02T09:44:12Z","receivedAt":"2023-02-02T09:45:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix error reporting added in ae9c606ed22 (imap-send: support CRAM-MD5\nauthentication, 2010-02-15), the use of \"srvc->host\" here was\nseemingly copy/pasted from other uses added in the same commit.\n\nBut here we're complaining about the \"auth_method\" being incorrect, so\nlet's note it, and not the hostname.\n\nIn a subsequent commit we'll alter other uses of \"host\" here after\ngetting rid of the non-tunnel OpenSSL codepath. This preparatory fix\nmakes that subsequent change smaller.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n imap-send.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex a50af56b827..b7902babd4c 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1121,7 +1121,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c\n \t\t\t\t\tgoto bail;\n \t\t\t\t}\n \t\t\t} else {\n-\t\t\t\tfprintf(stderr, \"Unknown authentication method:%s\\n\", srvc->host);\n+\t\t\t\tfprintf(stderr, \"Unknown authentication method:%s\\n\", srvc->auth_method);\n \t\t\t\tgoto bail;\n \t\t\t}\n \t\t} else {\n-- \n2.39.1.1392.g63e6d408230\n\n"},{"id":"471319","messageId":"cover-v2-0.6-00000000000-20230202T093706Z-avarab@gmail.com","threadId":"59173","inReplyTo":"patch-1.1-3bea1312322-20230201T225915Z-avarab@gmail.com","subject":"[PATCH v2 0/6] imap-send: replace auto-probe libcurl with hard dependency","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-02T09:44:11Z","receivedAt":"2023-02-02T09:45:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A polished-up v2 of [1]. I started by splitting out the \"imap-send:\nmake --curl no-optional\" per Junio's comment, but then noticed that we\nactually can delete quite a bit of code in imap-send.c as a result of\nthis change.\n\nComments on indivdiual patches below:\n\n1. https://lore.kernel.org/git/xmqqlelhx973.fsf@gitster.g/\n\nÆvar Arnfjörð Bjarmason (6):\n imap-send: note \"auth_method\", not \"host\" on auth method failure\n imap-send doc: the imap.sslVerify is used with imap.tunnel\n\nBoth existing issues, but we'll end up changing adjacent code & these\ndocs later.\n\n imap-send: replace auto-probe libcurl with hard dependency\n imap-send: make --curl no-optional\n\nThe previous patch, but now split into two...\n\n imap-send: remove old --no-curl codepath\n\n...or three, if we're counting this larger code deletion. This could\nhave been squashed into the above, but I thought in this case that\nthis refactoring was easier to reason about when split off from the\nfunctional change.\n\n imap-send: correctly report \"host\" when using \"tunnel\"\n\nAn existing issue, but one that's now easy to fix with the\nabove. Previously the reporting for the OpenSSL codepath had to deal\nwith both \"host\" and \"tunnel\", now that it only handles \"tunnel\" we\ncan correct and simplify it.\n\nThis is split from the above because there's a functional bugfix\nchange here, unlike the pure code deletion in the preceding commit.\n\nCI & branch for this at:\nhttps://github.com/avar/git/tree/avar/git-imap-send-curl-only-2\n\n Documentation/config/imap.txt | 8 +-\n Documentation/git-imap-send.txt | 11 --\n INSTALL | 8 +-\n Makefile | 18 +---\n imap-send.c | 182 +++++---------------------------\n 5 files changed, 41 insertions(+), 186 deletions(-)\n\nRange-diff against v1:\n-: ----------- > 1: 3187a643035 imap-send: note \"auth_method\", not \"host\" on auth method failure\n-: ----------- > 2: 1dfee9bf08e imap-send doc: the imap.sslVerify is used with imap.tunnel\n1: 3bea1312322 ! 3: 354b6a65a78 imap-send: replace auto-probe libcurl with hard dependency\n @@ Commit message\n before this it had an optional dependency on both libcurl and OpenSSL,\n now only the OpenSSL dependency is optional.\n \n - This simplifies our dependency matrix my getting rid of yet another\n + This simplifies our dependency matrix by getting rid of yet another\n special-case. Given the prevalence of libcurl and portability of\n libcurl it seems reasonable to say that \"git imap-send\" cannot be used\n without libcurl, almost everyone building git needs to be able to push\n @@ Commit message\n \n So let's remove the previous \"USE_CURL_FOR_IMAP_SEND\" knob. Whether we\n build git-imap-send or not is now controlled by the \"NO_CURL\"\n - knob. Let's also hide the old --curl and --no-curl options, and die if\n - \"--no-curl\" is provided.\n + knob.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n + ## Documentation/config/imap.txt ##\n +@@ Documentation/config/imap.txt: imap.preformattedHTML::\n + \n + imap.authMethod::\n + \tSpecify authenticate method for authentication with IMAP server.\n +-\tIf Git was built with the NO_CURL option, or if your curl version is older\n +-\tthan 7.34.0, or if you're running git-imap-send with the `--no-curl`\n ++\tIf you're running git-imap-send with the `--no-curl`\n + \toption, the only supported method is 'CRAM-MD5'. If this is not set\n + \tthen 'git imap-send' uses the basic IMAP plaintext LOGIN command.\n +\n ## Documentation/git-imap-send.txt ##\n @@ Documentation/git-imap-send.txt: OPTIONS\n - --quiet::\n - \tBe quiet.\n \n ----curl::\n --\tUse libcurl to communicate with the IMAP server, unless tunneling\n --\tinto it. Ignored if Git was built without the USE_CURL_FOR_IMAP_SEND\n --\toption set.\n --\n ----no-curl::\n --\tTalk to the IMAP server using git's own IMAP routines instead of\n + --no-curl::\n + \tTalk to the IMAP server using git's own IMAP routines instead of\n -\tusing libcurl. Ignored if Git was built with the NO_OPENSSL option\n -\tset.\n --\n ++\tusing libcurl.\n + \n \n CONFIGURATION\n - -------------\n \n ## INSTALL ##\n @@ INSTALL: Issues of note:\n @@ imap-send.c\n \n static const char * const imap_send_usage[] = { \"git imap-send [-v] [-q] [--[no-]curl] < \", NULL };\n \n - static struct option imap_send_options[] = {\n - \tOPT__VERBOSITY(&verbosity),\n --\tOPT_BOOL(0, \"curl\", &use_curl, \"use libcurl to communicate with the IMAP server\"),\n -+\tOPT_HIDDEN_BOOL(0, \"curl\", &use_curl, \"use libcurl to communicate with the IMAP server\"),\n - \tOPT_END()\n - };\n - \n @@ imap-send.c: static int append_msgs_to_imap(struct imap_server_conf *server,\n \treturn 0;\n }\n @@ imap-send.c: int cmd_main(int argc, const char **argv)\n -\t\tuse_curl = 0;\n -\t}\n -#elif defined(NO_OPENSSL)\n --\tif (!use_curl) {\n --\t\twarning(\"--no-curl not supported in this build\");\n --\t\tuse_curl = 1;\n --\t}\n --#endif\n -+\tif (!use_curl)\n -+\t\tdie(_(\"the --no-curl option to imap-send has been deprecated\"));\n - \n - \tif (!server.port)\n - \t\tserver.port = server.use_ssl ? 993 : 143;\n ++#if defined(NO_OPENSSL)\n + \tif (!use_curl) {\n + \t\twarning(\"--no-curl not supported in this build\");\n + \t\tuse_curl = 1;\n @@ imap-send.c: int cmd_main(int argc, const char **argv)\n \tif (server.tunnel)\n \t\treturn append_msgs_to_imap(&server, &all_msgs, total);\n \n -#ifdef USE_CURL_FOR_IMAP_SEND\n --\tif (use_curl)\n --\t\treturn curl_append_msgs_to_imap(&server, &all_msgs, total);\n + \tif (use_curl)\n + \t\treturn curl_append_msgs_to_imap(&server, &all_msgs, total);\n -#endif\n --\n --\treturn append_msgs_to_imap(&server, &all_msgs, total);\n -+\treturn curl_append_msgs_to_imap(&server, &all_msgs, total);\n + \n + \treturn append_msgs_to_imap(&server, &all_msgs, total);\n }\n-: ----------- > 4: e9cc9bbed1e imap-send: make --curl no-optional\n-: ----------- > 5: 17c75e6381a imap-send: remove old --no-curl codepath\n-: ----------- > 6: 686febb8cdc imap-send: correctly report \"host\" when using \"tunnel\"\n-- \n2.39.1.1392.g63e6d408230\n\n"},{"id":"471320","messageId":"patch-v2-2.6-1dfee9bf08e-20230202T093706Z-avarab@gmail.com","threadId":"59173","inReplyTo":"cover-v2-0.6-00000000000-20230202T093706Z-avarab@gmail.com","subject":"[PATCH v2 2/6] imap-send doc: the imap.sslVerify is used with imap.tunnel","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-02T09:44:13Z","receivedAt":"2023-02-02T09:45:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This documentation added in [1] claims that imap.{host,port,sslVerify}\nis ignored if imap.tunnel is set. That's correct in the first two\ncases, but not for imap.sslVerify.\n\nWhen we're using the tunnel feature we'll call ssl_socket_connect()\nwith a 3rd \"verify\" argument set to the value of the \"imap.sslVerify\"\nconfig if we're on the !preauth path. There is also a call to\nssl_socket_connect() that's specific to the non-tunnel\ncodepath.\n\nPerhaps the documentation added in [1] was written for an earlier\nversion of [2] (which was introduced in the same series). There is an\nearlier version of the patch on-list[3] where there's still a \"FIXME\"\ncomment indicating that we should read the config in the future before\nsetting \"SSL_VERIFY_PEER\", which is what we'll do if \"imap.sslVerify\"\nis set.\n\n1. c82b0748e53 (Documentation: Improve documentation for\n git-imap-send(1), 2008-07-09)\n2. 684ec6c63cd (git-imap-send: Support SSL, 2008-07-09)\n3. https://lore.kernel.org/git/1096648c0806010829n71de92dcmc19ddb87da19931d@mail.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config/imap.txt | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/imap.txt b/Documentation/config/imap.txt\nindex 06166fb5c04..96b1c0927d8 100644\n--- a/Documentation/config/imap.txt\n+++ b/Documentation/config/imap.txt\n@@ -26,8 +26,7 @@ imap.port::\n \n imap.sslverify::\n \tA boolean to enable/disable verification of the server certificate\n-\tused by the SSL/TLS connection. Default is `true`. Ignored when\n-\timap.tunnel is set.\n+\tused by the SSL/TLS connection.\n \n imap.preformattedHTML::\n \tA boolean to enable/disable the use of html encoding when sending\n-- \n2.39.1.1392.g63e6d408230\n\n"},{"id":"471321","messageId":"patch-v2-3.6-354b6a65a78-20230202T093706Z-avarab@gmail.com","threadId":"59173","inReplyTo":"cover-v2-0.6-00000000000-20230202T093706Z-avarab@gmail.com","subject":"[PATCH v2 3/6] imap-send: replace auto-probe libcurl with hard dependency","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-02T09:44:14Z","receivedAt":"2023-02-02T09:45:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the \"imap-send\" command to have a hard dependency on libcurl,\nbefore this it had an optional dependency on both libcurl and OpenSSL,\nnow only the OpenSSL dependency is optional.\n\nThis simplifies our dependency matrix by getting rid of yet another\nspecial-case. Given the prevalence of libcurl and portability of\nlibcurl it seems reasonable to say that \"git imap-send\" cannot be used\nwithout libcurl, almost everyone building git needs to be able to push\nor pull over http(s), so they'll be building with libcurl already.\n\nSo let's remove the previous \"USE_CURL_FOR_IMAP_SEND\" knob. Whether we\nbuild git-imap-send or not is now controlled by the \"NO_CURL\"\nknob.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config/imap.txt | 3 +--\n Documentation/git-imap-send.txt | 3 +--\n INSTALL | 8 ++++----\n Makefile | 18 +++++-------------\n imap-send.c | 23 ++---------------------\n 5 files changed, 13 insertions(+), 42 deletions(-)\n\ndiff --git a/Documentation/config/imap.txt b/Documentation/config/imap.txt\nindex 96b1c0927d8..7f30080c409 100644\n--- a/Documentation/config/imap.txt\n+++ b/Documentation/config/imap.txt\n@@ -37,7 +37,6 @@ imap.preformattedHTML::\n \n imap.authMethod::\n \tSpecify authenticate method for authentication with IMAP server.\n-\tIf Git was built with the NO_CURL option, or if your curl version is older\n-\tthan 7.34.0, or if you're running git-imap-send with the `--no-curl`\n+\tIf you're running git-imap-send with the `--no-curl`\n \toption, the only supported method is 'CRAM-MD5'. If this is not set\n \tthen 'git imap-send' uses the basic IMAP plaintext LOGIN command.\ndiff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt\nindex f7b18515141..202e3e59094 100644\n--- a/Documentation/git-imap-send.txt\n+++ b/Documentation/git-imap-send.txt\n@@ -44,8 +44,7 @@ OPTIONS\n \n --no-curl::\n \tTalk to the IMAP server using git's own IMAP routines instead of\n-\tusing libcurl. Ignored if Git was built with the NO_OPENSSL option\n-\tset.\n+\tusing libcurl.\n \n \n CONFIGURATION\ndiff --git a/INSTALL b/INSTALL\nindex d5694f8c470..d9538bbcb45 100644\n--- a/INSTALL\n+++ b/INSTALL\n@@ -129,13 +129,13 @@ Issues of note:\n \t itself, e.g. Digest::MD5, File::Spec, File::Temp, Net::Domain,\n \t Net::SMTP, and Time::HiRes.\n \n-\t- git-imap-send needs the OpenSSL library to talk IMAP over SSL if\n-\t you are using libcurl older than 7.34.0. Otherwise you can use\n-\t NO_OPENSSL without losing git-imap-send.\n+\t- git-imap-send needs libcurl 7.34.0 or newer, in addition\n+\t OpenSSL is needed if using the \"imap.tunnel\" open to tunnel\n+\t over SSL. Define NO_OPENSSL to omit the OpenSSL prerequisite.\n \n \t- \"libcurl\" library is used for fetching and pushing\n \t repositories over http:// or https://, as well as by\n-\t git-imap-send if the curl version is >= 7.34.0. If you do\n+\t git-imap-send. If you do\n \t not need that functionality, use NO_CURL to build without\n \t it.\n \ndiff --git a/Makefile b/Makefile\nindex 45bd6ac9c3e..b08a855198c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -773,7 +773,9 @@ PROGRAMS += $(EXTRA_PROGRAMS)\n \n PROGRAM_OBJS += daemon.o\n PROGRAM_OBJS += http-backend.o\n+ifndef NO_CURL\n PROGRAM_OBJS += imap-send.o\n+endif\n PROGRAM_OBJS += sh-i18n--envsubst.o\n PROGRAM_OBJS += shell.o\n .PHONY: program-objs\n@@ -1583,7 +1585,6 @@ ifdef HAVE_ALLOCA_H\n \tBASIC_CFLAGS += -DHAVE_ALLOCA_H\n endif\n \n-IMAP_SEND_BUILDDEPS =\n IMAP_SEND_LDFLAGS =\n \n ifdef NO_CURL\n@@ -1592,6 +1593,7 @@ ifdef NO_CURL\n \tREMOTE_CURL_ALIASES =\n \tREMOTE_CURL_NAMES =\n \tEXCLUDED_PROGRAMS += git-http-fetch git-http-push\n+\tEXCLUDED_PROGRAMS += git-imap-send\n else\n \tifdef CURLDIR\n \t\t# Try \"-Wl,-rpath=$(CURLDIR)/$(lib)\" in such a case.\n@@ -1617,19 +1619,9 @@ else\n \tREMOTE_CURL_NAMES = $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_ALIASES)\n \tPROGRAM_OBJS += http-fetch.o\n \tPROGRAMS += $(REMOTE_CURL_NAMES)\n+\tIMAP_SEND_LDFLAGS += $(CURL_LIBCURL)\n \tifndef NO_EXPAT\n \t\tPROGRAM_OBJS += http-push.o\n-\tendif\n-\tcurl_check := $(shell (echo 072200; $(CURL_CONFIG) --vernum | sed -e '/^70[BC]/s/^/0/') 2>/dev/null | sort -r | sed -ne 2p)\n-\tifeq \"$(curl_check)\" \"072200\"\n-\t\tUSE_CURL_FOR_IMAP_SEND = YesPlease\n-\tendif\n-\tifdef USE_CURL_FOR_IMAP_SEND\n-\t\tBASIC_CFLAGS += -DUSE_CURL_FOR_IMAP_SEND\n-\t\tIMAP_SEND_BUILDDEPS = http.o\n-\t\tIMAP_SEND_LDFLAGS += $(CURL_LIBCURL)\n-\tendif\n-\tifndef NO_EXPAT\n \t\tifdef EXPATDIR\n \t\t\tBASIC_CFLAGS += -I$(EXPATDIR)/include\n \t\t\tEXPAT_LIBEXPAT = -L$(EXPATDIR)/$(lib) $(CC_LD_DYNPATH)$(EXPATDIR)/$(lib) -lexpat\n@@ -2786,7 +2778,7 @@ endif\n git-%$X: %.o GIT-LDFLAGS $(GITLIBS)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)\n \n-git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS)\n+git-imap-send$X: imap-send.o http.o GIT-LDFLAGS $(GITLIBS)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n \t\t$(IMAP_SEND_LDFLAGS) $(LIBS)\n \ndiff --git a/imap-send.c b/imap-send.c\nindex b7902babd4c..26f8f01e97a 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -30,20 +30,10 @@\n #if defined(NO_OPENSSL) && !defined(HAVE_OPENSSL_CSPRNG)\n typedef void *SSL;\n #endif\n-#ifdef USE_CURL_FOR_IMAP_SEND\n #include \"http.h\"\n-#endif\n-\n-#if defined(USE_CURL_FOR_IMAP_SEND)\n-/* Always default to curl if it's available. */\n-#define USE_CURL_DEFAULT 1\n-#else\n-/* We don't have curl, so continue to use the historical implementation */\n-#define USE_CURL_DEFAULT 0\n-#endif\n \n static int verbosity;\n-static int use_curl = USE_CURL_DEFAULT;\n+static int use_curl = 1;\n \n static const char * const imap_send_usage[] = { \"git imap-send [-v] [-q] [--[no-]curl] < \", NULL };\n \n@@ -1396,7 +1386,6 @@ static int append_msgs_to_imap(struct imap_server_conf *server,\n \treturn 0;\n }\n \n-#ifdef USE_CURL_FOR_IMAP_SEND\n static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)\n {\n \tCURL *curl;\n@@ -1515,7 +1504,6 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,\n \n \treturn res != CURLE_OK;\n }\n-#endif\n \n int cmd_main(int argc, const char **argv)\n {\n@@ -1531,12 +1519,7 @@ int cmd_main(int argc, const char **argv)\n \tif (argc)\n \t\tusage_with_options(imap_send_usage, imap_send_options);\n \n-#ifndef USE_CURL_FOR_IMAP_SEND\n-\tif (use_curl) {\n-\t\twarning(\"--curl not supported in this build\");\n-\t\tuse_curl = 0;\n-\t}\n-#elif defined(NO_OPENSSL)\n+#if defined(NO_OPENSSL)\n \tif (!use_curl) {\n \t\twarning(\"--no-curl not supported in this build\");\n \t\tuse_curl = 1;\n@@ -1580,10 +1563,8 @@ int cmd_main(int argc, const char **argv)\n \tif (server.tunnel)\n \t\treturn append_msgs_to_imap(&server, &all_msgs, total);\n \n-#ifdef USE_CURL_FOR_IMAP_SEND\n \tif (use_curl)\n \t\treturn curl_append_msgs_to_imap(&server, &all_msgs, total);\n-#endif\n \n \treturn append_msgs_to_imap(&server, &all_msgs, total);\n }\n-- \n2.39.1.1392.g63e6d408230\n\n"},{"id":"471322","messageId":"patch-v2-4.6-e9cc9bbed1e-20230202T093706Z-avarab@gmail.com","threadId":"59173","inReplyTo":"cover-v2-0.6-00000000000-20230202T093706Z-avarab@gmail.com","subject":"[PATCH v2 4/6] imap-send: make --curl no-optional","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-02T09:44:15Z","receivedAt":"2023-02-02T09:45:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In the preceding commit the old \"USE_CURL_FOR_IMAP_SEND\" define became\nalways true, as we now require libcurl for git-imap-send.\n\nBut as we require OpenSSL for the \"tunnel\" mode we still need to keep\nthe OpenSSL codepath around (ee [1] for an attempt to remove it). But\nwe don't need to keep supporting \"--no-curl\" to bypass the curl\ncodepath for the non-tunnel mode.\n\nAs almost all users of \"git\" use a version of it built with libcurl\nwe're making what's already the preferred & default codepath\nmandatory.\n\nThe \"imap.authMethod\" documentation being changed here has always been\nincomplete. It only mentioned \"--no-curl\", but omitted mentioning that\nthe same applied for \"imap.tunnel\". Let's fix it as we're amending it\nto be correct, now (as before) with \"imap.tunnel\" only\n\"imap.authMethod=CRAM-MD5\" is supported.\n\n1. https://lore.kernel.org/git/ab866314-608b-eaca-b335-12cffe165526@morey-chaisemartin.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config/imap.txt | 4 ++--\n Documentation/git-imap-send.txt | 10 ----------\n imap-send.c | 15 ++++-----------\n 3 files changed, 6 insertions(+), 23 deletions(-)\n\ndiff --git a/Documentation/config/imap.txt b/Documentation/config/imap.txt\nindex 7f30080c409..5cc46d87216 100644\n--- a/Documentation/config/imap.txt\n+++ b/Documentation/config/imap.txt\n@@ -37,6 +37,6 @@ imap.preformattedHTML::\n \n imap.authMethod::\n \tSpecify authenticate method for authentication with IMAP server.\n-\tIf you're running git-imap-send with the `--no-curl`\n-\toption, the only supported method is 'CRAM-MD5'. If this is not set\n+\tIf you're using imap.tunnel, the only supported method is 'CRAM-MD5'.\n+\tIf this is not set\n \tthen 'git imap-send' uses the basic IMAP plaintext LOGIN command.\ndiff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt\nindex 202e3e59094..ddbbe819315 100644\n--- a/Documentation/git-imap-send.txt\n+++ b/Documentation/git-imap-send.txt\n@@ -37,16 +37,6 @@ OPTIONS\n --quiet::\n \tBe quiet.\n \n---curl::\n-\tUse libcurl to communicate with the IMAP server, unless tunneling\n-\tinto it. Ignored if Git was built without the USE_CURL_FOR_IMAP_SEND\n-\toption set.\n-\n---no-curl::\n-\tTalk to the IMAP server using git's own IMAP routines instead of\n-\tusing libcurl.\n-\n-\n CONFIGURATION\n -------------\n \ndiff --git a/imap-send.c b/imap-send.c\nindex 26f8f01e97a..9d7cb22285d 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -39,7 +39,7 @@ static const char * const imap_send_usage[] = { \"git imap-send [-v] [-q] [--[no-\n \n static struct option imap_send_options[] = {\n \tOPT__VERBOSITY(&verbosity),\n-\tOPT_BOOL(0, \"curl\", &use_curl, \"use libcurl to communicate with the IMAP server\"),\n+\tOPT_HIDDEN_BOOL(0, \"curl\", &use_curl, \"use libcurl to communicate with the IMAP server\"),\n \tOPT_END()\n };\n \n@@ -1519,12 +1519,8 @@ int cmd_main(int argc, const char **argv)\n \tif (argc)\n \t\tusage_with_options(imap_send_usage, imap_send_options);\n \n-#if defined(NO_OPENSSL)\n-\tif (!use_curl) {\n-\t\twarning(\"--no-curl not supported in this build\");\n-\t\tuse_curl = 1;\n-\t}\n-#endif\n+\tif (!use_curl)\n+\t\tdie(_(\"the --no-curl option to imap-send has been deprecated\"));\n \n \tif (!server.port)\n \t\tserver.port = server.use_ssl ? 993 : 143;\n@@ -1560,10 +1556,7 @@ int cmd_main(int argc, const char **argv)\n \n \t/* write it to the imap server */\n \n-\tif (server.tunnel)\n-\t\treturn append_msgs_to_imap(&server, &all_msgs, total);\n-\n-\tif (use_curl)\n+\tif (!server.tunnel)\n \t\treturn curl_append_msgs_to_imap(&server, &all_msgs, total);\n \n \treturn append_msgs_to_imap(&server, &all_msgs, total);\n-- \n2.39.1.1392.g63e6d408230\n\n"},{"id":"471323","messageId":"patch-v2-6.6-686febb8cdc-20230202T093706Z-avarab@gmail.com","threadId":"59173","inReplyTo":"cover-v2-0.6-00000000000-20230202T093706Z-avarab@gmail.com","subject":"[PATCH v2 6/6] imap-send: correctly report \"host\" when using \"tunnel\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-02T09:44:17Z","receivedAt":"2023-02-02T09:45:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Before [1] we'd force the \"imap.host\" to be set, even if the\n\"imap.tunnel\" was set, and then proceed to not use the \"host\" for\nestablishing a connection, as we'd use the tunneling command.\n\nHowever, we'd still use the \"imap.host\" if it was set as the \"host\"\nfield given to the credential helper, and in messages that were shared\nwith the non-tunnel mode, until a preceding commit made these OpenSSL\ncodepaths tunnel-only.\n\nLet's always give \"host=tunnel\" to the credential helper when in the\n\"imap.tunnel\" mode, and rephrase the relevant messages to indicate\nthat we're tunneling. This changes the existing behavior, but that\nbehavior was emergent and didn't make much sense. If we were using\n\"imap.tunnel\" the value in \"imap.host\" might be entirely unrelated to\nthe host we're tunneling to. Let's not pretend to know more than we do\nin that case.\n\n1. 34b5cd1fe9f (Don't force imap.host to be set when imap.tunnel is\n set, 2008-04-22)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n imap-send.c | 17 +++++++----------\n 1 file changed, 7 insertions(+), 10 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 9712a8d4f93..24b30c143a7 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -917,7 +917,7 @@ static void server_fill_credential(struct imap_server_conf *srvc, struct credent\n \t\treturn;\n \n \tcred->protocol = xstrdup(srvc->use_ssl ? \"imaps\" : \"imap\");\n-\tcred->host = xstrdup(srvc->host);\n+\tcred->host = xstrdup(srvc->tunnel ? \"tunnel\" : srvc->host);\n \n \tcred->username = xstrdup_or_null(srvc->user);\n \tcred->password = xstrdup_or_null(srvc->pass);\n@@ -1004,7 +1004,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c\n \t\t\t\tif (!CAP(AUTH_CRAM_MD5)) {\n \t\t\t\t\tfprintf(stderr, \"You specified \"\n \t\t\t\t\t\t\"CRAM-MD5 as authentication method, \"\n-\t\t\t\t\t\t\"but %s doesn't support it.\\n\", srvc->host);\n+\t\t\t\t\t\t\"but tunnel doesn't support it.\\n\");\n \t\t\t\t\tgoto bail;\n \t\t\t\t}\n \t\t\t\t/* CRAM-MD5 */\n@@ -1021,8 +1021,8 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c\n \t\t\t}\n \t\t} else {\n \t\t\tif (CAP(NOLOGIN)) {\n-\t\t\t\tfprintf(stderr, \"Skipping account %s@%s, server forbids LOGIN\\n\",\n-\t\t\t\t\tsrvc->user, srvc->host);\n+\t\t\t\tfprintf(stderr, \"Skipping account %s, server forbids LOGIN\\n\",\n+\t\t\t\t\tsrvc->user);\n \t\t\t\tgoto bail;\n \t\t\t}\n \t\t\tif (!imap->buf.sock.ssl)\n@@ -1434,12 +1434,9 @@ int cmd_main(int argc, const char **argv)\n \t\tfprintf(stderr, \"no imap store specified\\n\");\n \t\treturn 1;\n \t}\n-\tif (!server.host) {\n-\t\tif (!server.tunnel) {\n-\t\t\tfprintf(stderr, \"no imap host specified\\n\");\n-\t\t\treturn 1;\n-\t\t}\n-\t\tserver.host = \"tunnel\";\n+\tif (!server.host && !server.tunnel) {\n+\t\tfprintf(stderr, \"no imap host specified\\n\");\n+\t\treturn 1;\n \t}\n \n \t/* read the messages */\n-- \n2.39.1.1392.g63e6d408230\n\n"},{"id":"471324","messageId":"patch-v2-5.6-17c75e6381a-20230202T093706Z-avarab@gmail.com","threadId":"59173","inReplyTo":"cover-v2-0.6-00000000000-20230202T093706Z-avarab@gmail.com","subject":"[PATCH v2 5/6] imap-send: remove old --no-curl codepath","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-02T09:44:16Z","receivedAt":"2023-02-02T09:45:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In the preceding the \"--curl\" codepath was made mandatory, so now we\nwon't use the OpenSSL implementation codepaths in imap-send.c except\nfor \"imap.tunnel\".\n\nSo let's follow-up and delete the code on that path which was specific\nto the \"imap.host\" mode.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n imap-send.c | 127 +++++++---------------------------------------------\n 1 file changed, 16 insertions(+), 111 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 9d7cb22285d..9712a8d4f93 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -197,14 +197,7 @@ static void socket_perror(const char *func, struct imap_socket *sock, int ret)\n \t}\n }\n \n-#ifdef NO_OPENSSL\n-static int ssl_socket_connect(struct imap_socket *sock, int use_tls_only, int verify)\n-{\n-\tfprintf(stderr, \"SSL requested but SSL support not compiled in\\n\");\n-\treturn -1;\n-}\n-\n-#else\n+#ifndef NO_OPENSSL\n \n static int host_matches(const char *host, const char *pattern)\n {\n@@ -253,7 +246,7 @@ static int verify_hostname(X509 *cert, const char *hostname)\n \t\t cname, hostname);\n }\n \n-static int ssl_socket_connect(struct imap_socket *sock, int use_tls_only, int verify)\n+static int ssl_socket_connect(struct imap_socket *sock, int verify)\n {\n #if (OPENSSL_VERSION_NUMBER >= 0x10000000L)\n \tconst SSL_METHOD *meth;\n@@ -279,8 +272,7 @@ static int ssl_socket_connect(struct imap_socket *sock, int use_tls_only, int ve\n \t\treturn -1;\n \t}\n \n-\tif (use_tls_only)\n-\t\tSSL_CTX_set_options(ctx, SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3);\n+\tSSL_CTX_set_options(ctx, SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3);\n \n \tif (verify)\n \t\tSSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, NULL);\n@@ -944,7 +936,8 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c\n \tstruct imap_store *ctx;\n \tstruct imap *imap;\n \tchar *arg, *rsp;\n-\tint s = -1, preauth;\n+\tint preauth;\n+\tstruct child_process tunnel = CHILD_PROCESS_INIT;\n \n \tCALLOC_ARRAY(ctx, 1);\n \n@@ -953,107 +946,19 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c\n \timap->in_progress_append = &imap->in_progress;\n \n \t/* open connection to IMAP server */\n+\timap_info(\"Starting tunnel '%s'... \", srvc->tunnel);\n \n-\tif (srvc->tunnel) {\n-\t\tstruct child_process tunnel = CHILD_PROCESS_INIT;\n-\n-\t\timap_info(\"Starting tunnel '%s'... \", srvc->tunnel);\n-\n-\t\tstrvec_push(&tunnel.args, srvc->tunnel);\n-\t\ttunnel.use_shell = 1;\n-\t\ttunnel.in = -1;\n-\t\ttunnel.out = -1;\n-\t\tif (start_command(&tunnel))\n-\t\t\tdie(\"cannot start proxy %s\", srvc->tunnel);\n-\n-\t\timap->buf.sock.fd[0] = tunnel.out;\n-\t\timap->buf.sock.fd[1] = tunnel.in;\n-\n-\t\timap_info(\"ok\\n\");\n-\t} else {\n-#ifndef NO_IPV6\n-\t\tstruct addrinfo hints, *ai0, *ai;\n-\t\tint gai;\n-\t\tchar portstr[6];\n-\n-\t\txsnprintf(portstr, sizeof(portstr), \"%d\", srvc->port);\n-\n-\t\tmemset(&hints, 0, sizeof(hints));\n-\t\thints.ai_socktype = SOCK_STREAM;\n-\t\thints.ai_protocol = IPPROTO_TCP;\n-\n-\t\timap_info(\"Resolving %s... \", srvc->host);\n-\t\tgai = getaddrinfo(srvc->host, portstr, &hints, &ai);\n-\t\tif (gai) {\n-\t\t\tfprintf(stderr, \"getaddrinfo: %s\\n\", gai_strerror(gai));\n-\t\t\tgoto bail;\n-\t\t}\n-\t\timap_info(\"ok\\n\");\n-\n-\t\tfor (ai0 = ai; ai; ai = ai->ai_next) {\n-\t\t\tchar addr[NI_MAXHOST];\n-\n-\t\t\ts = socket(ai->ai_family, ai->ai_socktype,\n-\t\t\t\t ai->ai_protocol);\n-\t\t\tif (s < 0)\n-\t\t\t\tcontinue;\n+\tstrvec_push(&tunnel.args, srvc->tunnel);\n+\ttunnel.use_shell = 1;\n+\ttunnel.in = -1;\n+\ttunnel.out = -1;\n+\tif (start_command(&tunnel))\n+\t\tdie(\"cannot start proxy %s\", srvc->tunnel);\n \n-\t\t\tgetnameinfo(ai->ai_addr, ai->ai_addrlen, addr,\n-\t\t\t\t sizeof(addr), NULL, 0, NI_NUMERICHOST);\n-\t\t\timap_info(\"Connecting to [%s]:%s... \", addr, portstr);\n+\timap->buf.sock.fd[0] = tunnel.out;\n+\timap->buf.sock.fd[1] = tunnel.in;\n \n-\t\t\tif (connect(s, ai->ai_addr, ai->ai_addrlen) < 0) {\n-\t\t\t\tclose(s);\n-\t\t\t\ts = -1;\n-\t\t\t\tperror(\"connect\");\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\n-\t\t\tbreak;\n-\t\t}\n-\t\tfreeaddrinfo(ai0);\n-#else /* NO_IPV6 */\n-\t\tstruct hostent *he;\n-\t\tstruct sockaddr_in addr;\n-\n-\t\tmemset(&addr, 0, sizeof(addr));\n-\t\taddr.sin_port = htons(srvc->port);\n-\t\taddr.sin_family = AF_INET;\n-\n-\t\timap_info(\"Resolving %s... \", srvc->host);\n-\t\the = gethostbyname(srvc->host);\n-\t\tif (!he) {\n-\t\t\tperror(\"gethostbyname\");\n-\t\t\tgoto bail;\n-\t\t}\n-\t\timap_info(\"ok\\n\");\n-\n-\t\taddr.sin_addr.s_addr = *((int *) he->h_addr_list[0]);\n-\n-\t\ts = socket(PF_INET, SOCK_STREAM, 0);\n-\n-\t\timap_info(\"Connecting to %s:%hu... \", inet_ntoa(addr.sin_addr), ntohs(addr.sin_port));\n-\t\tif (connect(s, (struct sockaddr *)&addr, sizeof(addr))) {\n-\t\t\tclose(s);\n-\t\t\ts = -1;\n-\t\t\tperror(\"connect\");\n-\t\t}\n-#endif\n-\t\tif (s < 0) {\n-\t\t\tfputs(\"Error: unable to connect to server.\\n\", stderr);\n-\t\t\tgoto bail;\n-\t\t}\n-\n-\t\timap->buf.sock.fd[0] = s;\n-\t\timap->buf.sock.fd[1] = dup(s);\n-\n-\t\tif (srvc->use_ssl &&\n-\t\t ssl_socket_connect(&imap->buf.sock, 0, srvc->ssl_verify)) {\n-\t\t\tclose(s);\n-\t\t\tgoto bail;\n-\t\t}\n-\t\timap_info(\"ok\\n\");\n-\t}\n+\timap_info(\"ok\\n\");\n \n \t/* read the greeting string */\n \tif (buffer_gets(&imap->buf, &rsp)) {\n@@ -1081,7 +986,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c\n \t\tif (!srvc->use_ssl && CAP(STARTTLS)) {\n \t\t\tif (imap_exec(ctx, NULL, \"STARTTLS\") != RESP_OK)\n \t\t\t\tgoto bail;\n-\t\t\tif (ssl_socket_connect(&imap->buf.sock, 1,\n+\t\t\tif (ssl_socket_connect(&imap->buf.sock,\n \t\t\t\t\t srvc->ssl_verify))\n \t\t\t\tgoto bail;\n \t\t\t/* capabilities may have changed, so get the new capabilities */\n-- \n2.39.1.1392.g63e6d408230\n\n"},{"id":"471381","messageId":"xmqq7cwzvq5m.fsf@gitster.g","threadId":"59173","inReplyTo":"patch-v2-1.6-3187a643035-20230202T093706Z-avarab@gmail.com","subject":"Re: [PATCH v2 1/6] imap-send: note \"auth_method\", not \"host\" on auth method failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-02T19:11:17Z","receivedAt":"2023-02-02T19:11:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Fix error reporting added in ae9c606ed22 (imap-send: support CRAM-MD5\n> authentication, 2010-02-15), the use of \"srvc->host\" here was\n> seemingly copy/pasted from other uses added in the same commit.\n\nObviously correct ;-).\n"},{"id":"471387","messageId":"xmqqk00zuakx.fsf@gitster.g","threadId":"59173","inReplyTo":"patch-v2-3.6-354b6a65a78-20230202T093706Z-avarab@gmail.com","subject":"Re: [PATCH v2 3/6] imap-send: replace auto-probe libcurl with hard dependency","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-02T19:33:02Z","receivedAt":"2023-02-02T19:33:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"> imap.authMethod::\n> \tSpecify authenticate method for authentication with IMAP server.\n> -\tIf Git was built with the NO_CURL option, or if your curl version is older\n> -\tthan 7.34.0, or if you're running git-imap-send with the `--no-curl`\n> +\tIf you're running git-imap-send with the `--no-curl`\n> \toption, the only supported method is 'CRAM-MD5'. If this is not set\n> \tthen 'git imap-send' uses the basic IMAP plaintext LOGIN command.\n\nOK.\n\n> diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt\n> index f7b18515141..202e3e59094 100644\n> --- a/Documentation/git-imap-send.txt\n> +++ b/Documentation/git-imap-send.txt\n> @@ -44,8 +44,7 @@ OPTIONS\n> \n> --no-curl::\n> \tTalk to the IMAP server using git's own IMAP routines instead of\n> -\tusing libcurl. Ignored if Git was built with the NO_OPENSSL option\n> -\tset.\n> +\tusing libcurl.\n\nHmph, let's read on to resolve \"when built with NO_OPENSSL, giving\n--no-curl now errors out or do something else? do we need to? why?\",\nwhich was my knee-jerk reaction.\n\n> diff --git a/INSTALL b/INSTALL\n> index d5694f8c470..d9538bbcb45 100644\n> --- a/INSTALL\n> +++ b/INSTALL\n> @@ -129,13 +129,13 @@ Issues of note:\n> \t itself, e.g. Digest::MD5, File::Spec, File::Temp, Net::Domain,\n> \t Net::SMTP, and Time::HiRes.\n> \n> -\t- git-imap-send needs the OpenSSL library to talk IMAP over SSL if\n> -\t you are using libcurl older than 7.34.0. Otherwise you can use\n> -\t NO_OPENSSL without losing git-imap-send.\n> +\t- git-imap-send needs libcurl 7.34.0 or newer, in addition\n> +\t OpenSSL is needed if using the \"imap.tunnel\" open to tunnel\n> +\t over SSL. Define NO_OPENSSL to omit the OpenSSL prerequisite.\n\n\"if using the imap.tunnel to open a tunnel over ssl\"?\nbecause I think there are some grammo there.\n\n\"to omit the OpenSSL prerequisite\" -> \"if you do not need it\"?\nbecause the original sounds like losing prereq without any penalty.\n\n> \t- \"libcurl\" library is used for fetching and pushing\n> \t repositories over http:// or https://, as well as by\n> -\t git-imap-send if the curl version is >= 7.34.0. If you do\n> +\t git-imap-send. If you do\n> \t not need that functionality, use NO_CURL to build without\n> \t it.\n\nOK.\n\n> diff --git a/Makefile b/Makefile\n> index 45bd6ac9c3e..b08a855198c 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -773,7 +773,9 @@ PROGRAMS += $(EXTRA_PROGRAMS)\n> \n> PROGRAM_OBJS += daemon.o\n> PROGRAM_OBJS += http-backend.o\n> +ifndef NO_CURL\n> PROGRAM_OBJS += imap-send.o\n> +endif\n\nNice.\n\n> @@ -1592,6 +1593,7 @@ ifdef NO_CURL\n> \tREMOTE_CURL_ALIASES =\n> \tREMOTE_CURL_NAMES =\n> \tEXCLUDED_PROGRAMS += git-http-fetch git-http-push\n> +\tEXCLUDED_PROGRAMS += git-imap-send\n\nOK.\n\n> @@ -1617,19 +1619,9 @@ else\n> \tREMOTE_CURL_NAMES = $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_ALIASES)\n> \tPROGRAM_OBJS += http-fetch.o\n> \tPROGRAMS += $(REMOTE_CURL_NAMES)\n> +\tIMAP_SEND_LDFLAGS += $(CURL_LIBCURL)\n\nOK. That is a natural consequence of losing USE_CURL_FOR_IMAP_SEND\nconditional, which is good.\n\n> @@ -2786,7 +2778,7 @@ endif\n> git-%$X: %.o GIT-LDFLAGS $(GITLIBS)\n> \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)\n> \n> -git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS)\n> +git-imap-send$X: imap-send.o http.o GIT-LDFLAGS $(GITLIBS)\n\nAnd this too is a natural consequence of http.o that serves as a\nlinkage between us and libcURL being always used. Good.\n\n> diff --git a/imap-send.c b/imap-send.c\n> index b7902babd4c..26f8f01e97a 100644\n> --- a/imap-send.c\n> +++ b/imap-send.c\n> @@ -30,20 +30,10 @@\n> ...\n> +static int use_curl = 1;\n\nOK.\n\n> int cmd_main(int argc, const char **argv)\n> {\n> @@ -1531,12 +1519,7 @@ int cmd_main(int argc, const char **argv)\n> \tif (argc)\n> \t\tusage_with_options(imap_send_usage, imap_send_options);\n> \n> -#ifndef USE_CURL_FOR_IMAP_SEND\n> -\tif (use_curl) {\n> -\t\twarning(\"--curl not supported in this build\");\n> -\t\tuse_curl = 0;\n> -\t}\n\nNaturally ;-)\n\n> -#elif defined(NO_OPENSSL)\n> +#if defined(NO_OPENSSL)\n> \tif (!use_curl) {\n> \t\twarning(\"--no-curl not supported in this build\");\n> \t\tuse_curl = 1;\n\nIn the original, this part reached iff we had USE_CURL_FOR_IMAP_SEND,\nso \"if we are using curl and do not have openssl, then --no-curl is\nrejected and we forced use of curl\" was how the original behaved here.\n\nHere, we always link with curl, so the updated code is doing exactly\nthe same thing. Good.\n\nBut then the documentation change above that puzzled me was there\nnot because it was needed to match updated behaviour (the behaviour\nstayed the same). So was it meant as a documentation improvement?\n\n> @@ -1580,10 +1563,8 @@ int cmd_main(int argc, const char **argv)\n> \tif (server.tunnel)\n> \t\treturn append_msgs_to_imap(&server, &all_msgs, total);\n> \n> -#ifdef USE_CURL_FOR_IMAP_SEND\n> \tif (use_curl)\n> \t\treturn curl_append_msgs_to_imap(&server, &all_msgs, total);\n> -#endif\n\nNaturally.\n\n> \treturn append_msgs_to_imap(&server, &all_msgs, total);\n> }\n\nLooking good. Thanks.\n"},{"id":"471389","messageId":"xmqqfsbnu7dk.fsf@gitster.g","threadId":"59173","inReplyTo":"patch-v2-4.6-e9cc9bbed1e-20230202T093706Z-avarab@gmail.com","subject":"Re: [PATCH v2 4/6] imap-send: make --curl no-optional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-02T20:42:15Z","receivedAt":"2023-02-02T20:42:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> In the preceding commit the old \"USE_CURL_FOR_IMAP_SEND\" define became\n> always true, as we now require libcurl for git-imap-send.\n>\n> But as we require OpenSSL for the \"tunnel\" mode we still need to keep\n> the OpenSSL codepath around (ee [1] for an attempt to remove it). But\n\n\"(ee\" -> ???\n\n> we don't need to keep supporting \"--no-curl\" to bypass the curl\n> codepath for the non-tunnel mode.\n\nWe do not need to because...?\n\n> @@ -1519,12 +1519,8 @@ int cmd_main(int argc, const char **argv)\n> \tif (argc)\n> \t\tusage_with_options(imap_send_usage, imap_send_options);\n> \n> -#if defined(NO_OPENSSL)\n> -\tif (!use_curl) {\n> -\t\twarning(\"--no-curl not supported in this build\");\n> -\t\tuse_curl = 1;\n> -\t}\n> -#endif\n> +\tif (!use_curl)\n> +\t\tdie(_(\"the --no-curl option to imap-send has been deprecated\"));\n\nWe used to force use of cURL when there is no other way to make the\nprogram work (i.e. there is no direct OpenSSL codepath available),\ninstead of refusing to work (and forcing user to say --curl or to\nstop saying --no-curl, which is one unnecessary roadblock for the\nuser). Why do we want to change the error handling strategy that\nhas been in place?\n\nI think I made the same comment in some other thread, but the\nprinciple is the same. If there is no other choice the user can\ntake, do we force users to stop and be explicit to choose that only\navailable choice, or do we let the program choose the only available\noption for the user while clearly telling the user that is what we\ndid? Here, changing the behaviour sounds like a disservice to the\nusers.\n\n"},{"id":"471390","messageId":"xmqqbkmbu7bx.fsf@gitster.g","threadId":"59173","inReplyTo":"patch-v2-5.6-17c75e6381a-20230202T093706Z-avarab@gmail.com","subject":"Re: [PATCH v2 5/6] imap-send: remove old --no-curl codepath","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-02T20:43:14Z","receivedAt":"2023-02-02T20:43:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> In the preceding the \"--curl\" codepath was made mandatory, so now we\n> won't use the OpenSSL implementation codepaths in imap-send.c except\n> for \"imap.tunnel\".\n>\n> So let's follow-up and delete the code on that path which was specific\n> to the \"imap.host\" mode.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> imap-send.c | 127 +++++++---------------------------------------------\n> 1 file changed, 16 insertions(+), 111 deletions(-)\n\nNice code reduction.\n"},{"id":"471393","messageId":"xmqq5ycju6q1.fsf@gitster.g","threadId":"59173","inReplyTo":"patch-v2-6.6-686febb8cdc-20230202T093706Z-avarab@gmail.com","subject":"Re: [PATCH v2 6/6] imap-send: correctly report \"host\" when using \"tunnel\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-02T20:56:22Z","receivedAt":"2023-02-02T20:56:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Before [1] we'd force the \"imap.host\" to be set, even if the\n> \"imap.tunnel\" was set, and then proceed to not use the \"host\" for\n> establishing a connection, as we'd use the tunneling command.\n>\n> However, we'd still use the \"imap.host\" if it was set as the \"host\"\n> field given to the credential helper, and in messages that were shared\n> with the non-tunnel mode, until a preceding commit made these OpenSSL\n> codepaths tunnel-only.\n>\n> Let's always give \"host=tunnel\" to the credential helper when in the\n> \"imap.tunnel\" mode, and rephrase the relevant messages to indicate\n> that we're tunneling. This changes the existing behavior, but that\n> behavior was emergent and didn't make much sense. If we were using\n> \"imap.tunnel\" the value in \"imap.host\" might be entirely unrelated to\n> the host we're tunneling to. Let's not pretend to know more than we do\n> in that case.\n>\n> 1. 34b5cd1fe9f (Don't force imap.host to be set when imap.tunnel is\n> set, 2008-04-22)\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n\nI agree with the above flow of thought in principle, but I wonder if\n\"tunnel\" is distinct enough to allow credential helpers to tell that\nthey are dealing with a \"tunnel\", and not a host whose name happens\nto be \"tunnel\". Would it help to use a token that can never be a\nvalid hostname instead, I wonder?\n\n> @@ -1004,7 +1004,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c\n> \t\t\t\tif (!CAP(AUTH_CRAM_MD5)) {\n> \t\t\t\t\tfprintf(stderr, \"You specified \"\n> \t\t\t\t\t\t\"CRAM-MD5 as authentication method, \"\n> -\t\t\t\t\t\t\"but %s doesn't support it.\\n\", srvc->host);\n> +\t\t\t\t\t\t\"but tunnel doesn't support it.\\n\");\n\nDo we need some article before \"tunnel\"?\n\n> \t\t\tif (CAP(NOLOGIN)) {\n> -\t\t\t\tfprintf(stderr, \"Skipping account %s@%s, server forbids LOGIN\\n\",\n> -\t\t\t\t\tsrvc->user, srvc->host);\n> +\t\t\t\tfprintf(stderr, \"Skipping account %s, server forbids LOGIN\\n\",\n> +\t\t\t\t\tsrvc->user);\n> \t\t\t\tgoto bail;\n\nOK. We are talking to whatever \"tunnel\" is that was spawned to talk\nsomewhere we do not have a way to know, so trying to say \nat is futile. Makes sense.\n\n> -\tif (!server.host) {\n> -\t\tif (!server.tunnel) {\n> -\t\t\tfprintf(stderr, \"no imap host specified\\n\");\n> -\t\t\treturn 1;\n> -\t\t}\n> -\t\tserver.host = \"tunnel\";\n> +\tif (!server.host && !server.tunnel) {\n> +\t\tfprintf(stderr, \"no imap host specified\\n\");\n> +\t\treturn 1;\n> \t}\n\nOK, this is a natural consequence that we no longer abuse\nserver.host in the tunneling case. Makes sense.\n\nThanks. Will queue.\n"},{"id":"471451","messageId":"Y91J+P5P9gV1Dygm@coredump.intra.peff.net","threadId":"59173","inReplyTo":"patch-v2-6.6-686febb8cdc-20230202T093706Z-avarab@gmail.com","subject":"Re: [PATCH v2 6/6] imap-send: correctly report \"host\" when using \"tunnel\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-03T17:53:17Z","receivedAt":"2023-02-03T17:53:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 02, 2023 at 10:44:17AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> Before [1] we'd force the \"imap.host\" to be set, even if the\n> \"imap.tunnel\" was set, and then proceed to not use the \"host\" for\n> establishing a connection, as we'd use the tunneling command.\n> \n> However, we'd still use the \"imap.host\" if it was set as the \"host\"\n> field given to the credential helper, and in messages that were shared\n> with the non-tunnel mode, until a preceding commit made these OpenSSL\n> codepaths tunnel-only.\n> \n> Let's always give \"host=tunnel\" to the credential helper when in the\n> \"imap.tunnel\" mode, and rephrase the relevant messages to indicate\n> that we're tunneling. This changes the existing behavior, but that\n> behavior was emergent and didn't make much sense. If we were using\n> \"imap.tunnel\" the value in \"imap.host\" might be entirely unrelated to\n> the host we're tunneling to. Let's not pretend to know more than we do\n> in that case.\n\nIf you tunnel to two different hosts, how is the credential system\nsupposed to know which is which?\n\nIf you really want to distinguish connecting to $host versus tunneling\nto $host, I think you'd have to invent some new URL scheme\n(imap-tunnel:// or something).\n\nBut IMHO it is not really worth it. Your statement of \"the value in\nimap.host might be entirely unrelated\" does not match my experience. I\ndon't use imap-send, but I've been doing imap-tunneling with various\nprograms for two decades, and it's pretty normal to configure both, and\nto consider the tunnel command as an implementation detail for getting\nto the host. For example, my mutt config is like[1]:\n\n set folder = imap://example.com/\n set tunnel = \"ssh example.com /etc/rimapd\"\n\nand I expect to be able to refer to folders as imap://example.com/foo,\netc (well, in mutt you'd use the shorthand \"=foo\", but the idea is the\nsame). So if we see:\n\n [imap]\n host = example.com\n tunnel = ssh example.com /etc/rimapd\n\nwe should likewise think of it as example.com, but with an\nimplementation detail of how to contact the server.\n\nOf course if you don't set imap.host, then we don't have anything useful\nto say. But as you saw, in that case imap-send will default the host\nfield to the word \"tunnel\".\n\n-Peff\n\n[1] In my experience the main reason to tunnel is to avoid auth\n altogether, so for those cases the credential code wouldn't matter\n either way. But I imagine there may be some people who use it to pierce\n a firewall or some other network obstacle, and really do want it to\n be otherwise just like a connection to $host.\n"},{"id":"471460","messageId":"230203.86bkmabfjr.gmgdl@evledraar.gmail.com","threadId":"59173","inReplyTo":"Y91J+P5P9gV1Dygm@coredump.intra.peff.net","subject":"Re: [PATCH v2 6/6] imap-send: correctly report \"host\" when using \"tunnel\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-03T21:12:27Z","receivedAt":"2023-02-03T21:33:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Feb 03 2023, Jeff King wrote:\n\n> On Thu, Feb 02, 2023 at 10:44:17AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Before [1] we'd force the \"imap.host\" to be set, even if the\n>> \"imap.tunnel\" was set, and then proceed to not use the \"host\" for\n>> establishing a connection, as we'd use the tunneling command.\n>> \n>> However, we'd still use the \"imap.host\" if it was set as the \"host\"\n>> field given to the credential helper, and in messages that were shared\n>> with the non-tunnel mode, until a preceding commit made these OpenSSL\n>> codepaths tunnel-only.\n>> \n>> Let's always give \"host=tunnel\" to the credential helper when in the\n>> \"imap.tunnel\" mode, and rephrase the relevant messages to indicate\n>> that we're tunneling. This changes the existing behavior, but that\n>> behavior was emergent and didn't make much sense. If we were using\n>> \"imap.tunnel\" the value in \"imap.host\" might be entirely unrelated to\n>> the host we're tunneling to. Let's not pretend to know more than we do\n>> in that case.\n>\n> If you tunnel to two different hosts, how is the credential system\n> supposed to know which is which?\n>\n> If you really want to distinguish connecting to $host versus tunneling\n> to $host, I think you'd have to invent some new URL scheme\n> (imap-tunnel:// or something).\n>\n> But IMHO it is not really worth it. Your statement of \"the value in\n> imap.host might be entirely unrelated\" does not match my experience. I\n> don't use imap-send, but I've been doing imap-tunneling with various\n> programs for two decades, and it's pretty normal to configure both, and\n> to consider the tunnel command as an implementation detail for getting\n> to the host. For example, my mutt config is like[1]:\n>\n> set folder = imap://example.com/\n> set tunnel = \"ssh example.com /etc/rimapd\"\n>\n> and I expect to be able to refer to folders as imap://example.com/foo,\n> etc (well, in mutt you'd use the shorthand \"=foo\", but the idea is the\n> same). So if we see:\n>\n> [imap]\n> host = example.com\n> tunnel = ssh example.com /etc/rimapd\n>\n> we should likewise think of it as example.com, but with an\n> implementation detail of how to contact the server.\n\nExcept that mutt config is different than the imap-send case in that it\nwould presumably break if you changed:\n\n\tset folder = imap://example.com/\n\tset tunnel = \"ssh example.com /etc/rimapd\"\n\nSo that one of the two was example.org instead of example.com (or\nwhatever).\n\nI agree that \"give this to the auth helper\" might be useful in general,\nbut our current documentation says:\n\n\tTo use the tool, `imap.folder` and either `imap.tunnel` or `imap.host` must be set\n\tto appropriate values.\n\nAnd the docs for \"imap.tunnel\" say \"Required when imap.host is not set\",\nand \"imap.host\" says \"Ignored when imap.tunnel is set, but required\notherwise\".\n\nPerhaps we should bless this as an accidental feature instead of my\nproposed patch, but that's why I made this change. It seemed like an\nunintentional bug that nobody intended.\n\nEspecially as you're focusing on the case where it contrary to the docs\nwould do what you mean, but consider (same as the doc examples, but the\ndomains are changed):\n\n\t[imap]\n\t folder = \"INBOX.Drafts\"\n\t host = imap://imap.bar.com\n\t user = bob\n\t pass = p4ssw0rd\n\n\t[imap]\n\t folder = \"INBOX.Drafts\"\n\t tunnel = \"ssh -q -C user@foo.com /usr/bin/imapd ./Maildir 2> /dev/null\"\n\t\nI.e. I have a config for \"bar.com\" I tried earlier, but now I'm trying\nto connect to \"foo.com\", because I read the docs and notice it prefers\n\"tunnel\" to \"host\" I think it's going to ignore that \"imap.host\", but\nit's going to provide the password for bar.com to foo.com if challenged.\n\nSo I think if we want to keep this it would be better to have a\nimap.tunnel.credentialHost or something, to avoid conflating the two.\n\nBut I think it's okey to just remove this until someone has this\nexplicit use-case. I doubt that we have any users relying on this, as\nit's not only undocumented, but the documentation explicitly states that\nit doesn't work like this.\n\n> Of course if you don't set imap.host, then we don't have anything useful\n> to say. But as you saw, in that case imap-send will default the host\n> field to the word \"tunnel\".\n\nIsn't that more of a suggestion that nobody cares about this? Presumably\nif we had users trying to get this to work someone would have complained\nthat they wanted a custom string rather than \"tunnel\", as the auth\nhelper isn't very helpful in that case...\n"},{"id":"471461","messageId":"230203.867cwyberv.gmgdl@evledraar.gmail.com","threadId":"59173","inReplyTo":"xmqqfsbnu7dk.fsf@gitster.g","subject":"Re: [PATCH v2 4/6] imap-send: make --curl no-optional","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-03T21:46:11Z","receivedAt":"2023-02-03T21:49:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Feb 02 2023, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> In the preceding commit the old \"USE_CURL_FOR_IMAP_SEND\" define became\n>> always true, as we now require libcurl for git-imap-send.\n>>\n>> But as we require OpenSSL for the \"tunnel\" mode we still need to keep\n>> the OpenSSL codepath around (ee [1] for an attempt to remove it). But\n>\n> \"(ee\" -> ???\n\nShould be \"e.g.\", will fix.\n\n>> we don't need to keep supporting \"--no-curl\" to bypass the curl\n>> codepath for the non-tunnel mode.\n>\n> We do not need to because...?\n\nWe don't have that code anymore, will clarify.\n\n>> @@ -1519,12 +1519,8 @@ int cmd_main(int argc, const char **argv)\n>> \tif (argc)\n>> \t\tusage_with_options(imap_send_usage, imap_send_options);\n>> \n>> -#if defined(NO_OPENSSL)\n>> -\tif (!use_curl) {\n>> -\t\twarning(\"--no-curl not supported in this build\");\n>> -\t\tuse_curl = 1;\n>> -\t}\n>> -#endif\n>> +\tif (!use_curl)\n>> +\t\tdie(_(\"the --no-curl option to imap-send has been deprecated\"));\n>\n> We used to force use of cURL when there is no other way to make the\n> program work (i.e. there is no direct OpenSSL codepath available),\n> instead of refusing to work (and forcing user to say --curl or to\n> stop saying --no-curl, which is one unnecessary roadblock for the\n> user). Why do we want to change the error handling strategy that\n> has been in place?\n\nI can change this to a soft error, but it seemed more sensible to rip\nthe band-aid off an option that's never going to do anything now,\nwhereas before it would do something based on how you compiled git.\n\n> I think I made the same comment in some other thread, but the\n> principle is the same. If there is no other choice the user can\n> take, do we force users to stop and be explicit to choose that only\n> available choice, or do we let the program choose the only available\n> option for the user while clearly telling the user that is what we\n> did? Here, changing the behaviour sounds like a disservice to the\n> users.\n\nAt best we can make --no-curl use curl anyway with a warning, would that\nbe better?\n"},{"id":"471473","messageId":"xmqqy1peknrt.fsf@gitster.g","threadId":"59173","inReplyTo":"230203.867cwyberv.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 4/6] imap-send: make --curl no-optional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-04T05:22:46Z","receivedAt":"2023-02-04T05:22:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Thu, Feb 02 2023, Junio C Hamano wrote:\n>\n>> Ævar Arnfjörð Bjarmason writes:\n>>\n>>> In the preceding commit the old \"USE_CURL_FOR_IMAP_SEND\" define became\n>>> always true, as we now require libcurl for git-imap-send.\n>>>\n>>> But as we require OpenSSL for the \"tunnel\" mode we still need to keep\n>>> the OpenSSL codepath around (ee [1] for an attempt to remove it). But\n>>\n>> \"(ee\" -> ???\n>\n> Should be \"e.g.\", will fix.\n\nI think it's more like a missing 'S' before 'ee'.\n\n>>> -#if defined(NO_OPENSSL)\n>>> -\tif (!use_curl) {\n>>> -\t\twarning(\"--no-curl not supported in this build\");\n>>> -\t\tuse_curl = 1;\n>>> -\t}\n>>> -#endif\n>>> +\tif (!use_curl)\n>>> +\t\tdie(_(\"the --no-curl option to imap-send has been deprecated\"));\n>>\n>> We used to force use of cURL when there is no other way to make the\n>> program work (i.e. there is no direct OpenSSL codepath available),\n>> instead of refusing to work (and forcing user to say --curl or to\n>> stop saying --no-curl, which is one unnecessary roadblock for the\n>> user). Why do we want to change the error handling strategy that\n>> has been in place?\n>\n> I can change this to a soft error, but it seemed more sensible to rip\n> the band-aid off an option that's never going to do anything now,\n> whereas before it would do something based on how you compiled git.\n\nStopping and forcing the user to spend an extra step does not sound\nsensible to me. Let's never do this kind of behaviour change \"while\nat it\".\n\nThanks.\n"},{"id":"471477","messageId":"Y94866yd3adoC1o9@coredump.intra.peff.net","threadId":"59173","inReplyTo":"230203.86bkmabfjr.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 6/6] imap-send: correctly report \"host\" when using \"tunnel\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-04T11:09:31Z","receivedAt":"2023-02-04T11:09:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 03, 2023 at 10:12:27PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> Except that mutt config is different than the imap-send case in that it\n> would presumably break if you changed:\n> \n> \tset folder = imap://example.com/\n> \tset tunnel = \"ssh example.com /etc/rimapd\"\n> \n> So that one of the two was example.org instead of example.com (or\n> whatever).\n\nNo, it would be perfectly happy (and in fact they do not exactly match\nin my config). When you have a tunnel, that is the authoritative way to\nget to the host, and the hostname mostly becomes a label. But\nimportantly, it is still used for certain things like local cache keys,\nwhich would be shared with a non-tunnel connection to the same name.\nRecent versions of mutt do also use the hostname for TLS verification,\nif your tunnel is not itself secure (though this is not the default; you\nhave to set special options to tell it so).\n\n> I agree that \"give this to the auth helper\" might be useful in general,\n> but our current documentation says:\n> \n> \tTo use the tool, `imap.folder` and either `imap.tunnel` or `imap.host` must be set\n> \tto appropriate values.\n> \n> And the docs for \"imap.tunnel\" say \"Required when imap.host is not set\",\n> and \"imap.host\" says \"Ignored when imap.tunnel is set, but required\n> otherwise\".\n> \n> Perhaps we should bless this as an accidental feature instead of my\n> proposed patch, but that's why I made this change. It seemed like an\n> unintentional bug that nobody intended.\n\nYes, I agree that the scenario I'm giving is contrary to what the docs\nsay. But IMHO it is worth preferring what the code does now versus what\nthe docs say. The current behavior misbehaves if you configure things\nbadly (accidentally mix and match imap.host and imap.tunnel). Your new\nbehavior misbehaves if you have two correctly-configure imap stanzas\n(both with a host/tunnel combo). Those are both fairly unlikely\nscenarios, and the outcomes are similar (we mix up credentials), but:\n\n 1. In general, all things being equal, I'd rather trust the code as\n the status quo. People will complain if you break their working\n setup. They won't if you fix the documentation.\n\n 2. In the current behavior, if it's doing the wrong thing, your next\n step is to fix your configuration (don't mix and match imap.host\n and imap.tunnel). In your proposed behavior, there is no fix. You\n are simply not allowed to use two different imap tunnels with\n credential helpers, because the helpers don't receive enough\n context to distinguish them.\n\n And that is not even \"two imap tunnels in the same config\". It is\n really per user. If I have two repositories, each with\n \"imap.tunnel\" in their local config, they will still invoke the\n same credential helpers, and both will just see host=tunnel. The\n namespace for \"host\" really is global and should be unique (ideally\n across the Internet, but at the very least among the hosts that the\n user contacts).\n\nOne fix would be to pass the tunnel command as the hostname. But even\nthat has potential problems. Certainly you'd have to tweak it (it's a\nshell command, and hostnames have syntactic restrictions, including\nforbidding newlines). And you'd probably want to swap out protocol=imap\nfor something else. But I'm not sure if helpers may complain (e.g., I\nseem to recall that the osxkeychain helper translates protocol strings\ninto integer constants that the keychain API understands).\n\n> Especially as you're focusing on the case where it contrary to the docs\n> would do what you mean, but consider (same as the doc examples, but the\n> domains are changed):\n> \n> \t[imap]\n> \t folder = \"INBOX.Drafts\"\n> \t host = imap://imap.bar.com\n> \t user = bob\n> \t pass = p4ssw0rd\n> \n> \t[imap]\n> \t folder = \"INBOX.Drafts\"\n> \t tunnel = \"ssh -q -C user@foo.com /usr/bin/imapd ./Maildir 2> /dev/null\"\n> \t\n> I.e. I have a config for \"bar.com\" I tried earlier, but now I'm trying\n> to connect to \"foo.com\", because I read the docs and notice it prefers\n> \"tunnel\" to \"host\" I think it's going to ignore that \"imap.host\", but\n> it's going to provide the password for bar.com to foo.com if challenged.\n\nYes, that's a rather unfortunate effect of the way we do config parsing\n(it looks to the user like stanzas, but we don't parse it that way; the\nsecond stanza could even be in a different file!).\n\nThough as I said above, I still think this case does not justify making\nthe code change, I do think it's the most compelling argument, and would\nmake sense to include in the commit message if we did want to do this.\n\n> So I think if we want to keep this it would be better to have a\n> imap.tunnel.credentialHost or something, to avoid conflating the two.\n\nYes, there are many config schemes that would avoid this problem. If you\nare going to tie the two together, I think it would make sense to use\nreal subsections based on the host-name, like:\n\n # hostname is the subsection key; it also becomes a label when\n # necessary\n [imap \"example.com\"]\n\n # does not even need to mention a hostname. We'll assume example.com\n # here.\n tunnel = \"any-command\"\n\n # assumes example.com as hostname; not needed if you are using a\n # tunnel, of course\n protocol = imaps\n\nBut I would not bother going to that work myself. IMHO imap-send is\nsomewhat of an abomination, and I'd actually be just as happy if it went\naway. But what you are doing seems to go totally in the wrong direction\nto me (keeping it, but breaking a rare but working use case to the\nbenefit of a rare but broken misconfiguration).\n\n> > Of course if you don't set imap.host, then we don't have anything useful\n> > to say. But as you saw, in that case imap-send will default the host\n> > field to the word \"tunnel\".\n> \n> Isn't that more of a suggestion that nobody cares about this? Presumably\n> if we had users trying to get this to work someone would have complained\n> that they wanted a custom string rather than \"tunnel\", as the auth\n> helper isn't very helpful in that case...\n\nNot if they did:\n\n [imap]\n host = example.com\n tunnel = some-command\n\n-Peff\n"},{"id":"471547","messageId":"230205.86ilgf7osb.gmgdl@evledraar.gmail.com","threadId":"59173","inReplyTo":"Y94866yd3adoC1o9@coredump.intra.peff.net","subject":"Re: [PATCH v2 6/6] imap-send: correctly report \"host\" when using \"tunnel\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-05T21:51:04Z","receivedAt":"2023-02-05T22:03:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Feb 04 2023, Jeff King wrote:\n\n> On Fri, Feb 03, 2023 at 10:12:27PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> [...]\n>> I agree that \"give this to the auth helper\" might be useful in general,\n>> but our current documentation says:\n>> \n>> \tTo use the tool, `imap.folder` and either `imap.tunnel` or `imap.host` must be set\n>> \tto appropriate values.\n>> \n>> And the docs for \"imap.tunnel\" say \"Required when imap.host is not set\",\n>> and \"imap.host\" says \"Ignored when imap.tunnel is set, but required\n>> otherwise\".\n>> \n>> Perhaps we should bless this as an accidental feature instead of my\n>> proposed patch, but that's why I made this change. It seemed like an\n>> unintentional bug that nobody intended.\n>\n> Yes, I agree that the scenario I'm giving is contrary to what the docs\n> say. But IMHO it is worth preferring what the code does now versus what\n> the docs say. The current behavior misbehaves if you configure things\n> badly (accidentally mix and match imap.host and imap.tunnel). Your new\n> behavior misbehaves if you have two correctly-configure imap stanzas\n> (both with a host/tunnel combo). Those are both fairly unlikely\n> scenarios, and the outcomes are similar (we mix up credentials), but:\n>\n> 1. In general, all things being equal, I'd rather trust the code as\n> the status quo. People will complain if you break their working\n> setup. They won't if you fix the documentation.\n>\n> 2. In the current behavior, if it's doing the wrong thing, your next\n> step is to fix your configuration (don't mix and match imap.host\n> and imap.tunnel). In your proposed behavior, there is no fix. You\n> are simply not allowed to use two different imap tunnels with\n> credential helpers, because the helpers don't receive enough\n> context to distinguish them.\n>\n> And that is not even \"two imap tunnels in the same config\". It is\n> really per user. If I have two repositories, each with\n> \"imap.tunnel\" in their local config, they will still invoke the\n> same credential helpers, and both will just see host=tunnel. The\n> namespace for \"host\" really is global and should be unique (ideally\n> across the Internet, but at the very least among the hosts that the\n> user contacts).\n>\n> One fix would be to pass the tunnel command as the hostname. But even\n> that has potential problems. Certainly you'd have to tweak it (it's a\n> shell command, and hostnames have syntactic restrictions, including\n> forbidding newlines). And you'd probably want to swap out protocol=imap\n> for something else. But I'm not sure if helpers may complain (e.g., I\n> seem to recall that the osxkeychain helper translates protocol strings\n> into integer constants that the keychain API understands).\n>\n>> Especially as you're focusing on the case where it contrary to the docs\n>> would do what you mean, but consider (same as the doc examples, but the\n>> domains are changed):\n>> \n>> \t[imap]\n>> \t folder = \"INBOX.Drafts\"\n>> \t host = imap://imap.bar.com\n>> \t user = bob\n>> \t pass = p4ssw0rd\n>> \n>> \t[imap]\n>> \t folder = \"INBOX.Drafts\"\n>> \t tunnel = \"ssh -q -C user@foo.com /usr/bin/imapd ./Maildir 2> /dev/null\"\n>> \t\n>> I.e. I have a config for \"bar.com\" I tried earlier, but now I'm trying\n>> to connect to \"foo.com\", because I read the docs and notice it prefers\n>> \"tunnel\" to \"host\" I think it's going to ignore that \"imap.host\", but\n>> it's going to provide the password for bar.com to foo.com if challenged.\n>\n> Yes, that's a rather unfortunate effect of the way we do config parsing\n> (it looks to the user like stanzas, but we don't parse it that way; the\n> second stanza could even be in a different file!).\n>\n> Though as I said above, I still think this case does not justify making\n> the code change, I do think it's the most compelling argument, and would\n> make sense to include in the commit message if we did want to do this.\n>\n>> So I think if we want to keep this it would be better to have a\n>> imap.tunnel.credentialHost or something, to avoid conflating the two.\n>\n> Yes, there are many config schemes that would avoid this problem. If you\n> are going to tie the two together, I think it would make sense to use\n> real subsections based on the host-name, like:\n>\n> # hostname is the subsection key; it also becomes a label when\n> # necessary\n> [imap \"example.com\"]\n>\n> # does not even need to mention a hostname. We'll assume example.com\n> # here.\n> tunnel = \"any-command\"\n>\n> # assumes example.com as hostname; not needed if you are using a\n> # tunnel, of course\n> protocol = imaps\n>\n> But I would not bother going to that work myself. IMHO imap-send is\n> somewhat of an abomination, and I'd actually be just as happy if it went\n> away. But what you are doing seems to go totally in the wrong direction\n> to me (keeping it, but breaking a rare but working use case to the\n> benefit of a rare but broken misconfiguration).\n>\n>> > Of course if you don't set imap.host, then we don't have anything useful\n>> > to say. But as you saw, in that case imap-send will default the host\n>> > field to the word \"tunnel\".\n>> \n>> Isn't that more of a suggestion that nobody cares about this? Presumably\n>> if we had users trying to get this to work someone would have complained\n>> that they wanted a custom string rather than \"tunnel\", as the auth\n>> helper isn't very helpful in that case...\n>\n> Not if they did:\n>\n> [imap]\n> host = example.com\n> tunnel = some-command\n\nYes, but how would they have ended up doing that? By discarding the\ndocumentation and throwing things at the wall & hoping they'd stick? \n\nI take all your general points above, and generally agree with them. Re\n#1 we should generally prefer current behavior over the docs, and re #2:\nYes, I agree this might be useful in princple, and hardcoding\n\"host=tunnel\" doesn't leave a way to pass a custom \"host to the auth\nhelper.\n\nI also don't care enough to argue about it, so I'll leave the first hunk\nhere out of any re-roll. We'll continue to pass \"host\" down in that\ncase, i.e. I'll only adjust the error messages.\n\nI just don't get how anyone could have come to rely on this so that we'd\ncare about supporting it.\n\nBecause mutt has a feature that looks similar, users might have\nconfigured git-imap-send thinking it might do the same thing, and gotten\nlucky?\n\nI guess in principle that could be true, but I think it's more likely\nthat nobody's ever had reason to use it that way. I.e. if you use the\n\"tunnel\" the way the docs suggest you won't hit the credential helper,\nas you're authenticating with \"ssh\", and using \"imapd\" to directly\noperate on a Maildir path.\n"},{"id":"471610","messageId":"xmqq1qn25v4r.fsf@gitster.g","threadId":"59173","inReplyTo":"Y94866yd3adoC1o9@coredump.intra.peff.net","subject":"Re: [PATCH v2 6/6] imap-send: correctly report \"host\" when using \"tunnel\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-06T21:41:56Z","receivedAt":"2023-02-06T21:42:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yes, I agree that the scenario I'm giving is contrary to what the docs\n> say. But IMHO it is worth preferring what the code does now versus what\n> the docs say. The current behavior misbehaves if you configure things\n> badly (accidentally mix and match imap.host and imap.tunnel). Your new\n> behavior misbehaves if you have two correctly-configure imap stanzas\n> (both with a host/tunnel combo). Those are both fairly unlikely\n> scenarios, and the outcomes are similar (we mix up credentials), but:\n>\n> 1. In general, all things being equal, I'd rather trust the code as\n> the status quo. People will complain if you break their working\n> setup. They won't if you fix the documentation.\n>\n> 2. In the current behavior, if it's doing the wrong thing, your next\n> step is to fix your configuration (don't mix and match imap.host\n> and imap.tunnel). In your proposed behavior, there is no fix. You\n> are simply not allowed to use two different imap tunnels with\n> credential helpers, because the helpers don't receive enough\n> context to distinguish them.\n>\n> And that is not even \"two imap tunnels in the same config\". It is\n> really per user. If I have two repositories, each with\n> \"imap.tunnel\" in their local config, they will still invoke the\n> same credential helpers, and both will just see host=tunnel. The\n> namespace for \"host\" really is global and should be unique (ideally\n> across the Internet, but at the very least among the hosts that the\n> user contacts).\n\nAll good points.\n\n> Yes, there are many config schemes that would avoid this problem. If you\n> are going to tie the two together, I think it would make sense to use\n> real subsections based on the host-name, like:\n>\n> # hostname is the subsection key; it also becomes a label when\n> # necessary\n> [imap \"example.com\"]\n>\n> # does not even need to mention a hostname. We'll assume example.com\n> # here.\n> tunnel = \"any-command\"\n>\n> # assumes example.com as hostname; not needed if you are using a\n> # tunnel, of course\n> protocol = imaps\n>\n> But I would not bother going to that work myself. IMHO imap-send is\n> somewhat of an abomination, and I'd actually be just as happy if it went\n> away. But what you are doing seems to go totally in the wrong direction\n> to me (keeping it, but breaking a rare but working use case to the\n> benefit of a rare but broken misconfiguration).\n\nYup.\n"},{"id":"471715","messageId":"Y+KYwsBjty0aaLes@coredump.intra.peff.net","threadId":"59173","inReplyTo":"230205.86ilgf7osb.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 6/6] imap-send: correctly report \"host\" when using \"tunnel\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-07T18:30:26Z","receivedAt":"2023-02-07T18:30:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 05, 2023 at 10:51:04PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > Not if they did:\n> >\n> > [imap]\n> > host = example.com\n> > tunnel = some-command\n> \n> Yes, but how would they have ended up doing that? By discarding the\n> documentation and throwing things at the wall & hoping they'd stick? \n\nThat's what I would have tried without reading the documentation at all,\nbased on using other programs that tunnel imap. I'm just one data point,\nof course.\n\n> I just don't get how anyone could have come to rely on this so that we'd\n> care about supporting it.\n> \n> Because mutt has a feature that looks similar, users might have\n> configured git-imap-send thinking it might do the same thing, and gotten\n> lucky?\n\nIt's less \"mutt happens to do it this way\" and more \"associating a host\nis strictly more useful, because it lets you interact with all the other\nhost-like features\". It's only imap-send's funky config scheme that\nmakes it easy to mis-configure.\n\n> I guess in principle that could be true, but I think it's more likely\n> that nobody's ever had reason to use it that way. I.e. if you use the\n> \"tunnel\" the way the docs suggest you won't hit the credential helper,\n> as you're authenticating with \"ssh\", and using \"imapd\" to directly\n> operate on a Maildir path.\n\nAs I said, my main use of tunneling is to trigger the imap server's\npreauth mode. But there are other reasons one might want to do so, like\npiercing a firewall. E.g.:\n\n [imap]\n host = internal.example.com\n tunnel = \"ssh bastion-server nc internal.example.com 143\"\n\n-Peff\n"},{"id":"471721","messageId":"230207.86fsbh2nqo.gmgdl@evledraar.gmail.com","threadId":"59173","inReplyTo":"Y+KYwsBjty0aaLes@coredump.intra.peff.net","subject":"Re: [PATCH v2 6/6] imap-send: correctly report \"host\" when using \"tunnel\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-07T20:39:48Z","receivedAt":"2023-02-07T21:03:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Feb 07 2023, Jeff King wrote:\n\n> On Sun, Feb 05, 2023 at 10:51:04PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > Not if they did:\n>> >\n>> > [imap]\n>> > host = example.com\n>> > tunnel = some-command\n>> \n>> Yes, but how would they have ended up doing that? By discarding the\n>> documentation and throwing things at the wall & hoping they'd stick? \n>\n> That's what I would have tried without reading the documentation at all,\n> based on using other programs that tunnel imap. I'm just one data point,\n> of course.\n>\n>> I just don't get how anyone could have come to rely on this so that we'd\n>> care about supporting it.\n>> \n>> Because mutt has a feature that looks similar, users might have\n>> configured git-imap-send thinking it might do the same thing, and gotten\n>> lucky?\n>\n> It's less \"mutt happens to do it this way\" and more \"associating a host\n> is strictly more useful, because it lets you interact with all the other\n> host-like features\". It's only imap-send's funky config scheme that\n> makes it easy to mis-configure.\n>\n>> I guess in principle that could be true, but I think it's more likely\n>> that nobody's ever had reason to use it that way. I.e. if you use the\n>> \"tunnel\" the way the docs suggest you won't hit the credential helper,\n>> as you're authenticating with \"ssh\", and using \"imapd\" to directly\n>> operate on a Maildir path.\n\n*nod* I'll just note that you elided the part where I noted that I don't\nreally care, and will submit some re-roll that's compatible with the\ncurrent imap.{host,tunnel} interaction.\n\nI think you might be right that people might rely on this after having\ndiscovered this undocumented interaction by accident.\n\nBut I also think that the lack of questions about how to get imap-send's\ntunnel mode to work with auth helpers (at least I couldn't find any\non-list), which is what you'd run into if you went by the documentation\n& were trying to get htat ot work, is a pretty good sign that this may\nbe either entirely unused by anyone, or at best very obscure.\n\n> As I said, my main use of tunneling is to trigger the imap server's\n> preauth mode. But there are other reasons one might want to do so, like\n> piercing a firewall. E.g.:\n>\n> [imap]\n> host = internal.example.com\n> tunnel = \"ssh bastion-server nc internal.example.com 143\"\n\nI'll definitely leave this out of a re-roll of this topic, but I did\ncome up with an opinionated replacement on top.\n\nThat commitdwhich rips out non-PREAUTH (i.e. any authentication)\nsupport, as well as SSL support that isn't using curl from\ngit-imap-send.c.\n\nHere:\nhttps://github.com/avar/git/commit/8498089f8e5a3d050b44008a7947ef3cefe2a2dd\n\nI.e. if we just say that we're not going to support this use-case\nanymore we can get rid of all of the OpenSSL reliance in-tree, except\nfor the optional (and hardly ever used) OPENSSL_SHA1, and\nuses-only-one-API-function \"HAVE_OPENSSL_CSPRNG\" use.\n\nI.e. we'd support tunneling like this still (from the manpage):\n\n\t[imap]\n\t\tfolder = \"INBOX.Drafts\"\n\t\ttunnel = \"ssh -q -C user@example.com /usr/bin/imapd ./Maildir 2> /dev/null\"\n\nBut if your use of imap.tunnel is to essentially use git-imap-send.c for\nwhat you could use another shell (or systemd or whatever) to invoke a\n\"ssh\" or \"stunnel\" command for you, we'd say too bad, just do that instead.\n\nSo your example of:\n\n\t[imap]\n\thost = internal.example.com\n\ttunnel = \"ssh bastion-server nc internal.example.com 143\"\n\nWould instead be:\n\n\t1. Arrange for the equivalent of that to run outside of\n\t git-imap-send, e.g.:\n\n\t ssh -N -R 1430:internal.example.com:143 bastion-server\n\n\t2. Use \"imap.host\" to connect to that \"remote\" box with libcurl,\n\t but just use \"localhost:1430\"\n\nGiven the obscurity of git-imap-send overall, and how trivial the\nworkaround is I don't think that's unreasonable, even with an aggressive\ntransition period.\n\nAs that commit shows we have a surprising amount of code required to\nsupport just this one use-case (and I'm not even sure I got all of\nit). Or at least:\n\n\t7 files changed, 89 insertions(+), 509 deletions(-)\n\nWith most being OpenSSL library use, so if we can find a way to not\nkeeping supporting that...\n"},{"id":"471724","messageId":"xmqq8rh9yxot.fsf@gitster.g","threadId":"59173","inReplyTo":"230207.86fsbh2nqo.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 6/6] imap-send: correctly report \"host\" when using \"tunnel\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-07T21:26:10Z","receivedAt":"2023-02-07T21:26:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I think you might be right that people might rely on this after having\n> discovered this undocumented interaction by accident.\n>\n> But I also think that the lack of questions about how to get imap-send's\n> tunnel mode to work with auth helpers (at least I couldn't find any\n> on-list), which is what you'd run into if you went by the documentation\n> & were trying to get htat ot work, is a pretty good sign that this may\n> be either entirely unused by anyone, or at best very obscure.\n\nI actually think the misconfiguration (from documentation's point of\nview) Peff is taking advantage of is a behaviour you would naturally\nexpect, if you do not read the documentation but are merely aware of\nthe presence of .host and .tunnel and guess what these do. And\nthose who felt it was a natural design would probably not have asked\nany question about it. Documenting the current behaviour better\nwould not hurt. Updating the behaviour and documenting the new\nbehaviour would not help anybody.\n\n\n"},{"id":"471727","messageId":"230207.86357h2kmh.gmgdl@evledraar.gmail.com","threadId":"59173","inReplyTo":"xmqq8rh9yxot.fsf@gitster.g","subject":"Re: [PATCH v2 6/6] imap-send: correctly report \"host\" when using \"tunnel\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-07T22:04:22Z","receivedAt":"2023-02-07T22:09:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Feb 07 2023, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> I think you might be right that people might rely on this after having\n>> discovered this undocumented interaction by accident.\n>>\n>> But I also think that the lack of questions about how to get imap-send's\n>> tunnel mode to work with auth helpers (at least I couldn't find any\n>> on-list), which is what you'd run into if you went by the documentation\n>> & were trying to get htat ot work, is a pretty good sign that this may\n>> be either entirely unused by anyone, or at best very obscure.\n>\n> I actually think the misconfiguration (from documentation's point of\n> view) Peff is taking advantage of is a behaviour you would naturally\n> expect, if you do not read the documentation but are merely aware of\n> the presence of .host and .tunnel and guess what these do. And\n> those who felt it was a natural design would probably not have asked\n> any question about it. Documenting the current behaviour better\n> would not hurt. Updating the behaviour and documenting the new\n> behaviour would not help anybody.\n\nSure, we don't have to belabor the point. It's moot for a re-roll of\nthis topic in any case (I won't be changing this behavior).\n\nBut do I take it from the non-reply to what came afterwards that you're\nnot interested in a (not a part of this topic) proposal for us to say\n\"if you want that, arrange for ssh to do it for you\", which would allow\nfor finally dropping libssl as a non-trivial direct dependency? Or just\nthat you didn't get to considering that?\n"},{"id":"471728","messageId":"Y+LNitGAude1vogv@coredump.intra.peff.net","threadId":"59173","inReplyTo":"230207.86fsbh2nqo.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 6/6] imap-send: correctly report \"host\" when using \"tunnel\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-07T22:15:38Z","receivedAt":"2023-02-07T22:15:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 07, 2023 at 09:39:48PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> *nod* I'll just note that you elided the part where I noted that I don't\n> really care, and will submit some re-roll that's compatible with the\n> current imap.{host,tunnel} interaction.\n\nYeah, sorry, I should have said \"yes, thank you\" there. :) I wasn't\nmeaning to continue arguing, but just trying to answer your \"how would\nthey even find this?\" confusion.\n\n> I.e. if we just say that we're not going to support this use-case\n> anymore we can get rid of all of the OpenSSL reliance in-tree, except\n> for the optional (and hardly ever used) OPENSSL_SHA1, and\n> uses-only-one-API-function \"HAVE_OPENSSL_CSPRNG\" use.\n\nYeah, getting rid of that openssl code is a reasonable goal. And this\nmay seem counter-intuitive, but I'm actually _more_ in favor of that\nthan the change you proposed here, even though it potentially breaks\nmore users. That's because I feel like we're buying something useful\nwith it, whereas with the patch we've been discussing, the tradeoff was\nless clear to me.\n\nThat said, it seems like there should be a path forward for supporting\ntunnels via curl, and then we could be getting rid of the openssl\ndependency _and_ all of the custom and rarely-run imap code. But that's\nan even bigger task, and I not only wouldn't want to work on it, I'm not\neven sure I'd want to review it. I'm slightly regretting getting\ninvolved here at all, because it seems like none of us actually care at\nall about imap-send, and this has turned into a big discussion. I mostly\nchimed in because it seemed like I had a perspective you didn't on how\npeople might use tunnels, and it felt like I should speak up for folks\nwhose use cases might be getting broken.\n\n Side note: If somebody were proposing to add imap-send at all today,\n I'd probably say \"no, that should be a separate project, and you\n should probably write it in some language that has a decent imap\n library\". It really has nothing at all to do with Git in terms of\n implementation, and I suspect it's not super well maintained in\n general. But perhaps it is too late for that.\n\n> So your example of:\n> \n> \t[imap]\n> \thost = internal.example.com\n> \ttunnel = \"ssh bastion-server nc internal.example.com 143\"\n> \n> Would instead be:\n> \n> \t1. Arrange for the equivalent of that to run outside of\n> \t git-imap-send, e.g.:\n> \n> \t ssh -N -R 1430:internal.example.com:143 bastion-server\n> \n> \t2. Use \"imap.host\" to connect to that \"remote\" box with libcurl,\n> \t but just use \"localhost:1430\"\n\nHaving done something like that before, the \"arrange\" step is more\nfinicky than you might think (because sometimes it goes away, and you\nreally want to trigger it on demand).\n\n-Peff\n"},{"id":"471729","messageId":"Y+LNz+o3V8te7/c6@coredump.intra.peff.net","threadId":"59173","inReplyTo":"xmqq8rh9yxot.fsf@gitster.g","subject":"Re: [PATCH v2 6/6] imap-send: correctly report \"host\" when using \"tunnel\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-07T22:16:47Z","receivedAt":"2023-02-07T22:16:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 07, 2023 at 01:26:10PM -0800, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n> \n> > I think you might be right that people might rely on this after having\n> > discovered this undocumented interaction by accident.\n> >\n> > But I also think that the lack of questions about how to get imap-send's\n> > tunnel mode to work with auth helpers (at least I couldn't find any\n> > on-list), which is what you'd run into if you went by the documentation\n> > & were trying to get htat ot work, is a pretty good sign that this may\n> > be either entirely unused by anyone, or at best very obscure.\n> \n> I actually think the misconfiguration (from documentation's point of\n> view) Peff is taking advantage of is a behaviour you would naturally\n> expect, if you do not read the documentation but are merely aware of\n> the presence of .host and .tunnel and guess what these do. \n\nJust to be clear, I am not taking advantage of anything. I do not use\nimap-send myself, because a much better solution is to have a decent\nmail client that can access both imap and local maildirs. ;)\n\nI was only trying to offer some perspective as a general imap-tunnel\nuser.\n\n-Peff\n"},{"id":"471730","messageId":"xmqqcz6lw1u3.fsf@gitster.g","threadId":"59173","inReplyTo":"Y+LNitGAude1vogv@coredump.intra.peff.net","subject":"Re: [PATCH v2 6/6] imap-send: correctly report \"host\" when using \"tunnel\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-07T22:24:52Z","receivedAt":"2023-02-07T22:24:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Side note: If somebody were proposing to add imap-send at all today,\n> I'd probably say \"no, that should be a separate project, and you\n> should probably write it in some language that has a decent imap\n> library\". It really has nothing at all to do with Git in terms of\n> implementation, and I suspect it's not super well maintained in\n> general. But perhaps it is too late for that.\n\namen..\n\n"},{"id":"471749","messageId":"230208.86a61p0x9n.gmgdl@evledraar.gmail.com","threadId":"59173","inReplyTo":"Y+LNitGAude1vogv@coredump.intra.peff.net","subject":"Re: [PATCH v2 6/6] imap-send: correctly report \"host\" when using \"tunnel\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-08T01:06:55Z","receivedAt":"2023-02-08T01:19:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Feb 07 2023, Jeff King wrote:\n\n> On Tue, Feb 07, 2023 at 09:39:48PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> *nod* I'll just note that you elided the part where I noted that I don't\n>> really care, and will submit some re-roll that's compatible with the\n>> current imap.{host,tunnel} interaction.\n>\n> Yeah, sorry, I should have said \"yes, thank you\" there. :) I wasn't\n> meaning to continue arguing, but just trying to answer your \"how would\n> they even find this?\" confusion.\n\n*nod*\n\n>> I.e. if we just say that we're not going to support this use-case\n>> anymore we can get rid of all of the OpenSSL reliance in-tree, except\n>> for the optional (and hardly ever used) OPENSSL_SHA1, and\n>> uses-only-one-API-function \"HAVE_OPENSSL_CSPRNG\" use.\n>\n> Yeah, getting rid of that openssl code is a reasonable goal. And this\n> may seem counter-intuitive, but I'm actually _more_ in favor of that\n> than the change you proposed here, even though it potentially breaks\n> more users. That's because I feel like we're buying something useful\n> with it, whereas with the patch we've been discussing, the tradeoff was\n> less clear to me.\n\nMakes sense.\n\n> That said, it seems like there should be a path forward for supporting\n> tunnels via curl, and then we could be getting rid of the openssl\n> dependency _and_ all of the custom and rarely-run imap code.\n\nI really think it's obscure enough that just offerng users a documented\nway out should be neough.\n\n> [...]\n> Side note: If somebody were proposing to add imap-send at all today,\n> I'd probably say \"no, that should be a separate project, and you\n> should probably write it in some language that has a decent imap\n> library\". It really has nothing at all to do with Git in terms of\n> implementation, and I suspect it's not super well maintained in\n> general. But perhaps it is too late for that.\n\nI think it's a reasonable feature, but in hindsight our mistake was to\nthink that we should be perma-forking isync, which has since moved\non. I've used isync's \"mbsync\" extensively for IMAP in other contexts,\nand it works well for that.\n\nSo if we were going back to the drawing board a \"git-imap-sync\" really\nshould just be something in our mail tooling that can produce a Maildir,\nand if we wanted an IMAP helper it could invoke mbsync, offlineimap or\nvarious other \"maildir to IMAP\" bidirectional syncing utilities to\n\"send\" via IMAP.\n\nSo, just some hook support for format-patch with some documented\nexamples should do it, but I won't be working on that task...\n"},{"id":"472267","messageId":"Y+/olg2szMxLIkXp@coredump.intra.peff.net","threadId":"59173","inReplyTo":"230208.86a61p0x9n.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 6/6] imap-send: correctly report \"host\" when using \"tunnel\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-17T20:50:30Z","receivedAt":"2023-02-17T20:50:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 08, 2023 at 02:06:55AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > Side note: If somebody were proposing to add imap-send at all today,\n> > I'd probably say \"no, that should be a separate project, and you\n> > should probably write it in some language that has a decent imap\n> > library\". It really has nothing at all to do with Git in terms of\n> > implementation, and I suspect it's not super well maintained in\n> > general. But perhaps it is too late for that.\n> \n> I think it's a reasonable feature, but in hindsight our mistake was to\n> think that we should be perma-forking isync, which has since moved\n> on. I've used isync's \"mbsync\" extensively for IMAP in other contexts,\n> and it works well for that.\n> \n> So if we were going back to the drawing board a \"git-imap-sync\" really\n> should just be something in our mail tooling that can produce a Maildir,\n> and if we wanted an IMAP helper it could invoke mbsync, offlineimap or\n> various other \"maildir to IMAP\" bidirectional syncing utilities to\n> \"send\" via IMAP.\n> \n> So, just some hook support for format-patch with some documented\n> examples should do it, but I won't be working on that task...\n\nYes, I think format-patch plus a sync program would be good. I did\nbriefly look at the state of imap sync programs and was a bit\ndisappointed. Many older recommendations are for software that is no\nlonger packaged, or hard to find. And none of the ones I looked at do\nsomething as simple as \"copy these messages to this imap server\".\nThey're all very interested in bidirectional sync, incremental updates,\nand so on. But I do think one could make mbsync or offlineimap work, if\nyou used a dedicated folder on the server as the destination.\n\nBut yeah, I don't think you or I needs to come up with a solution there.\nI was more proposing along the lines of: let's drop imap-send, and\ninterested people can then make a solution based on other tools, or even\nspin off imap-send into its own repository.\n\nBut I get that even that is some work, and it may mean complaining\nusers.\n\n-Peff\n"}]}