Hi,

On Fri, 27 Sep 2019, Phillip Wood wrote:

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

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

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