[1/5] sequencer.c: factor out a function

Commit Message

Phillip Wood Sept. 8, 2021, 1:41 p.m. UTC

From: Phillip Wood <[e-mail]>

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 <[e-mail]>
---
 sequencer.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

Comments

Eric Sunshine Sept. 8, 2021, 5:51 p.m. UTC | #1

On Wed, Sep 8, 2021 at 9:41 AM Phillip Wood via GitGitGadget
<[e-mail]> wrote:
> 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.

s/Lets/Let's/ ... or just drop "Let's" altogether and start with "Move".

Phillip Wood Sept. 9, 2021, 10:10 a.m. UTC | #2

On 08/09/2021 18:51, Eric Sunshine wrote:
> On Wed, Sep 8, 2021 at 9:41 AM Phillip Wood via GitGitGadget
> <[e-mail]> wrote:
>> 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.
> 
> s/Lets/Let's/ ... or just drop "Let's" altogether and start with "Move".

Thanks, well spotted as always

Phillip

Johannes Schindelin Sept. 9, 2021, 10:44 a.m. UTC | #3

Hi Phillip,

On Wed, 8 Sep 2021, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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

>  		}
>
>  		todo_list->current++;
> --
> gitgitgadget
>
>

Patch

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;
 		}
 
 		todo_list->current++;