From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 02 Oct 2019 09:32:33 GMT
Subject: Re: [PATCH v1 4/5] rebase: fill `squash_onto' in get_replay_opts()
Message-ID: <94dbb49e-2c11-ce30-5d50-05be19ebb3bb@gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.1910021011010.46@tvgsbejvaqbjf.bet>

Hi Dscho

On 02/10/2019 09:16, Johannes Schindelin wrote:
> Hi,
> 
> On Fri, 27 Sep 2019, Phillip Wood wrote:
> 
>> Hi Alban
>>
>> On 25/09/2019 21:13, Alban Gruin wrote:
 >>> [...]
>>>    builtin/rebase.c | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>>> index e8319d5946..2097d41edc 100644
>>> --- a/builtin/rebase.c
>>> +++ b/builtin/rebase.c
>>> @@ -117,6 +117,11 @@ static struct replay_opts get_replay_opts(const struct
>>> rebase_options *opts)
>>>     if (opts->strategy_opts)
>>>      parse_strategy_opts(&replay, opts->strategy_opts);
>>>    +	if (opts->squash_onto) {
> 
> I guess it does not matter much, but shouldn't this be guarded against
> the case where `replay.squash_onto` was already initialized? Like, `if
> (opts->squash_onto && !replay.have_squash_onto)`?

replay is uninitialized as this function takes a `struct rebase_opitons` 
and returns a new `struct replay_options` based on that.

Best Wishes

Phillip

> 
> Other than that, this patch makes sense to me.
> 
> Ciao,
> Dscho
> 
>>> +		oidcpy(&replay.squash_onto, opts->squash_onto);
>>> +		replay.have_squash_onto = 1;
>>> +	}
>>> +
>>>    	return replay;
>>>    }
>>>
>>>
>>
>>