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? From the following
patches (which I shouldn't have to read to understand this one)
it looks like it is about strdup'ed strings from option parsing.
Or would there be something more in the future?
Would sequencer as a library function be called multiple times,
or only once?
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...
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
> ---
> sequencer.c | 13 +++++++++++++
> sequencer.h | 8 +++++++-
> 2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index c4b223b..b5be0f9 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -114,9 +114,22 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
> return 1;
> }
>
> +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'?
> +}
> +
> 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...
> + free(opts->owned);
>
> strbuf_addf(&dir, "%s", get_dir(opts));
> remove_dir_recursively(&dir, 0);
> 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...
> };
> -#define REPLAY_OPTS_INIT { -1, -1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, NULL, NULL, NULL, 0, 0, NULL }
> +#define REPLAY_OPTS_INIT { -1, -1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, NULL, NULL, NULL, 0, 0, NULL, NULL, 0, 0 }
Nb. it is a pity that we cannot use named initializers for structs,
so called designated inits. It would make this macro more readable.
> +
> +void *sequencer_entrust(struct replay_opts *opts, void *set_me_free_after_use);
>
> int sequencer_pick_revisions(struct replay_opts *opts);
>
>