From: Jonathan Tan <jonathantanmy@google.com>
To: alban.gruin@gmail.com
Cc: git@vger.kernel.org, Johannes.Schindelin@gmx.de,
phillip.wood@dunelm.org.uk, gitster@pobox.com,
jonathantanmy@google.com
Subject: Re: [PATCH v4 4/5] rebase: fill `squash_onto' in get_replay_opts()
Date: Tue, 26 Nov 2019 10:27:34 -0800 [thread overview]
Message-ID: <20191126182734.142503-1-jonathantanmy@google.com> (raw)
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().
> 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 ’.
next prev parent reply other threads:[~2019-11-26 18:27 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20190925201315.19722-1-alban.gruin@gmail.com>
2019-09-25 20:13 ` [PATCH v1 1/5] sequencer: update `total_nr' when adding an item to a todo list Alban Gruin
2019-10-02 2:10 ` Junio C Hamano
2019-10-02 8:06 ` Johannes Schindelin
2019-10-02 8:59 ` Junio C Hamano
2019-10-02 9:48 ` Johannes Schindelin
2019-10-02 18:03 ` Alban Gruin
2019-09-25 20:13 ` [PATCH v1 2/5] sequencer: update `done_nr' when skipping commands in " Alban Gruin
2019-09-25 20:13 ` [PATCH v1 3/5] sequencer: move the code writing total_nr on the disk to a new function Alban Gruin
2019-09-25 20:13 ` [PATCH v1 4/5] rebase: fill `squash_onto' in get_replay_opts() Alban Gruin
2019-09-27 13:30 ` Phillip Wood
2019-10-02 8:16 ` Johannes Schindelin
2019-10-02 9:32 ` Phillip Wood
2019-10-02 12:06 ` Johannes Schindelin
2019-09-25 20:13 ` [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action() Alban Gruin
2019-09-27 13:26 ` Phillip Wood
2019-10-02 8:20 ` Johannes Schindelin
2019-10-02 18:03 ` Alban Gruin
2019-10-02 2:38 ` Junio C Hamano
2019-10-02 18:37 ` Alban Gruin
2019-10-26 7:47 ` René Scharfe
2019-10-26 11:27 ` Alban Gruin
2019-10-28 1:39 ` Junio C Hamano
2019-10-28 3:20 ` Junio C Hamano
2019-09-25 20:20 ` [PATCH v1 0/5] Use complete_action's todo list to do the rebase Alban Gruin
2019-09-27 13:32 ` [PATCH v1 0/5] Use complete_action’s " Phillip Wood
2019-10-07 9:26 ` [PATCH v2 0/5] Use complete_action's " Alban Gruin
2019-10-07 9:26 ` [PATCH v2 1/5] sequencer: update `total_nr' when adding an item to a todo list Alban Gruin
2019-10-07 9:26 ` [PATCH v2 2/5] sequencer: update `done_nr' when skipping commands in " Alban Gruin
2019-10-07 9:26 ` [PATCH v2 3/5] sequencer: move the code writing total_nr on the disk to a new function Alban Gruin
2019-10-07 9:26 ` [PATCH v2 4/5] rebase: fill `squash_onto' in get_replay_opts() Alban Gruin
2019-10-07 9:26 ` [PATCH v2 5/5] sequencer: directly call pick_commits() from complete_action() Alban Gruin
2019-10-08 2:45 ` [PATCH v2 0/5] Use complete_action's todo list to do the rebase Junio C Hamano
2019-10-08 16:18 ` Alban Gruin
2019-10-14 12:49 ` Johannes Schindelin
2019-11-23 14:37 ` [PATCH v3 " Alban Gruin
2019-11-23 14:37 ` [PATCH v3 1/5] sequencer: update `total_nr' when adding an item to a todo list Alban Gruin
2019-11-23 14:37 ` [PATCH v3 2/5] sequencer: update `done_nr' when skipping commands in " Alban Gruin
2019-11-23 14:37 ` [PATCH v3 3/5] sequencer: move the code writing total_nr on the disk to a new function Alban Gruin
2019-11-23 14:37 ` [PATCH v3 4/5] rebase: fill `squash_onto' in get_replay_opts() Alban Gruin
2019-11-23 14:37 ` [PATCH v3 5/5] sequencer: directly call pick_commits() from complete_action() Alban Gruin
2019-11-24 17:43 ` [PATCH v4 0/5] Use complete_action's todo list to do the rebase Alban Gruin
2019-11-24 17:43 ` [PATCH v4 1/5] sequencer: update `total_nr' when adding an item to a todo list Alban Gruin
2019-11-24 17:43 ` [PATCH v4 2/5] sequencer: update `done_nr' when skipping commands in " Alban Gruin
2019-11-24 17:43 ` [PATCH v4 3/5] sequencer: move the code writing total_nr on the disk to a new function Alban Gruin
2019-11-24 17:43 ` [PATCH v4 4/5] rebase: fill `squash_onto' in get_replay_opts() Alban Gruin
2019-11-26 18:27 ` Jonathan Tan [this message]
2019-11-24 17:43 ` [PATCH v4 5/5] sequencer: directly call pick_commits() from complete_action() Alban Gruin
2019-11-26 18:41 ` Jonathan Tan
2019-11-27 19:56 ` Alban Gruin
2019-11-27 20:09 ` Alban Gruin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191126182734.142503-1-jonathantanmy@google.com \
--to=jonathantanmy@google.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=alban.gruin@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=phillip.wood@dunelm.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.