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

Johannes Schindelin [off-list ref] 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.

Hi Junio,

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

Johannes Schindelin [off-list ref] writes:

quoted

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.

quoted

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