Re: [PATCH v2 2/2] rebase: fix todo-list rereading

[Phillip Wood <phillip.wood123@gmail.com>]

On 24/09/2021 17:13, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> 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.
>