[PATCH v2 09/27] stash: always have the owner of "stash_info" free it
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Mar 23, 2022, 20:31 UTC
- Message-ID
- <patch-v2-09.27-88289157533-20220323T203149Z-avarab@gmail.com>
- In-Reply-To
- <cover-v2-00.27-00000000000-20220323T203149Z-avarab@gmail.com>
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 "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.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/stash.c | 70 ++++++++++++++++++++++++++----------------------- 1 file changed, 37 insertions(+), 33 deletions(-)
diff --git a/builtin/stash.c b/builtin/stash.c index 8a8093c5eea..4c8c8af8a09 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -117,6 +117,10 @@ struct stash_info { int has_u; }; +#define STASH_INFO_INIT { \ + .revision = STRBUF_INIT, \ +} + static void free_stash_info(struct stash_info *info) { strbuf_release(&info->revision); @@ -158,10 +162,8 @@ static int get_stash_info(struct stash_info *info, int argc, const char **argv) if (argc == 1) commit = argv[0]; - strbuf_init(&info->revision, 0); if (!commit) { if (!ref_exists(ref_stash)) { - free_stash_info(info); fprintf_ln(stderr, _("No stash entries found.")); return -1; } @@ -175,11 +177,8 @@ static int get_stash_info(struct stash_info *info, int argc, const char **argv) revision = info->revision.buf; - if (get_oid(revision, &info->w_commit)) { - error(_("%s is not a valid reference"), revision); - free_stash_info(info); - return -1; - } + if (get_oid(revision, &info->w_commit)) + return error(_("%s is not a valid reference"), revision); assert_stash_like(info, revision); @@ -198,7 +197,7 @@ static int get_stash_info(struct stash_info *info, int argc, const char **argv) info->is_stash_ref = !strcmp(expanded_ref, ref_stash); break; default: /* Invalid or ambiguous */ - free_stash_info(info); + break; } free(expanded_ref); @@ -599,10 +598,10 @@ static int do_apply_stash(const char *prefix, struct stash_info *info, static int apply_stash(int argc, const char **argv, const char *prefix) { - int ret; + int ret = -1; int quiet = 0; int index = 0; - struct stash_info info; + struct stash_info info = STASH_INFO_INIT; struct option options[] = { OPT__QUIET(&quiet, N_("be quiet, only report errors")), OPT_BOOL(0, "index", &index, @@ -614,9 +613,10 @@ static int apply_stash(int argc, const char **argv, const char *prefix) git_stash_apply_usage, 0); if (get_stash_info(&info, argc, argv)) - return -1; + goto cleanup; ret = do_apply_stash(prefix, &info, index, quiet); +cleanup: free_stash_info(&info); return ret; } @@ -663,9 +663,9 @@ static void assert_stash_ref(struct stash_info *info) static int drop_stash(int argc, const char **argv, const char *prefix) { - int ret; + int ret = -1; int quiet = 0; - struct stash_info info; + struct stash_info info = STASH_INFO_INIT; struct option options[] = { OPT__QUIET(&quiet, N_("be quiet, only report errors")), OPT_END() @@ -675,21 +675,22 @@ static int drop_stash(int argc, const char **argv, const char *prefix) git_stash_drop_usage, 0); if (get_stash_info(&info, argc, argv)) - return -1; + goto cleanup; assert_stash_ref(&info); ret = do_drop_stash(&info, quiet); +cleanup: free_stash_info(&info); return ret; } static int pop_stash(int argc, const char **argv, const char *prefix) { - int ret; + int ret = -1; int index = 0; int quiet = 0; - struct stash_info info; + struct stash_info info = STASH_INFO_INIT; struct option options[] = { OPT__QUIET(&quiet, N_("be quiet, only report errors")), OPT_BOOL(0, "index", &index, @@ -701,7 +702,7 @@ static int pop_stash(int argc, const char **argv, const char *prefix) git_stash_pop_usage, 0); if (get_stash_info(&info, argc, argv)) - return -1; + goto cleanup; assert_stash_ref(&info); if ((ret = do_apply_stash(prefix, &info, index, quiet))) @@ -710,15 +711,16 @@ static int pop_stash(int argc, const char **argv, const char *prefix) else ret = do_drop_stash(&info, quiet); +cleanup: free_stash_info(&info); return ret; } static int branch_stash(int argc, const char **argv, const char *prefix) { - int ret; + int ret = -1; const char *branch = NULL; - struct stash_info info; + struct stash_info info = STASH_INFO_INIT; struct child_process cp = CHILD_PROCESS_INIT; struct option options[] = { OPT_END() @@ -735,7 +737,7 @@ static int branch_stash(int argc, const char **argv, const char *prefix) branch = argv[0]; if (get_stash_info(&info, argc - 1, argv + 1)) - return -1; + goto cleanup; cp.git_cmd = 1; strvec_pushl(&cp.args, "checkout", "-b", NULL); @@ -747,8 +749,8 @@ static int branch_stash(int argc, const char **argv, const char *prefix) if (!ret && info.is_stash_ref) ret = do_drop_stash(&info, 0); +cleanup: free_stash_info(&info); - return ret; } @@ -826,8 +828,8 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op static int show_stash(int argc, const char **argv, const char *prefix) { int i; - int ret = 0; - struct stash_info info; + int ret = -1; + struct stash_info info = STASH_INFO_INIT; struct rev_info rev; struct strvec stash_args = STRVEC_INIT; struct strvec revision_args = STRVEC_INIT; @@ -862,10 +864,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 @@ -879,8 +879,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; } } @@ -913,8 +913,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); free_stash_info(&info); - return diff_result_code(&rev.diffopt, 0); + return ret; } static int do_store_stash(const struct object_id *w_commit, const char *stash_msg, @@ -1410,9 +1413,9 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b static int create_stash(int argc, const char **argv, const char *prefix) { - int ret = 0; + int ret; struct strbuf stash_msg_buf = STRBUF_INIT; - struct stash_info info; + struct stash_info info = STASH_INFO_INIT; struct pathspec ps; /* Starting with argv[1], since argv[0] is "create" */ @@ -1427,6 +1430,7 @@ static int create_stash(int argc, const char **argv, const char *prefix) if (!ret) printf_ln("%s", oid_to_hex(&info.w_commit)); + free_stash_info(&info); strbuf_release(&stash_msg_buf); return ret; } @@ -1435,7 +1439,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; struct strbuf patch = STRBUF_INIT; struct strbuf stash_msg_buf = STRBUF_INIT; struct strbuf untracked_files = STRBUF_INIT;
-- 2.35.1.1452.ga7cfc89151f