Re: [PATCH v4 10/27] stash: always have the owner of "stash_info" free it
Hi Ævar
On 31/03/2022 02:11, Ævar Arnfjörð Bjarmason wrote:
Show 20 quoted lines
> 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.
Show 32 quoted lines
>[...]
> @@ -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.
Show 11 quoted lines
> 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