Re: [PATCH] revision: introduce prepare_revision_walk_extended()

From: Jeff King <peff@peff.net>
Date: Dec 18, 2017, 15:10 UTC
Message-ID: <20171218151043.GA9449@sigill.intra.peff.net>
In-Reply-To: <6ace4f8f-824b-2825-ef18-1fccebb9fb5c@web.de>

On Sat, Dec 16, 2017 at 01:12:16PM +0100, René Scharfe wrote:

Show 9 quoted lines

> prepare_revision_walk() allows callers to take ownership of the array of
> pending objects by setting the rev_info flag "leak_pending" and copying
> the object_array "pending".  They use it to clear commit marks after
> setup is done.  This interface is brittle enough that it requires
> extensive comments.
> 
> Provide an easier way by adding a function that can hand over the array
> to a caller-supplied output parameter and converting all users of the
> flag "leak_pending" to call prepare_revision_walk_extended() instead.

I think this is _better_, but it's still kind of a funny interface.

The root of the matter is that the revision-walking code doesn't clean up after itself. In every case, the caller is just saving these to clean up commit marks, isn't it?

Could we instead have an interface like:

  revs.clear_commit_marks = 1;
  prepare_revision_walk(&revs);
  ...
  finish_revision_walk(&revs);

where that final function would do any cleanup, including clearing the commit marks. I suspect there are other small bits that get leaked because there's not really any "destructor" for a revision walk.

It's not as flexible as this whole "make a copy of the pending tips" thing, but it keeps all of the details abstracted away from the callers.

Alternatively:

Show 5 quoted lines

> +`prepare_revision_walk_extended`::
> +
> +	Like prepare_revision_walk(), but allows callers to take ownership
> +	of the array of pending objects by passing an object_array pointer
> +	as the second parameter; passing NULL clears the array.

What if we just got rid of this function and had callers do:

  object_array_copy(&old_pending, &revs);
  prepare_revision_walk(&revs);
  ...
  clear_commit_marks_for_object_array(&old_pending);

That sidesteps all of the memory ownership issues by just creating a copy. That's less efficient, but I'd be surprised if it matters in practice (we tend to do one or two revisions per process, there don't tend to be a lot of pending tips, and we're really just talking about copying some pointers here).

-Peff