| Message ID | 357d69fa8b538baba23cd110b8d16174234a58dc.1716983704.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Compile with `-Wwrite-strings` | expand |
Patrick Steinhardt <ps@pks.im> writes: > We never free any of the config strings that we populate into the > `struct imap_server_conf`. Fix this by creating a common exit path where > we can free resources. This is more like the previous step got rid of the anchor that made these strings reachable, so we need to turn around to free them, which is sort-of Meh, especially given that the leaked pieces of memory are small and very much bounded. The main benefit of this change is to allow us prepare on the constness change in the other (read: API this thing uses from elsewhere) parts of the system, which is a very worthy goal. > While at it, drop the unused variables `imap_server_conf::name` and > `nongit_ok`. The removal of the .name member may be correct, but I suspect the change to nongit_ok is a change in behaviour, and it could even be a regression. > - setup_git_directory_gently(&nongit_ok); > + setup_git_directory_gently(NULL); The general idea behind &nongit_ok is that - Usually setup_git_directory_gently() dies if NULL is passed instead of a pointer to &nongit_ok. Most of the Git command wants to make sure they have a repository to operate on, so this is a reasonable default behaviour. - Some commands would want to work also without having any repository, possibly with limited capability (e.g., "git apply" may want to work as a better "GNU patch", but obviously it cannot do "git apply --binary --3way" without having the object database). They tell setup_git_directory_gently() not to die when outside a repository by passing a pointer to &nongit_ok, and instead tell if we are in a repository by storing 0/1 in it. The idea is that a command that is willing to work outside a repository can disable selected features based on what it sees in nongit_ok. In the case of "imap-send", there is no such features that it needs to special case, perhaps because everything it does is supposed to work outside a repository? So the short version of what worries me in this change is that we used to be able to operate without having a repository at all, but now we would barf if run outside a repository, no? Thanks.
On Wed, May 29, 2024 at 01:55:13PM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > We never free any of the config strings that we populate into the > > `struct imap_server_conf`. Fix this by creating a common exit path where > > we can free resources. > > This is more like the previous step got rid of the anchor that made > these strings reachable, so we need to turn around to free them, > which is sort-of Meh, especially given that the leaked pieces of > memory are small and very much bounded. > > The main benefit of this change is to allow us prepare on the > constness change in the other (read: API this thing uses from > elsewhere) parts of the system, which is a very worthy goal. That's the motivation in this series at least. But I also see it as a good goal by itself to get rid of the global state that we had before the preceding patch. It may not be necessary, but it certainly helps me personally to reason about code better. > > While at it, drop the unused variables `imap_server_conf::name` and > > `nongit_ok`. > > The removal of the .name member may be correct, but I suspect the > change to nongit_ok is a change in behaviour, and it could even be a > regression. > > > - setup_git_directory_gently(&nongit_ok); > > + setup_git_directory_gently(NULL); > > The general idea behind &nongit_ok is that > > - Usually setup_git_directory_gently() dies if NULL is passed > instead of a pointer to &nongit_ok. Most of the Git command > wants to make sure they have a repository to operate on, so this > is a reasonable default behaviour. > > - Some commands would want to work also without having any > repository, possibly with limited capability (e.g., "git apply" > may want to work as a better "GNU patch", but obviously it cannot > do "git apply --binary --3way" without having the object > database). They tell setup_git_directory_gently() not to die > when outside a repository by passing a pointer to &nongit_ok, and > instead tell if we are in a repository by storing 0/1 in it. > > The idea is that a command that is willing to work outside a > repository can disable selected features based on what it sees in > nongit_ok. In the case of "imap-send", there is no such features > that it needs to special case, perhaps because everything it does is > supposed to work outside a repository? > > So the short version of what worries me in this change is that we > used to be able to operate without having a repository at all, but > now we would barf if run outside a repository, no? Oh, I wasn't aware that the parameter being `NULL` actually causes a change in behaviour. Which nicely demonstrates that we have some missing test coverage for git-imap-send(1). In fact, it's not only "some". We don't have any test coverage at all for git-imap-send(1) as far as I can see. Which does make me rest a bit uneasy. And I suspect that it wouldn't be trivial to add given that it kind of requires something that talks IMAP on the receiving end. Patrick
Patrick Steinhardt <ps@pks.im> writes: > Oh, I wasn't aware that the parameter being `NULL` actually causes a > change in behaviour. Which nicely demonstrates that we have some missing > test coverage for git-imap-send(1). > > In fact, it's not only "some". We don't have any test coverage at all > for git-imap-send(1) as far as I can see. Which does make me rest a bit > uneasy. And I suspect that it wouldn't be trivial to add given that it > kind of requires something that talks IMAP on the receiving end. Quite honestly, there is absolutely nothing that makes imap-send necessary to be part of Git suite. It does depend on Git as it uses our configuration files to store its knobs, but if it were written outside the context of the Git project as a handy way to move a draft message to your imap draft folder, it would have been perfectly fine as a standalone tool (and it would have used its own simple configuration file format). Other than its use of .gitconfig, it does not even know what a commit is, unlike, say send-email [*], which is also fairly unrelated to Git proper. Side note: send-email is only marginally closer to Git than imap-send, as it optionally can run format-patch as a part of its operation. Without the format-patch integration, it is just a MUA whose primary purpose is not to munge its payload. So I do not find it surprising at all that we have no tests for it. We could do something like this at least, perhaps on top of the recent topic that added a test to ensure commands that ought to be operable without any repository? t/t1517-outside-repo.sh | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git c/t/t1517-outside-repo.sh w/t/t1517-outside-repo.sh index 557808ffa7..6d5f07df93 100755 --- c/t/t1517-outside-repo.sh +++ w/t/t1517-outside-repo.sh @@ -56,4 +56,19 @@ test_expect_success 'grep outside repository' ' test_cmp expect actual ' +test_expect_success 'imap-send outside repository' ' + test_config_global imap.host imaps://localhost && + test_config_global imap.folder Drafts && + + echo nothing to send >expect && + test_must_fail git imap-send -v </dev/null 2>actual && + test_cmp expect actual && + + ( + cd non-repo && + test_must_fail git imap-send -v </dev/null 2>../actual + ) && + test_cmp expect actual +' + test_done
diff --git a/imap-send.c b/imap-send.c index 67a7a6c456..a6fb33806c 100644 --- a/imap-send.c +++ b/imap-send.c @@ -69,7 +69,6 @@ static void imap_warn(const char *, ...); static char *next_arg(char **); struct imap_server_conf { - const char *name; char *tunnel; char *host; int port; @@ -1300,23 +1299,28 @@ static int git_imap_config(const char *var, const char *val, { struct imap_server_conf *cfg = cb; - if (!strcmp("imap.sslverify", var)) + if (!strcmp("imap.sslverify", var)) { cfg->ssl_verify = git_config_bool(var, val); - else if (!strcmp("imap.preformattedhtml", var)) + } else if (!strcmp("imap.preformattedhtml", var)) { cfg->use_html = git_config_bool(var, val); - else if (!strcmp("imap.folder", var)) + } else if (!strcmp("imap.folder", var)) { + FREE_AND_NULL(cfg->folder); return git_config_string(&cfg->folder, var, val); - else if (!strcmp("imap.user", var)) + } else if (!strcmp("imap.user", var)) { + FREE_AND_NULL(cfg->folder); return git_config_string(&cfg->user, var, val); - else if (!strcmp("imap.pass", var)) + } else if (!strcmp("imap.pass", var)) { + FREE_AND_NULL(cfg->folder); return git_config_string(&cfg->pass, var, val); - else if (!strcmp("imap.tunnel", var)) + } else if (!strcmp("imap.tunnel", var)) { + FREE_AND_NULL(cfg->folder); return git_config_string(&cfg->tunnel, var, val); - else if (!strcmp("imap.authmethod", var)) + } else if (!strcmp("imap.authmethod", var)) { + FREE_AND_NULL(cfg->folder); return git_config_string(&cfg->auth_method, var, val); - else if (!strcmp("imap.port", var)) + } else if (!strcmp("imap.port", var)) { cfg->port = git_config_int(var, val, ctx->kvi); - else if (!strcmp("imap.host", var)) { + } else if (!strcmp("imap.host", var)) { if (!val) { return config_error_nonbool(var); } else { @@ -1330,8 +1334,9 @@ static int git_imap_config(const char *var, const char *val, val += 2; cfg->host = xstrdup(val); } - } else + } else { return git_default_config(var, val, ctx, cb); + } return 0; } @@ -1502,9 +1507,9 @@ int cmd_main(int argc, const char **argv) }; struct strbuf all_msgs = STRBUF_INIT; int total; - int nongit_ok; + int ret; - setup_git_directory_gently(&nongit_ok); + setup_git_directory_gently(NULL); git_config(git_imap_config, &server); argc = parse_options(argc, (const char **)argv, "", imap_send_options, imap_send_usage, 0); @@ -1529,42 +1534,55 @@ int cmd_main(int argc, const char **argv) if (!server.folder) { fprintf(stderr, "no imap store specified\n"); - return 1; + ret = 1; + goto out; } if (!server.host) { if (!server.tunnel) { fprintf(stderr, "no imap host specified\n"); - return 1; + ret = 1; + goto out; } - server.host = "tunnel"; + server.host = xstrdup("tunnel"); } /* read the messages */ if (strbuf_read(&all_msgs, 0, 0) < 0) { error_errno(_("could not read from stdin")); - return 1; + ret = 1; + goto out; } if (all_msgs.len == 0) { fprintf(stderr, "nothing to send\n"); - return 1; + ret = 1; + goto out; } total = count_messages(&all_msgs); if (!total) { fprintf(stderr, "no messages to send\n"); - return 1; + ret = 1; + goto out; } /* write it to the imap server */ if (server.tunnel) - return append_msgs_to_imap(&server, &all_msgs, total); - + ret = append_msgs_to_imap(&server, &all_msgs, total); #ifdef USE_CURL_FOR_IMAP_SEND - if (use_curl) - return curl_append_msgs_to_imap(&server, &all_msgs, total); + else if (use_curl) + ret = curl_append_msgs_to_imap(&server, &all_msgs, total); #endif - - return append_msgs_to_imap(&server, &all_msgs, total); + else + ret = append_msgs_to_imap(&server, &all_msgs, total); + +out: + free(server.tunnel); + free(server.host); + free(server.folder); + free(server.user); + free(server.pass); + free(server.auth_method); + return ret; }
We never free any of the config strings that we populate into the `struct imap_server_conf`. Fix this by creating a common exit path where we can free resources. While at it, drop the unused variables `imap_server_conf::name` and `nongit_ok`. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- imap-send.c | 68 +++++++++++++++++++++++++++++++++-------------------- 1 file changed, 43 insertions(+), 25 deletions(-)