Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks

[Junio C Hamano <gitster@pobox.com>]

Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr>
writes:

> Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> 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?