Hi Kuba,

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

> W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
> 
> > 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.)

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

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

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