Re: [PATCH RFC v2 03/19] rebase -i: reword executes pre-commit hook on interim commit
Hi Junio,
Junio C Hamano writes:
Fabian Ruch baf...@gmail.com writes:
The to-do list command `reword` replays a commit like `pick` but lets
the user also edit the commit's log message. This happens in two
steps. Firstly, the named commit is cherry-picked. Secondly, the
commit created in the first step is amended using an unchanged index
to edit the log message. The pre-commit hook is meant to verify the
changes introduced by a commit (for instance, catching inserted
trailing white space). Since the committed tree is not changed
between the two steps, do not execute the pre-commit hook in the
second step.
It is not like the second step is built as a child commit of the
result from the first step, recording the same tree without any
change. Why is it a good thing not to run the pre-commit hook (or
other hooks for that matter)? After all, the result of the second
step is what is recorded in the final history; it just feels wrong
not to test that one, even if it were a good idea to test only once.
if I understand correctly, the tree of the (amended) commit isn't tested
at all by git-rebase--interactive because git-cherry-pick, and therefore
pick_one, commits with both the pre-commit and commit-msg hook disabled
(see the git commit -n command line being built in
sequencer.c#run_git_commit since revision 9fa4db5).
The commit --amend we are concerned with here amends using an
untouched tree so that the history ought to record exactly the same
trees as prior to commit --amend. If git-cherry-pick was testing the
tree, then it would be unnecessary to test the tree a second time. Since
git-cherry-pick is not testing as of yet, testing and possibly rejecting
a tree in the second step would actually be wrong. I totally neglected
to look for a test case when I submitted this patch, so I'd like to
supply one late now:
test_expect_success 'reword a commit violating pre-commit' '
mkdir -p .git/hooks
PRE_COMMIT=.git/hooks/pre-commit
cat $PRE_COMMIT EOF
#!/bin/sh
echo running pre-commit
exit 1
EOF
chmod +x $PRE_COMMIT
test_must_fail test_commit file
test_commit --no-verify file
set_fake_editor
FAKE_LINES=pick 1 git rebase -i HEAD^
FAKE_LINES=pick 1 git rebase -i --no-ff HEAD^
FAKE_LINES=reword 1 git rebase -i HEAD^
'
(This addition to t3404-rebase--interactive.sh is based on the test case
rebase a commit violating pre-commit; test_commit --no-verify will
be implemented the obvious way.)
In both cases, it's correct to disable the pre-commit hook when editing
the log message and it just makes sense to test changes where you make
them. Unfortunately, it is not obvious that git commit --amend merely
edits the log message rather than committing a new tree.
For what it's worth, if we were to create an empty child commit and
squash it into the parent instead of amending immediately, then
git-rebase--interactive would disable tree verification in the second
step as well. This behaviour was introduced by commit c5b09fe. Although
the change seems to have been triggered by something completely
different back then, the correctness criterion remains the same, i.e.
recording previously committed trees.
Best regards,
Fabian
Specify the git-commit option `--no-verify` to disable the pre-commit
hook when editing the log message. Because `--no-verify` also skips
the commit-msg hook, execute the hook from within
git-rebase--interactive after the commit is created. Fortunately, the
commit message is still available in `$GIT_DIR/COMMIT_EDITMSG` after
git-commit terminates. Caveat: In case the commit-msg hook finds the
new log message ill-formatted, the user is only notified of the
failed commit-msg hook but the log message is used for the commit
anyway. git-commit ought to offer more fine-grained control over
which hooks are executed.
Signed-off-by: Fabian Ruch baf...@gmail.com
---
git-rebase--interactive.sh | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 689400e..b50770d 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -504,10 +504,19 @@ do_next () {
mark_action_done
do_pick $sha1 $rest
-git commit --allow-empty --amend --no-post-rewrite
${gpg_sign_opt:+$gpg_sign_opt} || {
-warn Could not amend commit after successfully picking
$sha1... $rest
-exit_with_patch $sha1 1
-}
+# TODO: Work around the fact that git-commit lets us
+# disable either both the pre-commit and the commit-msg
+# hook or none. Disable the pre-commit hook because the
+# tree is left unchanged but run the commit-msg hook
+# from here because the log message is altered.
+git commit --allow-empty --amend --no-post-rewrite -n
Re: [PATCH RFC v2 03/19] rebase -i: reword executes pre-commit hook on interim commit
Fabian Ruch baf...@gmail.com writes:
The to-do list command `reword` replays a commit like `pick` but lets
the user also edit the commit's log message. This happens in two
steps. Firstly, the named commit is cherry-picked. Secondly, the
commit created in the first step is amended using an unchanged index
to edit the log message. The pre-commit hook is meant to verify the
changes introduced by a commit (for instance, catching inserted
trailing white space). Since the committed tree is not changed
between the two steps, do not execute the pre-commit hook in the
second step.
It is not like the second step is built as a child commit of the
result from the first step, recording the same tree without any
change. Why is it a good thing not to run the pre-commit hook (or
other hooks for that matter)? After all, the result of the second
step is what is recorded in the final history; it just feels wrong
not to test that one, even if it were a good idea to test only once.
Specify the git-commit option `--no-verify` to disable the pre-commit
hook when editing the log message. Because `--no-verify` also skips
the commit-msg hook, execute the hook from within
git-rebase--interactive after the commit is created. Fortunately, the
commit message is still available in `$GIT_DIR/COMMIT_EDITMSG` after
git-commit terminates. Caveat: In case the commit-msg hook finds the
new log message ill-formatted, the user is only notified of the
failed commit-msg hook but the log message is used for the commit
anyway. git-commit ought to offer more fine-grained control over
which hooks are executed.
Signed-off-by: Fabian Ruch baf...@gmail.com
---
git-rebase--interactive.sh | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 689400e..b50770d 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -504,10 +504,19 @@ do_next () {
mark_action_done
do_pick $sha1 $rest
- git commit --allow-empty --amend --no-post-rewrite
${gpg_sign_opt:+$gpg_sign_opt} || {
- warn Could not amend commit after successfully picking
$sha1... $rest
- exit_with_patch $sha1 1
- }
+ # TODO: Work around the fact that git-commit lets us
+ # disable either both the pre-commit and the commit-msg
+ # hook or none. Disable the pre-commit hook because the
+ # tree is left unchanged but run the commit-msg hook
+ # from here because the log message is altered.
+ git commit --allow-empty --amend --no-post-rewrite -n
${gpg_sign_opt:+$gpg_sign_opt}
+ if test -x $GIT_DIR/hooks/commit-msg
+ then
+ $GIT_DIR/hooks/commit-msg
$GIT_DIR/COMMIT_EDITMSG
+ fi || {
+ warn Could not amend commit after successfully
picking $sha1... $rest
+ exit_with_patch $sha1 1
+ }
record_in_rewritten $sha1
;;
edit|e)
--
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 RFC v2 03/19] rebase -i: reword executes pre-commit hook on interim commit
The to-do list command `reword` replays a commit like `pick` but lets
the user also edit the commit's log message. This happens in two
steps. Firstly, the named commit is cherry-picked. Secondly, the
commit created in the first step is amended using an unchanged index
to edit the log message. The pre-commit hook is meant to verify the
changes introduced by a commit (for instance, catching inserted
trailing white space). Since the committed tree is not changed
between the two steps, do not execute the pre-commit hook in the
second step.
Specify the git-commit option `--no-verify` to disable the pre-commit
hook when editing the log message. Because `--no-verify` also skips
the commit-msg hook, execute the hook from within
git-rebase--interactive after the commit is created. Fortunately, the
commit message is still available in `$GIT_DIR/COMMIT_EDITMSG` after
git-commit terminates. Caveat: In case the commit-msg hook finds the
new log message ill-formatted, the user is only notified of the
failed commit-msg hook but the log message is used for the commit
anyway. git-commit ought to offer more fine-grained control over
which hooks are executed.
Signed-off-by: Fabian Ruch baf...@gmail.com
---
git-rebase--interactive.sh | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 689400e..b50770d 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -504,10 +504,19 @@ do_next () {
mark_action_done
do_pick $sha1 $rest
- git commit --allow-empty --amend --no-post-rewrite
${gpg_sign_opt:+$gpg_sign_opt} || {
- warn Could not amend commit after successfully picking
$sha1... $rest
- exit_with_patch $sha1 1
- }
+ # TODO: Work around the fact that git-commit lets us
+ # disable either both the pre-commit and the commit-msg
+ # hook or none. Disable the pre-commit hook because the
+ # tree is left unchanged but run the commit-msg hook
+ # from here because the log message is altered.
+ git commit --allow-empty --amend --no-post-rewrite -n
${gpg_sign_opt:+$gpg_sign_opt}
+ if test -x $GIT_DIR/hooks/commit-msg
+ then
+ $GIT_DIR/hooks/commit-msg
$GIT_DIR/COMMIT_EDITMSG
+ fi || {
+ warn Could not amend commit after successfully
picking $sha1... $rest
+ exit_with_patch $sha1 1
+ }
record_in_rewritten $sha1
;;
edit|e)
--
2.0.0
--
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
