| Message ID | 1ab1ad2ef07687c25c1d346b5b7b26f38bafe5b9.1690903412.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 36ac861a305720c54f759da814647d5987beb10b |
| Headers | show |
| Series | rebase -i: impove handling of failed commands | expand |
"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. 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);
On 01/08/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 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 <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);
Phillip Wood <phillip.wood123@gmail.com> 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.
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);