checkout: plug some leaks in git-restore

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/checkout.c b/builtin/checkout.c
index a6e30931b5c809da06c751ba6ef43e47a5a3e0ea..54684e1f943b2964d11d0bd32a34ce1cebd08729 100644 (file)
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1687,10 +1687,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;
@@ -1806,7 +1807,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) {
@@ -1815,7 +1816,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);
 
@@ -1835,7 +1836,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);
 
@@ -1885,9 +1886,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)
@@ -1905,8 +1913,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;
@@ -1936,13 +1942,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)
@@ -1960,8 +1961,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;
@@ -1980,11 +1979,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)
@@ -2003,8 +1999,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;
@@ -2019,9 +2013,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);
 }

diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh
index 16d6348b692806ab25faf89d77a73cd62fab4d93..ac404945d4c4e28b608f664f64c4b422557b2684 100755 (executable)
--- a/t/t2070-restore.sh
+++ b/t/t2070-restore.sh
@@ -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' '

diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh
index b5c5c0ff7e37ced9b6b50a06e4ec3e30c536cd53..9b25eb39c76ae75f34c6441773047270ec571d54 100755 (executable)
--- a/t/t2071-restore-patch.sh
+++ b/t/t2071-restore-patch.sh
@@ -2,6 +2,7 @@
 
 test_description='git restore --patch'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-patch-mode.sh
 
 test_expect_success PERL 'setup' '

diff --git a/t/t2072-restore-pathspec-file.sh b/t/t2072-restore-pathspec-file.sh
index 8198a1e5789cc963c32ecb028209a7a773144fd5..86c9c887881b94a586dc96b3f495a72c022fd827 100755 (executable)
--- a/t/t2072-restore-pathspec-file.sh
+++ b/t/t2072-restore-pathspec-file.sh
@@ -2,6 +2,7 @@
 
 test_description='restore --pathspec-from-file'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_tick

diff --git a/t/t6418-merge-text-auto.sh b/t/t6418-merge-text-auto.sh
index 41288a60ceb549295699f2efd9cb8e187d3e297b..48a62cb85568bfb7f77a8c597096617dfa5fcf4c 100755 (executable)
--- a/t/t6418-merge-text-auto.sh
+++ b/t/t6418-merge-text-auto.sh
@@ -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