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

From: Jakub Narębski <jnareb@gmail.com>
Date: Aug 29, 2016, 21:59 UTC
Message-ID: <733a899a-470b-79b3-b059-b38313a7057d@gmail.com>
In-Reply-To: <e4e7eab3d0610faa9d3173a585902e50128d8e15.1472457609.git.johannes.schindelin@gmx.de>

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

Show 11 quoted lines

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

Show 21 quoted lines

> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  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'?

Show 9 quoted lines

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

Show 16 quoted lines

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

Show 6 quoted lines

> +
> +void *sequencer_entrust(struct replay_opts *opts, void *set_me_free_after_use);
>  
>  int sequencer_pick_revisions(struct replay_opts *opts);
>  
>