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>

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 797681481d10b552e645237c99a8d9bff7df766a..4d9e0bd3ac13ab224e173daed9e00130cf4c82fb 100644 (file)
--- 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 ccdfdab44be6c492a6b1942354afab14850a0c00..8c90db479ce3578a25056c068a27eb716a39c9b9 100644 (file)
--- 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 ca085a03efc280deecd7cefce7ce00f5492a0d4c..d6800274bd51b300847a63965e149b04d9545fab 100644 (file)
--- 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 b72eb9fdbee8d8d7473c973973e88eb09e15f723..39cc010c628f5670b045f9e433d8fc016daa55ad 100644 (file)
--- 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 4963ba7def8d6633102b4b3f05f083b8d2df1ecc..5dd4b2e910c05047177d5d281e047abef606b84e 100644 (file)
--- 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 5bc59c7bfe1e35efe7fb24e93f9a093baa28b658..61c780fc4cd6ccac3e6a4809811717c3708af92f 100644 (file)
--- 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 d33f9272b724b562f5b854d306500e8c0b445c4c..922cf787f95adafa3c5d43be51c8b2fb89833548 100644 (file)
--- 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);
 }