Message ID | 20191124174332.30887-5-alban.gruin@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Use complete_action's todo list to do the rebase | expand |
Thanks for the better commit message! Just one note: > When sequencer_continue() is called by complete_action(), `opts' has > been filled by get_replay_opts(). I searched for "get_replay_opts" in complete_action() but couldn't find it, and had to do some searching to realize that what you mean is that whenever complete_action() is called by do_interactive_rebase() in builtin/rebase.c (its only caller), the "opts" argument was filled by get_replay_opts(). Maybe reword as: When do_interactive_rebase() in builtin/rebase.c calls complete_action(), it passes an "opts" argument generated by get_replay_opts(). complete_action() then passes it to sequencer_continue(). > Currently, it does not initialise the > `squash_onto' field (used by the `--root' mode), only > read_populate_opts() does. It’s not a problem yet since > sequencer_continue() calls it before pick_commits(), but it would lead > to incorrect results once complete_action() is modified to call > pick_commits() directly. > > Let’s change that. The rest is clear; thanks. I would like to make it explicit that pick_commits() uses "squash_onto", and make the antecedents of all the "it"s clear, so I would write it like this: Currently, get_replay_opts() does not initialize the "squash_onto" field (used by the "--root" mode); only read_populate_opts() does. This field is used by pick_commits(), called by sequencer_continue(). It's not a problem yet since sequencer_continue() currently calls read_populate_opts() before pick_commits(), but it would lead to incorrect results once complete_action() is modified to call pick_commits() directly (bypassing sequencer_continue() and, hence, read_populate_opts()). Let's change that. Also, I think it's better to use the regular quote ' instead of the smart quote ’.
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; }
When sequencer_continue() is called by complete_action(), `opts' has been filled by get_replay_opts(). Currently, it does not initialise the `squash_onto' field (used by the `--root' mode), only read_populate_opts() does. It’s not a problem yet since sequencer_continue() calls it before pick_commits(), but it would lead to incorrect results once complete_action() is modified to call pick_commits() directly. Let’s change that. Signed-off-by: Alban Gruin <alban.gruin@gmail.com> --- builtin/rebase.c | 5 +++++ 1 file changed, 5 insertions(+)