On 21/04/2023 18:22, Junio C Hamano wrote:
"Phillip Wood via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
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);
