Re: [PATCH] rebase -i: do not update "done" when rescheduling command

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mar 24, 2023, 16:22 UTC
Message-ID: <5e49f936-5281-6645-7bf7-78e658087c8a@dunelm.org.uk>
In-Reply-To: <xmqqfs9u409m.fsf@gitster.g>

Hi Junio

On 24/03/2023 15:49, Junio C Hamano wrote:

Show 18 quoted lines

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>>> 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.
>>
>> The command is only successfully executed once but may end up in
>> 'done' multiple times. While that means we can see which commands
>> ended up being rescheduled I'm not sure it is very useful and think
>> really we're just cluttering the 'done' file with failed attempts.
> 
> Sorry, but you utterly confused me.

Perhaps I should have not have added that last paragraph to the commit message.

>  I thought the point of this
> change was to avoid such a failed attempt to be recorded in "done",

No. This change fixes a bug where when we add the failed command back into "git-rebase-todo" we accidentally add the previous command to the "done" file. If "pick A" succeeds and "pick B" fails because it would overwrite an untracked file then currently when the rebase stops after the failed "done" will contain

	pick A
	pick B
	pick A

When it should contain

	pick A
	pick B

i.e. the last line should be the last command we tried to execute.

> and if that is the case, we (1) do not record any failing attempts,

unfortunately "done" is updated just before we try and execute the command so all the failing attempts are recorded. I'm not trying to change that in this patch, I mentioned it in the commit message as a suggestion for a future improvement.

> (2) record the successful execution, and (3) will not re-attempt
> once it is successful.  And if all of these three hold, we wont
> clutter 'done' with failed attempts at all, no?

Yes, unfortunately that's not how it works at the moment.

Show 24 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...
>>
>> The rescheduling code is a bit of a mess as rescheduling commands that
>> pick a commit does not use the "reschedule" variable and is handled
>> separately to other commands like "reset", "merge" and "exec" which do
>> use the "reschedule" varibale. I did try and add a preparatory step to
>> fix that but failed to find a good way of doing so.
> 
> I see.  It may be a sign, taken together with the fact that I found
> that it was very hard---even after reading the patch twice---to
> understand the verb "reschedule" in the proposed commit log to
> explain the change, that the concept of "reschedule" in this
> codepath may not be clearly capturing what it is attempting to do in
> the first place.

I'll try and come up with some better wording (if you have any suggestions please let me know). What's happening is that just before we try and execute a command it it removed from "git-rebase-todo" and added to "done". If the command then fails because it would overwrite an untracked file we need to add it back into "git-rebase-todo" before handing control to the user to remove the offending files. When the user runs "git rebase --continue" we'll try and execute the command again. It is the adding the command back into "git-rebase-todo" so that it is executed by "git rebase --continue" that "reschedule" was intended to capture.

The basic problem is that rebase updates "git-rebase-todo" and "done" before it has successfully executed the command (cherry-pick and revert only remove a command from their todo list after it is executed successfully). I fear it may be too late to change that now though.

Show 9 quoted lines

>> The reason I went
>> with hardcoded parameters is that for each call the purpose is fixed
>> and as you noticed the "reschedule" variable is only used for
>> rescheduling "reset", "merge" and "exec". I could expand the commit
>> message or do you think a couple of code comments be more helpful?
> 
> Yeah, at least it sounds like the code deserves a "NEEDSWORK: this
> is messy in such and such way and we need to clean it up to make it
> understandable" comment somehow.

I'll have another think about how we could clean it up, if that fails I'll add a code comment. I'll be offline next week so I'll re-roll after that.

Best Wishes

Phillip

> Thanks.