Re: [PATCH v2 1/6] rebase -i: move unlink() calls
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Apr 27, 2023, 10:15 UTC
- Message-ID
- <5d5413a2-d3fe-4a5b-25e5-d0132034c11f@gmail.com>
- In-Reply-To
- <xmqq8relkv52.fsf@gitster.g>
On 21/04/2023 18:22, Junio C Hamano wrote:
Show 24 quoted lines
> "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
Show 32 quoted lines
> 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);