prepare-commit-msg hook no longer run for cherry-pick?

prepare-commit-msg hook no longer run for cherry-pick?

[Dmitry Torokhov]

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

Re: prepare-commit-msg hook no longer run for cherry-pick?

[Dmitry Torokhov]

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

Re: prepare-commit-msg hook no longer run for cherry-pick?

[Junio C Hamano]

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.

Re: prepare-commit-msg hook no longer run for cherry-pick?

[Dmitry Torokhov]

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

Re: prepare-commit-msg hook no longer run for cherry-pick?

[Junio C Hamano]

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.

[PATCH 0/2] sequencer: run 'prepare-commit-msg' hook

[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

[PATCH 1/2] t7505: Add tests for cherry-pick and rebase -i/-p

[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

Re: [PATCH 1/2] t7505: Add tests for cherry-pick and rebase -i/-p

[Eric Sunshine]

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
> +'

[PATCH 2/2] sequencer: run 'prepare-commit-msg' hook

[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

[PATCH v2 0/2] sequencer: run 'prepare-commit-msg' hook​

[Phillip Wood]

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

I've updated the patches in response to comments, there are just a
couple of small changes. Thanks to Ramsay and Eric for their reviews.

Best Wishes

Phillip

Original cover letter:

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

[PATCH v2 1/2] t7505: Add tests for cherry-pick and rebase -i/-p

[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>
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
---

Notes:
    Changes since v1
      - use test_seq for portability (Thanks to Eric Sunshine)

 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..a3dd3545030c2a29a1ca4502f26b412f92f0375a 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 $(test_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

Re: [PATCH v2 1/2] t7505: Add tests for cherry-pick and rebase -i/-p

[Junio C Hamano]

Phillip Wood <phillip.wood@talktalk.net> writes:

> @@ -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

It is somewhat irritating that indentation is screwed up in this
part of the file.  Can we not make it even worse?

Re: [PATCH v2 1/2] t7505: Add tests for cherry-pick and rebase -i/-p

[Junio C Hamano]

Phillip Wood <phillip.wood@talktalk.net> writes:

> +			export GIT_SEQUENCE_EDITOR GIT_EDITOR &&
> +			test_must_fail git rebase $mode b &&
> +			echo x>a &&

"echo x >a"

[PATCH v2 2/2] sequencer: run 'prepare-commit-msg' hook

[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>
Reviewed-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---

Notes:
    Changes since v1:
      - marked run_prepare_commit_msg_hook() as static (Thanks to Ramsey Jones)

 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..5bfdc4044233d5f809f9f1fbc55ebe3da477e3f0 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);
 }
 
+static 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 a3dd3545030c2a29a1ca4502f26b412f92f0375a..512ebdde8acf38db5b3d11558e4f38c6804871c5 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

[PATCH v3 0/3] sequencer: run 'prepare-commit-msg' hook

[Phillip Wood]

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

The new test and the test hook scripts has been updated with some style
fixes spotted by Junio. I've added an extra commit at the beginning to
update the style of the original hook, so my later changes are
clearer.

Original cover letter:

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 (3):
  t7505: style fixes
  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 | 134 +++++++++++++++++++++++++++++++++++--
 t/t7505/expected-rebase-i          |  17 +++++
 t/t7505/expected-rebase-p          |  18 +++++
 6 files changed, 219 insertions(+), 22 deletions(-)
 create mode 100644 t/t7505/expected-rebase-i
 create mode 100644 t/t7505/expected-rebase-p

-- 
2.15.1

[PATCH v3 1/3] t7505: style fixes

[Phillip Wood]

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

Fix the indentation and style of the hook script in preparation for
further changes.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t7505-prepare-commit-msg-hook.sh | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index b13f72975ecce17887c4c8275c6935d78d4b09a0..cef709555eb9c3e3dec0016909a17ce7cf32650a 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -31,15 +31,17 @@ mkdir -p "$HOOKDIR"
 echo "#!$SHELL_PATH" > "$HOOK"
 cat >> "$HOOK" <<'EOF'
 
-if test "$2" = commit; then
-  source=$(git rev-parse "$3")
+if test "$2" = commit
+then
+	source=$(git rev-parse "$3")
 else
-  source=${2-default}
+	source=${2-default}
 fi
-if test "$GIT_EDITOR" = :; then
-  sed -e "1s/.*/$source (no editor)/" "$1" > msg.tmp
+if test "$GIT_EDITOR" = :
+then
+	sed -e "1s/.*/$source (no editor)/" "$1" >msg.tmp
 else
-  sed -e "1s/.*/$source/" "$1" > msg.tmp
+	sed -e "1s/.*/$source/" "$1" >msg.tmp
 fi
 mv msg.tmp "$1"
 exit 0
-- 
2.15.1

[PATCH v3 2/3] t7505: Add tests for cherry-pick and rebase -i/-p

[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>
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Reviewed-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7505-prepare-commit-msg-hook.sh | 126 +++++++++++++++++++++++++++++++++++--
 t/t7505/expected-rebase-i          |  17 +++++
 t/t7505/expected-rebase-p          |  18 ++++++
 3 files changed, 157 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 cef709555eb9c3e3dec0016909a17ce7cf32650a..c1f68cbcbf40b0204ae44491c60d0b12a9fd8687 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 $(test_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,19 +63,41 @@ 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" = :
+test "$GIT_EDITOR" = : && source="$source (no editor)"
+
+if test $rebasing = 1
 then
-	sed -e "1s/.*/$source (no editor)/" "$1" >msg.tmp
+	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"
@@ -158,6 +212,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
@@ -199,4 +310,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

Re: [PATCH v3 2/3] t7505: Add tests for cherry-pick and rebase -i/-p

[Eric Sunshine]

On Wed, Jan 24, 2018 at 7:34 AM, Phillip Wood <phillip.wood@talktalk.net> wrote:
> 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>
> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
> Reviewed-by: Junio C Hamano <gitster@pobox.com>

Let's drop the Reviewed-by: from me. Although I spotted a minor
portability issue while scanning a previous iteration, I did not read
the patch closely enough to draw any conclusion of its overall
correctness. Normally, a Reviewed-by: is given explicitly by a
reviewer when confident that the patch is correct and meets the stated
goals.

I suspect that Reviewed-by: Junio ought, similarly, to be dropped.

[PATCH v3 3/3] sequencer: run 'prepare-commit-msg' hook

[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>
Reviewed-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
 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..5bfdc4044233d5f809f9f1fbc55ebe3da477e3f0 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);
 }
 
+static 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 c1f68cbcbf40b0204ae44491c60d0b12a9fd8687..1f43b3cd4cd34ec6f4c3de4cfe8a26291e4e480f 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -252,10 +252,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 &&
@@ -310,7 +310,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

Re: [PATCH v3 3/3] sequencer: run 'prepare-commit-msg' hook

[Ramsay Jones]

On 24/01/18 12:34, Phillip Wood wrote:
> 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>
> Reviewed-by: Ramsay Jones <ramsay@ramsayjones.plus.com>

Echoing Eric's earlier email, I don't think this Reviewed-by is
warranted - I only requested the addition of a static keyword,
I didn't actually review the patch.

Thanks.

ATB,
Ramsay Jones

Re: [PATCH v3 3/3] sequencer: run 'prepare-commit-msg' hook

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> On 24/01/18 12:34, Phillip Wood wrote:
>> 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>
>> Reviewed-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>
> Echoing Eric's earlier email, I don't think this Reviewed-by is
> warranted - I only requested the addition of a static keyword,
> I didn't actually review the patch.

Thanks for clarification, and I tend to agree.  You, Eric and I
certainly did not review what is posted here, so if I "git am" these
patches as-is, we'd be lying.

Having said that, I think this round takes all the review comments
raised against the previous round(s) into account.  So I'm tempted
to tweak them with s/Reviewed-/Helped-/ and queue.

Thanks, all.

Re: [PATCH v3 3/3] sequencer: run 'prepare-commit-msg' hook

[Phillip Wood]

On 24/01/18 18:59, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
>> On 24/01/18 12:34, Phillip Wood wrote:
>>> 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>
>>> Reviewed-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>>
>> Echoing Eric's earlier email, I don't think this Reviewed-by is
>> warranted - I only requested the addition of a static keyword,
>> I didn't actually review the patch.
> 
> Thanks for clarification, and I tend to agree.  You, Eric and I
> certainly did not review what is posted here, so if I "git am" these
> patches as-is, we'd be lying.
> 
> Having said that, I think this round takes all the review comments
> raised against the previous round(s) into account.  So I'm tempted
> to tweak them with s/Reviewed-/Helped-/ and queue.
> 
Thanks Junio, I wasn't sure whether to go with Reviewed-by or Helped-by, 
I'll know for next time

Best Wishes

Phillip

Re: [PATCH v3 3/3] sequencer: run 'prepare-commit-msg' hook

[Johannes Schindelin]

Hi Phillip,

On Wed, 24 Jan 2018, Phillip Wood wrote:

> 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)

I would rather have stuck this into `commit.h` and `commit.c`, but it does
not really matter all that much. The rest looks good (if a little verbose
on the test front, I think testing just the cherry-pick would have
exercised the code path enough).

All three patches are Reviewed-by: me.

Ciao,
Dscho