From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 22 Apr 2024 15:43:59 GMT
Subject: Re: [PATCH 4/4] add: plug a leak on interactive_add
Message-ID: <55f1da17-6f07-4b25-9cf2-0de69bfb5005@gmail.com>
In-Reply-To: <a886c086-4b2e-4d41-ad40-b3fca20de42a@gmail.com>

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