Re: [PATCH] rebase -i: do not update "done" when rescheduling command
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 20, 2023, 17:46 UTC
- Message-ID
- <xmqq4jqfmi2k.fsf@gitster.g>
- In-Reply-To
- <pull.1492.git.1679237337683.gitgitgadget@gmail.com>
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 17 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk> > > As the sequencer executes todo commands it appends them to > .git/rebase-merge/done. This file is used by "git status" to show the > recently executed commands. Unfortunately when a command is rescheduled > the command preceding it is erroneously appended to the "done" file. > This means that when rebase stops after rescheduling "pick B" the "done" > file contains > > pick A > pick B > pick A > > instead of > > pick A > pick B
Here it may not be clear what you meant with the verb "reschedule" to those who weren't closely following the previous discussion that led to this fix.
Is it the same as "the command attempted to execute a step, couldn't complete it (e.g. due to conflicts), and gave control to the end user until they say 'git rebase --continue'"? What cases, other than interrupted step due to conflicts, involve "rescheduling"?
> Note that the rescheduled command will still be appended to the "done" > file again when it is successfully executed. Arguably it would be better > not to do that but fixing it would be more involved.
And without quite understanding what "reschedule" refers to, it is unclear why it is even arguable---it is perfectly sensible that a command that is rescheduled (hence not yet done) would not be sent to 'done'. If a command that was once rescheduled (hence it wasn't finished initially) gets finished now, shouldn't it be sent to 'done'? It is unclear why is it better not to.
> -static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
> +static int save_todo(struct todo_list *todo_list, struct replay_opts *opts,
> + int reschedule)
> {OK, all callers to save_todo() are in pick_commits() that knows what the value of "reschedule" is, and it is passed down to this helper ...
Show 18 quoted lines
> @@ -3389,7 +3390,7 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
> * rebase -i writes "git-rebase-todo" without the currently executing
> * command, appending it to "done" instead.
> */
> - if (is_rebase_i(opts))
> + if (is_rebase_i(opts) && !reschedule)
> next++;
>
> fd = hold_lock_file_for_update(&todo_lock, todo_path, 0);
> @@ -3402,7 +3403,7 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
> if (commit_lock_file(&todo_lock) < 0)
> return error(_("failed to finalize '%s'"), todo_path);
>
> - if (is_rebase_i(opts) && next > 0) {
> + if (is_rebase_i(opts) && !reschedule && next > 0) {
> const char *done = rebase_path_done();
> int fd = open(done, O_CREAT | O_WRONLY | O_APPEND, 0666);
> int ret = 0;... and the change here is quite straight-forward. With reschedule, we do not advance because by definition we haven't finished the step yet. OK.
Show 7 quoted lines
> @@ -4648,7 +4649,7 @@ static int pick_commits(struct repository *r, > const char *arg = todo_item_get_arg(todo_list, item); > int check_todo = 0; > > - if (save_todo(todo_list, opts)) > + if (save_todo(todo_list, opts, 0)) > return -1;
I wonder why we pass a hardcoded 0 here---shouldn't the value match the local variable 'reschedule'? here?
The same question for the other two callers, but I admit that when the second one is called, the local variable "reschedule" is not set...
Show 10 quoted lines
> if (is_rebase_i(opts)) {
> if (item->command != TODO_COMMENT) {
> @@ -4695,8 +4696,7 @@ static int pick_commits(struct repository *r,
> todo_list->current),
> get_item_line(todo_list,
> todo_list->current));
> - todo_list->current--;
> - if (save_todo(todo_list, opts))
> + if (save_todo(todo_list, opts, 1))
> return -1;... yet we call the helper with reschedule set to 1. Puzzled.
Show 8 quoted lines
> @@ -4788,8 +4788,7 @@ static int pick_commits(struct repository *r, > get_item_line_length(todo_list, > todo_list->current), > get_item_line(todo_list, todo_list->current)); > - todo_list->current--; > - if (save_todo(todo_list, opts)) > + if (save_todo(todo_list, opts, 1)) > return -1;
At this point, reschedule is set and passing it instead of 1 would be OK.
Thanks.