Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks
Matthieu Moy 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
Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks
Remi Galan Alfonso writes: > Junio C Hamano 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 writes: > 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? > > 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 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
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? -- 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 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? 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
Matthieu Moy 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 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
[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
rebase -i: drop, missing commits and static checks
Changes between versions:
In t3404:
Changed 'test_rebase_end' to 'rebase_setup_and_clean'.
Changed the indentation in 'rebase_setup_and_clean'.
Changed the names of the branches created in my tests (avoid names
like 'tmp').
Added 'test_might_fail' in front of 'git branch -D'.
Remove 'test_config rebase.missingCommitsCheck error' in the last test
('static check of bad SHA-1') because it was useless.
In git-rebase--interactive.sh:
Errors found in commands and SHA-1 (static check) are displayed on the
spot.
I used return values to signal to the calling functions if there is an
error.
The whole while block in 'check_bad_cmd_and_sha' with the return is in a
'(
[...]
)'
block because 'retval' would not have been correctly affected when
getting out of the loop since the while is in a pipe.
A thought occured to me:
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')
[PATCHv7 1/3] git-rebase -i: add command "drop" to remove a commit
[PATCHv7 2/3] git rebase -i: warn about removed commits
[PATCHv7 3/3] git rebase -i: add static check for commands and SHA-1
Rémi
(It seems that with 'send-email', 1 time out of 2 I get:
'5.7.8 Error: authentication failed: authentication failure')
--
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
