From: Phillip Wood Date: Tue, 01 Aug 2023 18:42:15 GMT Subject: Re: [PATCH v3 1/7] rebase -i: move unlink() calls Message-ID: <48fefaf0-6a31-0518-3424-73d93343f1d8@gmail.com> In-Reply-To: On 01/08/2023 18:22, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" writes: > >> From: Phillip Wood >> >> 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 >> --- >> 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);