Re: [PATCH 05/22] sequencer: allow the sequencer to take custody of malloc()ed data

Hi Kuba,

On Mon, 29 Aug 2016, Jakub Narębski wrote:

W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:

quoted

The sequencer is our attempt to lib-ify cherry-pick. Yet it behaves
like a one-shot command when it reads its configuration: memory is
allocated and released only when the command exits.

This is kind of okay for git-cherry-pick, which *is* a one-shot
command. All the work to make the sequencer its work horse was done to
allow using the functionality as a library function, though, including
proper clean-up after use.

This patch introduces an API to pass the responsibility of releasing
certain memory to the sequencer.

So how this API would be / is meant to be used?

I added an example to the commit message.

Would sequencer as a library function be called multiple times,
or only once?

The point of a library function is that it should not care.

I'm trying to find out how this is solved in other places of Git
code, and I have stumbled upon free_util in string_list...

I wanted this to be flexible enough to take care of any type of data, not
just strings.

And while the string_list has a void *util field, it would be rather silly
to add strings to a string list for the sole purpose of free()ing their
util fields in the end.

(That was the conclusion I came to after a search of my own.)

quoted

+void *sequencer_entrust(struct replay_opts *opts, void *set_me_free_after_use)
+{
+	ALLOC_GROW(opts->owned, opts->owned_nr + 1, opts->owned_alloc);
+	opts->owned[opts->owned_nr++] = set_me_free_after_use;
+
+	return set_me_free_after_use;

I was wondering what this 'set_me_free_after_use' parameter is about;
wouldn't it be more readable if this parameter was called 'owned_data'
or 'owned_ptr'?

If I read "owned_ptr" as a function's parameter, I would assume that the
associated memory is owned by the caller. So I would be puzzled reading
that name.

quoted

 static void remove_sequencer_state(const struct replay_opts *opts)
 {
 	struct strbuf dir = STRBUF_INIT;
+	int i;
+
+	for (i = 0; i < opts->owned_nr; i++)
+		free(opts->owned[i]);

I guess you can remove owned data in any order, regardless if you
store struct or its members first...

Indeed, this is not like a C++ destructor. It's free().

quoted

diff --git a/sequencer.h b/sequencer.h
index c955594..20b708a 100644
--- a/sequencer.h
+++ b/sequencer.h

@@ -43,8 +43,14 @@ struct replay_opts {
 
 	/* Only used by REPLAY_NONE */
 	struct rev_info *revs;
+
+	/* malloc()ed data entrusted to the sequencer */
+	void **owned;
+	int owned_nr, owned_alloc;

I'm not sure about naming conventions for those types of data, but
wouldn't 'owned_data' be a better name?  I could be wrong here...

The convention seemed to be "void *X; int X_nr, X_alloc;", so I stuck with
it.

Thanks for your review!
Johannes