| Message ID | 20190925201315.19722-5-alban.gruin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/5] sequencer: update `total_nr' when adding an item to a todo list | expand |
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'. > > 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) { > + oidcpy(&replay.squash_onto, opts->squash_onto); > + replay.have_squash_onto = 1; > + } > + > return replay; > } > >
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; > > } > > > > > >
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; >>> } >>> >>> >> >>
Hi Phillip, On Wed, 2 Oct 2019, Phillip Wood wrote: > On 02/10/2019 09:16, Johannes Schindelin wrote: > > > > 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. Ooops, you're right. The `.` in `replay.have_squash_onto` should have been a strong hint, eh? Thanks, Dscho > > 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; > > > > } > > > > > > > > > > > > > > >
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) { + oidcpy(&replay.squash_onto, opts->squash_onto); + replay.have_squash_onto = 1; + } + return replay; }
get_replay_opts() did not fill `squash_onto' if possible, meaning 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'. Let’s change that. Signed-off-by: Alban Gruin <alban.gruin@gmail.com> --- builtin/rebase.c | 5 +++++ 1 file changed, 5 insertions(+)