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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 17, 2016, 19:06 UTC
Message-ID: <xmqq4m4avlr7.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <b771bfbc88734b74acda0c377446e984ca81fd9c.1476450940.git.johannes.schindelin@gmx.de>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

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

Show 9 quoted lines

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