| Message ID | a886c086-4b2e-4d41-ad40-b3fca20de42a@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mark t3701-add-interactive.sh as leak-free | expand |
Hi Rubén On 21/04/2024 11:29, Rubén Justo wrote: > Plug a leak we have since 5a76aff1a6 (add: convert to use > parse_pathspec, 2013-07-14). > > This leak can be triggered with: > $ git add -p anything > > Fixing this leak allows us to mark as leak-free the following tests: > > + t3701-add-interactive.sh > + t7514-commit-patch.sh > > Mark them with "TEST_PASSES_SANITIZE_LEAK=true" to notice and fix > promply any new leak that may be introduced and triggered by them in the > future. This makes me wonder if we're freeing the pathspec properly when using '--patch' in checkout, reset, restore and stash. Best Wishes Phillip > Signed-off-by: Rubén Justo <rjusto@gmail.com> > --- > builtin/add.c | 9 ++++++--- > t/t3701-add-interactive.sh | 1 + > t/t7514-commit-patch.sh | 2 ++ > 3 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/builtin/add.c b/builtin/add.c > index ae723bc85e..b7d3ff1e28 100644 > --- a/builtin/add.c > +++ b/builtin/add.c > @@ -150,7 +150,7 @@ static int refresh(int verbose, const struct pathspec *pathspec) > int interactive_add(const char **argv, const char *prefix, int patch) > { > struct pathspec pathspec; > - int unused; > + int unused, ret; > > if (!git_config_get_bool("add.interactive.usebuiltin", &unused)) > warning(_("the add.interactive.useBuiltin setting has been removed!\n" > @@ -163,9 +163,12 @@ int interactive_add(const char **argv, const char *prefix, int patch) > prefix, argv); > > if (patch) > - return !!run_add_p(the_repository, ADD_P_ADD, NULL, &pathspec); > + ret = !!run_add_p(the_repository, ADD_P_ADD, NULL, &pathspec); > else > - return !!run_add_i(the_repository, &pathspec); > + ret = !!run_add_i(the_repository, &pathspec); > + > + clear_pathspec(&pathspec); > + return ret; > } > > static int edit_patch(int argc, const char **argv, const char *prefix) > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > index bc55255b0a..04d8333373 100755 > --- a/t/t3701-add-interactive.sh > +++ b/t/t3701-add-interactive.sh > @@ -4,6 +4,7 @@ test_description='add -i basic tests' > GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main > export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > . "$TEST_DIRECTORY"/lib-terminal.sh > > diff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh > index b4de10a5dd..03ba0c0e73 100755 > --- a/t/t7514-commit-patch.sh > +++ b/t/t7514-commit-patch.sh > @@ -1,6 +1,8 @@ > #!/bin/sh > > test_description='hunk edit with "commit -p -m"' > + > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > > test_expect_success 'setup (initial)' '
On 4/22/24 5:43 PM, Phillip Wood wrote: >> Plug a leak we have since 5a76aff1a6 (add: convert to use >> parse_pathspec, 2013-07-14). >> >> This leak can be triggered with: >> $ git add -p anything >> >> Fixing this leak allows us to mark as leak-free the following tests: >> >> + t3701-add-interactive.sh >> + t7514-commit-patch.sh >> >> Mark them with "TEST_PASSES_SANITIZE_LEAK=true" to notice and fix >> promply any new leak that may be introduced and triggered by them in the >> future. > > This makes me wonder if we're freeing the pathspec properly when using '--patch' in checkout, reset, restore and stash. I haven't checked it thoroughly, but I think we're fine in those cases.
diff --git a/builtin/add.c b/builtin/add.c index ae723bc85e..b7d3ff1e28 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -150,7 +150,7 @@ static int refresh(int verbose, const struct pathspec *pathspec) int interactive_add(const char **argv, const char *prefix, int patch) { struct pathspec pathspec; - int unused; + int unused, ret; if (!git_config_get_bool("add.interactive.usebuiltin", &unused)) warning(_("the add.interactive.useBuiltin setting has been removed!\n" @@ -163,9 +163,12 @@ int interactive_add(const char **argv, const char *prefix, int patch) prefix, argv); if (patch) - return !!run_add_p(the_repository, ADD_P_ADD, NULL, &pathspec); + ret = !!run_add_p(the_repository, ADD_P_ADD, NULL, &pathspec); else - return !!run_add_i(the_repository, &pathspec); + ret = !!run_add_i(the_repository, &pathspec); + + clear_pathspec(&pathspec); + return ret; } static int edit_patch(int argc, const char **argv, const char *prefix) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index bc55255b0a..04d8333373 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -4,6 +4,7 @@ test_description='add -i basic tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-terminal.sh diff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh index b4de10a5dd..03ba0c0e73 100755 --- a/t/t7514-commit-patch.sh +++ b/t/t7514-commit-patch.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='hunk edit with "commit -p -m"' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup (initial)' '
Plug a leak we have since 5a76aff1a6 (add: convert to use parse_pathspec, 2013-07-14). This leak can be triggered with: $ git add -p anything Fixing this leak allows us to mark as leak-free the following tests: + t3701-add-interactive.sh + t7514-commit-patch.sh Mark them with "TEST_PASSES_SANITIZE_LEAK=true" to notice and fix promply any new leak that may be introduced and triggered by them in the future. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- builtin/add.c | 9 ++++++--- t/t3701-add-interactive.sh | 1 + t/t7514-commit-patch.sh | 2 ++ 3 files changed, 9 insertions(+), 3 deletions(-)