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.gr...@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;
> > }
> >
> >
>
>
