From: Phillip Wood <phillip.wood123@gmail.com>
Date: Thu, 27 Apr 2023 10:15:39 GMT
Subject: Re: [PATCH v2 1/6] rebase -i: move unlink() calls
Message-ID: <5d5413a2-d3fe-4a5b-25e5-d0132034c11f@gmail.com>
In-Reply-To: <xmqq8relkv52.fsf@gitster.g>

On 21/04/2023 18:22, Junio C Hamano wrote:
> "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 so only need to be removed before
>> starting the loop, not in each iteration.
> 
> I do not doubt your reasoning is correct, but could you explain this
> a bit better?
> 
> I think the reason why others, e.g. author-script, need to be
> removed on every iteration is because the previous iteration that
> called do_pick_commit() can come back successfully after calling
> write_author_script(), and we would want to clear the deck before
> going into the next iteration, so I can guess that you meant by "if
> there are conflicts" that the loop will not iterate to the next step
> after conflicts happened (and these files like "amend" and
> "stopped-sha" may have been written)?  The latter, i.e. the loop
> will not iterate any further, is the more direct reason to justify
> this change, I think, and it would help readers of "git log" to say
> so, instead of forcing them to infer "are conflicts" imply "hence
> loop will stop".

Yes, that's right. I'll expand the commit message when I re-roll.

> Is this a pure clean-up, or will there be behaviour change?  I do
> not think there is with this patch alone, but does this change make
> future steps easier to understand or something?

It is a pure cleanup. I noticed it when adding the unlink() call in the 
next patch, it doesn't really have anything to do with the subject of 
this series, but I felt it was worth doing as a preparation for the next 
patch.

Best Wishes

Phillip

> IOW, the proposed log message may explain why this is not a wrong
> change to make, but it is unclear why this is a good change we want
> to have in this part of the series.
> 
> Thanks.
> 
>> diff --git a/sequencer.c b/sequencer.c
>> index d2c7698c48c..5073ec5902b 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -4639,6 +4639,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);
>> @@ -4662,10 +4666,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);