On Fri, Apr 01 2022, Phillip Wood wrote:
> 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.
This is just moving the exsting strvec_clear() shown above to the
"cleanup" branch, as we change return to "goto cleanup".
>> 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.
Hrm, I think you're right about that (but it was an existing leak). Will
fix.
