Re: [PATCH v2 3/6] leak_pending: use `object_array_clear()`, not `free()`

From: Jeff King <peff@peff.net>
Date: Sep 23, 2017, 03:47 UTC
Message-ID: <20170923034717.i65irpbmlxtzwo3h@sigill.intra.peff.net>
In-Reply-To: <d4df674923b0dbc5c2e232cdc56a75205653d275.1506120292.git.martin.agren@gmail.com>

On Sat, Sep 23, 2017 at 01:34:51AM +0200, Martin Ågren wrote:

Show 9 quoted lines

> 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()`.

Thanks for digging up the history here. Reviewing those it seems clear that doing a full object_array_clear() is the right thing.

Show 8 quoted lines

> 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.

This all looks good to me. Since the main use of "leak_pending" seems to be to clear commit marks, I have a feeling that a better API would have been something like:

  /* normal setup and use */
  init_revisions(&rev);
  setup_revisions(&rev, ...);
  prepare_revision_walk(&rev);
  while (get_revision(&rev))
	...;

  /* optional, but sensible if there may be other callers */
  clear_commit_marks(&rev);

  /* actually free any held memory */
  clear_revisions(&rev);

which would imply rev_info holding onto the "old" pending list automatically until we decommission the rev_info in the final step.

But that's obviously a much bigger change. What you have here looks like a nice clean fix to the leak problems.

-Peff