Re: [PATCH v1 4/5] rebase: fill `squash_onto' in get_replay_opts()

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 2, 2019, 08:16 UTC
Message-ID: <nycvar.QRO.7.76.6.1910021011010.46@tvgsbejvaqbjf.bet>
In-Reply-To: <66fcd66b-fad2-e5a1-cdd8-fd7b37c4abbf@gmail.com>

Hi,

On Fri, 27 Sep 2019, Phillip Wood wrote:

Show 11 quoted lines

> Hi Alban
>
> On 25/09/2019 21:13, Alban Gruin wrote:
> > get_replay_opts() did not fill `squash_onto' if possible, meaning that
>
> I'm not sure what you mean by 'if possible' here, I think the sentance makes
> sense without that.
>
> > this field should be read from the disk by the sequencer through
> > read_populate_opts().  Without this, calling `pick_commits()' directly
> > will result in incorrect results with `rebase --root'.

I guess I would have an easier time reading the commit message if this paragraph read like this:

	The `get_replay_opts()` function currently does not initialize
	the `squash_onto` field (which is used for the `--root` mode),
	only `read_populate_opts()` does.

	That would lead to incorrect results when calling
	`pick_commits()` without reading the options from disk first.

Show 23 quoted lines

> >
> > Let’s change that.
>
> Good catch
>
> Best Wishes
>
> Phillip
>
> > Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> > ---
> >   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)`?

Other than that, this patch makes sense to me.

Ciao, Dscho

Show 10 quoted lines

> > +		oidcpy(&replay.squash_onto, opts->squash_onto);
> > +		replay.have_squash_onto = 1;
> > +	}
> > +
> >   	return replay;
> >   }
> >
> >
>
>