From: Junio C Hamano Date: Tue, 30 Jun 2015 16:16:22 GMT Subject: Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks Message-ID: In-Reply-To: <1849903896.89424.1435676774069.JavaMail.zimbra@ensimag.grenoble-inp.fr> Remi Galan Alfonso writes: > Galan Rémi 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?