From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 01 Apr 2022 15:29:40 GMT
Subject: Re: [PATCH v4 10/27] stash: always have the owner of "stash_info" free it
Message-ID: <9a487124-ef1e-0644-9585-174343555914@gmail.com>
In-Reply-To: <patch-v4-10.27-145a0f74b6a-20220331T005325Z-avarab@gmail.com>

Hi Ævar

On 31/03/2022 02:11, Ævar Arnfjörð Bjarmason wrote:
> Change the initialization of the "revision" member of "struct
> stash_info" to be initialized vi a macro, and more importantly that
> that initializing function be tasked to free it, usually via a "goto
> cleanup" pattern.
> 
> Despite the "revision" name (and the topic of the series containing
> this commit) the "stash info" has nothing to do with the "struct
> rev_info". I'm making this change because in the subsequent commit
> when we do want to free the "struct rev_info" via a "goto cleanup"
> pattern we'd otherwise free() uninitialized memory in some cases, as
> we only strbuf_init() the string in get_stash_info().
> 
> So while it's the smallest possible change, let's convert all users of
> this pattern in the file while we're at it.
> 
> A good follow-up to this change would be to change all the "ret = -1;
> goto done;" in this file to instead use a "goto cleanup", and
> initialize "int ret = -1" at the start of the relevant functions. That
> would allow us to drop a lot of needless brace verbosity on two-line
> "if" statements, but let's leave that alone for now.

You seem to have made this change here.

>[...]
> @@ -861,10 +863,8 @@ static int show_stash(int argc, const char **argv, const char *prefix)
>   			strvec_push(&revision_args, argv[i]);
>   	}
>   
> -	ret = get_stash_info(&info, stash_args.nr, stash_args.v);
> -	strvec_clear(&stash_args);
> -	if (ret)
> -		return -1;
> +	if (get_stash_info(&info, stash_args.nr, stash_args.v))
> +		goto cleanup;
>   
>   	/*
>   	 * The config settings are applied only if there are not passed
> @@ -878,8 +878,8 @@ static int show_stash(int argc, const char **argv, const char *prefix)
>   			rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
>   
>   		if (!show_stat && !show_patch) {
> -			free_stash_info(&info);
> -			return 0;
> +			ret = 0;
> +			goto cleanup;
>   		}
>   	}
>   
> @@ -912,8 +912,11 @@ static int show_stash(int argc, const char **argv, const char *prefix)
>   	}
>   	log_tree_diff_flush(&rev);
>   
> +	ret = diff_result_code(&rev.diffopt, 0);;
> +cleanup:
> +	strvec_clear(&stash_args);

This seems to be fixing a leak that's not mentioned in the commit message.

>   	free_stash_info(&info);
> -	return diff_result_code(&rev.diffopt, 0);
> +	return ret;
>   }
>[...]
> @@ -1434,7 +1438,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
>   			 int keep_index, int patch_mode, int include_untracked, int only_staged)
>   {
>   	int ret = 0;
> -	struct stash_info info;
> +	struct stash_info info = STASH_INFO_INIT;

There doesn't seem to be a call to free_stash_info() in this function.

Best Wishes

Phillip