Git Mailing List Archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] imap-send: some -Wunused-parameter cleanups
@ 2023-07-03  6:32 Jeff King
  2023-07-03  6:33 ` [PATCH 1/3] imap-send: use server conf argument in setup_curl() Jeff King
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Jeff King @ 2023-07-03  6:32 UTC (permalink / raw)
  To: git

Here are a few patches where I opted to clean up code rather than
marking parameters with UNUSED. Plus a bonus cleanup in patch 3 (though
I am also OK to drop that one; I don't care much about the imap-send
code, and I'd be just as happy if we deprecated and eventually dropped
the tool).

  [1/3]: imap-send: use server conf argument in setup_curl()
  [2/3]: imap-send: drop unused parameter from imap_cmd_cb callback
  [3/3]: imap-send: drop unused fields from imap_cmd_cb

 imap-send.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

-Peff

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/3] imap-send: use server conf argument in setup_curl()
  2023-07-03  6:32 [PATCH 0/3] imap-send: some -Wunused-parameter cleanups Jeff King
@ 2023-07-03  6:33 ` Jeff King
  2023-07-05 17:23   ` Junio C Hamano
  2023-07-03  6:34 ` [PATCH 2/3] imap-send: drop unused parameter from imap_cmd_cb callback Jeff King
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 7+ messages in thread
From: Jeff King @ 2023-07-03  6:33 UTC (permalink / raw)
  To: git

Our caller passes in an imap_server_conf struct, but we ignore it
totally, and instead read the config directly from the global "server"
variable. This works OK, since our sole caller will pass in that same
global variable. But the intent seems to have been to use the passed-in
variable, as otherwise it has no purpose (and many other functions use
the same pattern).

Let's use the passed-in value, which also silences a -Wunused-parameter
warning.

It would be nice if "server" was not a global here, as we could avoid
making similar mistakes. But changing that would be a larger refactor,
as it must be accessed as a global in a few spots (e.g., filling it in
with the config callback).

Signed-off-by: Jeff King <peff@peff.net>
---
 imap-send.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 7f5426177a..0afd88de44 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1415,42 +1415,42 @@ static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)
 	if (!curl)
 		die("curl_easy_init failed");
 
-	server_fill_credential(&server, cred);
-	curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);
-	curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
+	server_fill_credential(srvc, cred);
+	curl_easy_setopt(curl, CURLOPT_USERNAME, srvc->user);
+	curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
 
-	strbuf_addstr(&path, server.use_ssl ? "imaps://" : "imap://");
-	strbuf_addstr(&path, server.host);
+	strbuf_addstr(&path, srvc->use_ssl ? "imaps://" : "imap://");
+	strbuf_addstr(&path, srvc->host);
 	if (!path.len || path.buf[path.len - 1] != '/')
 		strbuf_addch(&path, '/');
 
-	uri_encoded_folder = curl_easy_escape(curl, server.folder, 0);
+	uri_encoded_folder = curl_easy_escape(curl, srvc->folder, 0);
 	if (!uri_encoded_folder)
 		die("failed to encode server folder");
 	strbuf_addstr(&path, uri_encoded_folder);
 	curl_free(uri_encoded_folder);
 
 	curl_easy_setopt(curl, CURLOPT_URL, path.buf);
 	strbuf_release(&path);
-	curl_easy_setopt(curl, CURLOPT_PORT, server.port);
+	curl_easy_setopt(curl, CURLOPT_PORT, srvc->port);
 
-	if (server.auth_method) {
+	if (srvc->auth_method) {
 #ifndef GIT_CURL_HAVE_CURLOPT_LOGIN_OPTIONS
 		warning("No LOGIN_OPTIONS support in this cURL version");
 #else
 		struct strbuf auth = STRBUF_INIT;
 		strbuf_addstr(&auth, "AUTH=");
-		strbuf_addstr(&auth, server.auth_method);
+		strbuf_addstr(&auth, srvc->auth_method);
 		curl_easy_setopt(curl, CURLOPT_LOGIN_OPTIONS, auth.buf);
 		strbuf_release(&auth);
 #endif
 	}
 
-	if (!server.use_ssl)
+	if (!srvc->use_ssl)
 		curl_easy_setopt(curl, CURLOPT_USE_SSL, (long)CURLUSESSL_TRY);
 
-	curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, server.ssl_verify);
-	curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, server.ssl_verify);
+	curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, srvc->ssl_verify);
+	curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, srvc->ssl_verify);
 
 	curl_easy_setopt(curl, CURLOPT_READFUNCTION, fread_buffer);
 
-- 
2.41.0.586.g3c0cc15bc7


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/3] imap-send: drop unused parameter from imap_cmd_cb callback
  2023-07-03  6:32 [PATCH 0/3] imap-send: some -Wunused-parameter cleanups Jeff King
  2023-07-03  6:33 ` [PATCH 1/3] imap-send: use server conf argument in setup_curl() Jeff King
@ 2023-07-03  6:34 ` Jeff King
  2023-07-05 17:25   ` Junio C Hamano
  2023-07-03  6:34 ` [PATCH 3/3] imap-send: drop unused fields from imap_cmd_cb Jeff King
  2023-07-05 17:18 ` [PATCH 0/3] imap-send: some -Wunused-parameter cleanups Junio C Hamano
  3 siblings, 1 reply; 7+ messages in thread
From: Jeff King @ 2023-07-03  6:34 UTC (permalink / raw)
  To: git

There's a generic callback mechanism for handling plus-continuation of
IMAP commands. It takes the imap_cmd struct itself as an argument. That
seems reasonable, and in a larger imap-using program it might be used.
But in imap-send, we have only one such callback (auth_cram_md5) and it
doesn't use this value, triggering -Wunused-parameter warnings.

We could just mark the parameter as UNUSED. But since this is the only
such function, and because we are not likely to share code with the
upstream isync anymore, we can just simplify the interface to remove
this parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 imap-send.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 0afd88de44..81a87f434b 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -137,7 +137,7 @@ struct imap_store {
 };
 
 struct imap_cmd_cb {
-	int (*cont)(struct imap_store *ctx, struct imap_cmd *cmd, const char *prompt);
+	int (*cont)(struct imap_store *ctx, const char *prompt);
 	void (*done)(struct imap_store *ctx, struct imap_cmd *cmd, int response);
 	void *ctx;
 	char *data;
@@ -786,7 +786,7 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 				if (n != (int)cmdp->cb.dlen)
 					return RESP_BAD;
 			} else if (cmdp->cb.cont) {
-				if (cmdp->cb.cont(ctx, cmdp, cmd))
+				if (cmdp->cb.cont(ctx, cmd))
 					return RESP_BAD;
 			} else {
 				fprintf(stderr, "IMAP error: unexpected command continuation request\n");
@@ -917,7 +917,7 @@ static char *cram(const char *challenge_64, const char *user, const char *pass)
 
 #endif
 
-static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const char *prompt)
+static int auth_cram_md5(struct imap_store *ctx, const char *prompt)
 {
 	int ret;
 	char *response;
-- 
2.41.0.586.g3c0cc15bc7


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 3/3] imap-send: drop unused fields from imap_cmd_cb
  2023-07-03  6:32 [PATCH 0/3] imap-send: some -Wunused-parameter cleanups Jeff King
  2023-07-03  6:33 ` [PATCH 1/3] imap-send: use server conf argument in setup_curl() Jeff King
  2023-07-03  6:34 ` [PATCH 2/3] imap-send: drop unused parameter from imap_cmd_cb callback Jeff King
@ 2023-07-03  6:34 ` Jeff King
  2023-07-05 17:18 ` [PATCH 0/3] imap-send: some -Wunused-parameter cleanups Junio C Hamano
  3 siblings, 0 replies; 7+ messages in thread
From: Jeff King @ 2023-07-03  6:34 UTC (permalink / raw)
  To: git

The imap_cmd_cb struct has several fields which are totally unused.
Presumably they did useful things in the upstream isync code from which
this is derived, but they don't in our more limited program. This is
particularly confusing for the "done" callback, which (as of the
previous patch) no longer matches the signature of the adjacent "cont"
callback.

Since we're unlikely to share code with isync going forward, we should
feel free to simplify the code here. Note that "done" is examined but
never set, so we can also drop a little bit of code outside of the
struct definition.

Signed-off-by: Jeff King <peff@peff.net>
---
 imap-send.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 81a87f434b..c1952d99e8 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -138,11 +138,9 @@ struct imap_store {
 
 struct imap_cmd_cb {
 	int (*cont)(struct imap_store *ctx, const char *prompt);
-	void (*done)(struct imap_store *ctx, struct imap_cmd *cmd, int response);
 	void *ctx;
 	char *data;
 	int dlen;
-	int uid;
 };
 
 struct imap_cmd {
@@ -828,8 +826,6 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 			}
 			if ((resp2 = parse_response_code(ctx, &cmdp->cb, cmd)) > resp)
 				resp = resp2;
-			if (cmdp->cb.done)
-				cmdp->cb.done(ctx, cmdp, resp);
 			free(cmdp->cb.data);
 			free(cmdp->cmd);
 			free(cmdp);
-- 
2.41.0.586.g3c0cc15bc7

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/3] imap-send: some -Wunused-parameter cleanups
  2023-07-03  6:32 [PATCH 0/3] imap-send: some -Wunused-parameter cleanups Jeff King
                   ` (2 preceding siblings ...)
  2023-07-03  6:34 ` [PATCH 3/3] imap-send: drop unused fields from imap_cmd_cb Jeff King
@ 2023-07-05 17:18 ` Junio C Hamano
  3 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2023-07-05 17:18 UTC (permalink / raw)
  To: Jeff King; +Cc: git

Jeff King <peff@peff.net> writes:

> Here are a few patches where I opted to clean up code rather than
> marking parameters with UNUSED. Plus a bonus cleanup in patch 3 (though
> I am also OK to drop that one;

> I don't care much about the imap-send
> code, and I'd be just as happy if we deprecated and eventually dropped
> the tool).

Thanks---I too was wondering when we can drop 'imap-send' ;-)

>   [1/3]: imap-send: use server conf argument in setup_curl()
>   [2/3]: imap-send: drop unused parameter from imap_cmd_cb callback
>   [3/3]: imap-send: drop unused fields from imap_cmd_cb
>
>  imap-send.c | 34 +++++++++++++++-------------------
>  1 file changed, 15 insertions(+), 19 deletions(-)
>
> -Peff

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/3] imap-send: use server conf argument in setup_curl()
  2023-07-03  6:33 ` [PATCH 1/3] imap-send: use server conf argument in setup_curl() Jeff King
@ 2023-07-05 17:23   ` Junio C Hamano
  0 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2023-07-05 17:23 UTC (permalink / raw)
  To: Jeff King; +Cc: git

Jeff King <peff@peff.net> writes:

> Our caller passes in an imap_server_conf struct, but we ignore it
> totally, and instead read the config directly from the global "server"
> variable. This works OK, since our sole caller will pass in that same
> global variable.

This obviously is the right thing to do.

A much inferiour alternative would be to drop the srvc parameter to
the callee and that will make it clear that we are only working on
the singleton 'imap_server_conf' all the time (which is not entirely
incorrect), but I do not think there is no point in making any large
change to this program at this point in either direction.

> But the intent seems to have been to use the passed-in
> variable, as otherwise it has no purpose (and many other functions use
> the same pattern).

Yup.  Thanks.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/3] imap-send: drop unused parameter from imap_cmd_cb callback
  2023-07-03  6:34 ` [PATCH 2/3] imap-send: drop unused parameter from imap_cmd_cb callback Jeff King
@ 2023-07-05 17:25   ` Junio C Hamano
  0 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2023-07-05 17:25 UTC (permalink / raw)
  To: Jeff King; +Cc: git

Jeff King <peff@peff.net> writes:

> We could just mark the parameter as UNUSED. But since this is the only
> such function, and because we are not likely to share code with the
> upstream isync anymore, we can just simplify the interface to remove
> this parameter.

Sensible.  Thanks.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-07-05 17:25 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-03  6:32 [PATCH 0/3] imap-send: some -Wunused-parameter cleanups Jeff King
2023-07-03  6:33 ` [PATCH 1/3] imap-send: use server conf argument in setup_curl() Jeff King
2023-07-05 17:23   ` Junio C Hamano
2023-07-03  6:34 ` [PATCH 2/3] imap-send: drop unused parameter from imap_cmd_cb callback Jeff King
2023-07-05 17:25   ` Junio C Hamano
2023-07-03  6:34 ` [PATCH 3/3] imap-send: drop unused fields from imap_cmd_cb Jeff King
2023-07-05 17:18 ` [PATCH 0/3] imap-send: some -Wunused-parameter cleanups Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).