@matheusbsbs / matheus-bagliano / commits / 1878b5edc0

revision.[ch]: provide and start using a release_revisions()

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 <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Ævar Arnfjörð Bjarmason committed Apr 13, 2022 at 22:01 UTC 1878b5edc03c4bf685c7278fc4ced4704c876984

7 files changed +19 -8

builtin/checkout.c

+1 -1

@@ -629,7 +629,7 @@ static void show_local_changes(struct object *head,

629 diff_setup_done(&rev.diffopt);

630 add_pending_object(&rev, head, NULL);

631 run_diff_index(&rev, 0);

632 - object_array_clear(&rev.pending);

632 + release_revisions(&rev);

633 }

634

635 static void describe_detached_head(const char *msg, struct commit *commit)

builtin/stash.c

+2 -3

@@ -1047,7 +1047,6 @@ static int check_changes_tracked_files(const struct pathspec *ps)

1047 goto done;

1048 }

1049

1050 - object_array_clear(&rev.pending);

1050 result = run_diff_files(&rev, 0);

1051 if (diff_result_code(&rev.diffopt, result)) {

1052 ret = 1;

@@ -1056,6 +1055,7 @@ static int check_changes_tracked_files(const struct pathspec *ps)

1055

1056 done:

1057 clear_pathspec(&rev.prune_data);

1058 + release_revisions(&rev);

1059 return ret;

1060 }

1061

@@ -1266,9 +1266,8 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps

1266

1267 done:

1268 discard_index(&istate);

1269 - UNLEAK(rev);

1270 - object_array_clear(&rev.pending);

1269 clear_pathspec(&rev.prune_data);

1270 + release_revisions(&rev);

1271 strbuf_release(&diff_output);

1272 remove_path(stash_index_path.buf);

1273 return ret;

diff-lib.c

+1 -1

@@ -662,7 +662,7 @@ int index_differs_from(struct repository *r,

662 diff_flags_or(&rev.diffopt.flags, flags);

663 rev.diffopt.ita_invisible_in_index = ita_invisible_in_index;

664 run_diff_index(&rev, 1);

665 - object_array_clear(&rev.pending);

665 + release_revisions(&rev);

666 return (rev.diffopt.flags.has_changes != 0);

667 }

668

range-diff.c

+1 -1

@@ -596,6 +596,6 @@ int is_range_diff_range(const char *arg)

596 }

597

598 free(copy);

599 - object_array_clear(&revs.pending);

599 + release_revisions(&revs);

600 return negative > 0 && positive > 0;

601 }

revision.c

@@ -2922,6 +2922,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s

2922 return left;

2923 }

2924

2925 +void release_revisions(struct rev_info *revs)

2926 +{

2927 + object_array_clear(&revs->pending);

2928 +}

2929 +

2930 static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)

2931 {

2932 struct commit_list *l = xcalloc(1, sizeof(*l));

revision.h

@@ -377,6 +377,12 @@ void repo_init_revisions(struct repository *r,

377 int setup_revisions(int argc, const char **argv, struct rev_info *revs,

378 struct setup_revision_opt *);

379

380 +/**

381 + * Free data allocated in a "struct rev_info" after it's been

382 + * initialized with repo_init_revisions().

383 + */

384 +void release_revisions(struct rev_info *revs);

385 +

386 void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,

387 const struct option *options,

388 const char * const usagestr[]);

wt-status.c

+3 -2

@@ -662,7 +662,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)

662

663 copy_pathspec(&rev.prune_data, &s->pathspec);

664 run_diff_index(&rev, 1);

665 - object_array_clear(&rev.pending);

665 + release_revisions(&rev);

666 clear_pathspec(&rev.prune_data);

667 }

668

@@ -2545,6 +2545,7 @@ int has_unstaged_changes(struct repository *r, int ignore_submodules)

2545 rev_info.diffopt.flags.quick = 1;

2546 diff_setup_done(&rev_info.diffopt);

2547 result = run_diff_files(&rev_info, 0);

2548 + release_revisions(&rev_info);

2549 return diff_result_code(&rev_info.diffopt, result);

2550 }

2551

@@ -2577,7 +2578,7 @@ int has_uncommitted_changes(struct repository *r,

2578

2579 diff_setup_done(&rev_info.diffopt);

2580 result = run_diff_index(&rev_info, 1);

2580 - object_array_clear(&rev_info.pending);

2581 + release_revisions(&rev_info);

2582 return diff_result_code(&rev_info.diffopt, result);

2583 }

2584