{"thread":{"id":"61999","subject":"[PATCH] remote: plug memory leaks at early returns","startedAt":"2024-08-23T20:21:22Z","lastAt":"2024-08-26T15:12:01Z","messageCount":5,"participants":["René Scharfe","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"501611","messageId":"82cb986c-6830-4d9a-bad1-fe4cab6a76eb@web.de","threadId":"61999","inReplyTo":null,"subject":"[PATCH] remote: plug memory leaks at early returns","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-08-23T20:21:10Z","receivedAt":"2024-08-23T20:21:22Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Signed-off-by: René Scharfe \n---\n builtin/remote.c | 23 +++++++++++++++--------\n 1 file changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex d1f9292ed2..0acc547d69 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -164,6 +164,7 @@ static int add(int argc, const char **argv, const char *prefix)\n \tstruct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;\n \tconst char *name, *url;\n \tint i;\n+\tint result = 0;\n\n \tstruct option options[] = {\n \t\tOPT_BOOL('f', \"fetch\", &fetch, N_(\"fetch the remote branches\")),\n@@ -230,8 +231,10 @@ static int add(int argc, const char **argv, const char *prefix)\n \t\t\t fetch_tags == TAGS_SET ? \"--tags\" : \"--no-tags\");\n \t}\n\n-\tif (fetch && fetch_remote(name))\n-\t\treturn 1;\n+\tif (fetch && fetch_remote(name)) {\n+\t\tresult = 1;\n+\t\tgoto out;\n+\t}\n\n \tif (master) {\n \t\tstrbuf_reset(&buf);\n@@ -241,14 +244,15 @@ static int add(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_addf(&buf2, \"refs/remotes/%s/%s\", name, master);\n\n \t\tif (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, \"remote add\"))\n-\t\t\treturn error(_(\"Could not setup master '%s'\"), master);\n+\t\t\tresult = error(_(\"Could not setup master '%s'\"), master);\n \t}\n\n+out:\n \tstrbuf_release(&buf);\n \tstrbuf_release(&buf2);\n \tstring_list_clear(&track, 0);\n\n-\treturn 0;\n+\treturn result;\n }\n\n struct branch_info {\n@@ -715,6 +719,7 @@ static int mv(int argc, const char **argv, const char *prefix)\n \tstruct rename_info rename;\n \tint i, refs_renamed_nr = 0, refspec_updated = 0;\n \tstruct progress *progress = NULL;\n+\tint result = 0;\n\n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t builtin_remote_rename_usage, 0);\n@@ -747,9 +752,11 @@ static int mv(int argc, const char **argv, const char *prefix)\n\n \tstrbuf_addf(&buf, \"remote.%s\", rename.old_name);\n \tstrbuf_addf(&buf2, \"remote.%s\", rename.new_name);\n-\tif (repo_config_rename_section(the_repository, buf.buf, buf2.buf) < 1)\n-\t\treturn error(_(\"Could not rename config section '%s' to '%s'\"),\n-\t\t\t\tbuf.buf, buf2.buf);\n+\tif (repo_config_rename_section(the_repository, buf.buf, buf2.buf) < 1) {\n+\t\tresult = error(_(\"Could not rename config section '%s' to '%s'\"),\n+\t\t\t buf.buf, buf2.buf);\n+\t\tgoto out;\n+\t}\n\n \tif (oldremote->fetch.raw_nr) {\n \t\tstrbuf_reset(&buf);\n@@ -870,7 +877,7 @@ static int mv(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&buf);\n \tstrbuf_release(&buf2);\n \tstrbuf_release(&buf3);\n-\treturn 0;\n+\treturn result;\n }\n\n static int rm(int argc, const char **argv, const char *prefix)\n--\n2.30.2\n"},{"id":"501612","messageId":"xmqqseuvndsq.fsf@gitster.g","threadId":"61999","inReplyTo":"82cb986c-6830-4d9a-bad1-fe4cab6a76eb@web.de","subject":"Re: [PATCH] remote: plug memory leaks at early returns","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-23T21:13:09Z","receivedAt":"2024-08-23T21:13:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Signed-off-by: René Scharfe \n> ---\n> builtin/remote.c | 23 +++++++++++++++--------\n> 1 file changed, 15 insertions(+), 8 deletions(-)\n\nLooks straight-forward. Does this allow us to mark any test script\nas leak-free? I understand that Patrick has another round of\nleakfixes topic that is not yet published, and I'd prefer to see us\nnot step each other's toes.\n\nWill queue in the meantime but may drop it if Patrick says it\nalready is covered or something.\n\nThanks.\n\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index d1f9292ed2..0acc547d69 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -164,6 +164,7 @@ static int add(int argc, const char **argv, const char *prefix)\n> \tstruct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;\n> \tconst char *name, *url;\n> \tint i;\n> +\tint result = 0;\n>\n> \tstruct option options[] = {\n> \t\tOPT_BOOL('f', \"fetch\", &fetch, N_(\"fetch the remote branches\")),\n> @@ -230,8 +231,10 @@ static int add(int argc, const char **argv, const char *prefix)\n> \t\t\t fetch_tags == TAGS_SET ? \"--tags\" : \"--no-tags\");\n> \t}\n>\n> -\tif (fetch && fetch_remote(name))\n> -\t\treturn 1;\n> +\tif (fetch && fetch_remote(name)) {\n> +\t\tresult = 1;\n> +\t\tgoto out;\n> +\t}\n>\n> \tif (master) {\n> \t\tstrbuf_reset(&buf);\n> @@ -241,14 +244,15 @@ static int add(int argc, const char **argv, const char *prefix)\n> \t\tstrbuf_addf(&buf2, \"refs/remotes/%s/%s\", name, master);\n>\n> \t\tif (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, \"remote add\"))\n> -\t\t\treturn error(_(\"Could not setup master '%s'\"), master);\n> +\t\t\tresult = error(_(\"Could not setup master '%s'\"), master);\n> \t}\n>\n> +out:\n> \tstrbuf_release(&buf);\n> \tstrbuf_release(&buf2);\n> \tstring_list_clear(&track, 0);\n>\n> -\treturn 0;\n> +\treturn result;\n> }\n>\n> struct branch_info {\n> @@ -715,6 +719,7 @@ static int mv(int argc, const char **argv, const char *prefix)\n> \tstruct rename_info rename;\n> \tint i, refs_renamed_nr = 0, refspec_updated = 0;\n> \tstruct progress *progress = NULL;\n> +\tint result = 0;\n>\n> \targc = parse_options(argc, argv, prefix, options,\n> \t\t\t builtin_remote_rename_usage, 0);\n> @@ -747,9 +752,11 @@ static int mv(int argc, const char **argv, const char *prefix)\n>\n> \tstrbuf_addf(&buf, \"remote.%s\", rename.old_name);\n> \tstrbuf_addf(&buf2, \"remote.%s\", rename.new_name);\n> -\tif (repo_config_rename_section(the_repository, buf.buf, buf2.buf) < 1)\n> -\t\treturn error(_(\"Could not rename config section '%s' to '%s'\"),\n> -\t\t\t\tbuf.buf, buf2.buf);\n> +\tif (repo_config_rename_section(the_repository, buf.buf, buf2.buf) < 1) {\n> +\t\tresult = error(_(\"Could not rename config section '%s' to '%s'\"),\n> +\t\t\t buf.buf, buf2.buf);\n> +\t\tgoto out;\n> +\t}\n>\n> \tif (oldremote->fetch.raw_nr) {\n> \t\tstrbuf_reset(&buf);\n> @@ -870,7 +877,7 @@ static int mv(int argc, const char **argv, const char *prefix)\n> \tstrbuf_release(&buf);\n> \tstrbuf_release(&buf2);\n> \tstrbuf_release(&buf3);\n> -\treturn 0;\n> +\treturn result;\n> }\n>\n> static int rm(int argc, const char **argv, const char *prefix)\n> --\n> 2.30.2\n"},{"id":"501624","messageId":"56522704-3023-44a7-ba01-7b0dea6b9bdf@web.de","threadId":"61999","inReplyTo":"xmqqseuvndsq.fsf@gitster.g","subject":"Re: [PATCH] remote: plug memory leaks at early returns","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-08-24T08:06:10Z","receivedAt":"2024-08-24T08:06:21Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"\nAm 23.08.24 um 23:13 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> Signed-off-by: René Scharfe \n>> ---\n>> builtin/remote.c | 23 +++++++++++++++--------\n>> 1 file changed, 15 insertions(+), 8 deletions(-)\n>\n> Does this allow us to mark any test script as leak-free?\n\nI don't think so -- at least GIT_TEST_PASSING_SANITIZE_LEAK=check still\npasses for me.\n\n> Will queue in the meantime but may drop it if Patrick says it\n> already is covered or something.\n\nOK.\n\nRené\n"},{"id":"501637","messageId":"ZswUWj_dg7-kM8b0@tanuki","threadId":"61999","inReplyTo":"xmqqseuvndsq.fsf@gitster.g","subject":"Re: [PATCH] remote: plug memory leaks at early returns","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T05:36:26Z","receivedAt":"2024-08-26T05:36:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Aug 23, 2024 at 02:13:09PM -0700, Junio C Hamano wrote:\n> René Scharfe writes:\n> \n> > Signed-off-by: René Scharfe \n> > ---\n> > builtin/remote.c | 23 +++++++++++++++--------\n> > 1 file changed, 15 insertions(+), 8 deletions(-)\n> \n> Looks straight-forward. Does this allow us to mark any test script\n> as leak-free? I understand that Patrick has another round of\n> leakfixes topic that is not yet published, and I'd prefer to see us\n> not step each other's toes.\n\nNo, this doesn't conflict with anything I have. And even if it did, I'd\nbe happy to drop some patches from my local series :)\n\nThe changes themselves also look good to me, thanks!\n\nPatrick\n"},{"id":"501670","messageId":"xmqqmskznwsp.fsf@gitster.g","threadId":"61999","inReplyTo":"ZswUWj_dg7-kM8b0@tanuki","subject":"Re: [PATCH] remote: plug memory leaks at early returns","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-26T15:11:50Z","receivedAt":"2024-08-26T15:12:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Fri, Aug 23, 2024 at 02:13:09PM -0700, Junio C Hamano wrote:\n>> René Scharfe writes:\n>> \n>> > Signed-off-by: René Scharfe \n>> > ---\n>> > builtin/remote.c | 23 +++++++++++++++--------\n>> > 1 file changed, 15 insertions(+), 8 deletions(-)\n>> \n>> Looks straight-forward. Does this allow us to mark any test script\n>> as leak-free? I understand that Patrick has another round of\n>> leakfixes topic that is not yet published, and I'd prefer to see us\n>> not step each other's toes.\n>\n> No, this doesn't conflict with anything I have. And even if it did, I'd\n> be happy to drop some patches from my local series :)\n>\n> The changes themselves also look good to me, thanks!\n\nThanks. Will mark for \"next\".\n\n"}]}