diff mbox series

checkout: plug some leaks in git-restore

Message ID 04a9e001-5cd6-402c-86eb-f3751aa6f354@gmail.com (mailing list archive)
State Superseded
Headers show
Series checkout: plug some leaks in git-restore | expand

Commit Message

Rubén Justo March 14, 2024, 1:32 a.m. UTC
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 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.

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)
---

 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, 28 insertions(+), 27 deletions(-)

Comments

Eric Sunshine March 14, 2024, 2:39 a.m. UTC | #1
On Wed, Mar 13, 2024 at 9:33 PM Rubén Justo <rjusto@gmail.com> wrote:
> 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 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.
>
> As a consequence, mark: t2070, t2071, t2072 and t6418 as leak-free.
> ---

Missing sign-off.
diff mbox series

Patch

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 4fe049cf37..2ff4cf88a6 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1702,10 +1702,10 @@  static struct option *add_checkout_path_options(struct checkout_opts *opts,
 /* create-branch option (either b or 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,
-			 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)
 {
 	int parseopt_flags = 0;
 
@@ -1907,6 +1907,20 @@  static int checkout_main(int argc, const char **argv, const char *prefix,
 		return checkout_branch(opts, new_branch_info);
 }
 
+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);
+	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)
 {
 	struct checkout_opts opts;
@@ -1922,8 +1936,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 +1965,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 +1984,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 +2002,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 +2022,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 +2036,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 16d6348b69..ac404945d4 100755
--- 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 27e85be40a..42d5522119 100755
--- 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 'setup' '
diff --git a/t/t2072-restore-pathspec-file.sh b/t/t2072-restore-pathspec-file.sh
index 8198a1e578..86c9c88788 100755
--- 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 41288a60ce..48a62cb855 100755
--- 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