Matthieu Moy <matthieu....@grenoble-inp.fr> writes: > Remi Galan Alfonso <remi.galan-alfo...@ensimag.grenoble-inp.fr> writes: > > > Galan Rémi <remi.galan-alfo...@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? > > It would probably be better to run the check when running "git rebase > --continue". This way, we would have a guarantee that the todo-list is > syntactically correct when going through it. > > Just checking after --edit-todo would not guarantee that:
That's actually what I had in mind, my message wasn't clear enough, my bad. > But in any case, the most important is the initial edition. It covers > 99.9% of use-cases. So, I'd say: if you have time to add the checks at > the other relevant places, good, but not doing it shouldn't block the > inclusion of the series. Agreed, that's something that can be done in a future patch without interfering with this one. Junio C Hamano <gits...@pobox.com> writes: > 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. Makes sense but we would have the problem mentioned by Matthieu: > Warning: the command isn't recognized ... > > # Hmm, let's ignore that warning > $ git rebase --continue While in most cases it would be irrelevant (it would be the user that ignored the error on purpose), I can imagine the following situation: - 'git rebase --edit-todo' - get an error - go do something else, forgot where I was when I come back - 'git status', "you are in the middle of a rebasing" - 'git rebase --continue' > 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? Good point, maybe I'll try doing that at a later date, however as Matthieu said, I think it doesn't hurt to add this patch (if there is no further corrections to do) and do additional things in a later patch. Thanks, Rémi -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
