git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* prepare-commit-msg hook no longer run for cherry-pick?
@ 2018-01-05 18:48 Dmitry Torokhov
  2018-01-10  0:39 ` Dmitry Torokhov
  0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Torokhov @ 2018-01-05 18:48 UTC (permalink / raw)
  To: git

Hi,

I had prepare-commit-msg hook that would scrub "Patchwork-ID: NNNN" tags
form commit messages and would update input mailing list patchwork to
mark corresponding patches as "accepted" when I cherry pick form
WIP/review queue into branches that I publish, but that recently stopped
working if I do a simple cherry-pick. If I specify that I want to edit
the message, then the hook is executed:

dtor@dtor-ws:~/kernel/master (for-linus)$ GIT_TRACE=2 git cherry-pick ff162c1554efe951ba6c7a19a228fc76a91fe1ed    
10:43:12.832426 git.c:344               trace: built-in: git 'cherry-pick' 'ff162c1554efe951ba6c7a19a228fc76a91fe1ed'
[for-linus 48bc600a3659] Input: raydium_i2c_ts - include hardware version in firmware name
Author: Jeffrey Lin <jeffrey.lin@rad-ic.com>
Date: Thu Jan 4 21:35:23 2018 -0800
1 file changed, 12 insertions(+), 2 deletions(-)
dtor@dtor-ws:~/kernel/master (for-linus)$ gti reset --hard HEAD^            
HEAD is now at 02a0d9216d4d Input: xen-kbdfront - do not advertise multi-touch pressure support
dtor@dtor-ws:~/kernel/master (for-linus)$ GIT_TRACE=2 git cherry-pick -e ff162c1554efe951ba6c7a19a228fc76a91fe1ed
10:43:24.433162 git.c:344               trace: built-in: git 'cherry-pick' '-e' 'ff162c1554efe951ba6c7a19a228fc76a91fe1ed'
10:43:24.782355 run-command.c:627       trace: run_command: 'commit' '-n' '-e'
10:43:24.786460 git.c:344               trace: built-in: git 'commit' '-n' '-e'
10:43:25.082164 run-command.c:627       trace: run_command: '.git/hooks/prepare-commit-msg' '.git/COMMIT_EDITMSG' 'merge'
hint: Waiting for your editor to close the file...
10:43:31.491551 run-command.c:627       trace: run_command: 'vim' '/usr/local/goo gle/home/dtor/kernel/master/.git/COMMIT_EDITMSG'
[for-linus 039c57df0ec8] Input: raydium_i2c_ts - include hardware version in firmware name
Author: Jeffrey Lin <jeffrey.lin@rad-ic.com>
Date: Thu Jan 4 21:35:23 2018 -0800
1 file changed, 12 insertions(+), 2 deletions(-)
dtor@dtor-ws:~/kernel/master (for-linus)$

Also note that the argument to the hook is "merge" whereas I think it
used to be "cherry-pick" earlier.

Is this behavior intentional? dpkg reports version as 2.16.0~rc0+next.

Thanks!

-- 
Dmitry

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: prepare-commit-msg hook no longer run for cherry-pick?
  2018-01-05 18:48 prepare-commit-msg hook no longer run for cherry-pick? Dmitry Torokhov
@ 2018-01-10  0:39 ` Dmitry Torokhov
  2018-01-10  2:24   ` Junio C Hamano
  0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Torokhov @ 2018-01-10  0:39 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

Hi Junio,

On Fri, Jan 5, 2018 at 10:48 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi,
>
> I had prepare-commit-msg hook that would scrub "Patchwork-ID: NNNN" tags
> form commit messages and would update input mailing list patchwork to
> mark corresponding patches as "accepted" when I cherry pick form
> WIP/review queue into branches that I publish, but that recently stopped
> working if I do a simple cherry-pick.

This seems like a regression, at least for my use case. Unfortunately
my mail seems to get lost in the mailing list noise... Please let me
know if this is indeed broken or I need to adjust my workflow.

Thanks!

> If I specify that I want to edit
> the message, then the hook is executed:
>
> dtor@dtor-ws:~/kernel/master (for-linus)$ GIT_TRACE=2 git cherry-pick ff162c1554efe951ba6c7a19a228fc76a91fe1ed
> 10:43:12.832426 git.c:344               trace: built-in: git 'cherry-pick' 'ff162c1554efe951ba6c7a19a228fc76a91fe1ed'
> [for-linus 48bc600a3659] Input: raydium_i2c_ts - include hardware version in firmware name
> Author: Jeffrey Lin <jeffrey.lin@rad-ic.com>
> Date: Thu Jan 4 21:35:23 2018 -0800
> 1 file changed, 12 insertions(+), 2 deletions(-)
> dtor@dtor-ws:~/kernel/master (for-linus)$ gti reset --hard HEAD^
> HEAD is now at 02a0d9216d4d Input: xen-kbdfront - do not advertise multi-touch pressure support
> dtor@dtor-ws:~/kernel/master (for-linus)$ GIT_TRACE=2 git cherry-pick -e ff162c1554efe951ba6c7a19a228fc76a91fe1ed
> 10:43:24.433162 git.c:344               trace: built-in: git 'cherry-pick' '-e' 'ff162c1554efe951ba6c7a19a228fc76a91fe1ed'
> 10:43:24.782355 run-command.c:627       trace: run_command: 'commit' '-n' '-e'
> 10:43:24.786460 git.c:344               trace: built-in: git 'commit' '-n' '-e'
> 10:43:25.082164 run-command.c:627       trace: run_command: '.git/hooks/prepare-commit-msg' '.git/COMMIT_EDITMSG' 'merge'
> hint: Waiting for your editor to close the file...
> 10:43:31.491551 run-command.c:627       trace: run_command: 'vim' '/usr/local/goo gle/home/dtor/kernel/master/.git/COMMIT_EDITMSG'
> [for-linus 039c57df0ec8] Input: raydium_i2c_ts - include hardware version in firmware name
> Author: Jeffrey Lin <jeffrey.lin@rad-ic.com>
> Date: Thu Jan 4 21:35:23 2018 -0800
> 1 file changed, 12 insertions(+), 2 deletions(-)
> dtor@dtor-ws:~/kernel/master (for-linus)$
>
> Also note that the argument to the hook is "merge" whereas I think it
> used to be "cherry-pick" earlier.
>
> Is this behavior intentional? dpkg reports version as 2.16.0~rc0+next.
>
> Thanks!
>
> --
> Dmitry

-- 
Dmitry

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: prepare-commit-msg hook no longer run for cherry-pick?
  2018-01-10  0:39 ` Dmitry Torokhov
@ 2018-01-10  2:24   ` Junio C Hamano
  2018-01-10 19:25     ` Dmitry Torokhov
  0 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2018-01-10  2:24 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: git

Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:

>> I had prepare-commit-msg hook that would scrub "Patchwork-ID: NNNN" tags
>> form commit messages and would update input mailing list patchwork to
>> mark corresponding patches as "accepted" when I cherry pick form
>> WIP/review queue into branches that I publish, but that recently stopped
>> working if I do a simple cherry-pick.
>
> This seems like a regression, at least for my use case. Unfortunately
> my mail seems to get lost in the mailing list noise...

Possibly.  Can you bisect to see which commit broke things for you?
That would allow people who know what they themselves broke better
than I do to take a look ;-)

Thanks.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: prepare-commit-msg hook no longer run for cherry-pick?
  2018-01-10  2:24   ` Junio C Hamano
@ 2018-01-10 19:25     ` Dmitry Torokhov
  2018-01-10 21:14       ` Junio C Hamano
  2018-01-19 14:19       ` [PATCH 0/2] sequencer: run 'prepare-commit-msg' hook Phillip Wood
  0 siblings, 2 replies; 9+ messages in thread
From: Dmitry Torokhov @ 2018-01-10 19:25 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Phillip Wood

On Tue, Jan 9, 2018 at 6:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
>
> >> I had prepare-commit-msg hook that would scrub "Patchwork-ID: NNNN" tags
> >> form commit messages and would update input mailing list patchwork to
> >> mark corresponding patches as "accepted" when I cherry pick form
> >> WIP/review queue into branches that I publish, but that recently stopped
> >> working if I do a simple cherry-pick.
> >
> > This seems like a regression, at least for my use case. Unfortunately
> > my mail seems to get lost in the mailing list noise...
>
> Possibly.  Can you bisect to see which commit broke things for you?
> That would allow people who know what they themselves broke better
> than I do to take a look ;-)

Right, so it looks like the master works well, it is next(?) branch
that is troublesome (apparently we pack experimental internally?).

I bisected it down to:

commit 356ee4659bb551cd9464b317d691827276752c2d (refs/bisect/bad)
Author: Phillip Wood <phillip.wood@dunelm.org.uk>
Date:   Fri Nov 24 11:07:57 2017 +0000

   sequencer: try to commit without forking 'git commit'

   If the commit message does not need to be edited then create the
   commit without forking 'git commit'. Taking the best time of ten runs
   with a warm cache this reduces the time taken to cherry-pick 10
   commits by 27% (from 282ms to 204ms), and the time taken by 'git
   rebase --continue' to pick 10 commits by 45% (from 386ms to 212ms) on
   my computer running linux. Some of greater saving for rebase is
   because it no longer wastes time creating the commit summary just to
   throw it away.

   The code to create the commit is based on builtin/commit.c. It is
   simplified as it doesn't have to deal with merges and modified so that
   it does not die but returns an error to make sure the sequencer exits
   cleanly, as it would when forking 'git commit'

   Even when not forking 'git commit' the commit message is written to a
   file and CHERRY_PICK_HEAD is created unnecessarily. This could be
   eliminated in future. I hacked up a version that does not write these
   files and just passed an strbuf (with the wrong message for fixup and
   squash commands) to do_commit() but I couldn't measure any significant
   time difference when running cherry-pick or rebase. I think
   eliminating the writes properly for rebase would require a bit of
   effort as the code would need to be restructured.

   Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
   Signed-off-by: Junio C Hamano <gitster@pobox.com>

With this commit the hook is not being run unless I specify '-e' flag
to cherry-pick.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: prepare-commit-msg hook no longer run for cherry-pick?
  2018-01-10 19:25     ` Dmitry Torokhov
@ 2018-01-10 21:14       ` Junio C Hamano
  2018-01-19 14:19       ` [PATCH 0/2] sequencer: run 'prepare-commit-msg' hook Phillip Wood
  1 sibling, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2018-01-10 21:14 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: git, Phillip Wood

Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:

> Right, so it looks like the master works well, it is next(?) branch
> that is troublesome (apparently we pack experimental internally?).
>
> I bisected it down to:
>
> commit 356ee4659bb551cd9464b317d691827276752c2d (refs/bisect/bad)
> Author: Phillip Wood <phillip.wood@dunelm.org.uk>
> Date:   Fri Nov 24 11:07:57 2017 +0000
>
>    sequencer: try to commit without forking 'git commit'
> ...
>
> With this commit the hook is not being run unless I specify '-e' flag
> to cherry-pick.

Thanks.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 0/2] sequencer: run 'prepare-commit-msg' hook
  2018-01-10 19:25     ` Dmitry Torokhov
  2018-01-10 21:14       ` Junio C Hamano
@ 2018-01-19 14:19       ` Phillip Wood
  2018-01-19 14:19         ` [PATCH 1/2] t7505: Add tests for cherry-pick and rebase -i/-p Phillip Wood
  2018-01-19 14:19         ` [PATCH 2/2] sequencer: run 'prepare-commit-msg' hook Phillip Wood
  1 sibling, 2 replies; 9+ messages in thread
From: Phillip Wood @ 2018-01-19 14:19 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Johannes Schindelin, Junio C Hamano, Dmitry Torokhov, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

These two patches add some tests and fix the sequencer to run the
'prepare-commit-msg' hook when committing without forking 'git commit'

Phillip Wood (2):
  t7505: Add tests for cherry-pick and rebase -i/-p
  sequencer: run 'prepare-commit-msg' hook

 builtin/commit.c                   |   2 -
 sequencer.c                        |  69 ++++++++++++++++----
 sequencer.h                        |   1 +
 t/t7505-prepare-commit-msg-hook.sh | 127 +++++++++++++++++++++++++++++++++++--
 t/t7505/expected-rebase-i          |  17 +++++
 t/t7505/expected-rebase-p          |  18 ++++++
 6 files changed, 215 insertions(+), 19 deletions(-)
 create mode 100644 t/t7505/expected-rebase-i
 create mode 100644 t/t7505/expected-rebase-p

-- 
2.15.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] t7505: Add tests for cherry-pick and rebase -i/-p
  2018-01-19 14:19       ` [PATCH 0/2] sequencer: run 'prepare-commit-msg' hook Phillip Wood
@ 2018-01-19 14:19         ` Phillip Wood
  2018-01-20  0:48           ` Eric Sunshine
  2018-01-19 14:19         ` [PATCH 2/2] sequencer: run 'prepare-commit-msg' hook Phillip Wood
  1 sibling, 1 reply; 9+ messages in thread
From: Phillip Wood @ 2018-01-19 14:19 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Johannes Schindelin, Junio C Hamano, Dmitry Torokhov, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Check that cherry-pick and rebase call the 'prepare-commit-msg' hook
correctly. The expected values for the hook arguments are taken to
match the current master branch. I think there is scope for improving
the arguments passed so they make a bit more sense - for instance
cherry-pick currently passes different arguments depending on whether
the commit message is being edited. Also the arguments for rebase
could be improved. Commit 7c4188360ac ("rebase -i: proper
prepare-commit-msg hook argument when squashing", 2008-10-3) apparently
changed things so that when squashing rebase would pass 'squash' as
the argument to the hook but that has been lost.

I think that it would make more sense to pass 'message' for revert and
cherry-pick -x/-s (i.e. cases where there is a new message or the
current message in modified by the command), 'squash' when squashing
with a new message and 'commit HEAD/CHERRY_PICK_HEAD'
otherwise (picking and squashing without a new message).

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t7505-prepare-commit-msg-hook.sh | 127 +++++++++++++++++++++++++++++++++++--
 t/t7505/expected-rebase-i          |  17 +++++
 t/t7505/expected-rebase-p          |  18 ++++++
 3 files changed, 158 insertions(+), 4 deletions(-)
 create mode 100644 t/t7505/expected-rebase-i
 create mode 100644 t/t7505/expected-rebase-p

diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index b13f72975ecce17887c4c8275c6935d78d4b09a0..74b2eff71e886503d41b093953b9dd6ede29de3a 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -4,6 +4,38 @@ test_description='prepare-commit-msg hook'
 
 . ./test-lib.sh
 
+test_expect_success 'set up commits for rebasing' '
+	test_commit root &&
+	test_commit a a a &&
+	test_commit b b b &&
+	git checkout -b rebase-me root &&
+	test_commit rebase-a a aa &&
+	test_commit rebase-b b bb &&
+	for i in $(seq 1 13)
+	do
+		test_commit rebase-$i c $i
+	done &&
+	git checkout master &&
+
+	cat >rebase-todo <<-EOF
+	pick $(git rev-parse rebase-a)
+	pick $(git rev-parse rebase-b)
+	fixup $(git rev-parse rebase-1)
+	fixup $(git rev-parse rebase-2)
+	pick $(git rev-parse rebase-3)
+	fixup $(git rev-parse rebase-4)
+	squash $(git rev-parse rebase-5)
+	reword $(git rev-parse rebase-6)
+	squash $(git rev-parse rebase-7)
+	fixup $(git rev-parse rebase-8)
+	fixup $(git rev-parse rebase-9)
+	edit $(git rev-parse rebase-10)
+	squash $(git rev-parse rebase-11)
+	squash $(git rev-parse rebase-12)
+	edit $(git rev-parse rebase-13)
+	EOF
+'
+
 test_expect_success 'with no hook' '
 
 	echo "foo" > file &&
@@ -31,17 +63,40 @@ mkdir -p "$HOOKDIR"
 echo "#!$SHELL_PATH" > "$HOOK"
 cat >> "$HOOK" <<'EOF'
 
+GIT_DIR=$(git rev-parse --git-dir)
+if test -d "$GIT_DIR/rebase-merge"
+then
+  rebasing=1
+else
+  rebasing=0
+fi
+
+get_last_cmd () {
+  tail -n1 "$GIT_DIR/rebase-merge/done" | {
+    read cmd id _
+    git log --pretty="[$cmd %s]" -n1 $id
+  }
+}
+
 if test "$2" = commit; then
-  source=$(git rev-parse "$3")
+  if test $rebasing = 1
+  then
+    source="$3"
+  else
+    source=$(git rev-parse "$3")
+  fi
 else
   source=${2-default}
 fi
-if test "$GIT_EDITOR" = :; then
-  sed -e "1s/.*/$source (no editor)/" "$1" > msg.tmp
+test "$GIT_EDITOR" = : && source="$source (no editor)"
+
+if test $rebasing = 1
+then
+  echo "$source $(get_last_cmd)" >"$1"
 else
   sed -e "1s/.*/$source/" "$1" > msg.tmp
+  mv msg.tmp "$1"
 fi
-mv msg.tmp "$1"
 exit 0
 EOF
 chmod +x "$HOOK"
@@ -156,6 +211,63 @@ test_expect_success 'with hook and editor (merge)' '
 	test "$(git log -1 --pretty=format:%s)" = "merge"
 '
 
+test_rebase () {
+	expect=$1 &&
+	mode=$2 &&
+	test_expect_$expect C_LOCALE_OUTPUT "with hook (rebase $mode)" '
+		test_when_finished "\
+			git rebase --abort
+			git checkout -f master
+			git branch -D tmp" &&
+		git checkout -b tmp rebase-me &&
+		GIT_SEQUENCE_EDITOR="cp rebase-todo" &&
+		GIT_EDITOR="\"$FAKE_EDITOR\"" &&
+		(
+			export GIT_SEQUENCE_EDITOR GIT_EDITOR &&
+			test_must_fail git rebase $mode b &&
+			echo x>a &&
+			git add a &&
+			test_must_fail git rebase --continue &&
+			echo x>b &&
+			git add b &&
+			git commit &&
+			git rebase --continue &&
+			echo y>a &&
+			git add a &&
+			git commit &&
+			git rebase --continue &&
+			echo y>b &&
+			git add b &&
+			git rebase --continue
+		) &&
+		if test $mode = -p # reword amended after pick
+		then
+			n=18
+		else
+			n=17
+		fi &&
+		git log --pretty=%s -g -n$n HEAD@{1} >actual &&
+		test_cmp "$TEST_DIRECTORY/t7505/expected-rebase$mode" actual
+	'
+}
+
+test_rebase failure -i
+test_rebase failure -p
+
+test_expect_failure 'with hook (cherry-pick)' '
+	test_when_finished "git checkout -f master" &&
+	git checkout -B other b &&
+	git cherry-pick rebase-1 &&
+	test "$(git log -1 --pretty=format:%s)" = "message (no editor)"
+'
+
+test_expect_success 'with hook and editor (cherry-pick)' '
+	test_when_finished "git checkout -f master" &&
+	git checkout -B other b &&
+	git cherry-pick -e rebase-1 &&
+	test "$(git log -1 --pretty=format:%s)" = merge
+'
+
 cat > "$HOOK" <<'EOF'
 #!/bin/sh
 exit 1
@@ -197,4 +309,11 @@ test_expect_success 'with failing hook (merge)' '
 
 '
 
+test_expect_failure C_LOCALE_OUTPUT 'with failing hook (cherry-pick)' '
+	test_when_finished "git checkout -f master" &&
+	git checkout -B other b &&
+	test_must_fail git cherry-pick rebase-1 2>actual &&
+	test $(grep -c prepare-commit-msg actual) = 1
+'
+
 test_done
diff --git a/t/t7505/expected-rebase-i b/t/t7505/expected-rebase-i
new file mode 100644
index 0000000000000000000000000000000000000000..c514bdbb9422c0f699a3e1c1514b41e0796214a5
--- /dev/null
+++ b/t/t7505/expected-rebase-i
@@ -0,0 +1,17 @@
+message [edit rebase-13]
+message (no editor) [edit rebase-13]
+message [squash rebase-12]
+message (no editor) [squash rebase-11]
+default [edit rebase-10]
+message (no editor) [edit rebase-10]
+message [fixup rebase-9]
+message (no editor) [fixup rebase-8]
+message (no editor) [squash rebase-7]
+message [reword rebase-6]
+message [squash rebase-5]
+message (no editor) [fixup rebase-4]
+message (no editor) [pick rebase-3]
+message (no editor) [fixup rebase-2]
+message (no editor) [fixup rebase-1]
+merge [pick rebase-b]
+message [pick rebase-a]
diff --git a/t/t7505/expected-rebase-p b/t/t7505/expected-rebase-p
new file mode 100644
index 0000000000000000000000000000000000000000..93bada596e25f7148fdf0b955211cedfc0fbdba3
--- /dev/null
+++ b/t/t7505/expected-rebase-p
@@ -0,0 +1,18 @@
+message [edit rebase-13]
+message (no editor) [edit rebase-13]
+message [squash rebase-12]
+message (no editor) [squash rebase-11]
+default [edit rebase-10]
+message (no editor) [edit rebase-10]
+message [fixup rebase-9]
+message (no editor) [fixup rebase-8]
+message (no editor) [squash rebase-7]
+HEAD [reword rebase-6]
+message (no editor) [reword rebase-6]
+message [squash rebase-5]
+message (no editor) [fixup rebase-4]
+message (no editor) [pick rebase-3]
+message (no editor) [fixup rebase-2]
+message (no editor) [fixup rebase-1]
+merge [pick rebase-b]
+message [pick rebase-a]
-- 
2.15.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/2] sequencer: run 'prepare-commit-msg' hook
  2018-01-19 14:19       ` [PATCH 0/2] sequencer: run 'prepare-commit-msg' hook Phillip Wood
  2018-01-19 14:19         ` [PATCH 1/2] t7505: Add tests for cherry-pick and rebase -i/-p Phillip Wood
@ 2018-01-19 14:19         ` Phillip Wood
  1 sibling, 0 replies; 9+ messages in thread
From: Phillip Wood @ 2018-01-19 14:19 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Johannes Schindelin, Junio C Hamano, Dmitry Torokhov, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Commit 356ee4659b ("sequencer: try to commit without forking 'git
commit'", 2017-11-24) forgot to run the 'prepare-commit-msg' hook when
creating the commit. Fix this by writing the commit message to a
different file and running the hook. Using a different file means that
if the commit is cancelled the original message file is
unchanged. Also move the checks for an empty commit so the order
matches 'git commit'.

Reported-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/commit.c                   |  2 --
 sequencer.c                        | 69 +++++++++++++++++++++++++++++++-------
 sequencer.h                        |  1 +
 t/t7505-prepare-commit-msg-hook.sh |  8 ++---
 4 files changed, 61 insertions(+), 19 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 4a64428ca8ed8c3066aec7cfd8ad7b71217af7dd..5dd766af2842dddb80d30cd73b8be8ccb4956eac 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -66,8 +66,6 @@ N_("If you wish to skip this commit, use:\n"
 "Then \"git cherry-pick --continue\" will resume cherry-picking\n"
 "the remaining commits.\n");
 
-static GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG")
-
 static const char *use_message_buffer;
 static struct lock_file index_lock; /* real index */
 static struct lock_file false_lock; /* used only for partial commits */
diff --git a/sequencer.c b/sequencer.c
index 63a8ec9a61e7a7bf603ffa494621af79b60e0b76..79579ba118e2743c3463a8662368cb4008f02165 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -29,6 +29,8 @@
 const char sign_off_header[] = "Signed-off-by: ";
 static const char cherry_picked_prefix[] = "(cherry picked from commit ";
 
+GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG")
+
 GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
 
 static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
@@ -891,6 +893,31 @@ void commit_post_rewrite(const struct commit *old_head,
 	run_rewrite_hook(&old_head->object.oid, new_head);
 }
 
+int run_prepare_commit_msg_hook(struct strbuf *msg, const char *commit)
+{
+	struct argv_array hook_env = ARGV_ARRAY_INIT;
+	int ret;
+	const char *name;
+
+	name = git_path_commit_editmsg();
+	if (write_message(msg->buf, msg->len, name, 0))
+		return -1;
+
+	argv_array_pushf(&hook_env, "GIT_INDEX_FILE=%s", get_index_file());
+	argv_array_push(&hook_env, "GIT_EDITOR=:");
+	if (commit)
+		ret = run_hook_le(hook_env.argv, "prepare-commit-msg", name,
+				  "commit", commit, NULL);
+	else
+		ret = run_hook_le(hook_env.argv, "prepare-commit-msg", name,
+				  "message", NULL);
+	if (ret)
+		ret = error(_("'prepare-commit-msg' hook failed"));
+	argv_array_clear(&hook_env);
+
+	return ret;
+}
+
 static const char implicit_ident_advice_noconfig[] =
 N_("Your name and email address were configured automatically based\n"
 "on your username and hostname. Please check that they are accurate.\n"
@@ -1051,8 +1078,9 @@ static int try_to_commit(struct strbuf *msg, const char *author,
 	struct commit_list *parents = NULL;
 	struct commit_extra_header *extra = NULL;
 	struct strbuf err = STRBUF_INIT;
-	struct strbuf amend_msg = STRBUF_INIT;
+	struct strbuf commit_msg = STRBUF_INIT;
 	char *amend_author = NULL;
+	const char *hook_commit = NULL;
 	enum commit_msg_cleanup_mode cleanup;
 	int res = 0;
 
@@ -1069,8 +1097,9 @@ static int try_to_commit(struct strbuf *msg, const char *author,
 			const char *orig_message = NULL;
 
 			find_commit_subject(message, &orig_message);
-			msg = &amend_msg;
+			msg = &commit_msg;
 			strbuf_addstr(msg, orig_message);
+			hook_commit = "HEAD";
 		}
 		author = amend_author = get_author(message);
 		unuse_commit_buffer(current_head, message);
@@ -1084,16 +1113,6 @@ static int try_to_commit(struct strbuf *msg, const char *author,
 		commit_list_insert(current_head, &parents);
 	}
 
-	cleanup = (flags & CLEANUP_MSG) ? COMMIT_MSG_CLEANUP_ALL :
-					  opts->default_msg_cleanup;
-
-	if (cleanup != COMMIT_MSG_CLEANUP_NONE)
-		strbuf_stripspace(msg, cleanup == COMMIT_MSG_CLEANUP_ALL);
-	if (!opts->allow_empty_message && message_is_empty(msg, cleanup)) {
-		res = 1; /* run 'git commit' to display error message */
-		goto out;
-	}
-
 	if (write_cache_as_tree(tree.hash, 0, NULL)) {
 		res = error(_("git write-tree failed to write a tree"));
 		goto out;
@@ -1106,6 +1125,30 @@ static int try_to_commit(struct strbuf *msg, const char *author,
 		goto out;
 	}
 
+	if (find_hook("prepare-commit-msg")) {
+		res = run_prepare_commit_msg_hook(msg, hook_commit);
+		if (res)
+			goto out;
+		if (strbuf_read_file(&commit_msg, git_path_commit_editmsg(),
+				     2048) < 0) {
+			res = error_errno(_("unable to read commit message "
+					      "from '%s'"),
+					    git_path_commit_editmsg());
+			goto out;
+		}
+		msg = &commit_msg;
+	}
+
+	cleanup = (flags & CLEANUP_MSG) ? COMMIT_MSG_CLEANUP_ALL :
+					  opts->default_msg_cleanup;
+
+	if (cleanup != COMMIT_MSG_CLEANUP_NONE)
+		strbuf_stripspace(msg, cleanup == COMMIT_MSG_CLEANUP_ALL);
+	if (!opts->allow_empty_message && message_is_empty(msg, cleanup)) {
+		res = 1; /* run 'git commit' to display error message */
+		goto out;
+	}
+
 	if (commit_tree_extended(msg->buf, msg->len, tree.hash, parents,
 				 oid->hash, author, opts->gpg_sign, extra)) {
 		res = error(_("failed to write commit object"));
@@ -1124,7 +1167,7 @@ static int try_to_commit(struct strbuf *msg, const char *author,
 out:
 	free_commit_extra_headers(extra);
 	strbuf_release(&err);
-	strbuf_release(&amend_msg);
+	strbuf_release(&commit_msg);
 	free(amend_author);
 
 	return res;
diff --git a/sequencer.h b/sequencer.h
index 24401b07d57b7ca875dea939f465f3e6cf1162a5..e45b178dfc41d723bf186f20674c4515d7c7fa00 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -1,6 +1,7 @@
 #ifndef SEQUENCER_H
 #define SEQUENCER_H
 
+const char *git_path_commit_editmsg(void);
 const char *git_path_seq_dir(void);
 
 #define APPEND_SIGNOFF_DEDUP (1u << 0)
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 74b2eff71e886503d41b093953b9dd6ede29de3a..5df914a67cd74ce7101f792b4b9edcb913c665a7 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -251,10 +251,10 @@ test_rebase () {
 	'
 }
 
-test_rebase failure -i
-test_rebase failure -p
+test_rebase success -i
+test_rebase success -p
 
-test_expect_failure 'with hook (cherry-pick)' '
+test_expect_success 'with hook (cherry-pick)' '
 	test_when_finished "git checkout -f master" &&
 	git checkout -B other b &&
 	git cherry-pick rebase-1 &&
@@ -309,7 +309,7 @@ test_expect_success 'with failing hook (merge)' '
 
 '
 
-test_expect_failure C_LOCALE_OUTPUT 'with failing hook (cherry-pick)' '
+test_expect_success C_LOCALE_OUTPUT 'with failing hook (cherry-pick)' '
 	test_when_finished "git checkout -f master" &&
 	git checkout -B other b &&
 	test_must_fail git cherry-pick rebase-1 2>actual &&
-- 
2.15.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] t7505: Add tests for cherry-pick and rebase -i/-p
  2018-01-19 14:19         ` [PATCH 1/2] t7505: Add tests for cherry-pick and rebase -i/-p Phillip Wood
@ 2018-01-20  0:48           ` Eric Sunshine
  0 siblings, 0 replies; 9+ messages in thread
From: Eric Sunshine @ 2018-01-20  0:48 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Git Mailing List, Johannes Schindelin, Junio C Hamano, Dmitry Torokhov

On Fri, Jan 19, 2018 at 9:19 AM, Phillip Wood <phillip.wood@talktalk.net> wrote:
> Check that cherry-pick and rebase call the 'prepare-commit-msg' hook
> correctly. [...]
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
> @@ -4,6 +4,38 @@ test_description='prepare-commit-msg hook'
> +test_expect_success 'set up commits for rebasing' '
> +       test_commit root &&
> +       test_commit a a a &&
> +       test_commit b b b &&
> +       git checkout -b rebase-me root &&
> +       test_commit rebase-a a aa &&
> +       test_commit rebase-b b bb &&
> +       for i in $(seq 1 13)

For portability, use $(test_seq ...) rather than $(seq ...).

> +       do
> +               test_commit rebase-$i c $i
> +       done &&
> +       git checkout master &&
> +
> +       cat >rebase-todo <<-EOF
> +       pick $(git rev-parse rebase-a)
> +       pick $(git rev-parse rebase-b)
> +       fixup $(git rev-parse rebase-1)
> +       fixup $(git rev-parse rebase-2)
> +       pick $(git rev-parse rebase-3)
> +       fixup $(git rev-parse rebase-4)
> +       squash $(git rev-parse rebase-5)
> +       reword $(git rev-parse rebase-6)
> +       squash $(git rev-parse rebase-7)
> +       fixup $(git rev-parse rebase-8)
> +       fixup $(git rev-parse rebase-9)
> +       edit $(git rev-parse rebase-10)
> +       squash $(git rev-parse rebase-11)
> +       squash $(git rev-parse rebase-12)
> +       edit $(git rev-parse rebase-13)
> +       EOF
> +'

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-05 18:48 prepare-commit-msg hook no longer run for cherry-pick? Dmitry Torokhov
2018-01-10  0:39 ` Dmitry Torokhov
2018-01-10  2:24   ` Junio C Hamano
2018-01-10 19:25     ` Dmitry Torokhov
2018-01-10 21:14       ` Junio C Hamano
2018-01-19 14:19       ` [PATCH 0/2] sequencer: run 'prepare-commit-msg' hook Phillip Wood
2018-01-19 14:19         ` [PATCH 1/2] t7505: Add tests for cherry-pick and rebase -i/-p Phillip Wood
2018-01-20  0:48           ` Eric Sunshine
2018-01-19 14:19         ` [PATCH 2/2] sequencer: run 'prepare-commit-msg' hook Phillip Wood

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox