@@ -1704,10 +1704,11 @@ static char cb_option = 'b';
static int checkout_main(int argc, const char **argv, const char *prefix,
struct checkout_opts *opts, struct option *options,
- const char * const usagestr[],
- struct branch_info *new_branch_info)
+ const char * const usagestr[])
{
int parseopt_flags = 0;
+ struct branch_info new_branch_info = { 0 };
+ int ret;
opts->overwrite_ignore = 1;
opts->prefix = prefix;
@@ -1823,7 +1824,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
opts->track == BRANCH_TRACK_UNSPECIFIED &&
!opts->new_branch;
int n = parse_branchname_arg(argc, argv, dwim_ok,
- new_branch_info, opts, &rev);
+ &new_branch_info, opts, &rev);
argv += n;
argc -= n;
} else if (!opts->accept_ref && opts->from_treeish) {
@@ -1832,7 +1833,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
if (repo_get_oid_mb(the_repository, opts->from_treeish, &rev))
die(_("could not resolve %s"), opts->from_treeish);
- setup_new_branch_info_and_source_tree(new_branch_info,
+ setup_new_branch_info_and_source_tree(&new_branch_info,
opts, &rev,
opts->from_treeish);
@@ -1852,7 +1853,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
* Try to give more helpful suggestion.
* new_branch && argc > 1 will be caught later.
*/
- if (opts->new_branch && argc == 1 && !new_branch_info->commit)
+ if (opts->new_branch && argc == 1 && !new_branch_info.commit)
die(_("'%s' is not a commit and a branch '%s' cannot be created from it"),
argv[0], opts->new_branch);
@@ -1902,9 +1903,16 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
}
if (opts->patch_mode || opts->pathspec.nr)
- return checkout_paths(opts, new_branch_info);
+ ret = checkout_paths(opts, &new_branch_info);
else
- return checkout_branch(opts, new_branch_info);
+ ret = checkout_branch(opts, &new_branch_info);
+
+ branch_info_release(&new_branch_info);
+ clear_pathspec(&opts->pathspec);
+ free(opts->pathspec_from_file);
+ free(options);
+
+ return ret;
}
int cmd_checkout(int argc, const char **argv, const char *prefix)
@@ -1922,8 +1930,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode (default)")),
OPT_END()
};
- int ret;
- struct branch_info new_branch_info = { 0 };
memset(&opts, 0, sizeof(opts));
opts.dwim_new_local_branch = 1;
@@ -1953,13 +1959,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
options = add_common_switch_branch_options(&opts, options);
options = add_checkout_path_options(&opts, options);
- ret = checkout_main(argc, argv, prefix, &opts,
- options, checkout_usage, &new_branch_info);
- branch_info_release(&new_branch_info);
- clear_pathspec(&opts.pathspec);
- free(opts.pathspec_from_file);
- FREE_AND_NULL(options);
- return ret;
+ return checkout_main(argc, argv, prefix, &opts, options,
+ checkout_usage);
}
int cmd_switch(int argc, const char **argv, const char *prefix)
@@ -1977,8 +1978,6 @@ int cmd_switch(int argc, const char **argv, const char *prefix)
N_("throw away local modifications")),
OPT_END()
};
- int ret;
- struct branch_info new_branch_info = { 0 };
memset(&opts, 0, sizeof(opts));
opts.dwim_new_local_branch = 1;
@@ -1997,11 +1996,8 @@ int cmd_switch(int argc, const char **argv, const char *prefix)
cb_option = 'c';
- ret = checkout_main(argc, argv, prefix, &opts,
- options, switch_branch_usage, &new_branch_info);
- branch_info_release(&new_branch_info);
- FREE_AND_NULL(options);
- return ret;
+ return checkout_main(argc, argv, prefix, &opts, options,
+ switch_branch_usage);
}
int cmd_restore(int argc, const char **argv, const char *prefix)
@@ -2020,8 +2016,6 @@ int cmd_restore(int argc, const char **argv, const char *prefix)
OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode")),
OPT_END()
};
- int ret;
- struct branch_info new_branch_info = { 0 };
memset(&opts, 0, sizeof(opts));
opts.accept_ref = 0;
@@ -2036,9 +2030,6 @@ int cmd_restore(int argc, const char **argv, const char *prefix)
options = add_common_options(&opts, options);
options = add_checkout_path_options(&opts, options);
- ret = checkout_main(argc, argv, prefix, &opts,
- options, restore_usage, &new_branch_info);
- branch_info_release(&new_branch_info);
- FREE_AND_NULL(options);
- return ret;
+ return checkout_main(argc, argv, prefix, &opts, options,
+ restore_usage);
}
@@ -5,6 +5,7 @@ test_description='restore basic functionality'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
@@ -2,6 +2,7 @@
test_description='git restore --patch'
+TEST_PASSES_SANITIZE_LEAK=true
. ./lib-patch-mode.sh
test_expect_success 'setup' '
@@ -2,6 +2,7 @@
test_description='restore --pathspec-from-file'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_tick
@@ -15,6 +15,7 @@ test_description='CRLF merge conflict across text=auto change
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_have_prereq SED_STRIPS_CR && SED_OPTIONS=-b
In git-restore we need to free the pathspec and pathspec_from_file values from the struct checkout_opts. A simple fix could be to free them in cmd_restore, after the call to checkout_main returns, like we are doing [1][2] in the sibling function cmd_checkout. However, we can do even better. We have git-switch and git-restore, both of them spin-offs[3][4] of git-checkout. All three are implemented as thin wrappers around checkout_main. Considering this, it makes a lot of sense to do the cleanup closer to checkout_main. Move the cleanups, including the new_branch_info variable, to checkout_main. As a consequence, mark: t2070, t2071, t2072 and t6418 as leak-free. [1] 9081a421a6 (checkout: fix "branch info" memory leaks, 2021-11-16) [2] 7ce4088ab7 (parse-options: consistently allocate memory in fix_filename(), 2023-03-04) [3] d787d311db (checkout: split part of it to new command 'switch', 2019-03-29) [4] 46e91b663b (checkout: split part of it to new command 'restore', 2019-04-25) Signed-off-by: Rubén Justo <rjusto@gmail.com> --- Range-diff: 1: 93f900a81c ! 1: 2bfcb55424 checkout: plug some leaks in git-restore @@ Commit message checkout_main returns, like we are doing [1][2] in the sibling function cmd_checkout. - However, we can do better. + However, we can do even better. We have git-switch and git-restore, both of them spin-offs[3][4] of git-checkout. All three are implemented as thin wrappers around checkout_main. Considering this, it makes a lot of sense to do the cleanup closer to checkout_main. - Factor out the call to checkout_main in a function that does both the - work and the cleanup, and use it in the three wrappers. + Move the cleanups, including the new_branch_info variable, to + checkout_main. As a consequence, mark: t2070, t2071, t2072 and t6418 as leak-free. @@ Commit message Signed-off-by: Rubén Justo <rjusto@gmail.com> ## builtin/checkout.c ## -@@ builtin/checkout.c: static struct option *add_checkout_path_options(struct checkout_opts *opts, - /* create-branch option (either b or c) */ - static char cb_option = 'b'; +@@ builtin/checkout.c: static char cb_option = 'b'; --static int checkout_main(int argc, const char **argv, const char *prefix, -- struct checkout_opts *opts, struct option *options, + static int checkout_main(int argc, const char **argv, const char *prefix, + struct checkout_opts *opts, struct option *options, - const char * const usagestr[], - struct branch_info *new_branch_info) -+static int checkout_main_1(int argc, const char **argv, const char *prefix, -+ struct checkout_opts *opts, struct option *options, -+ const char * const usagestr[], -+ struct branch_info *new_branch_info) ++ const char * const usagestr[]) { int parseopt_flags = 0; ++ struct branch_info new_branch_info = { 0 }; ++ int ret; + opts->overwrite_ignore = 1; + opts->prefix = prefix; @@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const char *prefix, - return checkout_branch(opts, new_branch_info); - } + opts->track == BRANCH_TRACK_UNSPECIFIED && + !opts->new_branch; + int n = parse_branchname_arg(argc, argv, dwim_ok, +- new_branch_info, opts, &rev); ++ &new_branch_info, opts, &rev); + argv += n; + argc -= n; + } else if (!opts->accept_ref && opts->from_treeish) { +@@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const char *prefix, + if (repo_get_oid_mb(the_repository, opts->from_treeish, &rev)) + die(_("could not resolve %s"), opts->from_treeish); -+static int checkout_main(int argc, const char **argv, const char *prefix, -+ struct checkout_opts *opts, struct option *options, -+ const char * const usagestr[]) -+{ -+ struct branch_info new_branch_info = { 0 }; -+ int ret = checkout_main_1(argc, argv, prefix, opts, options, -+ checkout_usage, &new_branch_info); +- setup_new_branch_info_and_source_tree(new_branch_info, ++ setup_new_branch_info_and_source_tree(&new_branch_info, + opts, &rev, + opts->from_treeish); + +@@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const char *prefix, + * Try to give more helpful suggestion. + * new_branch && argc > 1 will be caught later. + */ +- if (opts->new_branch && argc == 1 && !new_branch_info->commit) ++ if (opts->new_branch && argc == 1 && !new_branch_info.commit) + die(_("'%s' is not a commit and a branch '%s' cannot be created from it"), + argv[0], opts->new_branch); + +@@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const char *prefix, + } + + if (opts->patch_mode || opts->pathspec.nr) +- return checkout_paths(opts, new_branch_info); ++ ret = checkout_paths(opts, &new_branch_info); + else +- return checkout_branch(opts, new_branch_info); ++ ret = checkout_branch(opts, &new_branch_info); ++ + branch_info_release(&new_branch_info); + clear_pathspec(&opts->pathspec); + free(opts->pathspec_from_file); + free(options); -+ return ret; -+} + ++ return ret; + } + int cmd_checkout(int argc, const char **argv, const char *prefix) - { - struct checkout_opts opts; @@ builtin/checkout.c: int cmd_checkout(int argc, const char **argv, const char *prefix) OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode (default)")), OPT_END() builtin/checkout.c | 51 +++++++++++++------------------- t/t2070-restore.sh | 1 + t/t2071-restore-patch.sh | 1 + t/t2072-restore-pathspec-file.sh | 1 + t/t6418-merge-text-auto.sh | 1 + 5 files changed, 25 insertions(+), 30 deletions(-) 25 < 30, :-) thanks Junio.