Hi Phillip,

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

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 54fd3243da ("rebase -i: reread the todo list if `exec` touched it",
> 2017-04-26) sought to reread the todo list after running an exec
> command only if it had been changed. To accomplish this it checks the
> stat data of the todo list after running an exec command to see if it
> has changed. Unfortunately there are two problems, firstly the
> implementation is buggy we actually reread the list after each exec
> which is quadratic in the number of commit lookups and secondly the
> design is predicated on using nanosecond time stamps which are not the
> default.
>
> The implementation bug stems from the fact that we write a new todo
> list to disk before running each command but do not update the stat
> data to reflect this[1].
>
> The design problem is that it is possible for the user to edit the
> todo list without changing its size or inode which means we have to
> rely on the mtime to tell us if it has changed. Unfortunately unless
> git is built with USE_NSEC it is possible for the original and edited
> list to share the same mtime.
>
> Ideally "git rebase --edit-todo" would set a flag that we would then
> check in sequencer.c. Unfortunately this is approach will not work as
> there are scripts in the wild that write to the todo list directly
> without running "git rebase --edit-todo". Instead of relying on stat
> data this patch simply reads the possibly edited todo list and
> compares it to the original with memcmp(). This is much faster than
> reparsing the todo list each time. This patch reduces the time to run
>
>    git rebase -r -xtrue v2.32.0~100 v2.32.0
>
> which runs 419 exec commands by 6.6%. For comparison fixing the
> implementation bug in stat based approach reduces the time by a
> further 1.4% and is indistinguishable from never rereading the todo
> list.

Thank you for fixing my bug _and_ for improving performance,
Dscho

>
> [1] https://lore.kernel.org/git/20191125131833.GD23183@szeder.dev/
>
> Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  sequencer.c | 19 ++++++++-----------
>  sequencer.h |  1 -
>  2 files changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index a248c886c27..d51440ddcd9 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2671,7 +2671,6 @@ static int read_populate_todo(struct repository *r,
>  			      struct todo_list *todo_list,
>  			      struct replay_opts *opts)
>  {
> -	struct stat st;
>  	const char *todo_file = get_todo_path(opts);
>  	int res;
>
> @@ -2679,11 +2678,6 @@ static int read_populate_todo(struct repository *r,
>  	if (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)
>  		return -1;
>
> -	res = stat(todo_file, &st);
> -	if (res)
> -		return error(_("could not stat '%s'"), todo_file);
> -	fill_stat_data(&todo_list->stat, &st);
> -
>  	res = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);
>  	if (res) {
>  		if (is_rebase_i(opts))
> @@ -4258,12 +4252,14 @@ static int reread_todo_if_changed(struct repository *r,
>  				  struct todo_list *todo_list,
>  				  struct replay_opts *opts)
>  {
> -	struct stat st;
> +	int offset;
> +	struct strbuf buf = STRBUF_INIT;
>
> -	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)) {
> +	if (strbuf_read_file_or_whine(&buf, get_todo_path(opts)) < 0)
> +		return -1;
> +	offset = get_item_line_offset(todo_list, todo_list->current + 1);
> +	if (buf.len != todo_list->buf.len - offset ||
> +	    memcmp(buf.buf, todo_list->buf.buf + offset, buf.len)) {
>  		/* Reread the todo file if it has changed. */
>  		todo_list_release(todo_list);
>  		if (read_populate_todo(r, todo_list, opts))
> @@ -4271,6 +4267,7 @@ static int reread_todo_if_changed(struct repository *r,
>  		/* `current` will be incremented on return */
>  		todo_list->current = -1;
>  	}
> +	strbuf_release(&buf);
>
>  	return 0;
>  }
> diff --git a/sequencer.h b/sequencer.h
> index d57d8ea23d7..cdeb0c6be47 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -116,7 +116,6 @@ struct todo_list {
>  	struct todo_item *items;
>  	int nr, alloc, current;
>  	int done_nr, total_nr;
> -	struct stat_data stat;
>  };
>
>  #define TODO_LIST_INIT { STRBUF_INIT }
> --
> gitgitgadget
>
>