From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 27 Jul 2021 19:34:33 GMT
Subject: Re: [PATCH 11/12] builtin/rebase: fix options.strategy memory lifecycle
Message-ID: <b03736e4-af30-7f91-d920-d917fc619d12@gmail.com>
In-Reply-To: <9f298c97-07d6-7117-baab-6a44359c44d2@ahunt.org>

Hi Andrzej

On 25/07/2021 14:03, Andrzej Hunt wrote:
> [...]
>>>> Given that we are
>>>> allocating a copy above I think maybe your alternative approach of
>>>> always freeing opts->strategy would be better.
> 
> I will go down this route for V2. Although on further thought: instead 
> of my original idea of moving the string to replay_opts (and NULL'ing 
> out rebase_options->strategy), I think it's better to create a new copy 
> when populating replay_opts. The move/NULL approach I suggested in V1 
> happens to work OK, but I think it's non-obvious and could break if we 
> ever wanted to use get_replay_opts() more than once - creating separate 
> copies reduces the number of surprises.

Copying the string sounds like a good approach. I've looked at the V2 
patch and it looks fine to me.

Thanks

Phillip