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

Commit Message

Phillip Wood Aug. 1, 2023, 3:23 p.m. UTC

From: Phillip Wood <[e-mail]>

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.

Signed-off-by: Phillip Wood <[e-mail]>
---
 sequencer.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Junio C Hamano Aug. 1, 2023, 5:22 p.m. UTC | #1

"Phillip Wood via GitGitGadget" <[e-mail]> writes:

> From: Phillip Wood <[e-mail]>
>
> 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.

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 <[e-mail]>
> ---
>  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);

Phillip Wood Aug. 1, 2023, 6:42 p.m. UTC | #2

On 01/08/2023 18:22, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <[e-mail]> writes:
> 
>> From: Phillip Wood <[e-mail]>
>>
>> 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

> 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 <[e-mail]>
>> ---
>>   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);

Junio C Hamano Aug. 1, 2023, 7:31 p.m. UTC | #3

Phillip Wood <[e-mail]> writes:

> 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.

Specifically, they are not created when we voluntarily leave the
loop via TODO_BREAK.  They are created when we leave the loop via
the other exit paths (e.g. path_message may be created from MERGE_MSG
in error_with_patch() but the control flow to reach error_with_patch()
in the loop would break out of the loop without ever reaching the
TODO_BREAK codepath).

Or something like that?  I didn't follow thru the other two files.

OK.  I am slow to read and understand a patch from more than 3
months ago X-<; sorry for the confusion.

Thanks.

Patch

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);