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