{"thread":{"id":"61116","subject":"[PATCH] checkout: plug some leaks in git-restore","startedAt":"2024-03-14T01:33:05Z","lastAt":"2024-03-14T18:57:42Z","messageCount":6,"participants":["Rubén Justo","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"490599","messageId":"04a9e001-5cd6-402c-86eb-f3751aa6f354@gmail.com","threadId":"61116","inReplyTo":null,"subject":"[PATCH] checkout: plug some leaks in git-restore","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-03-14T01:32:55Z","receivedAt":"2024-03-14T01:33:05Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In git-restore we need to free the pathspec and pathspec_from_file\nvalues from the struct checkout_opts.\n\nA simple fix could be to free them in cmd_restore, after the call to\ncheckout_main returns, like we are doing [1][2] in the sibling function\ncmd_checkout.\n\nHowever, we can do better.\n\nWe have git-switch and git-restore, both of them spin-offs[3][4] of\ngit-checkout.  All three are implemented as thin wrappers around\ncheckout_main.  Considering this, it makes a lot of sense to do the\ncleanup closer to checkout_main.\n\nFactor out the call to checkout_main in a function that does both the\nwork and the cleanup, and use it in the three wrappers.\n\nAs a consequence, mark: t2070, t2071, t2072 and t6418 as leak-free.\n\n [1] 9081a421a6 (checkout: fix \"branch info\" memory leaks, 2021-11-16)\n\n [2] 7ce4088ab7 (parse-options: consistently allocate memory in\n     fix_filename(), 2023-03-04)\n\n [3] d787d311db (checkout: split part of it to new command 'switch',\n     2019-03-29)\n\n [4] 46e91b663b (checkout: split part of it to new command 'restore',\n     2019-04-25)\n---\n\n builtin/checkout.c               | 51 +++++++++++++++-----------------\n t/t2070-restore.sh               |  1 +\n t/t2071-restore-patch.sh         |  1 +\n t/t2072-restore-pathspec-file.sh |  1 +\n t/t6418-merge-text-auto.sh       |  1 +\n 5 files changed, 28 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 4fe049cf37..2ff4cf88a6 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1702,10 +1702,10 @@ static struct option *add_checkout_path_options(struct checkout_opts *opts,\n /* create-branch option (either b or c) */\n static char cb_option = 'b';\n \n-static int checkout_main(int argc, const char **argv, const char *prefix,\n-\t\t\t struct checkout_opts *opts, struct option *options,\n-\t\t\t const char * const usagestr[],\n-\t\t\t struct branch_info *new_branch_info)\n+static int checkout_main_1(int argc, const char **argv, const char *prefix,\n+\t\t\t   struct checkout_opts *opts, struct option *options,\n+\t\t\t   const char * const usagestr[],\n+\t\t\t   struct branch_info *new_branch_info)\n {\n \tint parseopt_flags = 0;\n \n@@ -1907,6 +1907,20 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\treturn checkout_branch(opts, new_branch_info);\n }\n \n+static int checkout_main(int argc, const char **argv, const char *prefix,\n+\t\t\t      struct checkout_opts *opts, struct option *options,\n+\t\t\t      const char * const usagestr[])\n+{\n+\tstruct branch_info new_branch_info = { 0 };\n+\tint ret = checkout_main_1(argc, argv, prefix, opts, options,\n+\t\t\t\t  checkout_usage, &new_branch_info);\n+\tbranch_info_release(&new_branch_info);\n+\tclear_pathspec(&opts->pathspec);\n+\tfree(opts->pathspec_from_file);\n+\tfree(options);\n+\treturn ret;\n+}\n+\n int cmd_checkout(int argc, const char **argv, const char *prefix)\n {\n \tstruct checkout_opts opts;\n@@ -1922,8 +1936,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"overlay\", &opts.overlay_mode, N_(\"use overlay mode (default)\")),\n \t\tOPT_END()\n \t};\n-\tint ret;\n-\tstruct branch_info new_branch_info = { 0 };\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.dwim_new_local_branch = 1;\n@@ -1953,13 +1965,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \toptions = add_common_switch_branch_options(&opts, options);\n \toptions = add_checkout_path_options(&opts, options);\n \n-\tret = checkout_main(argc, argv, prefix, &opts,\n-\t\t\t    options, checkout_usage, &new_branch_info);\n-\tbranch_info_release(&new_branch_info);\n-\tclear_pathspec(&opts.pathspec);\n-\tfree(opts.pathspec_from_file);\n-\tFREE_AND_NULL(options);\n-\treturn ret;\n+\treturn checkout_main(argc, argv, prefix, &opts, options,\n+\t\t\t     checkout_usage);\n }\n \n int cmd_switch(int argc, const char **argv, const char *prefix)\n@@ -1977,8 +1984,6 @@ int cmd_switch(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"throw away local modifications\")),\n \t\tOPT_END()\n \t};\n-\tint ret;\n-\tstruct branch_info new_branch_info = { 0 };\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.dwim_new_local_branch = 1;\n@@ -1997,11 +2002,8 @@ int cmd_switch(int argc, const char **argv, const char *prefix)\n \n \tcb_option = 'c';\n \n-\tret = checkout_main(argc, argv, prefix, &opts,\n-\t\t\t    options, switch_branch_usage, &new_branch_info);\n-\tbranch_info_release(&new_branch_info);\n-\tFREE_AND_NULL(options);\n-\treturn ret;\n+\treturn checkout_main(argc, argv, prefix, &opts, options,\n+\t\t\t     switch_branch_usage);\n }\n \n int cmd_restore(int argc, const char **argv, const char *prefix)\n@@ -2020,8 +2022,6 @@ int cmd_restore(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"overlay\", &opts.overlay_mode, N_(\"use overlay mode\")),\n \t\tOPT_END()\n \t};\n-\tint ret;\n-\tstruct branch_info new_branch_info = { 0 };\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.accept_ref = 0;\n@@ -2036,9 +2036,6 @@ int cmd_restore(int argc, const char **argv, const char *prefix)\n \toptions = add_common_options(&opts, options);\n \toptions = add_checkout_path_options(&opts, options);\n \n-\tret = checkout_main(argc, argv, prefix, &opts,\n-\t\t\t    options, restore_usage, &new_branch_info);\n-\tbranch_info_release(&new_branch_info);\n-\tFREE_AND_NULL(options);\n-\treturn ret;\n+\treturn checkout_main(argc, argv, prefix, &opts, options,\n+\t\t\t     restore_usage);\n }\ndiff --git a/t/t2070-restore.sh b/t/t2070-restore.sh\nindex 16d6348b69..ac404945d4 100755\n--- a/t/t2070-restore.sh\n+++ b/t/t2070-restore.sh\n@@ -5,6 +5,7 @@ test_description='restore basic functionality'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh\nindex 27e85be40a..42d5522119 100755\n--- a/t/t2071-restore-patch.sh\n+++ b/t/t2071-restore-patch.sh\n@@ -2,6 +2,7 @@\n \n test_description='git restore --patch'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./lib-patch-mode.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t2072-restore-pathspec-file.sh b/t/t2072-restore-pathspec-file.sh\nindex 8198a1e578..86c9c88788 100755\n--- a/t/t2072-restore-pathspec-file.sh\n+++ b/t/t2072-restore-pathspec-file.sh\n@@ -2,6 +2,7 @@\n \n test_description='restore --pathspec-from-file'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_tick\ndiff --git a/t/t6418-merge-text-auto.sh b/t/t6418-merge-text-auto.sh\nindex 41288a60ce..48a62cb855 100755\n--- a/t/t6418-merge-text-auto.sh\n+++ b/t/t6418-merge-text-auto.sh\n@@ -15,6 +15,7 @@ test_description='CRLF merge conflict across text=auto change\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_have_prereq SED_STRIPS_CR && SED_OPTIONS=-b\n-- \n2.44.0.341.g37b2c3c964\n"},{"id":"490602","messageId":"CAPig+cSYB3uNZhNGh6++5KA03ehv5pE+_KK1X=hUbb5JHwMoJQ@mail.gmail.com","threadId":"61116","inReplyTo":"04a9e001-5cd6-402c-86eb-f3751aa6f354@gmail.com","subject":"Re: [PATCH] checkout: plug some leaks in git-restore","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-14T02:39:14Z","receivedAt":"2024-03-14T02:39:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 13, 2024 at 9:33 PM Rubén Justo <rjusto@gmail.com> wrote:\n> In git-restore we need to free the pathspec and pathspec_from_file\n> values from the struct checkout_opts.\n>\n> A simple fix could be to free them in cmd_restore, after the call to\n> checkout_main returns, like we are doing [1][2] in the sibling function\n> cmd_checkout.\n>\n> However, we can do better.\n>\n> We have git-switch and git-restore, both of them spin-offs[3][4] of\n> git-checkout.  All three are implemented as thin wrappers around\n> checkout_main.  Considering this, it makes a lot of sense to do the\n> cleanup closer to checkout_main.\n>\n> Factor out the call to checkout_main in a function that does both the\n> work and the cleanup, and use it in the three wrappers.\n>\n> As a consequence, mark: t2070, t2071, t2072 and t6418 as leak-free.\n> ---\n\nMissing sign-off.\n"},{"id":"490612","messageId":"8faa0cd5-25e8-4a8f-ad8b-5fc1b6e5138b@gmail.com","threadId":"61116","inReplyTo":"04a9e001-5cd6-402c-86eb-f3751aa6f354@gmail.com","subject":"[PATCH] checkout: plug some leaks in git-restore","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-03-14T07:36:25Z","receivedAt":"2024-03-14T07:36:31Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In git-restore we need to free the pathspec and pathspec_from_file\nvalues from the struct checkout_opts.\n\nA simple fix could be to free them in cmd_restore, after the call to\ncheckout_main returns, like we are doing [1][2] in the sibling function\ncmd_checkout.\n\nHowever, we can do better.\n\nWe have git-switch and git-restore, both of them spin-offs[3][4] of\ngit-checkout.  All three are implemented as thin wrappers around\ncheckout_main.  Considering this, it makes a lot of sense to do the\ncleanup closer to checkout_main.\n\nFactor out the call to checkout_main in a function that does both the\nwork and the cleanup, and use it in the three wrappers.\n\nAs a consequence, mark: t2070, t2071, t2072 and t6418 as leak-free.\n\n [1] 9081a421a6 (checkout: fix \"branch info\" memory leaks, 2021-11-16)\n\n [2] 7ce4088ab7 (parse-options: consistently allocate memory in\n     fix_filename(), 2023-03-04)\n\n [3] d787d311db (checkout: split part of it to new command 'switch',\n     2019-03-29)\n\n [4] 46e91b663b (checkout: split part of it to new command 'restore',\n     2019-04-25)\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\nRange-diff:\n1:  d54a2c4dcc ! 1:  78ad33fa02 checkout: plug some leaks in git-restore\n    @@ Commit message\n          [4] 46e91b663b (checkout: split part of it to new command 'restore',\n              2019-04-25)\n     \n    +    Signed-off-by: Rubén Justo <rjusto@gmail.com>\n    +\n      ## builtin/checkout.c ##\n     @@ builtin/checkout.c: static struct option *add_checkout_path_options(struct checkout_opts *opts,\n      /* create-branch option (either b or c) */\n\nThanks Eric.\n\n builtin/checkout.c               | 51 +++++++++++++++-----------------\n t/t2070-restore.sh               |  1 +\n t/t2071-restore-patch.sh         |  1 +\n t/t2072-restore-pathspec-file.sh |  1 +\n t/t6418-merge-text-auto.sh       |  1 +\n 5 files changed, 28 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 4fe049cf37..2ff4cf88a6 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1702,10 +1702,10 @@ static struct option *add_checkout_path_options(struct checkout_opts *opts,\n /* create-branch option (either b or c) */\n static char cb_option = 'b';\n \n-static int checkout_main(int argc, const char **argv, const char *prefix,\n-\t\t\t struct checkout_opts *opts, struct option *options,\n-\t\t\t const char * const usagestr[],\n-\t\t\t struct branch_info *new_branch_info)\n+static int checkout_main_1(int argc, const char **argv, const char *prefix,\n+\t\t\t   struct checkout_opts *opts, struct option *options,\n+\t\t\t   const char * const usagestr[],\n+\t\t\t   struct branch_info *new_branch_info)\n {\n \tint parseopt_flags = 0;\n \n@@ -1907,6 +1907,20 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\treturn checkout_branch(opts, new_branch_info);\n }\n \n+static int checkout_main(int argc, const char **argv, const char *prefix,\n+\t\t\t      struct checkout_opts *opts, struct option *options,\n+\t\t\t      const char * const usagestr[])\n+{\n+\tstruct branch_info new_branch_info = { 0 };\n+\tint ret = checkout_main_1(argc, argv, prefix, opts, options,\n+\t\t\t\t  checkout_usage, &new_branch_info);\n+\tbranch_info_release(&new_branch_info);\n+\tclear_pathspec(&opts->pathspec);\n+\tfree(opts->pathspec_from_file);\n+\tfree(options);\n+\treturn ret;\n+}\n+\n int cmd_checkout(int argc, const char **argv, const char *prefix)\n {\n \tstruct checkout_opts opts;\n@@ -1922,8 +1936,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"overlay\", &opts.overlay_mode, N_(\"use overlay mode (default)\")),\n \t\tOPT_END()\n \t};\n-\tint ret;\n-\tstruct branch_info new_branch_info = { 0 };\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.dwim_new_local_branch = 1;\n@@ -1953,13 +1965,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \toptions = add_common_switch_branch_options(&opts, options);\n \toptions = add_checkout_path_options(&opts, options);\n \n-\tret = checkout_main(argc, argv, prefix, &opts,\n-\t\t\t    options, checkout_usage, &new_branch_info);\n-\tbranch_info_release(&new_branch_info);\n-\tclear_pathspec(&opts.pathspec);\n-\tfree(opts.pathspec_from_file);\n-\tFREE_AND_NULL(options);\n-\treturn ret;\n+\treturn checkout_main(argc, argv, prefix, &opts, options,\n+\t\t\t     checkout_usage);\n }\n \n int cmd_switch(int argc, const char **argv, const char *prefix)\n@@ -1977,8 +1984,6 @@ int cmd_switch(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"throw away local modifications\")),\n \t\tOPT_END()\n \t};\n-\tint ret;\n-\tstruct branch_info new_branch_info = { 0 };\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.dwim_new_local_branch = 1;\n@@ -1997,11 +2002,8 @@ int cmd_switch(int argc, const char **argv, const char *prefix)\n \n \tcb_option = 'c';\n \n-\tret = checkout_main(argc, argv, prefix, &opts,\n-\t\t\t    options, switch_branch_usage, &new_branch_info);\n-\tbranch_info_release(&new_branch_info);\n-\tFREE_AND_NULL(options);\n-\treturn ret;\n+\treturn checkout_main(argc, argv, prefix, &opts, options,\n+\t\t\t     switch_branch_usage);\n }\n \n int cmd_restore(int argc, const char **argv, const char *prefix)\n@@ -2020,8 +2022,6 @@ int cmd_restore(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"overlay\", &opts.overlay_mode, N_(\"use overlay mode\")),\n \t\tOPT_END()\n \t};\n-\tint ret;\n-\tstruct branch_info new_branch_info = { 0 };\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.accept_ref = 0;\n@@ -2036,9 +2036,6 @@ int cmd_restore(int argc, const char **argv, const char *prefix)\n \toptions = add_common_options(&opts, options);\n \toptions = add_checkout_path_options(&opts, options);\n \n-\tret = checkout_main(argc, argv, prefix, &opts,\n-\t\t\t    options, restore_usage, &new_branch_info);\n-\tbranch_info_release(&new_branch_info);\n-\tFREE_AND_NULL(options);\n-\treturn ret;\n+\treturn checkout_main(argc, argv, prefix, &opts, options,\n+\t\t\t     restore_usage);\n }\ndiff --git a/t/t2070-restore.sh b/t/t2070-restore.sh\nindex 16d6348b69..ac404945d4 100755\n--- a/t/t2070-restore.sh\n+++ b/t/t2070-restore.sh\n@@ -5,6 +5,7 @@ test_description='restore basic functionality'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh\nindex 27e85be40a..42d5522119 100755\n--- a/t/t2071-restore-patch.sh\n+++ b/t/t2071-restore-patch.sh\n@@ -2,6 +2,7 @@\n \n test_description='git restore --patch'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./lib-patch-mode.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t2072-restore-pathspec-file.sh b/t/t2072-restore-pathspec-file.sh\nindex 8198a1e578..86c9c88788 100755\n--- a/t/t2072-restore-pathspec-file.sh\n+++ b/t/t2072-restore-pathspec-file.sh\n@@ -2,6 +2,7 @@\n \n test_description='restore --pathspec-from-file'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_tick\ndiff --git a/t/t6418-merge-text-auto.sh b/t/t6418-merge-text-auto.sh\nindex 41288a60ce..48a62cb855 100755\n--- a/t/t6418-merge-text-auto.sh\n+++ b/t/t6418-merge-text-auto.sh\n@@ -15,6 +15,7 @@ test_description='CRLF merge conflict across text=auto change\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_have_prereq SED_STRIPS_CR && SED_OPTIONS=-b\n-- \n2.43.0\n"},{"id":"490625","messageId":"xmqqbk7gwym8.fsf@gitster.g","threadId":"61116","inReplyTo":"8faa0cd5-25e8-4a8f-ad8b-5fc1b6e5138b@gmail.com","subject":"Re: [PATCH] checkout: plug some leaks in git-restore","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-14T16:45:51Z","receivedAt":"2024-03-14T16:45:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> In git-restore we need to free the pathspec and pathspec_from_file\n> values from the struct checkout_opts.\n>\n> A simple fix could be to free them in cmd_restore, after the call to\n> checkout_main returns, like we are doing [1][2] in the sibling function\n> cmd_checkout.\n>\n> However, we can do better.\n\nQuite honestly, my knee-jerk raction against _main_1() was \"Yuck\".\n\nIf the repetition of \"here are the things we need to clean up\"\nshared in the current checkout_main() looks so disturbing, I would\nhave gone in the opposite direction: the current callers of _main()\nwill do these clean-up actions after the call to _main() returns, so\nbundle them into a helper function and call it from the callers of\n_main(), without introducing an extra layer of opacity, and I would\nhave thought that would be what we would call \"doing better\".\n\nEven better, shouldn't you be able to do much better by not doing\nthe _main_1() thing at all?  If you look at checkout_main(), the\nonly way to leave thsi function, aside from die()'s, are \"return\"\nstatements to the caller at the very end of the function.  You\nshould be able to, instead of returning, capture the value you\nreceive from calling either checkout_paths() or checkout_branch(),\nand then do the common \"clean-up\" you stole from existing calls and\nmoved into _main_1(), and after doing so, return the value you\ncaptured from one of these calls that used to directly return, no?\n\nPerhaps something along this line, which should be an equilvalent to\nwhat your patch did?\n\n builtin/checkout.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git c/builtin/checkout.c w/builtin/checkout.c\nindex 15293a3013..a8ccdfa1f2 100644\n--- c/builtin/checkout.c\n+++ w/builtin/checkout.c\n@@ -1706,6 +1706,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\t struct branch_info *new_branch_info)\n {\n \tint parseopt_flags = 0;\n+\tint retval;\n \n \topts->overwrite_ignore = 1;\n \topts->prefix = prefix;\n@@ -1900,9 +1901,16 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t}\n \n \tif (opts->patch_mode || opts->pathspec.nr)\n-\t\treturn checkout_paths(opts, new_branch_info);\n+\t\tretval = checkout_paths(opts, new_branch_info);\n \telse\n-\t\treturn checkout_branch(opts, new_branch_info);\n+\t\tretval = checkout_branch(opts, new_branch_info);\n+\n+\tbranch_info_release(new_branch_info);\n+\tclear_pathspec(&opts->pathspec);\n+\tfree(opts->pathspec_from_file);\n+\tFREE_AND_NULL(options);\n+\n+\treturn retval;\n }\n \n int cmd_checkout(int argc, const char **argv, const char *prefix)\n@@ -1953,10 +1961,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \n \tret = checkout_main(argc, argv, prefix, &opts,\n \t\t\t    options, checkout_usage, &new_branch_info);\n-\tbranch_info_release(&new_branch_info);\n-\tclear_pathspec(&opts.pathspec);\n-\tfree(opts.pathspec_from_file);\n-\tFREE_AND_NULL(options);\n \treturn ret;\n }\n \n@@ -1997,8 +2001,6 @@ int cmd_switch(int argc, const char **argv, const char *prefix)\n \n \tret = checkout_main(argc, argv, prefix, &opts,\n \t\t\t    options, switch_branch_usage, &new_branch_info);\n-\tbranch_info_release(&new_branch_info);\n-\tFREE_AND_NULL(options);\n \treturn ret;\n }\n \n@@ -2036,7 +2038,5 @@ int cmd_restore(int argc, const char **argv, const char *prefix)\n \n \tret = checkout_main(argc, argv, prefix, &opts,\n \t\t\t    options, restore_usage, &new_branch_info);\n-\tbranch_info_release(&new_branch_info);\n-\tFREE_AND_NULL(options);\n \treturn ret;\n }\n\n\n\n[Footnote]\n\nIt is somewhat funny that we are moving more things into the\ncheckout_main() common function.  Maybe the true culprit of this\nmess was that the approach for splitting \"switch\" and \"restore\" out\nof \"checkout\" was misdesigned.\n\nPeople were so loudly against \"checkout\" that can check out a\nbranch, or check out files and directories out of a commit, and that\nwas why these two separate commands to do these two separate things\nwere created.  But instead of two separate functions that do two\nseparate and unrelated things, and making \"checkout\" call either one\nof these depending on which one of the two separate things it is\nasked to do, we ended up in a state where a single function _main()\nis shared among the three, which may have solved the \"these two are\nseparate operations and having them crammed together into one\ncommand is confusing from the user's point of view\" complaint, but\nthese two operations are still tightly coupled.  The fact that the\nthings that need to be cleaned up after calling checkout_paths() and\ncheckout_branch() are identical is quite telling ;-)\n\n"},{"id":"490638","messageId":"64c1c3cc-51d7-4168-9731-4389889e1449@gmail.com","threadId":"61116","inReplyTo":"8faa0cd5-25e8-4a8f-ad8b-5fc1b6e5138b@gmail.com","subject":"[PATCH v2] checkout: plug some leaks in git-restore","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-03-14T18:08:58Z","receivedAt":"2024-03-14T18:09:01Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In git-restore we need to free the pathspec and pathspec_from_file\nvalues from the struct checkout_opts.\n\nA simple fix could be to free them in cmd_restore, after the call to\ncheckout_main returns, like we are doing [1][2] in the sibling function\ncmd_checkout.\n\nHowever, we can do even better.\n\nWe have git-switch and git-restore, both of them spin-offs[3][4] of\ngit-checkout.  All three are implemented as thin wrappers around\ncheckout_main.  Considering this, it makes a lot of sense to do the\ncleanup closer to checkout_main.\n\nMove the cleanups, including the new_branch_info variable, to\ncheckout_main.\n\nAs a consequence, mark: t2070, t2071, t2072 and t6418 as leak-free.\n\n [1] 9081a421a6 (checkout: fix \"branch info\" memory leaks, 2021-11-16)\n\n [2] 7ce4088ab7 (parse-options: consistently allocate memory in\n     fix_filename(), 2023-03-04)\n\n [3] d787d311db (checkout: split part of it to new command 'switch',\n     2019-03-29)\n\n [4] 46e91b663b (checkout: split part of it to new command 'restore',\n     2019-04-25)\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\nRange-diff:\n1:  93f900a81c ! 1:  2bfcb55424 checkout: plug some leaks in git-restore\n    @@ Commit message\n         checkout_main returns, like we are doing [1][2] in the sibling function\n         cmd_checkout.\n     \n    -    However, we can do better.\n    +    However, we can do even better.\n     \n         We have git-switch and git-restore, both of them spin-offs[3][4] of\n         git-checkout.  All three are implemented as thin wrappers around\n         checkout_main.  Considering this, it makes a lot of sense to do the\n         cleanup closer to checkout_main.\n     \n    -    Factor out the call to checkout_main in a function that does both the\n    -    work and the cleanup, and use it in the three wrappers.\n    +    Move the cleanups, including the new_branch_info variable, to\n    +    checkout_main.\n     \n         As a consequence, mark: t2070, t2071, t2072 and t6418 as leak-free.\n     \n    @@ Commit message\n         Signed-off-by: Rubén Justo <rjusto@gmail.com>\n     \n      ## builtin/checkout.c ##\n    -@@ builtin/checkout.c: static struct option *add_checkout_path_options(struct checkout_opts *opts,\n    - /* create-branch option (either b or c) */\n    - static char cb_option = 'b';\n    +@@ builtin/checkout.c: static char cb_option = 'b';\n      \n    --static int checkout_main(int argc, const char **argv, const char *prefix,\n    --\t\t\t struct checkout_opts *opts, struct option *options,\n    + static int checkout_main(int argc, const char **argv, const char *prefix,\n    + \t\t\t struct checkout_opts *opts, struct option *options,\n     -\t\t\t const char * const usagestr[],\n     -\t\t\t struct branch_info *new_branch_info)\n    -+static int checkout_main_1(int argc, const char **argv, const char *prefix,\n    -+\t\t\t   struct checkout_opts *opts, struct option *options,\n    -+\t\t\t   const char * const usagestr[],\n    -+\t\t\t   struct branch_info *new_branch_info)\n    ++\t\t\t const char * const usagestr[])\n      {\n      \tint parseopt_flags = 0;\n    ++\tstruct branch_info new_branch_info = { 0 };\n    ++\tint ret;\n      \n    + \topts->overwrite_ignore = 1;\n    + \topts->prefix = prefix;\n     @@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const char *prefix,\n    - \t\treturn checkout_branch(opts, new_branch_info);\n    - }\n    + \t\t\topts->track == BRANCH_TRACK_UNSPECIFIED &&\n    + \t\t\t!opts->new_branch;\n    + \t\tint n = parse_branchname_arg(argc, argv, dwim_ok,\n    +-\t\t\t\t\t     new_branch_info, opts, &rev);\n    ++\t\t\t\t\t     &new_branch_info, opts, &rev);\n    + \t\targv += n;\n    + \t\targc -= n;\n    + \t} else if (!opts->accept_ref && opts->from_treeish) {\n    +@@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const char *prefix,\n    + \t\tif (repo_get_oid_mb(the_repository, opts->from_treeish, &rev))\n    + \t\t\tdie(_(\"could not resolve %s\"), opts->from_treeish);\n      \n    -+static int checkout_main(int argc, const char **argv, const char *prefix,\n    -+\t\t\t      struct checkout_opts *opts, struct option *options,\n    -+\t\t\t      const char * const usagestr[])\n    -+{\n    -+\tstruct branch_info new_branch_info = { 0 };\n    -+\tint ret = checkout_main_1(argc, argv, prefix, opts, options,\n    -+\t\t\t\t  checkout_usage, &new_branch_info);\n    +-\t\tsetup_new_branch_info_and_source_tree(new_branch_info,\n    ++\t\tsetup_new_branch_info_and_source_tree(&new_branch_info,\n    + \t\t\t\t\t\t      opts, &rev,\n    + \t\t\t\t\t\t      opts->from_treeish);\n    + \n    +@@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const char *prefix,\n    + \t\t * Try to give more helpful suggestion.\n    + \t\t * new_branch && argc > 1 will be caught later.\n    + \t\t */\n    +-\t\tif (opts->new_branch && argc == 1 && !new_branch_info->commit)\n    ++\t\tif (opts->new_branch && argc == 1 && !new_branch_info.commit)\n    + \t\t\tdie(_(\"'%s' is not a commit and a branch '%s' cannot be created from it\"),\n    + \t\t\t\targv[0], opts->new_branch);\n    + \n    +@@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const char *prefix,\n    + \t}\n    + \n    + \tif (opts->patch_mode || opts->pathspec.nr)\n    +-\t\treturn checkout_paths(opts, new_branch_info);\n    ++\t\tret = checkout_paths(opts, &new_branch_info);\n    + \telse\n    +-\t\treturn checkout_branch(opts, new_branch_info);\n    ++\t\tret = checkout_branch(opts, &new_branch_info);\n    ++\n     +\tbranch_info_release(&new_branch_info);\n     +\tclear_pathspec(&opts->pathspec);\n     +\tfree(opts->pathspec_from_file);\n     +\tfree(options);\n    -+\treturn ret;\n    -+}\n     +\n    ++\treturn ret;\n    + }\n    + \n      int cmd_checkout(int argc, const char **argv, const char *prefix)\n    - {\n    - \tstruct checkout_opts opts;\n     @@ builtin/checkout.c: int cmd_checkout(int argc, const char **argv, const char *prefix)\n      \t\tOPT_BOOL(0, \"overlay\", &opts.overlay_mode, N_(\"use overlay mode (default)\")),\n      \t\tOPT_END()\n\n builtin/checkout.c               | 51 +++++++++++++-------------------\n t/t2070-restore.sh               |  1 +\n t/t2071-restore-patch.sh         |  1 +\n t/t2072-restore-pathspec-file.sh |  1 +\n t/t6418-merge-text-auto.sh       |  1 +\n 5 files changed, 25 insertions(+), 30 deletions(-)\n\n25 < 30, :-) thanks Junio.\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 4fe049cf37..2e8b0d18f4 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1704,10 +1704,11 @@ static char cb_option = 'b';\n \n static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\t struct checkout_opts *opts, struct option *options,\n-\t\t\t const char * const usagestr[],\n-\t\t\t struct branch_info *new_branch_info)\n+\t\t\t const char * const usagestr[])\n {\n \tint parseopt_flags = 0;\n+\tstruct branch_info new_branch_info = { 0 };\n+\tint ret;\n \n \topts->overwrite_ignore = 1;\n \topts->prefix = prefix;\n@@ -1823,7 +1824,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\topts->track == BRANCH_TRACK_UNSPECIFIED &&\n \t\t\t!opts->new_branch;\n \t\tint n = parse_branchname_arg(argc, argv, dwim_ok,\n-\t\t\t\t\t     new_branch_info, opts, &rev);\n+\t\t\t\t\t     &new_branch_info, opts, &rev);\n \t\targv += n;\n \t\targc -= n;\n \t} else if (!opts->accept_ref && opts->from_treeish) {\n@@ -1832,7 +1833,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\tif (repo_get_oid_mb(the_repository, opts->from_treeish, &rev))\n \t\t\tdie(_(\"could not resolve %s\"), opts->from_treeish);\n \n-\t\tsetup_new_branch_info_and_source_tree(new_branch_info,\n+\t\tsetup_new_branch_info_and_source_tree(&new_branch_info,\n \t\t\t\t\t\t      opts, &rev,\n \t\t\t\t\t\t      opts->from_treeish);\n \n@@ -1852,7 +1853,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t * Try to give more helpful suggestion.\n \t\t * new_branch && argc > 1 will be caught later.\n \t\t */\n-\t\tif (opts->new_branch && argc == 1 && !new_branch_info->commit)\n+\t\tif (opts->new_branch && argc == 1 && !new_branch_info.commit)\n \t\t\tdie(_(\"'%s' is not a commit and a branch '%s' cannot be created from it\"),\n \t\t\t\targv[0], opts->new_branch);\n \n@@ -1902,9 +1903,16 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t}\n \n \tif (opts->patch_mode || opts->pathspec.nr)\n-\t\treturn checkout_paths(opts, new_branch_info);\n+\t\tret = checkout_paths(opts, &new_branch_info);\n \telse\n-\t\treturn checkout_branch(opts, new_branch_info);\n+\t\tret = checkout_branch(opts, &new_branch_info);\n+\n+\tbranch_info_release(&new_branch_info);\n+\tclear_pathspec(&opts->pathspec);\n+\tfree(opts->pathspec_from_file);\n+\tfree(options);\n+\n+\treturn ret;\n }\n \n int cmd_checkout(int argc, const char **argv, const char *prefix)\n@@ -1922,8 +1930,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"overlay\", &opts.overlay_mode, N_(\"use overlay mode (default)\")),\n \t\tOPT_END()\n \t};\n-\tint ret;\n-\tstruct branch_info new_branch_info = { 0 };\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.dwim_new_local_branch = 1;\n@@ -1953,13 +1959,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \toptions = add_common_switch_branch_options(&opts, options);\n \toptions = add_checkout_path_options(&opts, options);\n \n-\tret = checkout_main(argc, argv, prefix, &opts,\n-\t\t\t    options, checkout_usage, &new_branch_info);\n-\tbranch_info_release(&new_branch_info);\n-\tclear_pathspec(&opts.pathspec);\n-\tfree(opts.pathspec_from_file);\n-\tFREE_AND_NULL(options);\n-\treturn ret;\n+\treturn checkout_main(argc, argv, prefix, &opts, options,\n+\t\t\t     checkout_usage);\n }\n \n int cmd_switch(int argc, const char **argv, const char *prefix)\n@@ -1977,8 +1978,6 @@ int cmd_switch(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"throw away local modifications\")),\n \t\tOPT_END()\n \t};\n-\tint ret;\n-\tstruct branch_info new_branch_info = { 0 };\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.dwim_new_local_branch = 1;\n@@ -1997,11 +1996,8 @@ int cmd_switch(int argc, const char **argv, const char *prefix)\n \n \tcb_option = 'c';\n \n-\tret = checkout_main(argc, argv, prefix, &opts,\n-\t\t\t    options, switch_branch_usage, &new_branch_info);\n-\tbranch_info_release(&new_branch_info);\n-\tFREE_AND_NULL(options);\n-\treturn ret;\n+\treturn checkout_main(argc, argv, prefix, &opts, options,\n+\t\t\t     switch_branch_usage);\n }\n \n int cmd_restore(int argc, const char **argv, const char *prefix)\n@@ -2020,8 +2016,6 @@ int cmd_restore(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"overlay\", &opts.overlay_mode, N_(\"use overlay mode\")),\n \t\tOPT_END()\n \t};\n-\tint ret;\n-\tstruct branch_info new_branch_info = { 0 };\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.accept_ref = 0;\n@@ -2036,9 +2030,6 @@ int cmd_restore(int argc, const char **argv, const char *prefix)\n \toptions = add_common_options(&opts, options);\n \toptions = add_checkout_path_options(&opts, options);\n \n-\tret = checkout_main(argc, argv, prefix, &opts,\n-\t\t\t    options, restore_usage, &new_branch_info);\n-\tbranch_info_release(&new_branch_info);\n-\tFREE_AND_NULL(options);\n-\treturn ret;\n+\treturn checkout_main(argc, argv, prefix, &opts, options,\n+\t\t\t     restore_usage);\n }\ndiff --git a/t/t2070-restore.sh b/t/t2070-restore.sh\nindex 16d6348b69..ac404945d4 100755\n--- a/t/t2070-restore.sh\n+++ b/t/t2070-restore.sh\n@@ -5,6 +5,7 @@ test_description='restore basic functionality'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh\nindex 27e85be40a..42d5522119 100755\n--- a/t/t2071-restore-patch.sh\n+++ b/t/t2071-restore-patch.sh\n@@ -2,6 +2,7 @@\n \n test_description='git restore --patch'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./lib-patch-mode.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t2072-restore-pathspec-file.sh b/t/t2072-restore-pathspec-file.sh\nindex 8198a1e578..86c9c88788 100755\n--- a/t/t2072-restore-pathspec-file.sh\n+++ b/t/t2072-restore-pathspec-file.sh\n@@ -2,6 +2,7 @@\n \n test_description='restore --pathspec-from-file'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_tick\ndiff --git a/t/t6418-merge-text-auto.sh b/t/t6418-merge-text-auto.sh\nindex 41288a60ce..48a62cb855 100755\n--- a/t/t6418-merge-text-auto.sh\n+++ b/t/t6418-merge-text-auto.sh\n@@ -15,6 +15,7 @@ test_description='CRLF merge conflict across text=auto change\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_have_prereq SED_STRIPS_CR && SED_OPTIONS=-b\n-- \n2.44.0.341.g2bfcb55424\n"},{"id":"490641","messageId":"xmqq8r2kvdy4.fsf@gitster.g","threadId":"61116","inReplyTo":"64c1c3cc-51d7-4168-9731-4389889e1449@gmail.com","subject":"Re: [PATCH v2] checkout: plug some leaks in git-restore","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-14T18:57:39Z","receivedAt":"2024-03-14T18:57:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 4fe049cf37..2e8b0d18f4 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1704,10 +1704,11 @@ static char cb_option = 'b';\n>  \n>  static int checkout_main(int argc, const char **argv, const char *prefix,\n>  \t\t\t struct checkout_opts *opts, struct option *options,\n> -\t\t\t const char * const usagestr[],\n> -\t\t\t struct branch_info *new_branch_info)\n> +\t\t\t const char * const usagestr[])\n>  {\n>  \tint parseopt_flags = 0;\n> +\tstruct branch_info new_branch_info = { 0 };\n> +\tint ret;\n\nAh, nice.  This is one thing I missed.\n\nThe callers of checkout_main() do not even care about the\nnew_branch_info structure so there is no point having them allocate\nand pass a pointer to one to call this function.\n\nThe remainder is just as expected.  Will queue.  Thanks.\n"}]}