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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Nov 26, 2019, 18:27 UTC
Message-ID: <20191126182734.142503-1-jonathantanmy@google.com>
In-Reply-To: <20191124174332.30887-5-alban.gruin@gmail.com>

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

Show 8 quoted lines

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