Remi Galan Alfonso <remi.galan-alfonso@xxxxxxxxxxxxxxxxxxxxxxx> writes: > Galan Rémi <remi.galan-alfonso@xxxxxxxxxxxxxxxxxxxxxxx> writes: >> Shouldn't all the checking also be called in a 'rebase --continue', >> considering that it can be called after a 'rebase --edit-todo' ? >> (Right now it is only called after closing the editor in 'rebase -i') > > What's your opinion on it? > > Short example: > > 'git rebase -i HEAD~2' > pick commit_sha_1 commit_msg_1 > tick commit_sha_2 commit_msg_2 > An error is raised before anything is done. > 'git rebase --edit-todo' > pick commit_sha_1 commit_msg_1 > tick commit_sha_2 commit_msg_2 > (nothing changed) > 'git rebase --continue' > An error is raised after having picked the first commit. > > The same is relevent with bad sha and missing commits (in fact even > more relevant with missing commits since that would be silent loss of > information). The place where an error can be introduced is (assuming that what "rebase -i" writes out itself is perfect ;-) where we allow the user to edit, so instead of checking before "--continue", I would expect a sane design would check immediately after the editor we spawned returns. The codepath that allows the insn sheet getting edited and the codepath that handles --edit-todo have to do many things the same way (i.e. use collapse_todo_ids to prepare the file for editing, spawn the editor, receive the result and use expand_todo_ids to prepare the file to be used by us), and I would have expected for these two to be using a single helper---and a sanity check like the above can and should be done when we receive the result from the editor, immediately before running expand_todo_ids perhaps. If they are not using such a single helper right now, perhaps that is the preliminary restructuring of the code that is needed? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html