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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 24, 2023, 15:49 UTC
Message-ID: <xmqqfs9u409m.fsf@gitster.g>
In-Reply-To: <9e5093ab-2aa8-16e7-227a-f5c56983be9a@dunelm.org.uk>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 14 quoted lines

>>> 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. I thought the point of this change was to avoid such a failed attempt to be recorded in "done", and if that is the case, we (1) do not record any failing attempts, (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?

Show 17 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.

Show 5 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.

Thanks.