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.