leak_pending: use `object_array_clear()`, not `free()`

Setting `leak_pending = 1` tells `prepare_revision_walk()` not to
release the `pending` array, and makes that the caller's responsibility.
See 4a43d374f (revision: add leak_pending flag, 2011-10-01) and
353f5657a (bisect: use leak_pending flag, 2011-10-01).

Commit 1da1e07c8 (clean up name allocation in prepare_revision_walk,
2014-10-15) fixed a memory leak in `prepare_revision_walk()` by
switching from `free()` to `object_array_clear()`. However, where we use
the `leak_pending`-mechanism, we're still only calling `free()`.

Use `object_array_clear()` instead. Copy some helpful comments from
353f5657a to the other callers that we update to clarify the memory
responsibilities, and to highlight that the commits are not affected
when we clear the array -- it is indeed correct to both tidy up the
commit flags and clear the object array.

Document `leak_pending` in revision.h to help future users get this
right.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Reviewed-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/bisect.c b/bisect.c
index a9fd9fbc61a661ab19e18713b115f868daab2f98..fc797f6aea90597397f8f25753db0da439848449 100644 (file)
--- a/bisect.c
+++ b/bisect.c
@@ -826,7 +826,8 @@ static int check_ancestors(const char *prefix)
 
        /* Clean up objects used, as they will be reused. */
        clear_commit_marks_for_object_array(&pending_copy, ALL_REV_FLAGS);
-       free(pending_copy.objects);
+
+       object_array_clear(&pending_copy);
 
        return res;
 }

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2d75ac66c7f8447e846e0bc38072b76bddc3672b..52f1b67708b98a199efa842360faf1b065a37284 100644 (file)
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -796,9 +796,14 @@ static void orphaned_commit_warning(struct commit *old, struct commit *new)
        for_each_ref(add_pending_uninteresting_ref, &revs);
        add_pending_oid(&revs, "HEAD", &new->object.oid, UNINTERESTING);
 
+       /* Save pending objects, so they can be cleaned up later. */
        refs = revs.pending;
        revs.leak_pending = 1;
 
+       /*
+        * prepare_revision_walk (together with .leak_pending = 1) makes us
+        * the sole owner of the list of pending objects.
+        */
        if (prepare_revision_walk(&revs))
                die(_("internal error in revision walk"));
        if (!(old->object.flags & UNINTERESTING))
@@ -806,8 +811,10 @@ static void orphaned_commit_warning(struct commit *old, struct commit *new)
        else
                describe_detached_head(_("Previous HEAD position was"), old);
 
+       /* Clean up objects used, as they will be reused. */
        clear_commit_marks_for_object_array(&refs, ALL_REV_FLAGS);
-       free(refs.objects);
+
+       object_array_clear(&refs);
 }
 
 static int switch_branches(const struct checkout_opts *opts,

diff --git a/bundle.c b/bundle.c
index d15db03c84556af8a47783f4d4ee76379d721020..c092d5d68f32652c229fe232af72d02111f51de7 100644 (file)
--- a/bundle.c
+++ b/bundle.c
@@ -157,9 +157,14 @@ int verify_bundle(struct bundle_header *header, int verbose)
        req_nr = revs.pending.nr;
        setup_revisions(2, argv, &revs, NULL);
 
+       /* Save pending objects, so they can be cleaned up later. */
        refs = revs.pending;
        revs.leak_pending = 1;
 
+       /*
+        * prepare_revision_walk (together with .leak_pending = 1) makes us
+        * the sole owner of the list of pending objects.
+        */
        if (prepare_revision_walk(&revs))
                die(_("revision walk setup failed"));
 
@@ -176,8 +181,10 @@ int verify_bundle(struct bundle_header *header, int verbose)
                                refs.objects[i].name);
                }
 
+       /* Clean up objects used, as they will be reused. */
        clear_commit_marks_for_object_array(&refs, ALL_REV_FLAGS);
-       free(refs.objects);
+
+       object_array_clear(&refs);
 
        if (verbose) {
                struct ref_list *r;

diff --git a/revision.h b/revision.h
index bc18487d6fff5e86dff60748c084995a77d2b588..3162cc78e8d160c9ba77e2045272b32837527dd9 100644 (file)
--- a/revision.h
+++ b/revision.h
@@ -149,6 +149,17 @@ struct rev_info {
                        date_mode_explicit:1,
                        preserve_subject:1;
        unsigned int    disable_stdin:1;
+       /*
+        * Set `leak_pending` to prevent `prepare_revision_walk()` from clearing
+        * the array of pending objects (`pending`). It will still forget about
+        * the array and its entries, so they really are leaked. This can be
+        * useful if the `struct object_array` `pending` is copied before
+        * calling `prepare_revision_walk()`. By setting `leak_pending`, you
+        * effectively claim ownership of the old array, so you should most
+        * likely call `object_array_clear(&pending_copy)` once you are done.
+        * Observe that this is about ownership of the array and its entries,
+        * not the commits referenced by those entries.
+        */
        unsigned int    leak_pending:1;
        /* --show-linear-break */
        unsigned int    track_linear:1,