Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks
Matthieu Moy matthieu@imag.fr writes: Hi, Here are a few fixes to squash into the commits of the series. Other than that, the series looks good to me. Junio: do you prefer a reroll or do you want to apply locally? Matthieu Moy (3): fixup! git rebase -i: add static check for commands and SHA-1 fixup! git rebase -i: warn about removed commits fixup! git rebase -i: warn about removed commits git-rebase--interactive.sh| 32 +--- t/t3404-rebase-interactive.sh | 4 ++-- 2 files changed, 23 insertions(+), 13 deletions(-) Thanks for the various fixes ! However, I am still wondering about: 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? 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). 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
Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks
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? 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks
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: $ git rebase --edit-todo # Add some syntax errors, save and quit Warning: the command isn't recognized ... # Hmm, let's ignore that warning $ git rebase --continue Unknown command: ... Please fix this using 'git rebase --edit-todo' 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. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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
Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks
Remi Galan Alfonso remi.galan-alfo...@ensimag.grenoble-inp.fr writes: 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 There's an alternative: $ git rebase --edit-todo # Make mistakes, save and quit Your todo-list has the following issues: - ... Do you want to edit again (no aborts the rebase) [Y/n]? There's a precedent with the 'e' command of git add -p. I have a slight preference for non-interactive commands so I prefer not going this way though. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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
Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks
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
Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks
Matthieu Moy matthieu@grenoble-inp.fr writes: There's an alternative: $ git rebase --edit-todo # Make mistakes, save and quit Your todo-list has the following issues: - ... Do you want to edit again (no aborts the rebase) [Y/n]? There's a precedent with the 'e' command of git add -p. I have a slight preference for non-interactive commands so I prefer not going this way though. edit-todo (and rebase -i in general) is all about going interactive, isn't it? I was almost going to suggest to keep spawning the editor until the user gets it right, but that would infinitely repeat when GIT_EDITOR is set to a broken script, so I didn't ;-). -- 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
[PATCH 0/3] rebase -i: drop, missing commits and static checks
Hi, Here are a few fixes to squash into the commits of the series. Other than that, the series looks good to me. Junio: do you prefer a reroll or do you want to apply locally? Matthieu Moy (3): fixup! git rebase -i: add static check for commands and SHA-1 fixup! git rebase -i: warn about removed commits fixup! git rebase -i: warn about removed commits git-rebase--interactive.sh| 32 +--- t/t3404-rebase-interactive.sh | 4 ++-- 2 files changed, 23 insertions(+), 13 deletions(-) -- 2.5.0.rc0.10.g7792c2a -- 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
