From b925fcf12951dba85313afd08c6154d8d978b03e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:30 +0200 Subject: [PATCH 01/27] t/helper/test-fast-rebase.c: don't leak "struct strbuf" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix a memory leak that's been with us since f9500261e0a (fast-rebase: write conflict state to working tree, index, and HEAD, 2021-05-20) changed this code to move these strbuf_release() into an if/else block. We'll also add to "reflog_msg" in the "else" arm of the "if" block being modified here, and we'll append to "branch_msg" in both cases. But after f9500261e0a only the "if" block would free these two "struct strbuf". Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- t/helper/test-fast-rebase.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/t/helper/test-fast-rebase.c b/t/helper/test-fast-rebase.c index fc2d460904..993b90eaed 100644 --- a/t/helper/test-fast-rebase.c +++ b/t/helper/test-fast-rebase.c @@ -201,8 +201,6 @@ int cmd__fast_rebase(int argc, const char **argv) } if (create_symref("HEAD", branch_name.buf, reflog_msg.buf) < 0) die(_("unable to update HEAD")); - strbuf_release(&reflog_msg); - strbuf_release(&branch_name); prime_cache_tree(the_repository, the_repository->index, result.tree); @@ -221,5 +219,8 @@ int cmd__fast_rebase(int argc, const char **argv) if (write_locked_index(&the_index, &lock, COMMIT_LOCK | SKIP_IF_UNCHANGED)) die(_("unable to write %s"), get_index_file()); + + strbuf_release(&reflog_msg); + strbuf_release(&branch_name); return (result.clean == 0); } From 4b59b2db97af0b40fc2688069c3a91ba28f8883d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:31 +0200 Subject: [PATCH 02/27] blame: use "goto cleanup" for cleanup_scoreboard() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Amend a freeing pattern added in 0906ac2b54b (blame: use changed-path Bloom filters, 2020-04-16) to use a "goto cleanup", so that we can be sure that we call cleanup_scoreboard(). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/blame.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/builtin/blame.c b/builtin/blame.c index 8d15b68afc..885b381ab8 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -1167,7 +1167,7 @@ parse_done: if (!incremental) setup_pager(); else - return 0; + goto cleanup; blame_sort_final(&sb); @@ -1201,6 +1201,7 @@ parse_done: printf("num commits: %d\n", sb.num_commits); } +cleanup: cleanup_scoreboard(&sb); return 0; } From f2605051427af9fceaacb4a9d0effc6e2b0af227 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:32 +0200 Subject: [PATCH 03/27] string_list API users: use string_list_init_{no,}dup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up on the introduction of string_list_init_nodup() and string_list_init_dup() in the series merged in bd4232fac33 (Merge branch 'ab/struct-init', 2021-07-16) and convert code that implicitly relied on xcalloc() being equivalent to the initializer to use xmalloc() and string_list_init_{no,}dup() instead. In the case of get_unmerged() in merge-recursive.c we used the combination of xcalloc() and assigning "1" to "strdup_strings" to get what we'd get via string_list_init_dup(), let's use that instead. Adjacent code in cmd_format_patch() will be changed in a subsequent commit, since we're changing that let's change the other in-tree patterns that do the same. Let's also convert a "x == NULL" to "!x" per our CodingGuidelines, as we need to change the "if" line anyway. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/log.c | 9 ++++++--- builtin/shortlog.c | 6 ++++-- merge-recursive.c | 4 ++-- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index c211d66d1d..634dc782cc 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -231,7 +231,8 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, } if (mailmap) { - rev->mailmap = xcalloc(1, sizeof(struct string_list)); + rev->mailmap = xmalloc(sizeof(struct string_list)); + string_list_init_nodup(rev->mailmap); read_mailmap(rev->mailmap); } @@ -2173,8 +2174,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) prepare_bases(&bases, base, list, nr); } - if (in_reply_to || thread || cover_letter) - rev.ref_message_ids = xcalloc(1, sizeof(struct string_list)); + if (in_reply_to || thread || cover_letter) { + rev.ref_message_ids = xmalloc(sizeof(*rev.ref_message_ids)); + string_list_init_nodup(rev.ref_message_ids); + } if (in_reply_to) { const char *msgid = clean_message_id(in_reply_to); string_list_append(rev.ref_message_ids, msgid); diff --git a/builtin/shortlog.c b/builtin/shortlog.c index 26c5c0cf93..fcde07c936 100644 --- a/builtin/shortlog.c +++ b/builtin/shortlog.c @@ -81,8 +81,10 @@ static void insert_one_record(struct shortlog *log, format_subject(&subject, oneline, " "); buffer = strbuf_detach(&subject, NULL); - if (item->util == NULL) - item->util = xcalloc(1, sizeof(struct string_list)); + if (!item->util) { + item->util = xmalloc(sizeof(struct string_list)); + string_list_init_nodup(item->util); + } string_list_append(item->util, buffer); } } diff --git a/merge-recursive.c b/merge-recursive.c index 1ee6364e8b..32bbba5fbb 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -522,10 +522,10 @@ static struct stage_data *insert_stage_data(struct repository *r, */ static struct string_list *get_unmerged(struct index_state *istate) { - struct string_list *unmerged = xcalloc(1, sizeof(struct string_list)); + struct string_list *unmerged = xmalloc(sizeof(struct string_list)); int i; - unmerged->strdup_strings = 1; + string_list_init_dup(unmerged); /* TODO: audit for interaction with sparse-index. */ ensure_full_index(istate); From 89f45cf4eb031f28b5dd028983734c4ddfa99439 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:33 +0200 Subject: [PATCH 04/27] format-patch: don't leak "extra_headers" or "ref_message_ids" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix two memory leaks in "struct rev_info" by freeing that memory in cmd_format_patch(). These two are unusual special-cases in being in the "struct rev_info", but not being "owned" by the code in revision.c. I.e. they're members of the struct so that this code in "builtin/log.c" can conveniently pass information code in "log-tree.c". See e.g. the make_cover_letter() caller of log_write_email_headers() here in "builtin/log.c", and [1] for a demonstration of where the "extra_headers" and "ref_message_ids" struct members are used. See 20ff06805c6 (format-patch: resurrect extra headers from config, 2006-06-02) and d1566f7883f (git-format-patch: Make the second and subsequent mails replies to the first, 2006-07-14) for the initial introduction of "extra_headers" and "ref_message_ids". We can count on repo_init_revisions() memset()-ing this data to 0 however, so we can count on it being either NULL or something we allocated. In the case of "extra_headers" let's add a local "char *" variable to hold it, to avoid the eventual cast from "const char *" when we free() it. 1. https://lore.kernel.org/git/220401.868rsoogxf.gmgdl@evledraar.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/log.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/builtin/log.c b/builtin/log.c index 634dc782cc..6f9928fabf 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1747,6 +1747,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) struct commit *commit; struct commit **list = NULL; struct rev_info rev; + char *to_free = NULL; struct setup_revision_opt s_r_opt; int nr = 0, total, i; int use_stdout = 0; @@ -1947,7 +1948,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) strbuf_addch(&buf, '\n'); } - rev.extra_headers = strbuf_detach(&buf, NULL); + rev.extra_headers = to_free = strbuf_detach(&buf, NULL); if (from) { if (split_ident_line(&rev.from_ident, from, strlen(from))) @@ -2284,6 +2285,10 @@ done: strbuf_release(&rdiff1); strbuf_release(&rdiff2); strbuf_release(&rdiff_title); + free(to_free); + if (rev.ref_message_ids) + string_list_clear(rev.ref_message_ids, 0); + free(rev.ref_message_ids); UNLEAK(rev); return 0; } From bf20fe4ca8c8d2ba2087c9c44b4ee09a1d1467ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:34 +0200 Subject: [PATCH 05/27] cocci: add and apply free_commit_list() rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add and apply coccinelle rules to remove "if (E)" before "free_commit_list(E)", the function can accept NULL, and further change cases where "E = NULL" followed to also be unconditionally. The code changes in this commit were entirely made by the coccinelle rule being added here, and applied with: make contrib/coccinelle/free.cocci.patch patch -p1 Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/rev-list.c | 6 ++---- commit.c | 19 ++++++++----------- contrib/coccinelle/free.cocci | 27 +++++++++++++++++++++++++++ revision.c | 17 ++++++----------- submodule.c | 6 ++---- 5 files changed, 45 insertions(+), 30 deletions(-) diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 572da1472e..07c0ad11d8 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -213,10 +213,8 @@ static void show_commit(struct commit *commit, void *data) static void finish_commit(struct commit *commit) { - if (commit->parents) { - free_commit_list(commit->parents); - commit->parents = NULL; - } + free_commit_list(commit->parents); + commit->parents = NULL; free_commit_buffer(the_repository->parsed_objects, commit); } diff --git a/commit.c b/commit.c index 98b2e55665..0fee9b1122 100644 --- a/commit.c +++ b/commit.c @@ -397,17 +397,14 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b if (item->object.parsed) return 0; - - if (item->parents) { - /* - * Presumably this is leftover from an earlier failed parse; - * clear it out in preparation for us re-parsing (we'll hit the - * same error, but that's good, since it lets our caller know - * the result cannot be trusted. - */ - free_commit_list(item->parents); - item->parents = NULL; - } + /* + * Presumably this is leftover from an earlier failed parse; + * clear it out in preparation for us re-parsing (we'll hit the + * same error, but that's good, since it lets our caller know + * the result cannot be trusted. + */ + free_commit_list(item->parents); + item->parents = NULL; tail += size; if (tail <= bufptr + tree_entry_len + 1 || memcmp(bufptr, "tree ", 5) || diff --git a/contrib/coccinelle/free.cocci b/contrib/coccinelle/free.cocci index 4490069df9..6fb9eb6e88 100644 --- a/contrib/coccinelle/free.cocci +++ b/contrib/coccinelle/free.cocci @@ -2,13 +2,21 @@ expression E; @@ - if (E) +( free(E); +| + free_commit_list(E); +) @@ expression E; @@ - if (!E) +( free(E); +| + free_commit_list(E); +) @@ expression E; @@ -16,3 +24,22 @@ expression E; - free(E); + FREE_AND_NULL(E); - E = NULL; + +@@ +expression E; +@@ +- if (E) +- { + free_commit_list(E); + E = NULL; +- } + +@@ +expression E; +statement S; +@@ +- if (E) { ++ if (E) + S + free_commit_list(E); +- } diff --git a/revision.c b/revision.c index 7d435f8048..4963ba7def 100644 --- a/revision.c +++ b/revision.c @@ -1456,10 +1456,9 @@ static int limit_list(struct rev_info *revs) if (revs->left_only || revs->right_only) limit_left_right(newlist, revs); - if (bottom) { + if (bottom) limit_to_ancestry(bottom, newlist); - free_commit_list(bottom); - } + free_commit_list(bottom); /* * Check if any commits have become TREESAME by some of their parents @@ -4080,10 +4079,8 @@ static void create_boundary_commit_list(struct rev_info *revs) * boundary commits anyway. (This is what the code has always * done.) */ - if (revs->commits) { - free_commit_list(revs->commits); - revs->commits = NULL; - } + free_commit_list(revs->commits); + revs->commits = NULL; /* * Put all of the actual boundary commits from revs->boundary_commits @@ -4220,10 +4217,8 @@ struct commit *get_revision(struct rev_info *revs) graph_update(revs->graph, c); if (!c) { free_saved_parents(revs); - if (revs->previous_parents) { - free_commit_list(revs->previous_parents); - revs->previous_parents = NULL; - } + free_commit_list(revs->previous_parents); + revs->previous_parents = NULL; } return c; } diff --git a/submodule.c b/submodule.c index 5ace18a7d9..7797e5a4db 100644 --- a/submodule.c +++ b/submodule.c @@ -664,8 +664,7 @@ void show_submodule_diff_summary(struct diff_options *o, const char *path, print_submodule_diff_summary(sub, &rev, o); out: - if (merge_bases) - free_commit_list(merge_bases); + free_commit_list(merge_bases); clear_commit_marks(left, ~0); clear_commit_marks(right, ~0); if (sub) { @@ -754,8 +753,7 @@ void show_submodule_inline_diff(struct diff_options *o, const char *path, done: strbuf_release(&sb); - if (merge_bases) - free_commit_list(merge_bases); + free_commit_list(merge_bases); if (left) clear_commit_marks(left, ~0); if (right) From 1878b5edc03c4bf685c7278fc4ced4704c876984 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:35 +0200 Subject: [PATCH 06/27] revision.[ch]: provide and start using a release_revisions() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The users of the revision.[ch] API's "struct rev_info" are a major source of memory leaks in the test suite under SANITIZE=leak, which in turn adds a lot of noise when trying to mark up tests with "TEST_PASSES_SANITIZE_LEAK=true". The users of that API are largely one-shot, e.g. "git rev-list" or "git log", or the "git checkout" and "git stash" being modified here For these callers freeing the memory is arguably a waste of time, but in many cases they've actually been trying to free the memory, and just doing that in a buggy manner. Let's provide a release_revisions() function for these users, and start migrating them over per the plan outlined in [1]. Right now this only handles the "pending" member of the struct, but more will be added in subsequent commits. Even though we only clear the "pending" member now, let's not leave a trap in code like the pre-image of index_differs_from(), where we'd start doing the wrong thing as soon as the release_revisions() learned to clear its "diffopt". I.e. we need to call release_revisions() after we've inspected any state in "struct rev_info". This leaves in place e.g. clear_pathspec(&rev.prune_data) in stash_working_tree() in builtin/stash.c, subsequent commits will teach release_revisions() to free "prune_data" and other members that in some cases are individually cleared by users of "struct rev_info" by reaching into its members. Those subsequent commits will remove the relevant calls to e.g. clear_pathspec(). We avoid amending code in index_differs_from() in diff-lib.c as well as wt_status_collect_changes_index(), has_unstaged_changes() and has_uncommitted_changes() in wt-status.c in a way that assumes that we are already clearing the "diffopt" member. That will be handled in a subsequent commit. 1. https://lore.kernel.org/git/87a6k8daeu.fsf@evledraar.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/checkout.c | 2 +- builtin/stash.c | 5 ++--- diff-lib.c | 2 +- range-diff.c | 2 +- revision.c | 5 +++++ revision.h | 6 ++++++ wt-status.c | 5 +++-- 7 files changed, 19 insertions(+), 8 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index 797681481d..4d9e0bd3ac 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -629,7 +629,7 @@ static void show_local_changes(struct object *head, diff_setup_done(&rev.diffopt); add_pending_object(&rev, head, NULL); run_diff_index(&rev, 0); - object_array_clear(&rev.pending); + release_revisions(&rev); } static void describe_detached_head(const char *msg, struct commit *commit) diff --git a/builtin/stash.c b/builtin/stash.c index ccdfdab44b..8c90db479c 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -1047,7 +1047,6 @@ static int check_changes_tracked_files(const struct pathspec *ps) goto done; } - object_array_clear(&rev.pending); result = run_diff_files(&rev, 0); if (diff_result_code(&rev.diffopt, result)) { ret = 1; @@ -1056,6 +1055,7 @@ static int check_changes_tracked_files(const struct pathspec *ps) done: clear_pathspec(&rev.prune_data); + release_revisions(&rev); return ret; } @@ -1266,9 +1266,8 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps done: discard_index(&istate); - UNLEAK(rev); - object_array_clear(&rev.pending); clear_pathspec(&rev.prune_data); + release_revisions(&rev); strbuf_release(&diff_output); remove_path(stash_index_path.buf); return ret; diff --git a/diff-lib.c b/diff-lib.c index ca085a03ef..d6800274bd 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -662,7 +662,7 @@ int index_differs_from(struct repository *r, diff_flags_or(&rev.diffopt.flags, flags); rev.diffopt.ita_invisible_in_index = ita_invisible_in_index; run_diff_index(&rev, 1); - object_array_clear(&rev.pending); + release_revisions(&rev); return (rev.diffopt.flags.has_changes != 0); } diff --git a/range-diff.c b/range-diff.c index b72eb9fdbe..39cc010c62 100644 --- a/range-diff.c +++ b/range-diff.c @@ -596,6 +596,6 @@ int is_range_diff_range(const char *arg) } free(copy); - object_array_clear(&revs.pending); + release_revisions(&revs); return negative > 0 && positive > 0; } diff --git a/revision.c b/revision.c index 4963ba7def..5dd4b2e910 100644 --- a/revision.c +++ b/revision.c @@ -2922,6 +2922,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s return left; } +void release_revisions(struct rev_info *revs) +{ + object_array_clear(&revs->pending); +} + static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child) { struct commit_list *l = xcalloc(1, sizeof(*l)); diff --git a/revision.h b/revision.h index 5bc59c7bfe..61c780fc4c 100644 --- a/revision.h +++ b/revision.h @@ -377,6 +377,12 @@ void repo_init_revisions(struct repository *r, int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct setup_revision_opt *); +/** + * Free data allocated in a "struct rev_info" after it's been + * initialized with repo_init_revisions(). + */ +void release_revisions(struct rev_info *revs); + void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx, const struct option *options, const char * const usagestr[]); diff --git a/wt-status.c b/wt-status.c index d33f9272b7..922cf787f9 100644 --- a/wt-status.c +++ b/wt-status.c @@ -662,7 +662,7 @@ static void wt_status_collect_changes_index(struct wt_status *s) copy_pathspec(&rev.prune_data, &s->pathspec); run_diff_index(&rev, 1); - object_array_clear(&rev.pending); + release_revisions(&rev); clear_pathspec(&rev.prune_data); } @@ -2545,6 +2545,7 @@ int has_unstaged_changes(struct repository *r, int ignore_submodules) rev_info.diffopt.flags.quick = 1; diff_setup_done(&rev_info.diffopt); result = run_diff_files(&rev_info, 0); + release_revisions(&rev_info); return diff_result_code(&rev_info.diffopt, result); } @@ -2577,7 +2578,7 @@ int has_uncommitted_changes(struct repository *r, diff_setup_done(&rev_info.diffopt); result = run_diff_index(&rev_info, 1); - object_array_clear(&rev_info.pending); + release_revisions(&rev_info); return diff_result_code(&rev_info.diffopt, result); } From 2108fe4a1976f95821e13503fd331f075da398c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:36 +0200 Subject: [PATCH 07/27] revisions API users: add straightforward release_revisions() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a release_revisions() to various users of "struct rev_list" in those straightforward cases where we only need to add the release_revisions() call to the end of a block, and don't need to e.g. refactor anything to use a "goto cleanup" pattern. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- add-interactive.c | 1 + bisect.c | 2 ++ builtin/add.c | 1 + builtin/am.c | 3 +++ builtin/bisect--helper.c | 2 ++ builtin/blame.c | 1 + builtin/checkout.c | 1 + builtin/commit.c | 1 + builtin/describe.c | 2 ++ builtin/fast-export.c | 1 + builtin/merge.c | 2 ++ builtin/pack-objects.c | 2 ++ builtin/prune.c | 1 + builtin/reflog.c | 1 + builtin/shortlog.c | 2 ++ builtin/submodule--helper.c | 1 + fmt-merge-msg.c | 1 + merge-ort.c | 1 + merge-recursive.c | 1 + midx.c | 1 + pack-bitmap-write.c | 1 + ref-filter.c | 1 + remote.c | 1 + sequencer.c | 3 +++ shallow.c | 1 + submodule.c | 2 ++ t/helper/test-revision-walking.c | 1 + wt-status.c | 1 + 28 files changed, 39 insertions(+) diff --git a/add-interactive.c b/add-interactive.c index 7247210301..54cdfc8201 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -570,6 +570,7 @@ static int get_modified_files(struct repository *r, if (ps) clear_pathspec(&rev.prune_data); + release_revisions(&rev); } hashmap_clear_and_free(&s.file_map, struct pathname_entry, ent); if (unmerged_count) diff --git a/bisect.c b/bisect.c index 9e6a2b7f20..cc6b8b6230 100644 --- a/bisect.c +++ b/bisect.c @@ -884,6 +884,7 @@ static int check_ancestors(struct repository *r, int rev_nr, /* Clean up objects used, as they will be reused. */ clear_commit_marks_many(rev_nr, rev, ALL_REV_FLAGS); + release_revisions(&revs); return res; } @@ -964,6 +965,7 @@ static void show_diff_tree(struct repository *r, setup_revisions(ARRAY_SIZE(argv) - 1, argv, &opt, NULL); log_tree_commit(&opt, commit); + release_revisions(&opt); } /* diff --git a/builtin/add.c b/builtin/add.c index 3ffb86a433..f507d2191c 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -340,6 +340,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix) unlink(file); free(file); + release_revisions(&rev); return 0; } diff --git a/builtin/am.c b/builtin/am.c index 0f4111bafa..93bec62afa 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1397,6 +1397,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm add_pending_object(&rev_info, &commit->object, ""); diff_setup_done(&rev_info.diffopt); log_tree_commit(&rev_info, commit); + release_revisions(&rev_info); } /** @@ -1429,6 +1430,7 @@ static void write_index_patch(const struct am_state *state) add_pending_object(&rev_info, &tree->object, ""); diff_setup_done(&rev_info.diffopt); run_diff_index(&rev_info, 1); + release_revisions(&rev_info); } /** @@ -1582,6 +1584,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa add_pending_oid(&rev_info, "HEAD", &our_tree, 0); diff_setup_done(&rev_info.diffopt); run_diff_index(&rev_info, 1); + release_revisions(&rev_info); } if (run_apply(state, index_path)) diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 8b2b259ff0..e4d7b6779a 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -555,6 +555,7 @@ static int bisect_skipped_commits(struct bisect_terms *terms) reset_revision_walk(); strbuf_release(&commit_name); + release_revisions(&revs); fclose(fp); return 0; } @@ -1041,6 +1042,7 @@ static enum bisect_error bisect_skip(struct bisect_terms *terms, const char **ar oid_to_hex(&commit->object.oid)); reset_revision_walk(); + release_revisions(&revs); } else { strvec_push(&argv_state, argv[i]); } diff --git a/builtin/blame.c b/builtin/blame.c index 885b381ab8..24bac822c5 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -1203,5 +1203,6 @@ parse_done: cleanup: cleanup_scoreboard(&sb); + release_revisions(&revs); return 0; } diff --git a/builtin/checkout.c b/builtin/checkout.c index 4d9e0bd3ac..7ad4a7113c 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1082,6 +1082,7 @@ static void orphaned_commit_warning(struct commit *old_commit, struct commit *ne /* Clean up objects used, as they will be reused. */ repo_clear_commit_marks(the_repository, ALL_REV_FLAGS); + release_revisions(&revs); } static int switch_branches(const struct checkout_opts *opts, diff --git a/builtin/commit.c b/builtin/commit.c index 009a1de0a3..c7eda9bbb7 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1123,6 +1123,7 @@ static const char *find_author_by_nickname(const char *name) strbuf_release(&buf); format_commit_message(commit, "%aN <%aE>", &buf, &ctx); clear_mailmap(&mailmap); + release_revisions(&revs); return strbuf_detach(&buf, NULL); } die(_("--author '%s' is not 'Name ' and matches no existing author"), name); diff --git a/builtin/describe.c b/builtin/describe.c index 42159cd26b..a76f1a1a7a 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -517,6 +517,7 @@ static void describe_blob(struct object_id oid, struct strbuf *dst) traverse_commit_list(&revs, process_commit, process_object, &pcd); reset_revision_walk(); + release_revisions(&revs); } static void describe(const char *arg, int last_one) @@ -667,6 +668,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix) suffix = NULL; else suffix = dirty; + release_revisions(&revs); } describe("HEAD", 1); } else if (dirty) { diff --git a/builtin/fast-export.c b/builtin/fast-export.c index a7d72697fb..f34ae451ee 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -1275,6 +1275,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix) printf("done\n"); refspec_clear(&refspecs); + release_revisions(&revs); return 0; } diff --git a/builtin/merge.c b/builtin/merge.c index f178f5a3ee..d9784d4891 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -443,6 +443,7 @@ static void squash_message(struct commit *commit, struct commit_list *remotehead } write_file_buf(git_path_squash_msg(the_repository), out.buf, out.len); strbuf_release(&out); + release_revisions(&rev); } static void finish(struct commit *head_commit, @@ -998,6 +999,7 @@ static int evaluate_result(void) */ cnt += count_unmerged_entries(); + release_revisions(&rev); return cnt; } diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 09680fb6a8..5c3f317649 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -4167,11 +4167,13 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) read_object_list_from_stdin(); } else if (pfd.have_revs) { get_object_list(&pfd.revs, rp.nr, rp.v); + release_revisions(&pfd.revs); } else { struct rev_info revs; repo_init_revisions(the_repository, &revs, NULL); get_object_list(&revs, rp.nr, rp.v); + release_revisions(&revs); } cleanup_preferred_base(); if (include_tag && nr_result) diff --git a/builtin/prune.c b/builtin/prune.c index c2bcdc07db..df376b2ed1 100644 --- a/builtin/prune.c +++ b/builtin/prune.c @@ -196,5 +196,6 @@ int cmd_prune(int argc, const char **argv, const char *prefix) prune_shallow(show_only ? PRUNE_SHOW_ONLY : 0); } + release_revisions(&revs); return 0; } diff --git a/builtin/reflog.c b/builtin/reflog.c index 9407f835cb..592d5d3344 100644 --- a/builtin/reflog.c +++ b/builtin/reflog.c @@ -248,6 +248,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix) if (verbose) printf(_("Marking reachable objects...")); mark_reachable_objects(&revs, 0, 0, NULL); + release_revisions(&revs); if (verbose) putchar('\n'); } diff --git a/builtin/shortlog.c b/builtin/shortlog.c index fcde07c936..35825f075e 100644 --- a/builtin/shortlog.c +++ b/builtin/shortlog.c @@ -422,6 +422,8 @@ parse_done: else get_from_rev(&rev, &log); + release_revisions(&rev); + shortlog_output(&log); if (log.file != stdout) fclose(log.file); diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 5301612d24..24980863f6 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1269,6 +1269,7 @@ static int compute_summary_module_list(struct object_id *head_oid, run_diff_files(&rev, 0); prepare_submodule_summary(info, &list); strvec_clear(&diff_args); + release_revisions(&rev); return 0; } diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c index baca57d5b6..f48f44f9cd 100644 --- a/fmt-merge-msg.c +++ b/fmt-merge-msg.c @@ -699,6 +699,7 @@ int fmt_merge_msg(struct strbuf *in, struct strbuf *out, shortlog(origins.items[i].string, origins.items[i].util, head, &rev, opts, out); + release_revisions(&rev); } strbuf_complete_line(out); diff --git a/merge-ort.c b/merge-ort.c index 8545354daf..4c0d4eba19 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -1594,6 +1594,7 @@ static int find_first_merges(struct repository *repo, } object_array_clear(&merges); + release_revisions(&revs); return result->nr; } diff --git a/merge-recursive.c b/merge-recursive.c index 32bbba5fbb..acd13b2b06 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1160,6 +1160,7 @@ static int find_first_merges(struct repository *repo, } object_array_clear(&merges); + release_revisions(&revs); return result->nr; } diff --git a/midx.c b/midx.c index 865170bad0..702c8a9b17 100644 --- a/midx.c +++ b/midx.c @@ -1061,6 +1061,7 @@ static struct commit **find_commits_for_midx_bitmap(uint32_t *indexed_commits_nr if (indexed_commits_nr_p) *indexed_commits_nr_p = cb.commits_nr; + release_revisions(&revs); return cb.commits; } diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c index cab3eaa2ac..ea8e0b51cd 100644 --- a/pack-bitmap-write.c +++ b/pack-bitmap-write.c @@ -326,6 +326,7 @@ next: trace2_data_intmax("pack-bitmap-write", the_repository, "num_maximal_commits", num_maximal); + release_revisions(&revs); free_commit_list(reusable); } diff --git a/ref-filter.c b/ref-filter.c index 7838bd22b8..a91688bbf1 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -2392,6 +2392,7 @@ static void reach_filter(struct ref_array *array, clear_commit_marks(merge_commit, ALL_REV_FLAGS); } + release_revisions(&revs); free(to_clear); } diff --git a/remote.c b/remote.c index 42a4e7106e..fa3152a5d5 100644 --- a/remote.c +++ b/remote.c @@ -2172,6 +2172,7 @@ static int stat_branch_pair(const char *branch_name, const char *base, clear_commit_marks(theirs, ALL_REV_FLAGS); strvec_clear(&argv); + release_revisions(&revs); return 1; } diff --git a/sequencer.c b/sequencer.c index a1bb39383d..f9d7acd106 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1347,6 +1347,7 @@ void print_commit_summary(struct repository *r, log_tree_commit(&rev, commit); } + release_revisions(&rev); strbuf_release(&format); } @@ -3415,6 +3416,7 @@ static int make_patch(struct repository *r, unuse_commit_buffer(commit, commit_buffer); } strbuf_release(&buf); + release_revisions(&log_tree_opt); return res; } @@ -4525,6 +4527,7 @@ cleanup_head_ref: &log_tree_opt.diffopt); log_tree_diff_flush(&log_tree_opt); } + release_revisions(&log_tree_opt); } flush_rewritten_pending(); if (!stat(rebase_path_rewritten_list(), &st) && diff --git a/shallow.c b/shallow.c index 71e5876f37..2552f139f6 100644 --- a/shallow.c +++ b/shallow.c @@ -261,6 +261,7 @@ struct commit_list *get_shallow_commits_by_rev_list(int ac, const char **av, if ((o->flags & both_flags) == both_flags) o->flags &= ~not_shallow_flag; } + release_revisions(&revs); return result; } diff --git a/submodule.c b/submodule.c index 7797e5a4db..86ebd3f35c 100644 --- a/submodule.c +++ b/submodule.c @@ -900,9 +900,11 @@ static void collect_changed_submodules(struct repository *r, diff_rev.diffopt.format_callback_data = &data; diff_rev.dense_combined_merges = 1; diff_tree_combined_merge(commit, &diff_rev); + release_revisions(&diff_rev); } reset_revision_walk(); + release_revisions(&rev); } static void free_submodules_oids(struct string_list *submodules) diff --git a/t/helper/test-revision-walking.c b/t/helper/test-revision-walking.c index 625b2dbf82..4a45d5bac2 100644 --- a/t/helper/test-revision-walking.c +++ b/t/helper/test-revision-walking.c @@ -43,6 +43,7 @@ static int run_revision_walk(void) } reset_revision_walk(); + release_revisions(&rev); return got_revision; } diff --git a/wt-status.c b/wt-status.c index 922cf787f9..f910062137 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1152,6 +1152,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s) rev.diffopt.b_prefix = "w/"; run_diff_files(&rev, 0); } + release_revisions(&rev); } static void wt_longstatus_print_tracking(struct wt_status *s) From 296a1438455d366475570ac49afb466f591417dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:37 +0200 Subject: [PATCH 08/27] revision.[ch]: document and move code declared around "init" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A subsequent commit will add "REV_INFO_INIT" macro adjacent to repo_init_revisions(), unfortunately between the "struct rev_info" itself and that function we've added various miscellaneous code between the two over the years. Let's move that code either lower in revision.h, giving it API docs while we're at it, or in cases where it wasn't public API at all move it into revision.c No lines of code are changed here, only moved around. The only changes are the addition of new API comments. The "tree_difference" variable could also be declared like this, which I think would be a lot clearer, but let's leave that for now to keep this a move-only change: static enum { REV_TREE_SAME, REV_TREE_NEW, /* Only new files */ REV_TREE_OLD, /* Only files removed */ REV_TREE_DIFFERENT, /* Mixed changes */ } tree_difference = REV_TREE_SAME; Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- revision.c | 4 ++++ revision.h | 50 ++++++++++++++++++++++++-------------------------- 2 files changed, 28 insertions(+), 26 deletions(-) diff --git a/revision.c b/revision.c index 5dd4b2e910..472fff1e0a 100644 --- a/revision.c +++ b/revision.c @@ -606,6 +606,10 @@ static struct commit *one_relevant_parent(const struct rev_info *revs, * * 2. We saw anything except REV_TREE_NEW. */ +#define REV_TREE_SAME 0 +#define REV_TREE_NEW 1 /* Only new files */ +#define REV_TREE_OLD 2 /* Only files removed */ +#define REV_TREE_DIFFERENT 3 /* Mixed changes */ static int tree_difference = REV_TREE_SAME; static void file_add_remove(struct diff_options *options, diff --git a/revision.h b/revision.h index 61c780fc4c..b9070e4342 100644 --- a/revision.h +++ b/revision.h @@ -329,32 +329,6 @@ struct rev_info { struct tmp_objdir *remerge_objdir; }; -int ref_excluded(struct string_list *, const char *path); -void clear_ref_exclusion(struct string_list **); -void add_ref_exclusion(struct string_list **, const char *exclude); - - -#define REV_TREE_SAME 0 -#define REV_TREE_NEW 1 /* Only new files */ -#define REV_TREE_OLD 2 /* Only files removed */ -#define REV_TREE_DIFFERENT 3 /* Mixed changes */ - -/* revision.c */ -typedef void (*show_early_output_fn_t)(struct rev_info *, struct commit_list *); -extern volatile show_early_output_fn_t show_early_output; - -struct setup_revision_opt { - const char *def; - void (*tweak)(struct rev_info *, struct setup_revision_opt *); - unsigned int assume_dashdash:1, - allow_exclude_promisor_objects:1; - unsigned revarg_opt; -}; - -#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS -#define init_revisions(revs, prefix) repo_init_revisions(the_repository, revs, prefix) -#endif - /** * Initialize a rev_info structure with default values. The third parameter may * be NULL or can be prefix path, and then the `.prefix` variable will be set @@ -366,6 +340,9 @@ struct setup_revision_opt { void repo_init_revisions(struct repository *r, struct rev_info *revs, const char *prefix); +#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS +#define init_revisions(revs, prefix) repo_init_revisions(the_repository, revs, prefix) +#endif /** * Parse revision information, filling in the `rev_info` structure, and @@ -374,6 +351,13 @@ void repo_init_revisions(struct repository *r, * head of the argument list. The last parameter is used in case no * parameter given by the first two arguments. */ +struct setup_revision_opt { + const char *def; + void (*tweak)(struct rev_info *, struct setup_revision_opt *); + unsigned int assume_dashdash:1, + allow_exclude_promisor_objects:1; + unsigned revarg_opt; +}; int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct setup_revision_opt *); @@ -423,6 +407,14 @@ void mark_trees_uninteresting_sparse(struct repository *r, struct oidset *trees) void show_object_with_name(FILE *, struct object *, const char *); +/** + * Helpers to check if a "struct string_list" item matches with + * wildmatch(). + */ +int ref_excluded(struct string_list *, const char *path); +void clear_ref_exclusion(struct string_list **); +void add_ref_exclusion(struct string_list **, const char *exclude); + /** * This function can be used if you want to add commit objects as revision * information. You can use the `UNINTERESTING` object flag to indicate if @@ -478,4 +470,10 @@ int rewrite_parents(struct rev_info *revs, */ struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit *commit); +/** + * Global for the (undocumented) "--early-output" flag for "git log". + */ +typedef void (*show_early_output_fn_t)(struct rev_info *, struct commit_list *); +extern volatile show_early_output_fn_t show_early_output; + #endif From f196c1e908db9242206e1fc9c64f33baf92676bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:38 +0200 Subject: [PATCH 09/27] revisions API users: use release_revisions() needing REV_INFO_INIT MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use release_revisions() to various users of "struct rev_list" which need to have their "struct rev_info" zero-initialized before we can start using it. For the bundle.c code see the early exit case added in 3bbbe467f29 (bundle verify: error out if called without an object database, 2019-05-27). For the relevant bisect.c code see 45b6370812c (bisect: libify `check_good_are_ancestors_of_bad` and its dependents, 2020-02-17). For the submodule.c code see the "goto" on "(!left || !right || !sub)" added in 8e6df65015f (submodule: refactor show_submodule_summary with helper function, 2016-08-31). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- bisect.c | 18 ++++++++++++------ builtin/submodule--helper.c | 3 ++- bundle.c | 12 ++++++++---- revision.h | 21 ++++++++++++++++++++- submodule.c | 3 ++- 5 files changed, 44 insertions(+), 13 deletions(-) diff --git a/bisect.c b/bisect.c index cc6b8b6230..b63669cc9d 100644 --- a/bisect.c +++ b/bisect.c @@ -1010,7 +1010,7 @@ void read_bisect_terms(const char **read_bad, const char **read_good) */ enum bisect_error bisect_next_all(struct repository *r, const char *prefix) { - struct rev_info revs; + struct rev_info revs = REV_INFO_INIT; struct commit_list *tried; int reaches = 0, all = 0, nr, steps; enum bisect_error res = BISECT_OK; @@ -1035,7 +1035,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix) res = check_good_are_ancestors_of_bad(r, prefix, no_checkout); if (res) - return res; + goto cleanup; bisect_rev_setup(r, &revs, prefix, "%s", "^%s", 1); @@ -1060,14 +1060,16 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix) term_good, term_bad); - return BISECT_FAILED; + res = BISECT_FAILED; + goto cleanup; } if (!all) { fprintf(stderr, _("No testable commit found.\n" "Maybe you started with bad path arguments?\n")); - return BISECT_NO_TESTABLE_COMMIT; + res = BISECT_NO_TESTABLE_COMMIT; + goto cleanup; } bisect_rev = &revs.commits->item->object.oid; @@ -1087,7 +1089,8 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix) * for negative return values for early returns up * until the cmd_bisect__helper() caller. */ - return BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND; + res = BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND; + goto cleanup; } nr = all - reaches - 1; @@ -1106,7 +1109,10 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix) /* Clean up objects used, as they will be reused. */ repo_clear_commit_marks(r, ALL_REV_FLAGS); - return bisect_checkout(bisect_rev, no_checkout); + res = bisect_checkout(bisect_rev, no_checkout); +cleanup: + release_revisions(&revs); + return res; } static inline int log2i(int n) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 24980863f6..cda33ee4d2 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -766,7 +766,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid, { char *displaypath; struct strvec diff_files_args = STRVEC_INIT; - struct rev_info rev; + struct rev_info rev = REV_INFO_INIT; int diff_files_result; struct strbuf buf = STRBUF_INIT; const char *git_dir; @@ -853,6 +853,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid, cleanup: strvec_clear(&diff_files_args); free(displaypath); + release_revisions(&rev); } static void status_submodule_cb(const struct cache_entry *list_item, diff --git a/bundle.c b/bundle.c index d50cfb5aa7..6a870a6edb 100644 --- a/bundle.c +++ b/bundle.c @@ -196,14 +196,16 @@ int verify_bundle(struct repository *r, * to be verbose about the errors */ struct string_list *p = &header->prerequisites; - struct rev_info revs; + struct rev_info revs = REV_INFO_INIT; const char *argv[] = {NULL, "--all", NULL}; struct commit *commit; int i, ret = 0, req_nr; const char *message = _("Repository lacks these prerequisite commits:"); - if (!r || !r->objects || !r->objects->odb) - return error(_("need a repository to verify a bundle")); + if (!r || !r->objects || !r->objects->odb) { + ret = error(_("need a repository to verify a bundle")); + goto cleanup; + } repo_init_revisions(r, &revs, NULL); for (i = 0; i < p->nr; i++) { @@ -221,7 +223,7 @@ int verify_bundle(struct repository *r, error("%s %s", oid_to_hex(oid), name); } if (revs.pending.nr != p->nr) - return ret; + goto cleanup; req_nr = revs.pending.nr; setup_revisions(2, argv, &revs, NULL); @@ -284,6 +286,8 @@ int verify_bundle(struct repository *r, printf_ln("The bundle uses this filter: %s", list_objects_filter_spec(&header->filter)); } +cleanup: + release_revisions(&revs); return ret; } diff --git a/revision.h b/revision.h index b9070e4342..2621eb6d65 100644 --- a/revision.h +++ b/revision.h @@ -329,6 +329,25 @@ struct rev_info { struct tmp_objdir *remerge_objdir; }; +/** + * Initialize the "struct rev_info" structure with a macro. + * + * This will not fully initialize a "struct rev_info", the + * repo_init_revisions() function needs to be called before + * setup_revisions() and any revision walking takes place. + * + * Use REV_INFO_INIT to make the "struct rev_info" safe for passing to + * release_revisions() when it's inconvenient (e.g. due to a "goto + * cleanup" pattern) to arrange for repo_init_revisions() to be called + * before release_revisions() is called. + * + * Initializing with this REV_INFO_INIT is redundant to invoking + * repo_init_revisions(). If repo_init_revisions() is guaranteed to be + * called before release_revisions() the "struct rev_info" can be left + * uninitialized. + */ +#define REV_INFO_INIT { 0 } + /** * Initialize a rev_info structure with default values. The third parameter may * be NULL or can be prefix path, and then the `.prefix` variable will be set @@ -363,7 +382,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, /** * Free data allocated in a "struct rev_info" after it's been - * initialized with repo_init_revisions(). + * initialized with repo_init_revisions() or REV_INFO_INIT. */ void release_revisions(struct rev_info *revs); diff --git a/submodule.c b/submodule.c index 86ebd3f35c..e9c320d16d 100644 --- a/submodule.c +++ b/submodule.c @@ -638,7 +638,7 @@ void show_submodule_diff_summary(struct diff_options *o, const char *path, struct object_id *one, struct object_id *two, unsigned dirty_submodule) { - struct rev_info rev; + struct rev_info rev = REV_INFO_INIT; struct commit *left = NULL, *right = NULL; struct commit_list *merge_bases = NULL; struct repository *sub; @@ -665,6 +665,7 @@ void show_submodule_diff_summary(struct diff_options *o, const char *path, out: free_commit_list(merge_bases); + release_revisions(&rev); clear_commit_marks(left, ~0); clear_commit_marks(right, ~0); if (sub) { From 5e480176fed01aeb47735f525002203ac6e462d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:39 +0200 Subject: [PATCH 10/27] stash: always have the owner of "stash_info" free it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 not 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. To ensure that there's a 1=1 mapping between owners of the "struct stash_info" and free_stash_info() change the assert_stash_ref() function to be a trivial get_stash_info_assert() wrapper. The caller will call free_stash_info(), and by returning -1 we'll eventually (via !!ret) exit with status 1 anyway. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/stash.c | 107 ++++++++++++++++++++++++++---------------------- 1 file changed, 59 insertions(+), 48 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index 8c90db479c..a96d84a5a9 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -116,6 +116,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); @@ -157,10 +161,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; } @@ -174,11 +176,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); @@ -197,7 +196,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); @@ -598,10 +597,10 @@ restore_untracked: 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, @@ -613,9 +612,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; } @@ -651,20 +651,25 @@ static int do_drop_stash(struct stash_info *info, int quiet) return 0; } -static void assert_stash_ref(struct stash_info *info) +static int get_stash_info_assert(struct stash_info *info, int argc, + const char **argv) { - if (!info->is_stash_ref) { - error(_("'%s' is not a stash reference"), info->revision.buf); - free_stash_info(info); - exit(1); - } + int ret = get_stash_info(info, argc, argv); + + if (ret < 0) + return ret; + + if (!info->is_stash_ref) + return error(_("'%s' is not a stash reference"), info->revision.buf); + + return 0; } 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() @@ -673,22 +678,21 @@ static int drop_stash(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, git_stash_drop_usage, 0); - if (get_stash_info(&info, argc, argv)) - return -1; - - assert_stash_ref(&info); + if (get_stash_info_assert(&info, argc, argv)) + goto cleanup; 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, @@ -699,25 +703,25 @@ static int pop_stash(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, git_stash_pop_usage, 0); - if (get_stash_info(&info, argc, argv)) - return -1; + if (get_stash_info_assert(&info, argc, argv)) + goto cleanup; - assert_stash_ref(&info); if ((ret = do_apply_stash(prefix, &info, index, quiet))) printf_ln(_("The stash entry is kept in case " "you need it again.")); 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() @@ -734,7 +738,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); @@ -746,8 +750,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; } @@ -825,8 +829,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; @@ -844,6 +848,7 @@ static int show_stash(int argc, const char **argv, const char *prefix) UNTRACKED_ONLY, PARSE_OPT_NONEG), OPT_END() }; + int do_usage = 0; init_diff_ui_defaults(); git_config(git_diff_ui_config, NULL); @@ -861,10 +866,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,16 +881,14 @@ 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; } } argc = setup_revisions(revision_args.nr, revision_args.v, &rev, NULL); - if (argc > 1) { - free_stash_info(&info); - usage_with_options(git_stash_show_usage, options); - } + if (argc > 1) + goto usage; if (!rev.diffopt.output_format) { rev.diffopt.output_format = DIFF_FORMAT_PATCH; diff_setup_done(&rev.diffopt); @@ -912,8 +913,16 @@ 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); + if (do_usage) + usage_with_options(git_stash_show_usage, options); + return ret; +usage: + do_usage = 1; + goto cleanup; } static int do_store_stash(const struct object_id *w_commit, const char *stash_msg, @@ -1409,9 +1418,9 @@ done: 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" */ @@ -1426,6 +1435,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; } @@ -1434,7 +1444,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; @@ -1632,6 +1642,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q } done: + free_stash_info(&info); strbuf_release(&stash_msg_buf); return ret; } From 0139c58ab951e8620d6066eb687d0a96e490436a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:40 +0200 Subject: [PATCH 11/27] revisions API users: add "goto cleanup" for release_revisions() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a release_revisions() to various users of "struct rev_info" which requires a minor refactoring to a "goto cleanup" pattern to use that function. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/diff-files.c | 8 ++++++-- builtin/rev-list.c | 19 ++++++++++++------- builtin/stash.c | 1 + builtin/submodule--helper.c | 10 +++++++--- sequencer.c | 23 ++++++++++++++++------- t/helper/test-fast-rebase.c | 18 +++++++++++++----- 6 files changed, 55 insertions(+), 24 deletions(-) diff --git a/builtin/diff-files.c b/builtin/diff-files.c index 70103c4095..2bfaf9ba7a 100644 --- a/builtin/diff-files.c +++ b/builtin/diff-files.c @@ -77,8 +77,12 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix) if (read_cache_preload(&rev.diffopt.pathspec) < 0) { perror("read_cache_preload"); - return -1; + result = -1; + goto cleanup; } +cleanup: result = run_diff_files(&rev, options); - return diff_result_code(&rev.diffopt, result); + result = diff_result_code(&rev.diffopt, result); + release_revisions(&rev); + return result; } diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 07c0ad11d8..30fd8e83ea 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -500,6 +500,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) int use_bitmap_index = 0; int filter_provided_objects = 0; const char *show_progress = NULL; + int ret = 0; if (argc == 2 && !strcmp(argv[1], "-h")) usage(rev_list_usage); @@ -583,7 +584,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) } if (!strcmp(arg, "--test-bitmap")) { test_bitmap_walk(&revs); - return 0; + goto cleanup; } if (skip_prefix(arg, "--progress=", &arg)) { show_progress = arg; @@ -672,11 +673,11 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) if (use_bitmap_index) { if (!try_bitmap_count(&revs, filter_provided_objects)) - return 0; + goto cleanup; if (!try_bitmap_disk_usage(&revs, filter_provided_objects)) - return 0; + goto cleanup; if (!try_bitmap_traversal(&revs, filter_provided_objects)) - return 0; + goto cleanup; } if (prepare_revision_walk(&revs)) @@ -696,8 +697,10 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) find_bisection(&revs.commits, &reaches, &all, bisect_flags); - if (bisect_show_vars) - return show_bisect_vars(&info, reaches, all); + if (bisect_show_vars) { + ret = show_bisect_vars(&info, reaches, all); + goto cleanup; + } } if (filter_provided_objects) { @@ -752,5 +755,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) if (show_disk_usage) printf("%"PRIuMAX"\n", (uintmax_t)total_disk_usage); - return 0; +cleanup: + release_revisions(&revs); + return ret; } diff --git a/builtin/stash.c b/builtin/stash.c index a96d84a5a9..7c9c5751f5 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -917,6 +917,7 @@ static int show_stash(int argc, const char **argv, const char *prefix) cleanup: strvec_clear(&stash_args); free_stash_info(&info); + release_revisions(&rev); if (do_usage) usage_with_options(git_stash_show_usage, options); return ret; diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index cda33ee4d2..1bd31c8594 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1232,6 +1232,7 @@ static int compute_summary_module_list(struct object_id *head_oid, struct strvec diff_args = STRVEC_INIT; struct rev_info rev; struct module_cb_list list = MODULE_CB_LIST_INIT; + int ret = 0; strvec_push(&diff_args, get_diff_cmd(diff_cmd)); if (info->cached) @@ -1257,11 +1258,13 @@ static int compute_summary_module_list(struct object_id *head_oid, setup_work_tree(); if (read_cache_preload(&rev.diffopt.pathspec) < 0) { perror("read_cache_preload"); - return -1; + ret = -1; + goto cleanup; } } else if (read_cache() < 0) { perror("read_cache"); - return -1; + ret = -1; + goto cleanup; } if (diff_cmd == DIFF_INDEX) @@ -1269,9 +1272,10 @@ static int compute_summary_module_list(struct object_id *head_oid, else run_diff_files(&rev, 0); prepare_submodule_summary(info, &list); +cleanup: strvec_clear(&diff_args); release_revisions(&rev); - return 0; + return ret; } static int module_summary(int argc, const char **argv, const char *prefix) diff --git a/sequencer.c b/sequencer.c index f9d7acd106..41ae5e2527 100644 --- a/sequencer.c +++ b/sequencer.c @@ -5354,6 +5354,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc, int rebase_merges = flags & TODO_LIST_REBASE_MERGES; int reapply_cherry_picks = flags & TODO_LIST_REAPPLY_CHERRY_PICKS; int skipped_commit = 0; + int ret = 0; repo_init_revisions(r, &revs, NULL); revs.verbose_header = 1; @@ -5377,14 +5378,20 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc, pp.fmt = revs.commit_format; pp.output_encoding = get_log_output_encoding(); - if (setup_revisions(argc, argv, &revs, NULL) > 1) - return error(_("make_script: unhandled options")); + if (setup_revisions(argc, argv, &revs, NULL) > 1) { + ret = error(_("make_script: unhandled options")); + goto cleanup; + } - if (prepare_revision_walk(&revs) < 0) - return error(_("make_script: error preparing revisions")); + if (prepare_revision_walk(&revs) < 0) { + ret = error(_("make_script: error preparing revisions")); + goto cleanup; + } - if (rebase_merges) - return make_script_with_merges(&pp, &revs, out, flags); + if (rebase_merges) { + ret = make_script_with_merges(&pp, &revs, out, flags); + goto cleanup; + } while ((commit = get_revision(&revs))) { int is_empty = is_original_commit_empty(commit); @@ -5408,7 +5415,9 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc, if (skipped_commit) advise_if_enabled(ADVICE_SKIPPED_CHERRY_PICKS, _("use --reapply-cherry-picks to include skipped commits")); - return 0; +cleanup: + release_revisions(&revs); + return ret; } /* diff --git a/t/helper/test-fast-rebase.c b/t/helper/test-fast-rebase.c index 993b90eaed..4e5553e202 100644 --- a/t/helper/test-fast-rebase.c +++ b/t/helper/test-fast-rebase.c @@ -99,6 +99,7 @@ int cmd__fast_rebase(int argc, const char **argv) struct merge_result result; struct strbuf reflog_msg = STRBUF_INIT; struct strbuf branch_name = STRBUF_INIT; + int ret = 0; /* * test-tool stuff doesn't set up the git directory by default; need to @@ -137,13 +138,17 @@ int cmd__fast_rebase(int argc, const char **argv) revs.topo_order = 1; strvec_pushl(&rev_walk_args, "", argv[4], "--not", argv[3], NULL); - if (setup_revisions(rev_walk_args.nr, rev_walk_args.v, &revs, NULL) > 1) - return error(_("unhandled options")); + if (setup_revisions(rev_walk_args.nr, rev_walk_args.v, &revs, NULL) > 1) { + ret = error(_("unhandled options")); + goto cleanup; + } strvec_clear(&rev_walk_args); - if (prepare_revision_walk(&revs) < 0) - return error(_("error preparing revisions")); + if (prepare_revision_walk(&revs) < 0) { + ret = error(_("error preparing revisions")); + goto cleanup; + } init_merge_options(&merge_opt, the_repository); memset(&result, 0, sizeof(result)); @@ -220,7 +225,10 @@ int cmd__fast_rebase(int argc, const char **argv) COMMIT_LOCK | SKIP_IF_UNCHANGED)) die(_("unable to write %s"), get_index_file()); + ret = (result.clean == 0); +cleanup: strbuf_release(&reflog_msg); strbuf_release(&branch_name); - return (result.clean == 0); + release_revisions(&revs); + return ret; } From b78ce337def2db3a6fceee157f49202e5d1ce46a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:41 +0200 Subject: [PATCH 12/27] revisions API users: use release_revisions() in http-push.c MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the case of cmd_main() in http-push.c we need to move the deceleration of the "struct rev-list" into the loop over the "remote_refs" when adding a release_revisions(). We'd previously set up the "revs" for each remote, but would potentially leak memory on each one. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- http-push.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/http-push.c b/http-push.c index f0c044dcf7..01e7c2ac5c 100644 --- a/http-push.c +++ b/http-push.c @@ -1689,7 +1689,6 @@ int cmd_main(int argc, const char **argv) struct refspec rs = REFSPEC_INIT_PUSH; struct remote_lock *ref_lock = NULL; struct remote_lock *info_ref_lock = NULL; - struct rev_info revs; int delete_branch = 0; int force_delete = 0; int objects_to_send; @@ -1825,6 +1824,7 @@ int cmd_main(int argc, const char **argv) new_refs = 0; for (ref = remote_refs; ref; ref = ref->next) { + struct rev_info revs; struct strvec commit_argv = STRVEC_INIT; if (!ref->peer_ref) @@ -1941,6 +1941,7 @@ int cmd_main(int argc, const char **argv) unlock_remote(ref_lock); check_locks(); strvec_clear(&commit_argv); + release_revisions(&revs); } /* Update remote server info if appropriate */ From f6bfea0ad0151a941686b2ed1875ec7038623ac0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:42 +0200 Subject: [PATCH 13/27] revisions API users: use release_revisions() in builtin/log.c MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In preparation for having the "log" family of functions make wider use of release_revisions() let's have them call it just before exiting. This changes the "log", "whatchanged", "show", "format-patch", etc. commands, all of which live in this file. The release_revisions() API still only frees the "pending" member, but will learn to release more members of "struct rev_info" in subsequent commits. In the case of "format-patch" revert the addition of UNLEAK() in dee839a2633 (format-patch: mark rev_info with UNLEAK, 2021-12-16), which will cause several tests that previously passed under "TEST_PASSES_SANITIZE_LEAK=true" to start failing. In subsequent commits we'll now be able to use those tests to check whether that part of the API is really leaking memory, and will fix all of those memory leaks. Removing the UNLEAK() allows us to make incremental progress in that direction. See [1] for further details about this approach. Note that the release_revisions() will not be sufficient to deal with the code in cmd_show() added in 5d7eeee2ac6 (git-show: grok blobs, trees and tags, too, 2006-12-14) which clobbers the "pending" array in the case of "OBJ_COMMIT". That will need to be dealt with by some future follow-up work. 1. https://lore.kernel.org/git/220218.861r00ib86.gmgdl@evledraar.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/log.c | 20 ++++++++++++-------- t/t4126-apply-empty.sh | 2 -- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index 6f9928fabf..c40ebe1c3f 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -295,6 +295,12 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix, cmd_log_init_finish(argc, argv, prefix, rev, opt); } +static int cmd_log_deinit(int ret, struct rev_info *rev) +{ + release_revisions(rev); + return ret; +} + /* * This gives a rough estimate for how many commits we * will print out in the list. @@ -558,7 +564,7 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix) cmd_log_init(argc, argv, prefix, &rev, &opt); if (!rev.diffopt.output_format) rev.diffopt.output_format = DIFF_FORMAT_RAW; - return cmd_log_walk(&rev); + return cmd_log_deinit(cmd_log_walk(&rev), &rev); } static void show_tagger(const char *buf, struct rev_info *rev) @@ -677,7 +683,7 @@ int cmd_show(int argc, const char **argv, const char *prefix) cmd_log_init(argc, argv, prefix, &rev, &opt); if (!rev.no_walk) - return cmd_log_walk(&rev); + return cmd_log_deinit(cmd_log_walk(&rev), &rev); count = rev.pending.nr; objects = rev.pending.objects; @@ -732,8 +738,7 @@ int cmd_show(int argc, const char **argv, const char *prefix) ret = error(_("unknown type: %d"), o->type); } } - free(objects); - return ret; + return cmd_log_deinit(ret, &rev); } /* @@ -761,7 +766,7 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix) rev.always_show_header = 1; cmd_log_init_finish(argc, argv, prefix, &rev, &opt); - return cmd_log_walk(&rev); + return cmd_log_deinit(cmd_log_walk(&rev), &rev); } static void log_setup_revisions_tweak(struct rev_info *rev, @@ -792,7 +797,7 @@ int cmd_log(int argc, const char **argv, const char *prefix) opt.revarg_opt = REVARG_COMMITTISH; opt.tweak = log_setup_revisions_tweak; cmd_log_init(argc, argv, prefix, &rev, &opt); - return cmd_log_walk(&rev); + return cmd_log_deinit(cmd_log_walk(&rev), &rev); } /* format-patch */ @@ -2289,8 +2294,7 @@ done: if (rev.ref_message_ids) string_list_clear(rev.ref_message_ids, 0); free(rev.ref_message_ids); - UNLEAK(rev); - return 0; + return cmd_log_deinit(0, &rev); } static int add_pending_commit(const char *arg, struct rev_info *revs, int flags) diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh index 33860d3829..66a7ba8ab8 100755 --- a/t/t4126-apply-empty.sh +++ b/t/t4126-apply-empty.sh @@ -2,8 +2,6 @@ test_description='apply empty' - -TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' From bf1b32d0991cfead7e5604897d0866b789bf602e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:43 +0200 Subject: [PATCH 14/27] revisions API users: use release_revisions() with UNLEAK() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use a release_revisions() with those "struct rev_list" users which already "UNLEAK" the struct. It may seem odd to simultaneously attempt to free() memory, but also to explicitly ignore whether we have memory leaks in the same. As explained in preceding commits this is being done to use the built-in commands as a guinea pig for whether the release_revisions() function works as expected, we'd like to test e.g. whether we segfault as we change it. In subsequent commits we'll then remove these UNLEAK() as the function is made to free the memory that caused us to add them in the first place. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/diff-index.c | 4 +++- builtin/diff.c | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/builtin/diff-index.c b/builtin/diff-index.c index 5fd23ab5b6..3a83183c31 100644 --- a/builtin/diff-index.c +++ b/builtin/diff-index.c @@ -71,5 +71,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) } result = run_diff_index(&rev, option); UNLEAK(rev); - return diff_result_code(&rev.diffopt, result); + result = diff_result_code(&rev.diffopt, result); + release_revisions(&rev); + return result; } diff --git a/builtin/diff.c b/builtin/diff.c index bb7fafca61..dd48336da5 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -595,6 +595,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) if (1 < rev.diffopt.skip_stat_unmatch) refresh_index_quietly(); UNLEAK(rev); + release_revisions(&rev); UNLEAK(ent); UNLEAK(blob); return result; From f0cb6b8053eb9146e9d5255b6b0473d020c6e8bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:44 +0200 Subject: [PATCH 15/27] revisions API users: use release_revisions() for "prune_data" users MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use release_revisions() for users of "struct rev_list" that reach into the "struct rev_info" and clear the "prune_data" already. In a subsequent commit we'll teach release_revisions() to clear this itself, but in the meantime let's invoke release_revisions() here to clear anything else we may have missed, and for reasons of having consistent boilerplate. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/add.c | 1 + diff-lib.c | 1 + wt-status.c | 1 + 3 files changed, 3 insertions(+) diff --git a/builtin/add.c b/builtin/add.c index f507d2191c..115a26ea63 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -143,6 +143,7 @@ int add_files_to_cache(const char *prefix, rev.max_count = 0; /* do not compare unmerged paths with stage #2 */ run_diff_files(&rev, DIFF_RACY_IS_MODIFIED); clear_pathspec(&rev.prune_data); + release_revisions(&rev); return !!data.add_errors; } diff --git a/diff-lib.c b/diff-lib.c index d6800274bd..0f16281253 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -642,6 +642,7 @@ int do_diff_cache(const struct object_id *tree_oid, struct diff_options *opt) if (diff_cache(&revs, tree_oid, NULL, 1)) exit(128); clear_pathspec(&revs.prune_data); + release_revisions(&revs); return 0; } diff --git a/wt-status.c b/wt-status.c index f910062137..a14fad1e03 100644 --- a/wt-status.c +++ b/wt-status.c @@ -617,6 +617,7 @@ static void wt_status_collect_changes_worktree(struct wt_status *s) copy_pathspec(&rev.prune_data, &s->pathspec); run_diff_files(&rev, 0); clear_pathspec(&rev.prune_data); + release_revisions(&rev); } static void wt_status_collect_changes_index(struct wt_status *s) From e966fc5a89b4db275f3855cfdff157c1a759c7c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:45 +0200 Subject: [PATCH 16/27] revisions API: have release_revisions() release "commits" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extend the the release_revisions() function so that it frees the "commits" in the "struct rev_info". We don't expect to use this "struct rev_info" again, so there's no reason to NULL out revs->commits, as e.g. simplify_merges() and create_boundary_commit_list() do. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- revision.c | 1 + 1 file changed, 1 insertion(+) diff --git a/revision.c b/revision.c index 472fff1e0a..553f7de825 100644 --- a/revision.c +++ b/revision.c @@ -2928,6 +2928,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s void release_revisions(struct rev_info *revs) { + free_commit_list(revs->commits); object_array_clear(&revs->pending); } From a52f07afcb02b2e09702d54118527fbb9d640895 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:46 +0200 Subject: [PATCH 17/27] revisions API: have release_revisions() release "mailmap" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extend the the release_revisions() function so that it frees the "mailmap" in the "struct rev_info". The log family of functions now calls the clear_mailmap() function added in fa8afd18e5a (revisions API: provide and use a release_revisions(), 2021-09-19), allowing us to whitelist some tests with "TEST_PASSES_SANITIZE_LEAK=true". Unfortunately having a pointer to a mailmap in "struct rev_info" instead of an embedded member that we "own" get a bit messy, as can be seen in the change to builtin/commit.c. When we free() this data we won't be able to tell apart a pointer to a "mailmap" on the heap from one on the stack. As seen in ea57bc0d41b (log: add --use-mailmap option, 2013-01-05) the "log" family allocates it on the heap, but in the find_author_by_nickname() code added in ea16794e430 (commit: search author pattern against mailmap, 2013-08-23) we allocated it on the stack instead. Ideally we'd simply change that member to a "struct string_list mailmap" and never free() the "mailmap" itself, but that would be a much larger change to the revisions API. We have code that needs to hand an existing "mailmap" to a "struct rev_info", while we could change all of that, let's not go there now. The complexity isn't in the ownership of the "mailmap" per-se, but that various things assume a "rev_info.mailmap == NULL" means "doesn't want mailmap", if we changed that to an init'd "struct string_list we'd need to carefully refactor things to change those assumptions. Let's instead always free() it, and simply declare that if you add such a "mailmap" it must be allocated on the heap. Any modern libc will correctly panic if we free() a stack variable, so this should be safe going forward. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/commit.c | 5 ++--- revision.c | 9 +++++++++ t/t0056-git-C.sh | 1 + t/t3302-notes-index-expensive.sh | 1 + t/t4055-diff-context.sh | 1 + t/t4066-diff-emit-delay.sh | 1 + t/t7008-filter-branch-null-sha1.sh | 1 + 7 files changed, 16 insertions(+), 3 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index c7eda9bbb7..cd6cebcf8c 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1100,7 +1100,6 @@ static const char *find_author_by_nickname(const char *name) struct rev_info revs; struct commit *commit; struct strbuf buf = STRBUF_INIT; - struct string_list mailmap = STRING_LIST_INIT_NODUP; const char *av[20]; int ac = 0; @@ -1111,7 +1110,8 @@ static const char *find_author_by_nickname(const char *name) av[++ac] = buf.buf; av[++ac] = NULL; setup_revisions(ac, av, &revs, NULL); - revs.mailmap = &mailmap; + revs.mailmap = xmalloc(sizeof(struct string_list)); + string_list_init_nodup(revs.mailmap); read_mailmap(revs.mailmap); if (prepare_revision_walk(&revs)) @@ -1122,7 +1122,6 @@ static const char *find_author_by_nickname(const char *name) ctx.date_mode.type = DATE_NORMAL; strbuf_release(&buf); format_commit_message(commit, "%aN <%aE>", &buf, &ctx); - clear_mailmap(&mailmap); release_revisions(&revs); return strbuf_detach(&buf, NULL); } diff --git a/revision.c b/revision.c index 553f7de825..622f0faecc 100644 --- a/revision.c +++ b/revision.c @@ -2926,10 +2926,19 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s return left; } +static void release_revisions_mailmap(struct string_list *mailmap) +{ + if (!mailmap) + return; + clear_mailmap(mailmap); + free(mailmap); +} + void release_revisions(struct rev_info *revs) { free_commit_list(revs->commits); object_array_clear(&revs->pending); + release_revisions_mailmap(revs->mailmap); } static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child) diff --git a/t/t0056-git-C.sh b/t/t0056-git-C.sh index 2630e756da..752aa8c945 100755 --- a/t/t0056-git-C.sh +++ b/t/t0056-git-C.sh @@ -2,6 +2,7 @@ test_description='"-C " option and its effects on other path-related options' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success '"git -C " runs git from the directory ' ' diff --git a/t/t3302-notes-index-expensive.sh b/t/t3302-notes-index-expensive.sh index bb5fea02a0..d0c4d38b4d 100755 --- a/t/t3302-notes-index-expensive.sh +++ b/t/t3302-notes-index-expensive.sh @@ -8,6 +8,7 @@ test_description='Test commit notes index (expensive!)' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh create_repo () { diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh index 741e0803c1..73048d0a52 100755 --- a/t/t4055-diff-context.sh +++ b/t/t4055-diff-context.sh @@ -5,6 +5,7 @@ test_description='diff.context configuration' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t4066-diff-emit-delay.sh b/t/t4066-diff-emit-delay.sh index a1de63b77f..0ecb391541 100755 --- a/t/t4066-diff-emit-delay.sh +++ b/t/t4066-diff-emit-delay.sh @@ -4,6 +4,7 @@ test_description='test combined/stat/moved interaction' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # This test covers a weird 3-way interaction between "--cc -p", which will run diff --git a/t/t7008-filter-branch-null-sha1.sh b/t/t7008-filter-branch-null-sha1.sh index 9ba9f24ad2..93fbc92b8d 100755 --- a/t/t7008-filter-branch-null-sha1.sh +++ b/t/t7008-filter-branch-null-sha1.sh @@ -1,6 +1,7 @@ #!/bin/sh test_description='filter-branch removal of trees with null sha1' + . ./test-lib.sh test_expect_success 'setup: base commits' ' From 7a98d9ab00dc0a238b624b20c3884b50fe504e2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:47 +0200 Subject: [PATCH 18/27] revisions API: have release_revisions() release "cmdline" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extend the the release_revisions() function so that it frees the "cmdline" in the "struct rev_info". This in combination with a preceding change to free "commits" and "mailmap" means that we can whitelist another test under "TEST_PASSES_SANITIZE_LEAK=true". There was a proposal in [1] to do away with xstrdup()-ing this add_rev_cmdline(), perhaps that would be worthwhile, but for now let's just free() it. We could also make that a "char *" in "struct rev_cmdline_entry" itself, but since we own it let's expose it as a constant to outside callers. I proposed that in [2] but have since changed my mind. See 14d30cdfc04 (ref-filter: fix memory leak in `free_array_item()`, 2019-07-10), c514c62a4fd (checkout: fix leak of non-existent branch names, 2020-08-14) and other log history hits for "free((char *)" for prior art. This includes the tests we had false-positive passes on before my 6798b08e848 (perl Git.pm: don't ignore signalled failure in _cmd_close(), 2022-02-01), now they pass for real. Since there are 66 tests matching t/t[0-9]*git-svn*.sh it's easier to list those that don't pass than to touch most of those 66. So let's introduce a "TEST_FAILS_SANITIZE_LEAK=true", which if set in the tests won't cause lib-git-svn.sh to set "TEST_PASSES_SANITIZE_LEAK=true. This change also marks all the tests that we removed "TEST_FAILS_SANITIZE_LEAK=true" from in an earlier commit due to removing the UNLEAK() from cmd_format_patch(), we can now assert that its API use doesn't leak any "struct rev_info" memory. This change also made commit "t5503-tagfollow.sh" pass on current master, but that would regress when combined with ps/fetch-atomic-fixup's de004e848a9 (t5503: simplify setup of test which exercises failure of backfill, 2022-03-03) (through no fault of that topic, that change started using "git clone" in the test, which has an outstanding leak). Let's leave that test out for now to avoid in-flight semantic conflicts. 1. https://lore.kernel.org/git/YUj%2FgFRh6pwrZalY@carlos-mbp.lan/ 2. https://lore.kernel.org/git/87o88obkb1.fsf@evledraar.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- revision.c | 10 ++++++++++ t/lib-git-svn.sh | 4 ++++ t/t0062-revision-walking.sh | 1 + t/t0101-at-syntax.sh | 2 ++ t/t1060-object-corruption.sh | 1 + t/t3303-notes-subtrees.sh | 1 + t/t3305-notes-fanout.sh | 1 + t/t3408-rebase-multi-line.sh | 1 + t/t4027-diff-submodule.sh | 1 + t/t4128-apply-root.sh | 1 + t/t4212-log-corrupt.sh | 1 + t/t5515-fetch-merge-logic.sh | 1 + t/t5518-fetch-exit-status.sh | 1 + t/t6002-rev-list-bisect.sh | 1 + t/t6003-rev-list-topo-order.sh | 1 + t/t6005-rev-list-count.sh | 1 + t/t6018-rev-list-glob.sh | 1 + t/t6100-rev-list-in-order.sh | 1 + t/t9100-git-svn-basic.sh | 1 + t/t9101-git-svn-props.sh | 2 ++ t/t9104-git-svn-follow-parent.sh | 2 ++ t/t9106-git-svn-commit-diff-clobber.sh | 2 ++ t/t9115-git-svn-dcommit-funky-renames.sh | 1 + t/t9116-git-svn-log.sh | 2 ++ t/t9122-git-svn-author.sh | 2 ++ t/t9127-git-svn-partial-rebuild.sh | 2 ++ t/t9129-git-svn-i18n-commitencoding.sh | 1 + t/t9132-git-svn-broken-symlink.sh | 1 + t/t9139-git-svn-non-utf8-commitencoding.sh | 1 + t/t9146-git-svn-empty-dirs.sh | 2 ++ t/t9148-git-svn-propset.sh | 1 + t/t9151-svn-mergeinfo.sh | 1 + t/t9160-git-svn-preserve-empty-dirs.sh | 1 + t/t9162-git-svn-dcommit-interactive.sh | 2 ++ t/t9164-git-svn-dcommit-concurrent.sh | 2 ++ t/t9501-gitweb-standalone-http-status.sh | 1 + 36 files changed, 58 insertions(+) diff --git a/revision.c b/revision.c index 622f0faecc..de4076e77d 100644 --- a/revision.c +++ b/revision.c @@ -2926,6 +2926,15 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s return left; } +static void release_revisions_cmdline(struct rev_cmdline_info *cmdline) +{ + unsigned int i; + + for (i = 0; i < cmdline->nr; i++) + free((char *)cmdline->rev[i].name); + free(cmdline->rev); +} + static void release_revisions_mailmap(struct string_list *mailmap) { if (!mailmap) @@ -2938,6 +2947,7 @@ void release_revisions(struct rev_info *revs) { free_commit_list(revs->commits); object_array_clear(&revs->pending); + release_revisions_cmdline(&revs->cmdline); release_revisions_mailmap(revs->mailmap); } diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh index 2fde2353fd..ea28971e8e 100644 --- a/t/lib-git-svn.sh +++ b/t/lib-git-svn.sh @@ -1,3 +1,7 @@ +if test -z "$TEST_FAILS_SANITIZE_LEAK" +then + TEST_PASSES_SANITIZE_LEAK=true +fi . ./test-lib.sh if test -n "$NO_SVN_TESTS" diff --git a/t/t0062-revision-walking.sh b/t/t0062-revision-walking.sh index 8e215867b8..b9480c8178 100755 --- a/t/t0062-revision-walking.sh +++ b/t/t0062-revision-walking.sh @@ -5,6 +5,7 @@ test_description='Test revision walking api' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh cat >run_twice_expected <<-EOF diff --git a/t/t0101-at-syntax.sh b/t/t0101-at-syntax.sh index a1998b558f..878aadd64c 100755 --- a/t/t0101-at-syntax.sh +++ b/t/t0101-at-syntax.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='various @{whatever} syntax tests' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh index bc89371f53..e8a58b1589 100755 --- a/t/t1060-object-corruption.sh +++ b/t/t1060-object-corruption.sh @@ -1,6 +1,7 @@ #!/bin/sh test_description='see how we handle various forms of corruption' + . ./test-lib.sh # convert "1234abcd" to ".git/objects/12/34abcd" diff --git a/t/t3303-notes-subtrees.sh b/t/t3303-notes-subtrees.sh index eac193757b..bc9b791d1b 100755 --- a/t/t3303-notes-subtrees.sh +++ b/t/t3303-notes-subtrees.sh @@ -5,6 +5,7 @@ test_description='Test commit notes organized in subtrees' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh number_of_commits=100 diff --git a/t/t3305-notes-fanout.sh b/t/t3305-notes-fanout.sh index 9976d787f4..64a9915761 100755 --- a/t/t3305-notes-fanout.sh +++ b/t/t3305-notes-fanout.sh @@ -2,6 +2,7 @@ test_description='Test that adding/removing many notes triggers automatic fanout restructuring' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh path_has_fanout() { diff --git a/t/t3408-rebase-multi-line.sh b/t/t3408-rebase-multi-line.sh index cde3562e3a..7b4607d72f 100755 --- a/t/t3408-rebase-multi-line.sh +++ b/t/t3408-rebase-multi-line.sh @@ -5,6 +5,7 @@ test_description='rebasing a commit with multi-line first paragraph.' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh index 295da987cc..40164ae07d 100755 --- a/t/t4027-diff-submodule.sh +++ b/t/t4027-diff-submodule.sh @@ -2,6 +2,7 @@ test_description='difference in submodules' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-diff.sh diff --git a/t/t4128-apply-root.sh b/t/t4128-apply-root.sh index f6db5a79dd..ed94c90204 100755 --- a/t/t4128-apply-root.sh +++ b/t/t4128-apply-root.sh @@ -2,6 +2,7 @@ test_description='apply same filename' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh index 0244888a5a..30a219894b 100755 --- a/t/t4212-log-corrupt.sh +++ b/t/t4212-log-corrupt.sh @@ -2,6 +2,7 @@ test_description='git log with invalid commit headers' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh index 320d26796d..c100a809c5 100755 --- a/t/t5515-fetch-merge-logic.sh +++ b/t/t5515-fetch-merge-logic.sh @@ -14,6 +14,7 @@ export GIT_TEST_PROTOCOL_VERSION GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh build_script () { diff --git a/t/t5518-fetch-exit-status.sh b/t/t5518-fetch-exit-status.sh index 5c4ac2556e..c13120088f 100755 --- a/t/t5518-fetch-exit-status.sh +++ b/t/t5518-fetch-exit-status.sh @@ -8,6 +8,7 @@ test_description='fetch exit status test' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh index b95a0212ad..162cf50778 100755 --- a/t/t6002-rev-list-bisect.sh +++ b/t/t6002-rev-list-bisect.sh @@ -4,6 +4,7 @@ # test_description='Tests git rev-list --bisect functionality' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-t6000.sh # t6xxx specific functions diff --git a/t/t6003-rev-list-topo-order.sh b/t/t6003-rev-list-topo-order.sh index 24d1836f41..1f7d7dd20c 100755 --- a/t/t6003-rev-list-topo-order.sh +++ b/t/t6003-rev-list-topo-order.sh @@ -5,6 +5,7 @@ test_description='Tests git rev-list --topo-order functionality' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-t6000.sh # t6xxx specific functions diff --git a/t/t6005-rev-list-count.sh b/t/t6005-rev-list-count.sh index e960049f64..0729f800c3 100755 --- a/t/t6005-rev-list-count.sh +++ b/t/t6005-rev-list-count.sh @@ -2,6 +2,7 @@ test_description='git rev-list --max-count and --skip test' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh index 24b34add83..e1abc5c2b3 100755 --- a/t/t6018-rev-list-glob.sh +++ b/t/t6018-rev-list-glob.sh @@ -5,6 +5,7 @@ test_description='rev-list/rev-parse --glob' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh commit () { diff --git a/t/t6100-rev-list-in-order.sh b/t/t6100-rev-list-in-order.sh index e934bc239c..88ed7bd75a 100755 --- a/t/t6100-rev-list-in-order.sh +++ b/t/t6100-rev-list-in-order.sh @@ -2,6 +2,7 @@ test_description='rev-list testing in-commit-order' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup a commit history with trees, blobs' ' diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh index fea41b3c36..7c5b847f58 100755 --- a/t/t9100-git-svn-basic.sh +++ b/t/t9100-git-svn-basic.sh @@ -8,6 +8,7 @@ test_description='git svn basic tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_FAILS_SANITIZE_LEAK=true . ./lib-git-svn.sh prepare_utf8_locale diff --git a/t/t9101-git-svn-props.sh b/t/t9101-git-svn-props.sh index 8b5681dd68..d043e80fc3 100755 --- a/t/t9101-git-svn-props.sh +++ b/t/t9101-git-svn-props.sh @@ -4,6 +4,8 @@ # test_description='git svn property tests' + +TEST_FAILS_SANITIZE_LEAK=true . ./lib-git-svn.sh mkdir import diff --git a/t/t9104-git-svn-follow-parent.sh b/t/t9104-git-svn-follow-parent.sh index c7d8e0bf00..5cf2ef4b8b 100755 --- a/t/t9104-git-svn-follow-parent.sh +++ b/t/t9104-git-svn-follow-parent.sh @@ -4,6 +4,8 @@ # test_description='git svn fetching' + +TEST_FAILS_SANITIZE_LEAK=true . ./lib-git-svn.sh test_expect_success 'initialize repo' ' diff --git a/t/t9106-git-svn-commit-diff-clobber.sh b/t/t9106-git-svn-commit-diff-clobber.sh index aec45bca3b..3cab0b9720 100755 --- a/t/t9106-git-svn-commit-diff-clobber.sh +++ b/t/t9106-git-svn-commit-diff-clobber.sh @@ -2,6 +2,8 @@ # # Copyright (c) 2006 Eric Wong test_description='git svn commit-diff clobber' + +TEST_FAILS_SANITIZE_LEAK=true . ./lib-git-svn.sh test_expect_success 'initialize repo' ' diff --git a/t/t9115-git-svn-dcommit-funky-renames.sh b/t/t9115-git-svn-dcommit-funky-renames.sh index 743fbe1fe4..419f055721 100755 --- a/t/t9115-git-svn-dcommit-funky-renames.sh +++ b/t/t9115-git-svn-dcommit-funky-renames.sh @@ -5,6 +5,7 @@ test_description='git svn dcommit can commit renames of files with ugly names' +TEST_FAILS_SANITIZE_LEAK=true . ./lib-git-svn.sh test_expect_success 'load repository with strange names' ' diff --git a/t/t9116-git-svn-log.sh b/t/t9116-git-svn-log.sh index 0a9f1ef366..34f6c80dea 100755 --- a/t/t9116-git-svn-log.sh +++ b/t/t9116-git-svn-log.sh @@ -4,6 +4,8 @@ # test_description='git svn log tests' + +TEST_FAILS_SANITIZE_LEAK=true . ./lib-git-svn.sh test_expect_success 'setup repository and import' ' diff --git a/t/t9122-git-svn-author.sh b/t/t9122-git-svn-author.sh index 9e8fe38e7e..527ba3d293 100755 --- a/t/t9122-git-svn-author.sh +++ b/t/t9122-git-svn-author.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='git svn authorship' + +TEST_FAILS_SANITIZE_LEAK=true . ./lib-git-svn.sh test_expect_success 'setup svn repository' ' diff --git a/t/t9127-git-svn-partial-rebuild.sh b/t/t9127-git-svn-partial-rebuild.sh index 2e4789d061..90b1b30dde 100755 --- a/t/t9127-git-svn-partial-rebuild.sh +++ b/t/t9127-git-svn-partial-rebuild.sh @@ -4,6 +4,8 @@ # test_description='git svn partial-rebuild tests' + +TEST_FAILS_SANITIZE_LEAK=true . ./lib-git-svn.sh test_expect_success 'initialize svnrepo' ' diff --git a/t/t9129-git-svn-i18n-commitencoding.sh b/t/t9129-git-svn-i18n-commitencoding.sh index 01e1e8a8f7..185248a4cd 100755 --- a/t/t9129-git-svn-i18n-commitencoding.sh +++ b/t/t9129-git-svn-i18n-commitencoding.sh @@ -4,6 +4,7 @@ test_description='git svn honors i18n.commitEncoding in config' +TEST_FAILS_SANITIZE_LEAK=true . ./lib-git-svn.sh compare_git_head_with () { diff --git a/t/t9132-git-svn-broken-symlink.sh b/t/t9132-git-svn-broken-symlink.sh index aeceffaf7b..4d8d0584b7 100755 --- a/t/t9132-git-svn-broken-symlink.sh +++ b/t/t9132-git-svn-broken-symlink.sh @@ -2,6 +2,7 @@ test_description='test that git handles an svn repository with empty symlinks' +TEST_FAILS_SANITIZE_LEAK=true . ./lib-git-svn.sh test_expect_success 'load svn dumpfile' ' svnadmin load "$rawsvnrepo" < Date: Wed, 13 Apr 2022 22:01:48 +0200 Subject: [PATCH 19/27] revisions API: have release_revisions() release "filter" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extend the the release_revisions() function so that it frees the "filter" in the "struct rev_info". This in combination with a preceding change to free "cmdline" means that we can mark another set of tests as passing under "TEST_PASSES_SANITIZE_LEAK=true". The "filter" member was added recently in ffaa137f646 (revision: put object filter into struct rev_info, 2022-03-09), and this fixes leaks intruded in the subsequent leak 7940941de1f (pack-objects: use rev.filter when possible, 2022-03-09) and 105c6f14ad3 (bundle: parse filter capability, 2022-03-09). The "builtin/pack-objects.c" leak in 7940941de1f was effectively with us already, but the variable was referred to by a "static" file-scoped variable. The "bundle.c " leak in 105c6f14ad3 was newly introduced with the new "filter" feature for bundles. The "t5600-clone-fail-cleanup.sh" change here to add "TEST_PASSES_SANITIZE_LEAK=true" is one of the cases where run-command.c in not carrying the abort() exit code upwards would have had that test passing before, but now it *actually* passes[1]. We should fix the lack of 1=1 mapping of SANITIZE=leak testing to actual leaks some other time, but it's an existing edge case, let's just mark the really-passing test as passing for now. 1. https://lore.kernel.org/git/220303.86fsnz5o9w.gmgdl@evledraar.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- revision.c | 1 + t/t1060-object-corruption.sh | 1 + t/t2015-checkout-unborn.sh | 1 + t/t4207-log-decoration-colors.sh | 1 + t/t5301-sliding-window.sh | 2 ++ t/t5313-pack-bounds-checks.sh | 2 ++ t/t5316-pack-delta-depth.sh | 2 ++ t/t5320-delta-islands.sh | 2 ++ t/t5322-pack-objects-sparse.sh | 1 + t/t5506-remote-groups.sh | 1 + t/t5513-fetch-track.sh | 1 + t/t5532-fetch-proxy.sh | 2 ++ t/t5600-clone-fail-cleanup.sh | 1 + t/t5900-repo-selection.sh | 2 ++ t/t6101-rev-parse-parents.sh | 1 + t/t6114-keep-packs.sh | 2 ++ t/t7702-repack-cyclic-alternate.sh | 2 ++ t/t9127-git-svn-partial-rebuild.sh | 1 - 18 files changed, 25 insertions(+), 1 deletion(-) diff --git a/revision.c b/revision.c index de4076e77d..a9d6d3a8dc 100644 --- a/revision.c +++ b/revision.c @@ -2948,6 +2948,7 @@ void release_revisions(struct rev_info *revs) free_commit_list(revs->commits); object_array_clear(&revs->pending); release_revisions_cmdline(&revs->cmdline); + list_objects_filter_release(&revs->filter); release_revisions_mailmap(revs->mailmap); } diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh index e8a58b1589..5b8e47e346 100755 --- a/t/t1060-object-corruption.sh +++ b/t/t1060-object-corruption.sh @@ -2,6 +2,7 @@ test_description='see how we handle various forms of corruption' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # convert "1234abcd" to ".git/objects/12/34abcd" diff --git a/t/t2015-checkout-unborn.sh b/t/t2015-checkout-unborn.sh index a9721215fa..9425aae639 100755 --- a/t/t2015-checkout-unborn.sh +++ b/t/t2015-checkout-unborn.sh @@ -4,6 +4,7 @@ test_description='checkout from unborn branch' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh index b870942498..36ac6aff1e 100755 --- a/t/t4207-log-decoration-colors.sh +++ b/t/t4207-log-decoration-colors.sh @@ -8,6 +8,7 @@ test_description='Test for "git log --decorate" colors' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t5301-sliding-window.sh b/t/t5301-sliding-window.sh index 76f9798ab9..3ccaaeb397 100755 --- a/t/t5301-sliding-window.sh +++ b/t/t5301-sliding-window.sh @@ -4,6 +4,8 @@ # test_description='mmap sliding window tests' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success \ diff --git a/t/t5313-pack-bounds-checks.sh b/t/t5313-pack-bounds-checks.sh index 535313e4dc..cc4cfaa9d3 100755 --- a/t/t5313-pack-bounds-checks.sh +++ b/t/t5313-pack-bounds-checks.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='bounds-checking of access to mmapped on-disk file formats' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh clear_base () { diff --git a/t/t5316-pack-delta-depth.sh b/t/t5316-pack-delta-depth.sh index e9045009a1..eb4ef3dda4 100755 --- a/t/t5316-pack-delta-depth.sh +++ b/t/t5316-pack-delta-depth.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='pack-objects breaks long cross-pack delta chains' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # This mirrors a repeated push setup: diff --git a/t/t5320-delta-islands.sh b/t/t5320-delta-islands.sh index fea92a5777..124d47603d 100755 --- a/t/t5320-delta-islands.sh +++ b/t/t5320-delta-islands.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='exercise delta islands' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # returns true iff $1 is a delta based on $2 diff --git a/t/t5322-pack-objects-sparse.sh b/t/t5322-pack-objects-sparse.sh index d39958c066..770695c927 100755 --- a/t/t5322-pack-objects-sparse.sh +++ b/t/t5322-pack-objects-sparse.sh @@ -4,6 +4,7 @@ test_description='pack-objects object selection using sparse algorithm' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup repo' ' diff --git a/t/t5506-remote-groups.sh b/t/t5506-remote-groups.sh index 8f150c0793..5bac03ede8 100755 --- a/t/t5506-remote-groups.sh +++ b/t/t5506-remote-groups.sh @@ -4,6 +4,7 @@ test_description='git remote group handling' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh mark() { diff --git a/t/t5513-fetch-track.sh b/t/t5513-fetch-track.sh index 65d1e05bd6..c46c4dbaef 100755 --- a/t/t5513-fetch-track.sh +++ b/t/t5513-fetch-track.sh @@ -2,6 +2,7 @@ test_description='fetch follows remote-tracking branches correctly' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t5532-fetch-proxy.sh b/t/t5532-fetch-proxy.sh index 9c2798603b..d664912799 100755 --- a/t/t5532-fetch-proxy.sh +++ b/t/t5532-fetch-proxy.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='fetching via git:// using core.gitproxy' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup remote repo' ' diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh index 34b3df4027..c814afa565 100755 --- a/t/t5600-clone-fail-cleanup.sh +++ b/t/t5600-clone-fail-cleanup.sh @@ -13,6 +13,7 @@ Unless the directory already exists, in which case we clean up only what we wrote. ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh corrupt_repo () { diff --git a/t/t5900-repo-selection.sh b/t/t5900-repo-selection.sh index 14e59c5b3e..a84faac242 100755 --- a/t/t5900-repo-selection.sh +++ b/t/t5900-repo-selection.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='selecting remote repo in ambiguous cases' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh reset() { diff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh index c571fa5179..a3a41c7a3e 100755 --- a/t/t6101-rev-parse-parents.sh +++ b/t/t6101-rev-parse-parents.sh @@ -8,6 +8,7 @@ test_description='Test git rev-parse with different parent options' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_cmp_rev_output () { diff --git a/t/t6114-keep-packs.sh b/t/t6114-keep-packs.sh index 9239d8aa46..44246f8a63 100755 --- a/t/t6114-keep-packs.sh +++ b/t/t6114-keep-packs.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='rev-list with .keep packs' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t7702-repack-cyclic-alternate.sh b/t/t7702-repack-cyclic-alternate.sh index 93b74867ac..f3cdb98eec 100755 --- a/t/t7702-repack-cyclic-alternate.sh +++ b/t/t7702-repack-cyclic-alternate.sh @@ -4,6 +4,8 @@ # test_description='repack involving cyclic alternate' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t9127-git-svn-partial-rebuild.sh b/t/t9127-git-svn-partial-rebuild.sh index 90b1b30dde..97f495bd49 100755 --- a/t/t9127-git-svn-partial-rebuild.sh +++ b/t/t9127-git-svn-partial-rebuild.sh @@ -5,7 +5,6 @@ test_description='git svn partial-rebuild tests' -TEST_FAILS_SANITIZE_LEAK=true . ./lib-git-svn.sh test_expect_success 'initialize svnrepo' ' From f41fb662f57abccf24d036bb0fd4294eb72261af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:49 +0200 Subject: [PATCH 20/27] revisions API: have release_revisions() release "grep_filter" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extend the the release_revisions() function so that it frees the "grep_filter" in the "struct rev_info".This allows us to mark a test as passing under "TEST_PASSES_SANITIZE_LEAK=true". Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- revision.c | 1 + t/t9151-svn-mergeinfo.sh | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/revision.c b/revision.c index a9d6d3a8dc..1db58c3e4d 100644 --- a/revision.c +++ b/revision.c @@ -2950,6 +2950,7 @@ void release_revisions(struct rev_info *revs) release_revisions_cmdline(&revs->cmdline); list_objects_filter_release(&revs->filter); release_revisions_mailmap(revs->mailmap); + free_grep_patterns(&revs->grep_filter); } static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child) diff --git a/t/t9151-svn-mergeinfo.sh b/t/t9151-svn-mergeinfo.sh index 85221d439b..c93a5beab2 100755 --- a/t/t9151-svn-mergeinfo.sh +++ b/t/t9151-svn-mergeinfo.sh @@ -5,7 +5,6 @@ test_description='git-svn svn mergeinfo properties' -TEST_FAILS_SANITIZE_LEAK=true . ./lib-git-svn.sh test_expect_success 'load svn dump' " From 689a8e80dd4f7b3a20be88146bdea833c7759603 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:50 +0200 Subject: [PATCH 21/27] revisions API: have release_revisions() release "prune_data" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extend the the release_revisions() function so that it frees the "prune_data" in the "struct rev_info". This means that any code that calls "release_revisions()" already can get rid of adjacent calls to clear_pathspec(). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- add-interactive.c | 2 -- builtin/add.c | 1 - builtin/stash.c | 2 -- diff-lib.c | 1 - revision.c | 1 + wt-status.c | 2 -- 6 files changed, 1 insertion(+), 8 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 54cdfc8201..6047e8f648 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -568,8 +568,6 @@ static int get_modified_files(struct repository *r, run_diff_files(&rev, 0); } - if (ps) - clear_pathspec(&rev.prune_data); release_revisions(&rev); } hashmap_clear_and_free(&s.file_map, struct pathname_entry, ent); diff --git a/builtin/add.c b/builtin/add.c index 115a26ea63..fc729e14c1 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -142,7 +142,6 @@ int add_files_to_cache(const char *prefix, rev.diffopt.flags.override_submodule_config = 1; rev.max_count = 0; /* do not compare unmerged paths with stage #2 */ run_diff_files(&rev, DIFF_RACY_IS_MODIFIED); - clear_pathspec(&rev.prune_data); release_revisions(&rev); return !!data.add_errors; } diff --git a/builtin/stash.c b/builtin/stash.c index 7c9c5751f5..e6b4c12ebb 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -1064,7 +1064,6 @@ static int check_changes_tracked_files(const struct pathspec *ps) } done: - clear_pathspec(&rev.prune_data); release_revisions(&rev); return ret; } @@ -1276,7 +1275,6 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps done: discard_index(&istate); - clear_pathspec(&rev.prune_data); release_revisions(&rev); strbuf_release(&diff_output); remove_path(stash_index_path.buf); diff --git a/diff-lib.c b/diff-lib.c index 0f16281253..298265e5b5 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -641,7 +641,6 @@ int do_diff_cache(const struct object_id *tree_oid, struct diff_options *opt) if (diff_cache(&revs, tree_oid, NULL, 1)) exit(128); - clear_pathspec(&revs.prune_data); release_revisions(&revs); return 0; } diff --git a/revision.c b/revision.c index 1db58c3e4d..6f444eaaeb 100644 --- a/revision.c +++ b/revision.c @@ -2949,6 +2949,7 @@ void release_revisions(struct rev_info *revs) object_array_clear(&revs->pending); release_revisions_cmdline(&revs->cmdline); list_objects_filter_release(&revs->filter); + clear_pathspec(&revs->prune_data); release_revisions_mailmap(revs->mailmap); free_grep_patterns(&revs->grep_filter); } diff --git a/wt-status.c b/wt-status.c index a14fad1e03..61e0c1022f 100644 --- a/wt-status.c +++ b/wt-status.c @@ -616,7 +616,6 @@ static void wt_status_collect_changes_worktree(struct wt_status *s) rev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score; copy_pathspec(&rev.prune_data, &s->pathspec); run_diff_files(&rev, 0); - clear_pathspec(&rev.prune_data); release_revisions(&rev); } @@ -664,7 +663,6 @@ static void wt_status_collect_changes_index(struct wt_status *s) copy_pathspec(&rev.prune_data, &s->pathspec); run_diff_index(&rev, 1); release_revisions(&rev); - clear_pathspec(&rev.prune_data); } static int add_file_to_list(const struct object_id *oid, From ab1f6926e90e00ac2ea649e67355f0a0669c0541 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:51 +0200 Subject: [PATCH 22/27] revisions API: clear "boundary_commits" in release_revisions() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clear the "boundary_commits" object_array in release_revisions(). This makes a few more tests pass under SANITIZE=leak, including "t/t4126-apply-empty.sh" which started failed as an UNLEAK() in cmd_format_patch() was removed in a preceding commit. This also re-marks the various tests relying on "git format-patch" as passing under "SANITIZE=leak", in the preceding "revisions API users: use release_revisions() in builtin/log.c" commit those were marked as failing as we removed the UNLEAK(rev) from cmd_format_patch() in "builtin/log.c". Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- revision.c | 1 + t/t4021-format-patch-numbered.sh | 1 + t/t4028-format-patch-mime-headers.sh | 2 ++ t/t4036-format-patch-signer-mime.sh | 1 + t/t4122-apply-symlink-inside.sh | 1 + t/t4126-apply-empty.sh | 1 + t/t6110-rev-list-sparse.sh | 1 + t/t9001-send-email.sh | 1 + t/t9116-git-svn-log.sh | 1 - 9 files changed, 9 insertions(+), 1 deletion(-) diff --git a/revision.c b/revision.c index 6f444eaaeb..5aa6dec453 100644 --- a/revision.c +++ b/revision.c @@ -2947,6 +2947,7 @@ void release_revisions(struct rev_info *revs) { free_commit_list(revs->commits); object_array_clear(&revs->pending); + object_array_clear(&revs->boundary_commits); release_revisions_cmdline(&revs->cmdline); list_objects_filter_release(&revs->filter); clear_pathspec(&revs->prune_data); diff --git a/t/t4021-format-patch-numbered.sh b/t/t4021-format-patch-numbered.sh index 9be65fd444..1219aa226d 100755 --- a/t/t4021-format-patch-numbered.sh +++ b/t/t4021-format-patch-numbered.sh @@ -5,6 +5,7 @@ test_description='Format-patch numbering options' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t4028-format-patch-mime-headers.sh b/t/t4028-format-patch-mime-headers.sh index 204ba673cb..60cb819c42 100755 --- a/t/t4028-format-patch-mime-headers.sh +++ b/t/t4028-format-patch-mime-headers.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='format-patch mime headers and extra headers do not conflict' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'create commit with utf-8 body' ' diff --git a/t/t4036-format-patch-signer-mime.sh b/t/t4036-format-patch-signer-mime.sh index 98d9713d8b..48655bcc78 100755 --- a/t/t4036-format-patch-signer-mime.sh +++ b/t/t4036-format-patch-signer-mime.sh @@ -2,6 +2,7 @@ test_description='format-patch -s should force MIME encoding as needed' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh index aa52de401b..9696537303 100755 --- a/t/t4122-apply-symlink-inside.sh +++ b/t/t4122-apply-symlink-inside.sh @@ -4,6 +4,7 @@ test_description='apply to deeper directory without getting fooled with symlink' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh index 66a7ba8ab8..ece9fae207 100755 --- a/t/t4126-apply-empty.sh +++ b/t/t4126-apply-empty.sh @@ -2,6 +2,7 @@ test_description='apply empty' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t6110-rev-list-sparse.sh b/t/t6110-rev-list-sparse.sh index 13c1da5352..ddefc7f24e 100755 --- a/t/t6110-rev-list-sparse.sh +++ b/t/t6110-rev-list-sparse.sh @@ -4,6 +4,7 @@ test_description='operations that cull histories in unusual ways' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 84d0f40d76..dfa6b20f7a 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -4,6 +4,7 @@ test_description='git send-email' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # May be altered later in the test diff --git a/t/t9116-git-svn-log.sh b/t/t9116-git-svn-log.sh index 34f6c80dea..d74d7b2de6 100755 --- a/t/t9116-git-svn-log.sh +++ b/t/t9116-git-svn-log.sh @@ -5,7 +5,6 @@ test_description='git svn log tests' -TEST_FAILS_SANITIZE_LEAK=true . ./lib-git-svn.sh test_expect_success 'setup repository and import' ' From 81ffbf838070bf68970688e4a11399f58c562cce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:52 +0200 Subject: [PATCH 23/27] revisions API: release "reflog_info" in release revisions() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a missing reflog_walk_info_release() to "reflog-walk.c" and use it in release_revisions(). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- reflog-walk.c | 24 +++++++++++++++++++++++- reflog-walk.h | 1 + revision.c | 1 + t/t0100-previous.sh | 1 + t/t1401-symbolic-ref.sh | 2 ++ t/t1411-reflog-show.sh | 1 + t/t1412-reflog-loop.sh | 2 ++ t/t1415-worktree-refs.sh | 1 + 8 files changed, 32 insertions(+), 1 deletion(-) diff --git a/reflog-walk.c b/reflog-walk.c index 8ac4b284b6..7aa6595a51 100644 --- a/reflog-walk.c +++ b/reflog-walk.c @@ -8,7 +8,7 @@ struct complete_reflogs { char *ref; - const char *short_ref; + char *short_ref; struct reflog_info { struct object_id ooid, noid; char *email; @@ -51,9 +51,16 @@ static void free_complete_reflog(struct complete_reflogs *array) } free(array->items); free(array->ref); + free(array->short_ref); free(array); } +static void complete_reflogs_clear(void *util, const char *str) +{ + struct complete_reflogs *array = util; + free_complete_reflog(array); +} + static struct complete_reflogs *read_complete_reflog(const char *ref) { struct complete_reflogs *reflogs = @@ -116,6 +123,21 @@ void init_reflog_walk(struct reflog_walk_info **info) (*info)->complete_reflogs.strdup_strings = 1; } +void reflog_walk_info_release(struct reflog_walk_info *info) +{ + size_t i; + + if (!info) + return; + + for (i = 0; i < info->nr; i++) + free(info->logs[i]); + string_list_clear_func(&info->complete_reflogs, + complete_reflogs_clear); + free(info->logs); + free(info); +} + int add_reflog_for_walk(struct reflog_walk_info *info, struct commit *commit, const char *name) { diff --git a/reflog-walk.h b/reflog-walk.h index e9e00ffd47..8076f10d9f 100644 --- a/reflog-walk.h +++ b/reflog-walk.h @@ -8,6 +8,7 @@ struct reflog_walk_info; struct date_mode; void init_reflog_walk(struct reflog_walk_info **info); +void reflog_walk_info_release(struct reflog_walk_info *info); int add_reflog_for_walk(struct reflog_walk_info *info, struct commit *commit, const char *name); void show_reflog_message(struct reflog_walk_info *info, int, diff --git a/revision.c b/revision.c index 5aa6dec453..8cd849aa2b 100644 --- a/revision.c +++ b/revision.c @@ -2953,6 +2953,7 @@ void release_revisions(struct rev_info *revs) clear_pathspec(&revs->prune_data); release_revisions_mailmap(revs->mailmap); free_grep_patterns(&revs->grep_filter); + reflog_walk_info_release(revs->reflog_info); } static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child) diff --git a/t/t0100-previous.sh b/t/t0100-previous.sh index 69beb59f62..a16cc3d298 100755 --- a/t/t0100-previous.sh +++ b/t/t0100-previous.sh @@ -5,6 +5,7 @@ test_description='previous branch syntax @{-n}' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'branch -d @{-1}' ' diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh index 132a1b885a..9fb0b90f25 100755 --- a/t/t1401-symbolic-ref.sh +++ b/t/t1401-symbolic-ref.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='basic symbolic-ref tests' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # If the tests munging HEAD fail, they can break detection of diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh index 0bb319b944..3770ceffaf 100755 --- a/t/t1411-reflog-show.sh +++ b/t/t1411-reflog-show.sh @@ -4,6 +4,7 @@ test_description='Test reflog display routines' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' diff --git a/t/t1412-reflog-loop.sh b/t/t1412-reflog-loop.sh index 977603f7f1..ff30874f94 100755 --- a/t/t1412-reflog-loop.sh +++ b/t/t1412-reflog-loop.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='reflog walk shows repeated commits again' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup commits' ' diff --git a/t/t1415-worktree-refs.sh b/t/t1415-worktree-refs.sh index a3e6ea0808..3b531842dd 100755 --- a/t/t1415-worktree-refs.sh +++ b/t/t1415-worktree-refs.sh @@ -2,6 +2,7 @@ test_description='per-worktree refs' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' ' From 6ab75ac83926c154a58104ac832363f128f8ed56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 13 Apr 2022 22:01:53 +0200 Subject: [PATCH 24/27] revisions API: call diff_free(&revs->pruning) in revisions_release() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Call diff_free() on the "pruning" member of "struct rev_info". Doing so makes several tests pass under SANITIZE=leak. This was also the last missing piece that allows us to remove the UNLEAK() in "cmd_diff" and "cmd_diff_index", which allows us to use those commands as a canary for general leaks in the revisions API. See [1] for further rationale, and 886e1084d78 (builtin/: add UNLEAKs, 2017-10-01) for the commit that added the UNLEAK() there. 1. https://lore.kernel.org/git/220218.861r00ib86.gmgdl@evledraar.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/diff-index.c | 1 - builtin/diff.c | 1 - revision.c | 1 + t/t1001-read-tree-m-2way.sh | 1 + t/t1002-read-tree-m-u-2way.sh | 1 + t/t2200-add-update.sh | 1 + t/t4039-diff-assume-unchanged.sh | 1 + t/t4206-log-follow-harder-copies.sh | 2 ++ t/t6131-pathspec-icase.sh | 2 ++ 9 files changed, 9 insertions(+), 2 deletions(-) diff --git a/builtin/diff-index.c b/builtin/diff-index.c index 3a83183c31..7d158af6b6 100644 --- a/builtin/diff-index.c +++ b/builtin/diff-index.c @@ -70,7 +70,6 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) return -1; } result = run_diff_index(&rev, option); - UNLEAK(rev); result = diff_result_code(&rev.diffopt, result); release_revisions(&rev); return result; diff --git a/builtin/diff.c b/builtin/diff.c index dd48336da5..f539132ac6 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -594,7 +594,6 @@ int cmd_diff(int argc, const char **argv, const char *prefix) result = diff_result_code(&rev.diffopt, result); if (1 < rev.diffopt.skip_stat_unmatch) refresh_index_quietly(); - UNLEAK(rev); release_revisions(&rev); UNLEAK(ent); UNLEAK(blob); diff --git a/revision.c b/revision.c index 8cd849aa2b..63f17c085c 100644 --- a/revision.c +++ b/revision.c @@ -2953,6 +2953,7 @@ void release_revisions(struct rev_info *revs) clear_pathspec(&revs->prune_data); release_revisions_mailmap(revs->mailmap); free_grep_patterns(&revs->grep_filter); + diff_free(&revs->pruning); reflog_walk_info_release(revs->reflog_info); } diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh index 0710b1fb1e..516a6112fd 100755 --- a/t/t1001-read-tree-m-2way.sh +++ b/t/t1001-read-tree-m-2way.sh @@ -21,6 +21,7 @@ In the test, these paths are used: yomin - not in H or M ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-read-tree.sh diff --git a/t/t1002-read-tree-m-u-2way.sh b/t/t1002-read-tree-m-u-2way.sh index 46cbd5514a..bd5313caec 100755 --- a/t/t1002-read-tree-m-u-2way.sh +++ b/t/t1002-read-tree-m-u-2way.sh @@ -9,6 +9,7 @@ This is identical to t1001, but uses -u to update the work tree as well. ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-read-tree.sh diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh index 0c38f8e356..be394f1131 100755 --- a/t/t2200-add-update.sh +++ b/t/t2200-add-update.sh @@ -14,6 +14,7 @@ only the updates to dir/sub. Also tested are "git add -u" without limiting, and "git add -u" without contents changes, and other conditions' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t4039-diff-assume-unchanged.sh b/t/t4039-diff-assume-unchanged.sh index 0eb0314a8b..78090e6852 100755 --- a/t/t4039-diff-assume-unchanged.sh +++ b/t/t4039-diff-assume-unchanged.sh @@ -2,6 +2,7 @@ test_description='diff with assume-unchanged entries' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # external diff has been tested in t4020-diff-external.sh diff --git a/t/t4206-log-follow-harder-copies.sh b/t/t4206-log-follow-harder-copies.sh index 4871a5dc92..33ecf82c7f 100755 --- a/t/t4206-log-follow-harder-copies.sh +++ b/t/t4206-log-follow-harder-copies.sh @@ -6,6 +6,8 @@ test_description='Test --follow should always find copies hard in git log. ' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-diff.sh diff --git a/t/t6131-pathspec-icase.sh b/t/t6131-pathspec-icase.sh index 39fc3f6769..770cce026c 100755 --- a/t/t6131-pathspec-icase.sh +++ b/t/t6131-pathspec-icase.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='test case insensitive pathspec limiting' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh if test_have_prereq CASE_INSENSITIVE_FS From 9d5a7df3322551422f257c686f6621a22e0fa731 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 14 Apr 2022 07:56:38 +0200 Subject: [PATCH 25/27] revisions API: have release_revisions() release "date_mode" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extend the the release_revisions() function so that it frees the "date_mode" in the "struct ref_info". This uses the date_mode_release() function added in 974c919d36d (date API: add and use a date_mode_release(), 2022-02-16). As that commit notes "t7004-tag.sh" tests for the leaks that are being fixed here. That test now fails "only" 44 tests, instead of the 46 it failed before this change. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- revision.c | 1 + 1 file changed, 1 insertion(+) diff --git a/revision.c b/revision.c index 63f17c085c..307f41e889 100644 --- a/revision.c +++ b/revision.c @@ -2951,6 +2951,7 @@ void release_revisions(struct rev_info *revs) release_revisions_cmdline(&revs->cmdline); list_objects_filter_release(&revs->filter); clear_pathspec(&revs->prune_data); + date_mode_release(&revs->date_mode); release_revisions_mailmap(revs->mailmap); free_grep_patterns(&revs->grep_filter); diff_free(&revs->pruning); From ae1b383dfa945d98ec3cedd744619c2028d6edf7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 14 Apr 2022 07:56:39 +0200 Subject: [PATCH 26/27] revisions API: have release_revisions() release "topo_walk_info" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refactor the existing reset_topo_walk() into a thin wrapper for a release_revisions_topo_walk_info() + resetting the member to "NULL", and call release_revisions_topo_walk_info() from release_revisions(). This fixes memory leaks that have been with us ever since "topo_walk_info" was added to revision.[ch] in f0d9cc4196a (revision.c: begin refactoring --topo-order logic, 2018-11-01). Due to various other leaks this makes no tests pass in their entirety, but e.g. before this running this on git.git: ./git -P log --pretty=tformat:"%P %H | %s" --parents --full-history --topo-order -3 -- README.md Would report under SANITIZE=leak: SUMMARY: LeakSanitizer: 531064 byte(s) leaked in 6 allocation(s). Now we'll free all of that memory. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- revision.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/revision.c b/revision.c index 307f41e889..0107ac1077 100644 --- a/revision.c +++ b/revision.c @@ -2943,6 +2943,8 @@ static void release_revisions_mailmap(struct string_list *mailmap) free(mailmap); } +static void release_revisions_topo_walk_info(struct topo_walk_info *info); + void release_revisions(struct rev_info *revs) { free_commit_list(revs->commits); @@ -2956,6 +2958,7 @@ void release_revisions(struct rev_info *revs) free_grep_patterns(&revs->grep_filter); diff_free(&revs->pruning); reflog_walk_info_release(revs->reflog_info); + release_revisions_topo_walk_info(revs->topo_walk_info); } static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child) @@ -3468,17 +3471,22 @@ static void compute_indegrees_to_depth(struct rev_info *revs, indegree_walk_step(revs); } -static void reset_topo_walk(struct rev_info *revs) +static void release_revisions_topo_walk_info(struct topo_walk_info *info) { - struct topo_walk_info *info = revs->topo_walk_info; - + if (!info) + return; clear_prio_queue(&info->explore_queue); clear_prio_queue(&info->indegree_queue); clear_prio_queue(&info->topo_queue); clear_indegree_slab(&info->indegree); clear_author_date_slab(&info->author_date); + free(info); +} - FREE_AND_NULL(revs->topo_walk_info); +static void reset_topo_walk(struct rev_info *revs) +{ + release_revisions_topo_walk_info(revs->topo_walk_info); + revs->topo_walk_info = NULL; } static void init_topo_walk(struct rev_info *revs) From 54c8a7c379fc37a847b8a5ec5c419eae171322e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 14 Apr 2022 07:56:40 +0200 Subject: [PATCH 27/27] revisions API: add a TODO for diff_free(&revs->diffopt) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a TODO comment indicating that we should release "diffopt" in release_revisions(). In a preceding commit we started releasing the "pruning" member of the same type, but handling "diffopt" will require us to untangle the "no_free" conditions I added in e900d494dcf (diff: add an API for deferred freeing, 2021-02-11). Let's leave a TODO comment to that effect, and so that we don't forget refactor code that was changed to use release_revisions() in earlier commits to stop using the "diffopt" member after a call to release_revisions(). This works currently, but would become a logic error as soon as we started freeing "diffopt". Doing that change now doesn't harm anything, and future-proofs us against a later change to release_revisions(). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- diff-lib.c | 4 +++- revision.c | 1 + wt-status.c | 6 ++++-- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/diff-lib.c b/diff-lib.c index 298265e5b5..7eb66a417a 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -651,6 +651,7 @@ int index_differs_from(struct repository *r, { struct rev_info rev; struct setup_revision_opt opt; + unsigned has_changes; repo_init_revisions(r, &rev, NULL); memset(&opt, 0, sizeof(opt)); @@ -662,8 +663,9 @@ int index_differs_from(struct repository *r, diff_flags_or(&rev.diffopt.flags, flags); rev.diffopt.ita_invisible_in_index = ita_invisible_in_index; run_diff_index(&rev, 1); + has_changes = rev.diffopt.flags.has_changes; release_revisions(&rev); - return (rev.diffopt.flags.has_changes != 0); + return (has_changes != 0); } static struct strbuf *idiff_prefix_cb(struct diff_options *opt, void *data) diff --git a/revision.c b/revision.c index 0107ac1077..58d6212221 100644 --- a/revision.c +++ b/revision.c @@ -2956,6 +2956,7 @@ void release_revisions(struct rev_info *revs) date_mode_release(&revs->date_mode); release_revisions_mailmap(revs->mailmap); free_grep_patterns(&revs->grep_filter); + /* TODO (need to handle "no_free"): diff_free(&revs->diffopt) */ diff_free(&revs->pruning); reflog_walk_info_release(revs->reflog_info); release_revisions_topo_walk_info(revs->topo_walk_info); diff --git a/wt-status.c b/wt-status.c index 61e0c1022f..102d904adc 100644 --- a/wt-status.c +++ b/wt-status.c @@ -2545,8 +2545,9 @@ int has_unstaged_changes(struct repository *r, int ignore_submodules) rev_info.diffopt.flags.quick = 1; diff_setup_done(&rev_info.diffopt); result = run_diff_files(&rev_info, 0); + result = diff_result_code(&rev_info.diffopt, result); release_revisions(&rev_info); - return diff_result_code(&rev_info.diffopt, result); + return result; } /** @@ -2578,8 +2579,9 @@ int has_uncommitted_changes(struct repository *r, diff_setup_done(&rev_info.diffopt); result = run_diff_index(&rev_info, 1); + result = diff_result_code(&rev_info.diffopt, result); release_revisions(&rev_info); - return diff_result_code(&rev_info.diffopt, result); + return result; } /**