Hi Junio
On 24/03/2023 15:49, Junio C Hamano wrote:
Phillip Wood <phillip.wood123@xxxxxxxxx> 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.
@@ -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.
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.
