@matheusbsbs / matheus-bagliano / commits / 2b88fe0603

rebase: fix todo-list rereading

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. [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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Phillip Wood committed Sep 23, 2021 at 15:26 UTC 2b88fe0603a93314b96f94b22e197b7b1b838c80

2 files changed +8 -12

sequencer.c

+8 -11

@@ -2671,7 +2671,6 @@ static int read_populate_todo(struct repository *r,

2671 struct todo_list *todo_list,

2672 struct replay_opts *opts)

2673 {

2674 - struct stat st;

2674 const char *todo_file = get_todo_path(opts);

2675 int res;

2676

@@ -2679,11 +2678,6 @@ static int read_populate_todo(struct repository *r,

2678 if (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)

2679 return -1;

2680

2682 - res = stat(todo_file, &st);

2683 - if (res)

2684 - return error(_("could not stat '%s'"), todo_file);

2685 - fill_stat_data(&todo_list->stat, &st);

2686 -

2681 res = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);

2682 if (res) {

2683 if (is_rebase_i(opts))

@@ -4258,12 +4252,14 @@ static int reread_todo_if_changed(struct repository *r,

4252 struct todo_list *todo_list,

4253 struct replay_opts *opts)

4254 {

4261 - struct stat st;

4255 + int offset;

4256 + struct strbuf buf = STRBUF_INIT;

4257

4263 - if (stat(get_todo_path(opts), &st)) {

4264 - return error_errno(_("could not stat '%s'"),

4265 - get_todo_path(opts));

4266 - } else if (match_stat_data(&todo_list->stat, &st)) {

4258 + if (strbuf_read_file_or_whine(&buf, get_todo_path(opts)) < 0)

4259 + return -1;

4260 + offset = get_item_line_offset(todo_list, todo_list->current + 1);

4261 + if (buf.len != todo_list->buf.len - offset ||

4262 + memcmp(buf.buf, todo_list->buf.buf + offset, buf.len)) {

4263 /* Reread the todo file if it has changed. */

4264 todo_list_release(todo_list);

4265 if (read_populate_todo(r, todo_list, opts))

@@ -4271,6 +4267,7 @@ static int reread_todo_if_changed(struct repository *r,

4267 /* `current` will be incremented on return */

4268 todo_list->current = -1;

4269 }

4270 + strbuf_release(&buf);

4271

4272 return 0;

4273 }

sequencer.h

-1

@@ -116,7 +116,6 @@ struct todo_list {

116 struct todo_item *items;

117 int nr, alloc, current;

118 int done_nr, total_nr;

119 - struct stat_data stat;

119 };

120

121 #define TODO_LIST_INIT { STRBUF_INIT }