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

2014-11-10 Thread Junio C Hamano 
Bernhard Reiter  writes:

> Am 2014-11-09 um 14:00 schrieb Torsten Bögershausen:
>> On 2014-08-27 00.40, Bernhard Reiter wrote:
>>> Use libcurl's high-level API functions to implement git-imap-send
>>> instead of the previous low-level OpenSSL-based functions.
>>>
>> This doesn't seem to fully work under Debian 7:
>> /home/tb/projects/git/git.pu/imap-send.c:1546: undefined reference
>> to `curl_append_msgs_to_imap'
>
> Thx for the notice. I forgot to guard that with an #ifdef.
>
> The new patch below includes that, and the fix sent by Ramsay;
> hopefully the squashed/edited commit message is fine.

Queued with a small fix-ups, including

 - line-fold a couple of overlong lines;

 - avoid decl-after-stmt of "int prev_len";

 - reduce the scope of "struct struct auth" down to only the block
   it is used;

 - the footer of the log message now reads "helped-by ramsay", your
   sign-off and then mine.

Thanks.

diff --git a/imap-send.c b/imap-send.c
index 08271d9..4dfe4c2 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1349,7 +1349,8 @@ static void git_imap_config(void)
git_config_get_string("imap.authmethod", &server.auth_method);
 }
 
-static int append_msgs_to_imap(struct imap_server_conf *server, struct strbuf* 
all_msgs, int total)
+static int append_msgs_to_imap(struct imap_server_conf *server,
+  struct strbuf* all_msgs, int total)
 {
struct strbuf msg = STRBUF_INIT;
struct imap_store *ctx = NULL;
@@ -1391,7 +1392,6 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
 {
CURL *curl;
struct strbuf path = STRBUF_INIT;
-   struct strbuf auth = STRBUF_INIT;
 
if (curl_global_init(CURL_GLOBAL_ALL) != CURLE_OK)
die("curl_global_init failed");
@@ -1414,11 +1414,12 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
curl_easy_setopt(curl, CURLOPT_PORT, server.port);
 
if (server.auth_method) {
+   struct strbuf auth = STRBUF_INIT;
strbuf_addstr(&auth, "AUTH=");
strbuf_addstr(&auth, server.auth_method);
curl_easy_setopt(curl, CURLOPT_LOGIN_OPTIONS, auth.buf);
+   strbuf_release(&auth);
}
-   strbuf_release(&auth);
 
if (server.use_ssl)
curl_easy_setopt(curl, CURLOPT_USE_SSL, (long)CURLUSESSL_ALL);
@@ -1436,7 +1437,8 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
return curl;
 }
 
-static int curl_append_msgs_to_imap(struct imap_server_conf *server, struct 
strbuf* all_msgs, int total) {
+static int curl_append_msgs_to_imap(struct imap_server_conf *server,
+   struct strbuf* all_msgs, int total) {
int ofs = 0;
int n = 0;
struct buffer msgbuf = { STRBUF_INIT, 0 };
@@ -1449,17 +1451,19 @@ static int curl_append_msgs_to_imap(struct 
imap_server_conf *server, struct strb
fprintf(stderr, "sending %d message%s\n", total, (total != 1) ? "s" : 
"");
while (1) {
unsigned percent = n * 100 / total;
+   int prev_len;
 
fprintf(stderr, "%4u%% (%d/%d) done\r", percent, n, total);
 
-   int prev_len = msgbuf.buf.len;
+   prev_len = msgbuf.buf.len;
if (!split_msg(all_msgs, &msgbuf.buf, &ofs))
break;
if (server->use_html)
wrap_in_html(&msgbuf.buf);
lf_to_crlf(&msgbuf.buf);
 
-   curl_easy_setopt(curl, CURLOPT_INFILESIZE_LARGE, 
(curl_off_t)(msgbuf.buf.len-prev_len));
+   curl_easy_setopt(curl, CURLOPT_INFILESIZE_LARGE,
+(curl_off_t)(msgbuf.buf.len-prev_len));
 
res = curl_easy_perform(curl);
 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-11-09 Thread Junio C Hamano 
Ramsay Jones  writes:

>> In order to suppress a sparse warning about "using sizeof on a
>> function", we use the same solution used in commit 9371322a6
>> ("sparse: suppress some "using sizeof on a function" warnings",
>> 06-10-2013) which solved exactly this problem for the other commands
>> using libcurl.
>
> Although it doesn't hurt, I don't think this 'problem' deserves
> so many (or any) inches in the commit message. ;-)

Use smaller line pitch, then? ;-) I am sure myself 6 months down the
road as a "git show" reader of this patch would appreciate these
five lines when I scratch my head looking at the Makefile changes.

>> Signed-off-by: Bernhard Reiter 
>> Signed-off-by: Ramsay Jones 
>
> Since I didn't actually review this patch, or make any significant
> contribution to the code (it's not even enough to be copyright-able!),
> then this 'Signed-off-by:' should not be included. At *most* you might
> want to put a 'Helped-by:' _prior_ to your sign-off.

Yes, that is the appropriate thing to do, I would think.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-11-09 Thread Ramsay Jones 
On 09/11/14 14:55, Bernhard Reiter wrote:
> Am 2014-11-09 um 14:00 schrieb Torsten Bögershausen:
>> On 2014-08-27 00.40, Bernhard Reiter wrote:
>>> Use libcurl's high-level API functions to implement git-imap-send
>>> instead of the previous low-level OpenSSL-based functions.
>>>
>> This doesn't seem to fully work under Debian 7:
>> /home/tb/projects/git/git.pu/imap-send.c:1546: undefined reference to 
>> `curl_append_msgs_to_imap'
> 
> Thx for the notice. I forgot to guard that with an #ifdef.
> 
> The new patch below includes that, and the fix sent by Ramsay; hopefully the 
> squashed/edited commit message is fine.

Sorry for not responding earlier, I've been somewhat busy for the last
couple of days. :(

> Bernhard
> 
> -- >8 --
> 
> Use libcurl's high-level API functions to implement git-imap-send
> instead of the previous low-level OpenSSL-based functions.
> 
> Since version 7.30.0, libcurl's API has been able to communicate with
> IMAP servers. Using those high-level functions instead of the current
> ones would reduce imap-send.c by some 1200 lines of code. For now,
> the old ones are wrapped in #ifdefs, and the new functions are enabled
> by make if curl's version is >= 7.34.0, from which version on curl's
> CURLOPT_LOGIN_OPTIONS (enabling IMAP authentication) parameter has been
> available. The low-level functions will still be used for tunneling
> into the server for now.
> 
> As I don't have access to that many IMAP servers, I haven't been able to
> test the new code with a wide variety of parameter combinations. I did
> test both secure and insecure (imaps:// and imap://) connections and
> values of "PLAIN" and "LOGIN" for the authMethod.
> 
> In order to suppress a sparse warning about "using sizeof on a
> function", we use the same solution used in commit 9371322a6
> ("sparse: suppress some "using sizeof on a function" warnings",
> 06-10-2013) which solved exactly this problem for the other commands
> using libcurl.

Although it doesn't hurt, I don't think this 'problem' deserves
so many (or any) inches in the commit message. ;-)

> 
> Signed-off-by: Bernhard Reiter 
> Signed-off-by: Ramsay Jones 

Since I didn't actually review this patch, or make any significant
contribution to the code (it's not even enough to be copyright-able!),
then this 'Signed-off-by:' should not be included. At *most* you might
want to put a 'Helped-by:' _prior_ to your sign-off.

Thanks.

ATB,
Ramsay Jones


> ---
>  Documentation/git-imap-send.txt |  15 +++-
>  INSTALL |  15 ++--
>  Makefile|  18 +++-
>  imap-send.c | 176 
> ++--
>  4 files changed, 187 insertions(+), 37 deletions(-)
> 
> diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
> index 0897131..77aacf1 100644
> --- a/Documentation/git-imap-send.txt
> +++ b/Documentation/git-imap-send.txt
> @@ -9,7 +9,7 @@ git-imap-send - Send a collection of patches from stdin to an 
> IMAP folder
>  SYNOPSIS
>  
>  [verse]
> -'git imap-send' [-v] [-q]
> +'git imap-send' [-v] [-q] [--[no-]curl]
>  
>  
>  DESCRIPTION
> @@ -37,6 +37,15 @@ OPTIONS
>  --quiet::
>   Be quiet.
>  
> +--curl::
> + Use libcurl to communicate with the IMAP server, unless tunneling
> + into it.  Ignored if Git was built without the USE_CURL_FOR_IMAP_SEND
> + option set.
> +
> +--no-curl::
> + Talk to the IMAP server using git's own IMAP routines instead of
> + using libcurl.
> +
>  
>  CONFIGURATION
>  -
> @@ -87,7 +96,9 @@ imap.preformattedHTML::
>  
>  imap.authMethod::
>   Specify authenticate method for authentication with IMAP server.
> - Current supported method is 'CRAM-MD5' only. If this is not set
> + If Git was built with the NO_CURL option, or if your curl version is 
> older
> + than 7.34.0, or if you're running git-imap-send with the `--no-curl`
> + option, the only supported method is 'CRAM-MD5'. If this is not set
>   then 'git imap-send' uses the basic IMAP plaintext LOGIN command.
>  
>  Examples
> diff --git a/INSTALL b/INSTALL
> index 6ec7a24..ffb071e 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -108,18 +108,21 @@ Issues of note:
> so you might need to install additional packages other than Perl
> itself, e.g. Time::HiRes.
>  
> - - "openssl" library is used by git-imap-send to use IMAP over SSL.
> -   If you don't need it, use NO_OPENSSL.
> + - git-imap-send needs the OpenSSL library to talk IMAP over SSL if
> +   you are using libcurl older than 7.34.0.  Otherwise you can use
> +   NO_OPENSSL without losing git-imap-send.
>  
> By default, git uses OpenSSL for SHA1 but it will use its own
> library (inspired by Mozilla's) with either NO_OPENSSL or
> BLK_SHA1.  Also included is a version optimized for PowerPC
> (PPC_SHA1).
>  
> - - "libcurl" library is used by git-http-fetch and git-fetch.  You
> -   might

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

2014-11-09 Thread Bernhard Reiter 
Am 2014-11-09 um 14:00 schrieb Torsten Bögershausen:
> On 2014-08-27 00.40, Bernhard Reiter wrote:
>> Use libcurl's high-level API functions to implement git-imap-send
>> instead of the previous low-level OpenSSL-based functions.
>>
> This doesn't seem to fully work under Debian 7:
> /home/tb/projects/git/git.pu/imap-send.c:1546: undefined reference to 
> `curl_append_msgs_to_imap'

Thx for the notice. I forgot to guard that with an #ifdef.

The new patch below includes that, and the fix sent by Ramsay; hopefully the 
squashed/edited commit message is fine.

Bernhard

-- >8 --

Use libcurl's high-level API functions to implement git-imap-send
instead of the previous low-level OpenSSL-based functions.

Since version 7.30.0, libcurl's API has been able to communicate with
IMAP servers. Using those high-level functions instead of the current
ones would reduce imap-send.c by some 1200 lines of code. For now,
the old ones are wrapped in #ifdefs, and the new functions are enabled
by make if curl's version is >= 7.34.0, from which version on curl's
CURLOPT_LOGIN_OPTIONS (enabling IMAP authentication) parameter has been
available. The low-level functions will still be used for tunneling
into the server for now.

As I don't have access to that many IMAP servers, I haven't been able to
test the new code with a wide variety of parameter combinations. I did
test both secure and insecure (imaps:// and imap://) connections and
values of "PLAIN" and "LOGIN" for the authMethod.

In order to suppress a sparse warning about "using sizeof on a
function", we use the same solution used in commit 9371322a6
("sparse: suppress some "using sizeof on a function" warnings",
06-10-2013) which solved exactly this problem for the other commands
using libcurl.

Signed-off-by: Bernhard Reiter 
Signed-off-by: Ramsay Jones 
---
 Documentation/git-imap-send.txt |  15 +++-
 INSTALL |  15 ++--
 Makefile|  18 +++-
 imap-send.c | 176 ++--
 4 files changed, 187 insertions(+), 37 deletions(-)

diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
index 0897131..77aacf1 100644
--- a/Documentation/git-imap-send.txt
+++ b/Documentation/git-imap-send.txt
@@ -9,7 +9,7 @@ git-imap-send - Send a collection of patches from stdin to an 
IMAP folder
 SYNOPSIS
 
 [verse]
-'git imap-send' [-v] [-q]
+'git imap-send' [-v] [-q] [--[no-]curl]
 
 
 DESCRIPTION
@@ -37,6 +37,15 @@ OPTIONS
 --quiet::
Be quiet.
 
+--curl::
+   Use libcurl to communicate with the IMAP server, unless tunneling
+   into it.  Ignored if Git was built without the USE_CURL_FOR_IMAP_SEND
+   option set.
+
+--no-curl::
+   Talk to the IMAP server using git's own IMAP routines instead of
+   using libcurl.
+
 
 CONFIGURATION
 -
@@ -87,7 +96,9 @@ imap.preformattedHTML::
 
 imap.authMethod::
Specify authenticate method for authentication with IMAP server.
-   Current supported method is 'CRAM-MD5' only. If this is not set
+   If Git was built with the NO_CURL option, or if your curl version is 
older
+   than 7.34.0, or if you're running git-imap-send with the `--no-curl`
+   option, the only supported method is 'CRAM-MD5'. If this is not set
then 'git imap-send' uses the basic IMAP plaintext LOGIN command.
 
 Examples
diff --git a/INSTALL b/INSTALL
index 6ec7a24..ffb071e 100644
--- a/INSTALL
+++ b/INSTALL
@@ -108,18 +108,21 @@ Issues of note:
  so you might need to install additional packages other than Perl
  itself, e.g. Time::HiRes.
 
-   - "openssl" library is used by git-imap-send to use IMAP over SSL.
- If you don't need it, use NO_OPENSSL.
+   - git-imap-send needs the OpenSSL library to talk IMAP over SSL if
+ you are using libcurl older than 7.34.0.  Otherwise you can use
+ NO_OPENSSL without losing git-imap-send.
 
  By default, git uses OpenSSL for SHA1 but it will use its own
  library (inspired by Mozilla's) with either NO_OPENSSL or
  BLK_SHA1.  Also included is a version optimized for PowerPC
  (PPC_SHA1).
 
-   - "libcurl" library is used by git-http-fetch and git-fetch.  You
- might also want the "curl" executable for debugging purposes.
- If you do not use http:// or https:// repositories, you do not
- have to have them (use NO_CURL).
+   - "libcurl" library is used by git-http-fetch, git-fetch, and, if
+ the curl version >= 7.34.0, for git-imap-send.  You might also
+ want the "curl" executable for debugging purposes. If you do not
+ use http:// or https:// repositories, and do not want to put
+ patches into an IMAP mailbox, you do not have to have them
+ (use NO_CURL).
 
- "expat" library; git-http-push uses it for remote lock
  management over DAV.  Similar to "curl" above, this is optional
diff --git a

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

2014-11-09 Thread Torsten Bögershausen 
On 2014-08-27 00.40, Bernhard Reiter wrote:
> Use libcurl's high-level API functions to implement git-imap-send
> instead of the previous low-level OpenSSL-based functions.
> 
This doesn't seem to fully work under Debian 7:
/home/tb/projects/git/git.pu/imap-send.c:1546: undefined reference to 
`curl_append_msgs_to_imap'

Some more info:
 curl-config --vernum
071a00

 (echo 072200; curl-config --vernum) 2>/dev/null | sort -r 
072200
071a00

(echo 072200; curl-config --vernum) 2>/dev/null | sort -r  | sed -ne 2p
071a00



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-10-29 Thread Junio C Hamano 
Bernhard Reiter  writes:

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

Thanks.

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

> diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
> index 7d991d9..1c24e1f 100644
> --- a/Documentation/git-imap-send.txt
> +++ b/Documentation/git-imap-send.txt
> @@ -9,7 +9,7 @@ git-imap-send - Send a collection of patches from stdin to an 
> IMAP folder
>  SYNOPSIS
>  
>  [verse]
> -'git imap-send'
> +'git imap-send' [-v] [-q] --[no-]curl
>DESCRIPTION
> @@ ...

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

> diff --git a/Makefile b/Makefile
> index f34a2d4..69b2fbf 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -992,6 +992,9 @@ ifdef HAVE_ALLOCA_H
>   BASIC_CFLAGS += -DHAVE_ALLOCA_H
>  endif
>  +IMAP_SEND_BUILDDEPS =
> +IMAP_SEND_LDFLAGS = $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
> +

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

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

> diff --git a/imap-send.c b/imap-send.c
> index 70bcc7a..9cc80ae 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -26,11 +26,31 @@
>  #include "credential.h"
>  #include "exec_cmd.h"
>  #include "run-command.h"
> +#include "parse-options.h"
>  #ifdef NO_OPENSSL
>  typedef void *SSL;
>  #endif
> +#ifdef USE_CURL_FOR_IMAP_SEND
> +#include "http.h"
> +#endif
>  -static const char imap_send_usage[] = "git imap-send < ";
> +static int Verbose, Quiet;

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

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

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

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

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

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

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

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

For example, you may want to add

[imap]
useCurl = true

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

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

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

...
git_imap_config();
parse_options();

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

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

static int use_curl; /* strictly opt in */
...
main()
{

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

2014-10-22 Thread Junio C Hamano 
Bernhard Reiter  writes:

> *ping*
> Hope I didn't mess up formatting again...
> Or do I need to top-post, as the original thread is too old to keep posting 
> to it?

Please avoid top-posting on this list.

If you have some background material (e.g. summary of previous
discussions, list of things that have been changed in response to
previous review, etc.), you have three options:

 * Use a cover letter, even for a one-patch topic.  Use [PATCH 0/1]
   for the background material if you have tons of it and then make
   [PATCH 1/1] as a follow-up to that cover letter.

   See http://thread.gmane.org/gmane.comp.version-control.git/256386
   for how such an approach looks like.

 * Use a single message, [PATCH] (aka [PATCH 1/1]), and add your
   background material _after_ three-dash lines as comment.

   See http://thread.gmane.org/gmane.comp.version-control.git/258451
   for how such an approach looks like.

 * Use a single message, [PATCH] (aka [PATCH 1/1]), and add your
   background material _before_ a scissors line "-- >8 --",
   optionally followed by "From:" and/or "Subject:" (but in your
   case, you are sending your own patch as yourself with the same
   subject, so neither of them apply).

   See 
http://thread.gmane.org/gmane.comp.version-control.git/258470/focus=258474
   for how such an approach looks like.


Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-10-22 Thread Bernhard Reiter 
*ping*
Hope I didn't mess up formatting again...
Or do I need to top-post, as the original thread is too old to keep posting to 
it?

Bernhard

 Weitergeleitete Nachricht 
Betreff: Re: [PATCH] git-imap-send: use libcurl for implementation
Datum: Sun, 12 Oct 2014 17:22:20 +0200
Von: Bernhard Reiter 
An: Junio C Hamano 
Kopie (CC): git@vger.kernel.org, Jonathan Nieder , Jeff 
King , 434...@bugs.debian.org, René Scharfe , Tony 
Finch , Tanay Abhra , Dan Albert 
, Jeremy Huddleston , David Aguilar 
, Michael Haggerty , Oswald Buddenhagen 


Sorry for not getting back to this any sooner, I've been pretty busy
recently with Other Projects(tm).

Am 2014-08-27 um 19:20 schrieb Junio C Hamano:
> Bernhard Reiter  writes:
> 
>> [...] For now,
>> the old ones are wrapped in #ifdefs, and the new functions are enabled
>> by make if curl's version is >= 7.35.0, from which version on curl's
>> CURLOPT_LOGIN_OPTIONS (enabling IMAP authentication) parameter has been
>> available.
> 
> https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions
> says that this was introduced as of 7.34.0, though.

Strange, I thought I recalled having seen that in
http://curl.haxx.se/libcurl/c/CURLOPT_LOGIN_OPTIONS.html but it clearly
says 7.34.0 there too. I've now changed all occurrences of 7.35.0 to
7.34.0 (and the corresponding hex value in the Makefile).

>> As I don't have access to that many IMAP servers, I haven't been able to
>> test the new code with a wide variety of parameter combinations. I did
>> test both secure and insecure (imaps:// and imap://) connections and
>> values of "PLAIN" and "LOGIN" for the authMethod.
> 
> Perhaps CC'ing those who have touched git-imap-send code over the
> years and asking for their help testing might help?

CC'ing them (going back about 2 years, which already makes the list
quite long) and the people who have taken part in the initial discussion
on this feature in August. And the related Debian bug.

Please test this, folks!

>> Signed-off-by: Bernhard Reiter 
>> ---
>> I rebased the patch on the pu branch, hope that was the right thing to do.
> 
> Usually I would appreciate a patch for a new feature not meant for
> the maintenance tracks to be based on 'master', so that it can go to
> the next release without having to wait other changes that may
> conflict with it and that may not yet be ready.
> 
> I will try to apply this one to 'pu', rebase it on 'master' to make
> sure the result does not depend on the other topics in flight, and
> then merge it back to 'pu'.

Okay, I'll stick to master. I've rebased on master now that the first
couple related patches are there anyway.

> [...]
>>
>> diff --git a/Documentation/git-imap-send.txt 
>> b/Documentation/git-imap-send.txt
>> index 7d991d9..9d244c4 100644
>> --- a/Documentation/git-imap-send.txt
>> +++ b/Documentation/git-imap-send.txt
>> @@ -75,7 +75,8 @@ imap.preformattedHTML::
>>  
>>  imap.authMethod::
>>  Specify authenticate method for authentication with IMAP server.
>> -Current supported method is 'CRAM-MD5' only. If this is not set
>> +If you compiled git with the NO_CURL option or if your curl version is
>> +< 7.35.0, the only supported method is 'CRAM-MD5'. If this is not set
>>  then 'git imap-send' uses the basic IMAP plaintext LOGIN command.
> 
> Hmph, so there is no option that lets me say "I know my libcurl is
> new enough but I have some reason not to want to use the new code to
> interact with my imap server", at compile time or (more preferrably)
> at runtime?

Added a runtime option, see below.

>> diff --git a/INSTALL b/INSTALL
>> index 6ec7a24..e2770a0 100644
>> --- a/INSTALL
>> +++ b/INSTALL
>> @@ -108,18 +108,21 @@ Issues of note:
>>so you might need to install additional packages other than Perl
>>itself, e.g. Time::HiRes.
>>  
>> -- "openssl" library is used by git-imap-send to use IMAP over SSL.
>> -  If you don't need it, use NO_OPENSSL.
>> +- "openssl" library is used by git-imap-send to use IMAP over SSL,
>> +  unless you're using curl >= 7.35.0, in which case that will be
>> +  used. If you don't need git-imap-send, you can use NO_OPENSSL.
> 
> The last sentence makes it unclear which of the following is true:
> 
>  - I have sufficiently new libcurl.  I cannot say NO_OPENSSL because
>I do need git-imap-send.
> 
>  - I have sufficiently new libcurl, so "openssl" is not used by
>git-imap send f

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

2014-10-12 Thread Bernhard Reiter 
Sorry for not getting back to this any sooner, I've been pretty busy
recently with Other Projects(tm).

Am 2014-08-27 um 19:20 schrieb Junio C Hamano:
> Bernhard Reiter  writes:
> 
>> [...] For now,
>> the old ones are wrapped in #ifdefs, and the new functions are enabled
>> by make if curl's version is >= 7.35.0, from which version on curl's
>> CURLOPT_LOGIN_OPTIONS (enabling IMAP authentication) parameter has been
>> available.
> 
> https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions
> says that this was introduced as of 7.34.0, though.

Strange, I thought I recalled having seen that in
http://curl.haxx.se/libcurl/c/CURLOPT_LOGIN_OPTIONS.html but it clearly
says 7.34.0 there too. I've now changed all occurrences of 7.35.0 to
7.34.0 (and the corresponding hex value in the Makefile).

>> As I don't have access to that many IMAP servers, I haven't been able to
>> test the new code with a wide variety of parameter combinations. I did
>> test both secure and insecure (imaps:// and imap://) connections and
>> values of "PLAIN" and "LOGIN" for the authMethod.
> 
> Perhaps CC'ing those who have touched git-imap-send code over the
> years and asking for their help testing might help?

CC'ing them (going back about 2 years, which already makes the list
quite long) and the people who have taken part in the initial discussion
on this feature in August. And the related Debian bug.

Please test this, folks!

>> Signed-off-by: Bernhard Reiter 
>> ---
>> I rebased the patch on the pu branch, hope that was the right thing to do.
> 
> Usually I would appreciate a patch for a new feature not meant for
> the maintenance tracks to be based on 'master', so that it can go to
> the next release without having to wait other changes that may
> conflict with it and that may not yet be ready.
> 
> I will try to apply this one to 'pu', rebase it on 'master' to make
> sure the result does not depend on the other topics in flight, and
> then merge it back to 'pu'.

Okay, I'll stick to master. I've rebased on master now that the first
couple related patches are there anyway.

> [...]
>>
>> diff --git a/Documentation/git-imap-send.txt 
>> b/Documentation/git-imap-send.txt
>> index 7d991d9..9d244c4 100644
>> --- a/Documentation/git-imap-send.txt
>> +++ b/Documentation/git-imap-send.txt
>> @@ -75,7 +75,8 @@ imap.preformattedHTML::
>>  
>>  imap.authMethod::
>>  Specify authenticate method for authentication with IMAP server.
>> -Current supported method is 'CRAM-MD5' only. If this is not set
>> +If you compiled git with the NO_CURL option or if your curl version is
>> +< 7.35.0, the only supported method is 'CRAM-MD5'. If this is not set
>>  then 'git imap-send' uses the basic IMAP plaintext LOGIN command.
> 
> Hmph, so there is no option that lets me say "I know my libcurl is
> new enough but I have some reason not to want to use the new code to
> interact with my imap server", at compile time or (more preferrably)
> at runtime?

Added a runtime option, see below.

>> diff --git a/INSTALL b/INSTALL
>> index 6ec7a24..e2770a0 100644
>> --- a/INSTALL
>> +++ b/INSTALL
>> @@ -108,18 +108,21 @@ Issues of note:
>>so you might need to install additional packages other than Perl
>>itself, e.g. Time::HiRes.
>>  
>> -- "openssl" library is used by git-imap-send to use IMAP over SSL.
>> -  If you don't need it, use NO_OPENSSL.
>> +- "openssl" library is used by git-imap-send to use IMAP over SSL,
>> +  unless you're using curl >= 7.35.0, in which case that will be
>> +  used. If you don't need git-imap-send, you can use NO_OPENSSL.
> 
> The last sentence makes it unclear which of the following is true:
> 
>  - I have sufficiently new libcurl.  I cannot say NO_OPENSSL because
>I do need git-imap-send.
> 
>  - I have sufficiently new libcurl, so "openssl" is not used by
>git-imap send for me.  I can say NO_OPENSSL.
> 
> Perhaps
> 
>  - git-imap-send needs the OpenSSL library to talk IMAP over SSL if
>you are using libCurl older than 7.35.0.  Otherwise you can use
>NO_OPENSSL without losing git-imap-send.

Fixed.

>> diff --git a/git.spec.in b/git.spec.in
>> index d61d537..9535cc3 100644
>> --- a/git.spec.in
>> +++ b/git.spec.in
>> @@ -8,7 +8,7 @@ License: GPL
>>  Group:  Development/Tools
>>  URL:http://kernel.org/pub/software/scm/git/
>>  Source: http://kernel.org/pub/software/scm/git/%{name}-%{version}.tar.gz
>> -BuildRequires:  zlib-devel >= 1.2, openssl-devel, curl-devel, 
>> expat-devel, gettext  %{!?_without_docs:, xmlto, asciidoc > 6.0.3}
>> +BuildRequires:  zlib-devel >= 1.2, openssl-devel, curl-devel >= 7.35.0, 
>> expat-devel, gettext  %{!?_without_docs:, xmlto, asciidoc > 6.0.3}
> 
> This is very iffy.  It incompatible with the body of the patch where
> you allow older curl library and because you depend on openssl-devel
> you wouldn't lose imap-send.

Okay, removed the version requirement.

>> @@ -1391

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

2014-08-27 Thread Junio C Hamano 
Bernhard Reiter  writes:

> Use libcurl's high-level API functions to implement git-imap-send
> instead of the previous low-level OpenSSL-based functions.
>
> Since version 7.30.0, libcurl's API has been able to communicate with
> IMAP servers. Using those high-level functions instead of the current
> ones would reduce imap-send.c by some 1200 lines of code. For now,
> the old ones are wrapped in #ifdefs, and the new functions are enabled
> by make if curl's version is >= 7.35.0, from which version on curl's
> CURLOPT_LOGIN_OPTIONS (enabling IMAP authentication) parameter has been
> available.

https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions
says that this was introduced as of 7.34.0, though.

> As I don't have access to that many IMAP servers, I haven't been able to
> test the new code with a wide variety of parameter combinations. I did
> test both secure and insecure (imaps:// and imap://) connections and
> values of "PLAIN" and "LOGIN" for the authMethod.

Perhaps CC'ing those who have touched git-imap-send code over the
years and asking for their help testing might help?

> Signed-off-by: Bernhard Reiter 
> ---
> I rebased the patch on the pu branch, hope that was the right thing to do.

Usually I would appreciate a patch for a new feature not meant for
the maintenance tracks to be based on 'master', so that it can go to
the next release without having to wait other changes that may
conflict with it and that may not yet be ready.

I will try to apply this one to 'pu', rebase it on 'master' to make
sure the result does not depend on the other topics in flight, and
then merge it back to 'pu'.

Thanks; some comments below.

>  Documentation/git-imap-send.txt |   3 +-
>  INSTALL |  15 ++--
>  Makefile|  16 +++-
>  git.spec.in |   5 +-
>  imap-send.c | 165 
> +---
>  5 files changed, 167 insertions(+), 37 deletions(-)
>
> diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
> index 7d991d9..9d244c4 100644
> --- a/Documentation/git-imap-send.txt
> +++ b/Documentation/git-imap-send.txt
> @@ -75,7 +75,8 @@ imap.preformattedHTML::
>  
>  imap.authMethod::
>   Specify authenticate method for authentication with IMAP server.
> - Current supported method is 'CRAM-MD5' only. If this is not set
> + If you compiled git with the NO_CURL option or if your curl version is
> + < 7.35.0, the only supported method is 'CRAM-MD5'. If this is not set
>   then 'git imap-send' uses the basic IMAP plaintext LOGIN command.

Hmph, so there is no option that lets me say "I know my libcurl is
new enough but I have some reason not to want to use the new code to
interact with my imap server", at compile time or (more preferrably)
at runtime?

> diff --git a/INSTALL b/INSTALL
> index 6ec7a24..e2770a0 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -108,18 +108,21 @@ Issues of note:
> so you might need to install additional packages other than Perl
> itself, e.g. Time::HiRes.
>  
> - - "openssl" library is used by git-imap-send to use IMAP over SSL.
> -   If you don't need it, use NO_OPENSSL.
> + - "openssl" library is used by git-imap-send to use IMAP over SSL,
> +   unless you're using curl >= 7.35.0, in which case that will be
> +   used. If you don't need git-imap-send, you can use NO_OPENSSL.

The last sentence makes it unclear which of the following is true:

 - I have sufficiently new libcurl.  I cannot say NO_OPENSSL because
   I do need git-imap-send.

 - I have sufficiently new libcurl, so "openssl" is not used by
   git-imap send for me.  I can say NO_OPENSSL.

Perhaps

 - git-imap-send needs the OpenSSL library to talk IMAP over SSL if
   you are using libCurl older than 7.35.0.  Otherwise you can use
   NO_OPENSSL without losing git-imap-send.

> diff --git a/git.spec.in b/git.spec.in
> index d61d537..9535cc3 100644
> --- a/git.spec.in
> +++ b/git.spec.in
> @@ -8,7 +8,7 @@ License:  GPL
>  Group:   Development/Tools
>  URL: http://kernel.org/pub/software/scm/git/
>  Source:  http://kernel.org/pub/software/scm/git/%{name}-%{version}.tar.gz
> -BuildRequires:   zlib-devel >= 1.2, openssl-devel, curl-devel, 
> expat-devel, gettext  %{!?_without_docs:, xmlto, asciidoc > 6.0.3}
> +BuildRequires:   zlib-devel >= 1.2, openssl-devel, curl-devel >= 7.35.0, 
> expat-devel, gettext  %{!?_without_docs:, xmlto, asciidoc > 6.0.3}

This is very iffy.  It incompatible with the body of the patch where
you allow older curl library and because you depend on openssl-devel
you wouldn't lose imap-send.

> @@ -1391,29 +1518,13 @@ int main(int argc, char **argv)
>   }
>  
>   /* write it to the imap server */
> - ctx = imap_open_store(&server, server.folder);
> - if (!ctx) {
> - fprintf(stderr, "failed to open store\n");
> - return 1;
> -