@matheusbsbs / matheus-bagliano / commits / b2ccdf7fc1

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>

Martin Ågren committed Sep 23, 2017 at 01:34 UTC b2ccdf7fc15e866a883b706540055b5d05fb9aef

4 files changed +29 -3

bisect.c

+2 -1

@@ -826,7 +826,8 @@ static int check_ancestors(const char *prefix)

826

827 /* Clean up objects used, as they will be reused. */

828 clear_commit_marks_for_object_array(&pending_copy, ALL_REV_FLAGS);

829 - free(pending_copy.objects);

829 +

830 + object_array_clear(&pending_copy);

831

832 return res;

833 }

builtin/checkout.c

+8 -1

@@ -796,9 +796,14 @@ static void orphaned_commit_warning(struct commit *old, struct commit *new)

796 for_each_ref(add_pending_uninteresting_ref, &revs);

797 add_pending_oid(&revs, "HEAD", &new->object.oid, UNINTERESTING);

798

799 + /* Save pending objects, so they can be cleaned up later. */

800 refs = revs.pending;

801 revs.leak_pending = 1;

802

803 + /*

804 + * prepare_revision_walk (together with .leak_pending = 1) makes us

805 + * the sole owner of the list of pending objects.

806 + */

807 if (prepare_revision_walk(&revs))

808 die(_("internal error in revision walk"));

809 if (!(old->object.flags & UNINTERESTING))

@@ -806,8 +811,10 @@ static void orphaned_commit_warning(struct commit *old, struct commit *new)

811 else

812 describe_detached_head(_("Previous HEAD position was"), old);

813

814 + /* Clean up objects used, as they will be reused. */

815 clear_commit_marks_for_object_array(&refs, ALL_REV_FLAGS);

810 - free(refs.objects);

816 +

817 + object_array_clear(&refs);

818 }

819

820 static int switch_branches(const struct checkout_opts *opts,

bundle.c

+8 -1

@@ -157,9 +157,14 @@ int verify_bundle(struct bundle_header *header, int verbose)

157 req_nr = revs.pending.nr;

158 setup_revisions(2, argv, &revs, NULL);

159

160 + /* Save pending objects, so they can be cleaned up later. */

161 refs = revs.pending;

162 revs.leak_pending = 1;

163

164 + /*

165 + * prepare_revision_walk (together with .leak_pending = 1) makes us

166 + * the sole owner of the list of pending objects.

167 + */

168 if (prepare_revision_walk(&revs))

169 die(_("revision walk setup failed"));

170

@@ -176,8 +181,10 @@ int verify_bundle(struct bundle_header *header, int verbose)

181 refs.objects[i].name);

182 }

183

184 + /* Clean up objects used, as they will be reused. */

185 clear_commit_marks_for_object_array(&refs, ALL_REV_FLAGS);

180 - free(refs.objects);

186 +

187 + object_array_clear(&refs);

188

189 if (verbose) {

190 struct ref_list *r;

revision.h

+11

@@ -149,6 +149,17 @@ struct rev_info {

149 date_mode_explicit:1,

150 preserve_subject:1;

151 unsigned int disable_stdin:1;

152 + /*

153 + * Set `leak_pending` to prevent `prepare_revision_walk()` from clearing

154 + * the array of pending objects (`pending`). It will still forget about

155 + * the array and its entries, so they really are leaked. This can be

156 + * useful if the `struct object_array` `pending` is copied before

157 + * calling `prepare_revision_walk()`. By setting `leak_pending`, you

158 + * effectively claim ownership of the old array, so you should most

159 + * likely call `object_array_clear(&pending_copy)` once you are done.

160 + * Observe that this is about ownership of the array and its entries,

161 + * not the commits referenced by those entries.

162 + */

163 unsigned int leak_pending:1;

164 /* --show-linear-break */

165 unsigned int track_linear:1,