Re: [PATCH 1/5] sequencer.c: factor out a function
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Sep 9, 2021, 10:44 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2109091241270.59@tvgsbejvaqbjf.bet>
- In-Reply-To
- <53cde4825b408e5fb893bbd9a222e7387d69a408.1631108472.git.gitgitgadget@gmail.com>
Hi Phillip,
On Wed, 8 Sep 2021, Phillip Wood via GitGitGadget wrote:
Show 64 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> This code is heavily indented and obscures the high level logic within
> the loop. Lets move it to its own function before modifying it in the
> next commit.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> sequencer.c | 38 ++++++++++++++++++++++++--------------
> 1 file changed, 24 insertions(+), 14 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 7f07cd00f3f..a248c886c27 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4254,6 +4254,27 @@ static int stopped_at_head(struct repository *r)
>
> }
>
> +static int reread_todo_if_changed(struct repository *r,
> + struct todo_list *todo_list,
> + struct replay_opts *opts)
> +{
> + struct stat st;
> +
> + if (stat(get_todo_path(opts), &st)) {
> + return error_errno(_("could not stat '%s'"),
> + get_todo_path(opts));
> + } else if (match_stat_data(&todo_list->stat, &st)) {
> + /* Reread the todo file if it has changed. */
> + todo_list_release(todo_list);
> + if (read_populate_todo(r, todo_list, opts))
> + return -1; /* message was printed */
> + /* `current` will be incremented on return */
> + todo_list->current = -1;
> + }
> +
> + return 0;
> +}
> +
> static const char rescheduled_advice[] =
> N_("Could not execute the todo command\n"
> "\n"
> @@ -4433,20 +4454,9 @@ static int pick_commits(struct repository *r,
> item->commit,
> arg, item->arg_len,
> opts, res, 0);
> - } else if (is_rebase_i(opts) && check_todo && !res) {
> - struct stat st;
> -
> - if (stat(get_todo_path(opts), &st)) {
> - res = error_errno(_("could not stat '%s'"),
> - get_todo_path(opts));
> - } else if (match_stat_data(&todo_list->stat, &st)) {
> - /* Reread the todo file if it has changed. */
> - todo_list_release(todo_list);
> - if (read_populate_todo(r, todo_list, opts))
> - res = -1; /* message was printed */
> - /* `current` will be incremented below */
> - todo_list->current = -1;
> - }
> + } else if (is_rebase_i(opts) && check_todo && !res &&
> + reread_todo_if_changed(r, todo_list, opts)) {
> + return -1;It definitely looks like a good refactoring, but it does subtly change the behavior. If the `todo` file could not be found, we previously continued after printing out an error, but now we return with an error.
Likewise, if the `todo` file existed but had a parse error, we would print that error _but continue_. This is definitely a bug, if you ask me.
Now, this might be a desirable change in the first place: if an `exec` command removed the `todo` file, changes are that the user wanted to abort, right? And the bug needs to be fixed, too.
It does need to be called out in the commit message, though.
Thank you, Dscho
Show 7 quoted lines
> } > > todo_list->current++; > -- > gitgitgadget > >