Re: [PATCH v3 1/7] rebase -i: move unlink() calls

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Aug 1, 2023, 18:42 UTC
Message-ID: <48fefaf0-6a31-0518-3424-73d93343f1d8@gmail.com>
In-Reply-To: <xmqqa5vad6ea.fsf@gitster.g>

On 01/08/2023 18:22, Junio C Hamano wrote:

Show 17 quoted lines

> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> At the start of each iteration the loop that picks commits removes
>> state files from the previous pick. However some of these are only
>> written if there are conflicts and so we break out of the loop after
>> writing them. Therefore they only need to be removed when the rebase
>> continues, not in each iteration.
> 
> "They only need to be removed before the loop" assumes that the SOLE
> purpose of the removal is to give the next iteration a clean slate
> to work with, but is that really the case?  The original unlink's is
> followed by "if TODO_BREAK, break out of the loop", presumably to
> give control back to the end-user.  So three files that were not
> available to the user after "break" are now suddenly visible to
> them.

The files will never exist when the "if TODO_BREAK" is executed because we've removed them before entering the loop and as I tried and seemly failed to explain in the commit message they are only created when we're about to break out of the loop.

Best Wishes

Phillip

Show 42 quoted lines

> Perhaps that is the effect the series wanted to have.  Or it could
> be an unintended side effect that may be a regression.  Or perhaps
> the visibility of these three files (but not others?) is considered
> an implementation detail no users should ever depend on.
> 
> It is hard to tell from the above description and the patch text
> which is the case.  Care to enlighten?
> 
> Thanks.
> 
> 
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   sequencer.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/sequencer.c b/sequencer.c
>> index cc9821ece2c..de66bda9d5b 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -4656,6 +4656,10 @@ static int pick_commits(struct repository *r,
>>   	if (read_and_refresh_cache(r, opts))
>>   		return -1;
>>   
>> +	unlink(rebase_path_message());
>> +	unlink(rebase_path_stopped_sha());
>> +	unlink(rebase_path_amend());
>> +
>>   	while (todo_list->current < todo_list->nr) {
>>   		struct todo_item *item = todo_list->items + todo_list->current;
>>   		const char *arg = todo_item_get_arg(todo_list, item);
>> @@ -4679,10 +4683,7 @@ static int pick_commits(struct repository *r,
>>   						todo_list->total_nr,
>>   						opts->verbose ? "\n" : "\r");
>>   			}
>> -			unlink(rebase_path_message());
>>   			unlink(rebase_path_author_script());
>> -			unlink(rebase_path_stopped_sha());
>> -			unlink(rebase_path_amend());
>>   			unlink(git_path_merge_head(r));
>>   			unlink(git_path_auto_merge(r));
>>   			delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);