Re: [PATCH v4 05/25] sequencer: eventually release memory allocated for the option values

[Johannes Schindelin]

Hi Junio,

On Tue, 18 Oct 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> >> > To remedy that, we now take custody of the option values in question,
> >> > requiring those values to be malloc()ed or strdup()ed
> >> 
> >> That is the approach this patch takes, so "eventually release" in
> >> the title is no longer accurate, I would think.
> >
> > To the contrary, we now free() things in remove_state(), so we still
> > "eventually release" the memory.
> 
> OK.  We call a change to teach remove_state() to free the resource
> does more commonly as "plug leaks"; the word "eventually" gave me an
> impression that we are emphasizing the fact that we do not free(3)
> immediately but lazily do so at the end, hence my response.

I changed the commit subject, hopefully to your liking,
Dscho

Re: [PATCH v4 05/25] sequencer: eventually release memory allocated for the option values

[Junio C Hamano]

Johannes Schindelin  writes:

>> > To remedy that, we now take custody of the option values in question,
>> > requiring those values to be malloc()ed or strdup()ed
>> 
>> That is the approach this patch takes, so "eventually release" in
>> the title is no longer accurate, I would think.
>
> To the contrary, we now free() things in remove_state(), so we still
> "eventually release" the memory.

OK.  We call a change to teach remove_state() to free the resource
does more commonly as "plug leaks"; the word "eventually" gave me an
impression that we are emphasizing the fact that we do not free(3)
immediately but lazily do so at the end, hence my response.

Re: [PATCH v4 05/25] sequencer: eventually release memory allocated for the option values

[Johannes Schindelin]

Hi Junio,

On Mon, 17 Oct 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > 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.
> >
> > To remedy that, we now take custody of the option values in question,
> > requiring those values to be malloc()ed or strdup()ed
> 
> That is the approach this patch takes, so "eventually release" in
> the title is no longer accurate, I would think.

To the contrary, we now free() things in remove_state(), so we still
"eventually release" the memory.

> > Sadly, the current approach makes the code uglier, as we now have to
> > take care to strdup() the values passed via the command-line.
> 
> I obviously disagree with that statement and the _entrust was too
> ugly to live, but it is obviously subjective, and it boils down to
> who has a better taste.  Let's not go there.

Changed.

Thanks,
Dscho

Re: [PATCH v4 05/25] sequencer: eventually release memory allocated for the option values

[Junio C Hamano]

Johannes Schindelin  writes:

> 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.
>
> To remedy that, we now take custody of the option values in question,
> requiring those values to be malloc()ed or strdup()ed

That is the approach this patch takes, so "eventually release" in
the title is no longer accurate, I would think.

> Sadly, the current approach makes the code uglier, as we now have to
> take care to strdup() the values passed via the command-line.

I obviously disagree with that statement and the _entrust was too
ugly to live, but it is obviously subjective, and it boils down to
who has a better taste.  Let's not go there.

> +
> + /* These option values will be free()d */
> + opts->gpg_sign = xstrdup_or_null(opts->gpg_sign);
> + opts->strategy = xstrdup_or_null(opts->strategy);

xstrdup-or-null does make things cleaner.

> +static int git_config_string_dup(char **dest,
> +  const char *var, const char *value)
> +{
> + if (!value)
> + return config_error_nonbool(var);
> + free(*dest);
> + *dest = xstrdup(value);
> + return 0;
> +}

So does this.

[PATCH v4 05/25] sequencer: eventually release memory allocated for the option values

[Johannes Schindelin]

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.

To remedy that, we now take custody of the option values in question,
requiring those values to be malloc()ed or strdup()ed (an alternative
approach, to add a list of pointers to malloc()ed data and to ask the
sequencer to release all of them in the end, was proposed earlier but
rejected during review).

Sadly, the current approach makes the code uglier, as we now have to
take care to strdup() the values passed via the command-line.

Signed-off-by: Johannes Schindelin 
---
 builtin/revert.c |  4 
 sequencer.c  | 26 ++
 sequencer.h  |  6 +++---
 3 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 7365559..ba5a88c 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -174,6 +174,10 @@ static void parse_args(int argc, const char **argv, struct 
replay_opts *opts)
 
if (argc > 1)
usage_with_options(usage_str, options);
+
+   /* These option values will be free()d */
+   opts->gpg_sign = xstrdup_or_null(opts->gpg_sign);
+   opts->strategy = xstrdup_or_null(opts->strategy);
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
diff --git a/sequencer.c b/sequencer.c
index 8d272fb..04c55f2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -117,6 +117,13 @@ static int has_conforming_footer(struct strbuf *sb, struct 
strbuf *sob,
 static void remove_sequencer_state(const struct replay_opts *opts)
 {
struct strbuf dir = STRBUF_INIT;
+   int i;
+
+   free(opts->gpg_sign);
+   free(opts->strategy);
+   for (i = 0; i < opts->xopts_nr; i++)
+   free(opts->xopts[i]);
+   free(opts->xopts);
 
strbuf_addf(, "%s", get_dir(opts));
remove_dir_recursively(, 0);
@@ -280,7 +287,7 @@ static int do_recursive_merge(struct commit *base, struct 
commit *next,
struct merge_options o;
struct tree *result, *next_tree, *base_tree, *head_tree;
int clean;
-   const char **xopt;
+   char **xopt;
static struct lock_file index_lock;
 
hold_locked_index(_lock, 1);
@@ -583,7 +590,8 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
 
commit_list_insert(base, );
commit_list_insert(next, );
-   res |= try_merge_command(opts->strategy, opts->xopts_nr, 
opts->xopts,
+   res |= try_merge_command(opts->strategy,
+opts->xopts_nr, (const char 
**)opts->xopts,
common, sha1_to_hex(head), remotes);
free_commit_list(common);
free_commit_list(remotes);
@@ -783,6 +791,16 @@ static int read_populate_todo(struct commit_list 
**todo_list,
return 0;
 }
 
+static int git_config_string_dup(char **dest,
+const char *var, const char *value)
+{
+   if (!value)
+   return config_error_nonbool(var);
+   free(*dest);
+   *dest = xstrdup(value);
+   return 0;
+}
+
 static int populate_opts_cb(const char *key, const char *value, void *data)
 {
struct replay_opts *opts = data;
@@ -803,9 +821,9 @@ static int populate_opts_cb(const char *key, const char 
*value, void *data)
else if (!strcmp(key, "options.mainline"))
opts->mainline = git_config_int(key, value);
else if (!strcmp(key, "options.strategy"))
-   git_config_string(>strategy, key, value);
+   git_config_string_dup(>strategy, key, value);
else if (!strcmp(key, "options.gpg-sign"))
-   git_config_string(>gpg_sign, key, value);
+   git_config_string_dup(>gpg_sign, key, value);
else if (!strcmp(key, "options.strategy-option")) {
ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
opts->xopts[opts->xopts_nr++] = xstrdup(value);
diff --git a/sequencer.h b/sequencer.h
index dd4d33a..8453669 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -34,11 +34,11 @@ struct replay_opts {
 
int mainline;
 
-   const char *gpg_sign;
+   char *gpg_sign;
 
/* Merge strategy */
-   const char *strategy;
-   const char **xopts;
+   char *strategy;
+   char **xopts;
size_t xopts_nr, xopts_alloc;
 
/* Only used by REPLAY_NONE */
-- 
2.10.1.513.g00ef6dd