| Message ID | 3dfb2c6903bcc61258c72ba5c8e4201c9db2665b.1682089074.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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 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". 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? 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);
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);
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);