From: Phillip Wood Date: Tue, 28 Sep 2021 10:20:54 GMT Subject: Re: [PATCH v2 2/2] rebase: fix todo-list rereading Message-ID: In-Reply-To: On 24/09/2021 17:13, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" writes: > >> .... 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. > > Nice. Is that an egg of Columbus or what ;-) > >> + 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)) > > As we already have the contents of hte file in the buffer, we could > further refactor the code around read_populate_todo() to tell it not > to reopen and reread the rebase-todo file (which risks toctou race), > but that is OK for now, I would think. I did wonder about doing that but decided to punt it for now, I don't think the race is a concern - if another process is writing to the todo list while rebase is picking commits it is asking for trouble already. I suspect doing this will be easier once git-rebase--preserve-merges.sh is gone from master. >> @@ -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; > > Good riddance ;-) Hear, hear! > Will queue. Thanks. >