threads / patch / 36111

v2, 7 partsfix hunk editing with 'commit -p -m'

Subject: [PATCH V2 0/7] fix hunk editing with 'commit -p -m'

## tl;dr

50 messages between Mar 10, 2014 and Apr 3, 2014. Diffs are folded; open one to read it.

replies: 49people: 9as markdown or json

Benoit Pierre· Mar 10, 2014, 18:49 UTC · lore

This patch fixes the fact that hunk editing with 'commit -p -m' does not work: GIT_EDITOR is set to ':' to indicate to hooks that no editor will be launched, which result in the 'hunk edit' option not launching the editor (and selecting the whole hunk).

The fix consists in deferring the GIT_EDITOR override to the hook subprocess,
like it's already done for GIT_INDEX_FILE:
- rename 'run_hook' to 'run_hook_le' and change the first parameter to the environment to set
- add a 'run_hook_ve' variant that take a va_list
- add a new 'run_commit_hook' helper (to set both GIT_EDITOR and GIT_INDEX_FILE)
- the old 'run_hook' functionality is available as 'run_hook_with_custom_index'
  (and marked as deprecated in the last optional patch of this series)

N.B.: the merge builtin 'prepare-commit-msg' hook handling has also been updated to be consistent; i.e. GIT_EDITOR will not be set to ':' if the '--edit' option is used.

Benoit Pierre (7):
  merge hook tests: fix missing '&&' in test
  merge hook tests: use 'test_must_fail' instead of '!'
  test patch hunk editing with "commit -p -m"
  commit: fix patch hunk editing with "commit -p -m"
  merge: fix GIT_EDITOR override for commit hook
  merge hook tests: fix and update tests
  run-command: mark run_hook_with_custom_index as deprecated
 builtin/checkout.c                 |  8 +++----
 builtin/clone.c                    |  4 ++--
 builtin/commit.c                   | 35 ++++++++++++++++++++++++------
 builtin/gc.c                       |  2 +-
 builtin/merge.c                    |  6 +++---
 commit.h                           |  3 +++
 run-command.c                      | 44 +++++++++++++++++++++++++++-----------
 run-command.h                      |  7 +++++-
 t/t7505-prepare-commit-msg-hook.sh | 33 ++++++++++++++++++++--------
 t/t7513-commit_-p_-m_hunk_edit.sh  | 34 +++++++++++++++++++++++++++++
 10 files changed, 137 insertions(+), 39 deletions(-)
 create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh
-- 
1.9.0
Benoit Pierre· Mar 10, 2014, 18:49 UTC · re: Benoit Pierre · lore

[PATCH 1/7] merge hook tests: fix missing '&&' in test

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 t/t7505-prepare-commit-msg-hook.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t7505-prepare-commit-msg-hook.sh +1 −1
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 3573751..1c95652 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -174,7 +174,7 @@ test_expect_success 'with failing hook (merge)' '
 	git add file &&
 	rm -f "$HOOK" &&
 	git commit -m other &&
-	write_script "$HOOK" <<-EOF
+	write_script "$HOOK" <<-EOF &&
 	exit 1
 	EOF
 	git checkout - &&

-- 
1.9.0
Benoit Pierre· Mar 10, 2014, 18:49 UTC · re: Benoit Pierre · lore

[PATCH 2/7] merge hook tests: use 'test_must_fail' instead of '!'

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 t/t7505-prepare-commit-msg-hook.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to t/t7505-prepare-commit-msg-hook.sh +2 −2
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 1c95652..5531abb 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -154,7 +154,7 @@ test_expect_success 'with failing hook' '
 	head=`git rev-parse HEAD` &&
 	echo "more" >> file &&
 	git add file &&
-	! GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit -c $head
+	test_must_fail env GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit -c $head
 
 '
 
@@ -163,7 +163,7 @@ test_expect_success 'with failing hook (--no-verify)' '
 	head=`git rev-parse HEAD` &&
 	echo "more" >> file &&
 	git add file &&
-	! GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit --no-verify -c $head
+	test_must_fail env GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit --no-verify -c $head
 
 '
 

-- 
1.9.0
Benoit Pierre· Mar 10, 2014, 18:49 UTC · re: Benoit Pierre · lore

[PATCH 3/7] test patch hunk editing with "commit -p -m"

Add (failing) test: with commit changing the environment to let hooks now that no editor will be used (by setting GIT_EDITOR to ":"), the "edit hunk" functionality does not work (no editor is launched and the whole hunk is committed).

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 t/t7513-commit_-p_-m_hunk_edit.sh | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh
Show changes to t/t7513-commit_-p_-m_hunk_edit.sh +34 −0
diff --git a/t/t7513-commit_-p_-m_hunk_edit.sh b/t/t7513-commit_-p_-m_hunk_edit.sh
new file mode 100755
index 0000000..994939a
--- /dev/null
+++ b/t/t7513-commit_-p_-m_hunk_edit.sh
@@ -0,0 +1,34 @@
+#!/bin/sh
+
+test_description='hunk edit with "commit -p -m"'
+. ./test-lib.sh
+
+if ! test_have_prereq PERL
+then
+	skip_all="skipping '$test_description' tests, perl not available"
+	test_done
+fi
+
+test_expect_success 'setup (initial)' '
+	echo line1 >file &&
+	git add file &&
+	git commit -m commit1 &&
+	echo line3 >>file &&
+	cat >expect <<-\EOF
+	diff --git a/file b/file
+	index a29bdeb..c0d0fb4 100644
+	--- a/file
+	+++ b/file
+	@@ -1 +1,2 @@
+	 line1
+	+line2
+	EOF
+'
+
+test_expect_failure 'edit hunk "commit -p -m message"' '
+	echo e | env GIT_EDITOR="sed s/+line3\$/+line2/ -i" git commit -p -m commit2 file &&
+	git diff HEAD^ HEAD >actual &&
+	test_cmp expect actual
+'
+
+test_done

-- 
1.9.0
Eric Sunshine· Mar 10, 2014, 20:20 UTC · re: Benoit Pierre · lore

Re: [PATCH 3/7] test patch hunk editing with "commit -p -m"

On Mon, Mar 10, 2014 at 2:49 PM, Benoit Pierre <benoit.pierre@gmail.com> wrote:
Show 10 quoted lines
> Add (failing) test: with commit changing the environment to let hooks
> now that no editor will be used (by setting GIT_EDITOR to ":"), the
> "edit hunk" functionality does not work (no editor is launched and the
> whole hunk is committed).
>
> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
> ---
>  t/t7513-commit_-p_-m_hunk_edit.sh | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh

Is it possible to give this file a name less unusual and more consistent with other test scripts? Perhaps choose a more generic name which may allow other similar tests to be added to the file in the future (if needed)?

Show 31 quoted lines
> diff --git a/t/t7513-commit_-p_-m_hunk_edit.sh b/t/t7513-commit_-p_-m_hunk_edit.sh
> new file mode 100755
> index 0000000..994939a
> --- /dev/null
> +++ b/t/t7513-commit_-p_-m_hunk_edit.sh
> @@ -0,0 +1,34 @@
> +#!/bin/sh
> +
> +test_description='hunk edit with "commit -p -m"'
> +. ./test-lib.sh
> +
> +if ! test_have_prereq PERL
> +then
> +       skip_all="skipping '$test_description' tests, perl not available"
> +       test_done
> +fi
> +
> +test_expect_success 'setup (initial)' '
> +       echo line1 >file &&
> +       git add file &&
> +       git commit -m commit1 &&
> +       echo line3 >>file &&
> +       cat >expect <<-\EOF
> +       diff --git a/file b/file
> +       index a29bdeb..c0d0fb4 100644
> +       --- a/file
> +       +++ b/file
> +       @@ -1 +1,2 @@
> +        line1
> +       +line2
> +       EOF

In the previous review, the suggestion was that creation of 'expect' should be moved to the test below where it is actually used rather than into the 'setup' phase above.

Show 11 quoted lines
> +'
> +
> +test_expect_failure 'edit hunk "commit -p -m message"' '
> +       echo e | env GIT_EDITOR="sed s/+line3\$/+line2/ -i" git commit -p -m commit2 file &&
> +       git diff HEAD^ HEAD >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_done
> --
> 1.9.0
Junio C Hamano· Mar 11, 2014, 18:13 UTC · re: Eric Sunshine · lore

Re: [PATCH 3/7] test patch hunk editing with "commit -p -m"

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 16 quoted lines
> On Mon, Mar 10, 2014 at 2:49 PM, Benoit Pierre <benoit.pierre@gmail.com> wrote:
>> Add (failing) test: with commit changing the environment to let hooks
>> now that no editor will be used (by setting GIT_EDITOR to ":"), the
>> "edit hunk" functionality does not work (no editor is launched and the
>> whole hunk is committed).
>>
>> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
>> ---
>>  t/t7513-commit_-p_-m_hunk_edit.sh | 34 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 34 insertions(+)
>>  create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh
>
> Is it possible to give this file a name less unusual and more
> consistent with other test scripts? Perhaps choose a more generic name
> which may allow other similar tests to be added to the file in the
> future (if needed)?

Surely. There are "reset-patch" and "checkout-patch" tests, and if we were to add something like this, I'd imagine "commit-patch" would be a logical name for the new test.

Show 47 quoted lines
>> diff --git a/t/t7513-commit_-p_-m_hunk_edit.sh b/t/t7513-commit_-p_-m_hunk_edit.sh
>> new file mode 100755
>> index 0000000..994939a
>> --- /dev/null
>> +++ b/t/t7513-commit_-p_-m_hunk_edit.sh
>> @@ -0,0 +1,34 @@
>> +#!/bin/sh
>> +
>> +test_description='hunk edit with "commit -p -m"'
>> +. ./test-lib.sh
>> +
>> +if ! test_have_prereq PERL
>> +then
>> +       skip_all="skipping '$test_description' tests, perl not available"
>> +       test_done
>> +fi
>> +
>> +test_expect_success 'setup (initial)' '
>> +       echo line1 >file &&
>> +       git add file &&
>> +       git commit -m commit1 &&
>> +       echo line3 >>file &&
>> +       cat >expect <<-\EOF
>> +       diff --git a/file b/file
>> +       index a29bdeb..c0d0fb4 100644
>> +       --- a/file
>> +       +++ b/file
>> +       @@ -1 +1,2 @@
>> +        line1
>> +       +line2
>> +       EOF
>
> In the previous review, the suggestion was that creation of 'expect'
> should be moved to the test below where it is actually used rather
> than into the 'setup' phase above.
>
>> +'
>> +
>> +test_expect_failure 'edit hunk "commit -p -m message"' '
>> +       echo e | env GIT_EDITOR="sed s/+line3\$/+line2/ -i" git commit -p -m commit2 file &&
>> +       git diff HEAD^ HEAD >actual &&
>> +       test_cmp expect actual
>> +'
>> +
>> +test_done
>> --
>> 1.9.0
Philip Oakley· Mar 10, 2014, 20:27 UTC · re: Benoit Pierre · lore

Re: [PATCH 3/7] test patch hunk editing with "commit -p -m"

mincro nit.
From: "Benoit Pierre" <benoit.pierre@gmail.com>
> Add (failing) test: with commit changing the environment to let hooks
> now that no editor will be used (by setting GIT_EDITOR to ":"), the
s/now/know/
Show 57 quoted lines
> "edit hunk" functionality does not work (no editor is launched and the
> whole hunk is committed).
>
> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
> ---
> t/t7513-commit_-p_-m_hunk_edit.sh | 34 
> ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
> create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh
>
> diff --git a/t/t7513-commit_-p_-m_hunk_edit.sh 
> b/t/t7513-commit_-p_-m_hunk_edit.sh
> new file mode 100755
> index 0000000..994939a
> --- /dev/null
> +++ b/t/t7513-commit_-p_-m_hunk_edit.sh
> @@ -0,0 +1,34 @@
> +#!/bin/sh
> +
> +test_description='hunk edit with "commit -p -m"'
> +. ./test-lib.sh
> +
> +if ! test_have_prereq PERL
> +then
> + skip_all="skipping '$test_description' tests, perl not available"
> + test_done
> +fi
> +
> +test_expect_success 'setup (initial)' '
> + echo line1 >file &&
> + git add file &&
> + git commit -m commit1 &&
> + echo line3 >>file &&
> + cat >expect <<-\EOF
> + diff --git a/file b/file
> + index a29bdeb..c0d0fb4 100644
> + --- a/file
> + +++ b/file
> + @@ -1 +1,2 @@
> + line1
> + +line2
> + EOF
> +'
> +
> +test_expect_failure 'edit hunk "commit -p -m message"' '
> + echo e | env GIT_EDITOR="sed s/+line3\$/+line2/ -i" git commit -p -m 
> commit2 file &&
> + git diff HEAD^ HEAD >actual &&
> + test_cmp expect actual
> +'
> +
> +test_done
> -- 
> 1.9.0
>
> --
>
Junio C Hamano· Mar 11, 2014, 21:03 UTC · re: Benoit Pierre · lore

Re: [PATCH 3/7] test patch hunk editing with "commit -p -m"

Benoit Pierre <benoit.pierre@gmail.com> writes:
Show 12 quoted lines
> Add (failing) test: with commit changing the environment to let hooks
> now that no editor will be used (by setting GIT_EDITOR to ":"), the
> "edit hunk" functionality does not work (no editor is launched and the
> whole hunk is committed).
>
> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
> ---
>  t/t7513-commit_-p_-m_hunk_edit.sh | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh
>
> diff --git a/t/t7513-commit_-p_-m_hunk_edit.sh b/t/t7513-commit_-p_-m_hunk_edit.sh
I'll move this to t/t7514-commit-patch.sh for now while queuing.
Torsten Bögershausen· Mar 15, 2014, 12:28 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/7] test patch hunk editing with "commit -p -m"

On 2014-03-11 22.03, Junio C Hamano wrote:
Show 16 quoted lines
> Benoit Pierre <benoit.pierre@gmail.com> writes:
> 
>> Add (failing) test: with commit changing the environment to let hooks
>> now that no editor will be used (by setting GIT_EDITOR to ":"), the
>> "edit hunk" functionality does not work (no editor is launched and the
>> whole hunk is committed).
>>
>> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
>> ---
>>  t/t7513-commit_-p_-m_hunk_edit.sh | 34 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 34 insertions(+)
>>  create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh
>>
>> diff --git a/t/t7513-commit_-p_-m_hunk_edit.sh b/t/t7513-commit_-p_-m_hunk_edit.sh
> 
> I'll move this to t/t7514-commit-patch.sh for now while queuing.
This line is problematic:
	echo e | env GIT_EDITOR="sed s/+line3\$/+line2/ -i" git commit -p -m commit2 f

(sed -i is not portable: http://pubs.opengroup.org/onlinepubs/007908799/xcu/sed.html)

The whole test hangs in a forever loop loop under MacOS:
debug=t verbose=t ./t7514-commit-patch.sh 
Stage this hunk [y,n,q,a,d,/,e,?]? @@ -1 +1,2 @@
 line1
+line3
I think perl can be used instead of sed (but I haven't found the exact syntax yet)
Benoit Pierre· Mar 15, 2014, 16:11 UTC · re: Torsten Bögershausen · lore

Re: [PATCH 3/7] test patch hunk editing with "commit -p -m"

On Sat, Mar 15, 2014 at 1:28 PM, Torsten Bögershausen <tboegi@web.de> wrote:
Show 31 quoted lines
> On 2014-03-11 22.03, Junio C Hamano wrote:
>> Benoit Pierre <benoit.pierre@gmail.com> writes:
>>
>>> Add (failing) test: with commit changing the environment to let hooks
>>> now that no editor will be used (by setting GIT_EDITOR to ":"), the
>>> "edit hunk" functionality does not work (no editor is launched and the
>>> whole hunk is committed).
>>>
>>> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
>>> ---
>>>  t/t7513-commit_-p_-m_hunk_edit.sh | 34 ++++++++++++++++++++++++++++++++++
>>>  1 file changed, 34 insertions(+)
>>>  create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh
>>>
>>> diff --git a/t/t7513-commit_-p_-m_hunk_edit.sh b/t/t7513-commit_-p_-m_hunk_edit.sh
>>
>> I'll move this to t/t7514-commit-patch.sh for now while queuing.
>
> This line is problematic:
>         echo e | env GIT_EDITOR="sed s/+line3\$/+line2/ -i" git commit -p -m commit2 f
>
> (sed -i is not portable:
> http://pubs.opengroup.org/onlinepubs/007908799/xcu/sed.html)
>
> The whole test hangs in a forever loop loop under MacOS:
> debug=t verbose=t ./t7514-commit-patch.sh
> Stage this hunk [y,n,q,a,d,/,e,?]? @@ -1 +1,2 @@
>  line1
> +line3
>
> I think perl can be used instead of sed (but I haven't found the exact syntax yet)

Or maybe change the test to just 'touch' a temporary file or change its content like Jun Hao did with for its version of the tests:

https://github.com/bloomberg/git/compare/commit-patch-allow-hunk-editing
Should I make a third version? I'll simplify and move the tests to
t/t7514-commit-patch.sh and add a test for the '--dry-run' case. And
also:
- 'now' => 'know' in one of the commit message
- sign off the last patch (which I forgot to do)
- fix the indentation in one of the patch
-- 
A: Because it destroys the flow of conversation.
Q: Why is top posting dumb?
Torsten Bögershausen· Mar 15, 2014, 21:00 UTC · re: Benoit Pierre · lore

Re: [PATCH 3/7] test patch hunk editing with "commit -p -m"

On 2014-03-15 17.11, Benoit Pierre wrote:
Show 45 quoted lines
> On Sat, Mar 15, 2014 at 1:28 PM, Torsten Bögershausen <tboegi@web.de> wrote:
>> On 2014-03-11 22.03, Junio C Hamano wrote:
>>> Benoit Pierre <benoit.pierre@gmail.com> writes:
>>>
>>>> Add (failing) test: with commit changing the environment to let hooks
>>>> now that no editor will be used (by setting GIT_EDITOR to ":"), the
>>>> "edit hunk" functionality does not work (no editor is launched and the
>>>> whole hunk is committed).
>>>>
>>>> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
>>>> ---
>>>>  t/t7513-commit_-p_-m_hunk_edit.sh | 34 ++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 34 insertions(+)
>>>>  create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh
>>>>
>>>> diff --git a/t/t7513-commit_-p_-m_hunk_edit.sh b/t/t7513-commit_-p_-m_hunk_edit.sh
>>>
>>> I'll move this to t/t7514-commit-patch.sh for now while queuing.
>>
>> This line is problematic:
>>         echo e | env GIT_EDITOR="sed s/+line3\$/+line2/ -i" git commit -p -m commit2 f
>>
>> (sed -i is not portable:
>> http://pubs.opengroup.org/onlinepubs/007908799/xcu/sed.html)
>>
>> The whole test hangs in a forever loop loop under MacOS:
>> debug=t verbose=t ./t7514-commit-patch.sh
>> Stage this hunk [y,n,q,a,d,/,e,?]? @@ -1 +1,2 @@
>>  line1
>> +line3
>>
>> I think perl can be used instead of sed (but I haven't found the exact syntax yet)
> 
> Or maybe change the test to just 'touch' a temporary file or change
> its content like Jun Hao did with for its version of the tests:
> 
> https://github.com/bloomberg/git/compare/commit-patch-allow-hunk-editing
> 
> Should I make a third version? I'll simplify and move the tests to
> t/t7514-commit-patch.sh and add a test for the '--dry-run' case. And
> also:
> - 'now' => 'know' in one of the commit message
> - sign off the last patch (which I forgot to do)
> - fix the indentation in one of the patch
>

The following works for me, (diff against pu) (and if you want to send a 3rd version, please do so :-)

Show changes to t/t7514-commit-patch.sh +13 −3
diff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh
index 1f05d32..da92669 100755
--- a/t/t7514-commit-patch.sh
+++ b/t/t7514-commit-patch.sh
@@ -25,10 +25,20 @@ test_expect_success 'setup (initial)' '
 	EOF
 '
 
+write_script .git/FAKE_EDITOR <<\EOF
+	sed -e "s/+line3\$/+line2/" <"$1" >tmp &&
+	mv -f tmp "$1"
+	exit 0
+EOF
+
 test_expect_success 'edit hunk "commit -p -m message"' '
-	echo e | env GIT_EDITOR="sed s/+line3\$/+line2/ -i" git commit -p -m commit2 file &&
-	git diff HEAD^ HEAD >actual &&
-	test_cmp expect actual
+	(
+		GIT_EDITOR="\"$(pwd)/.git/FAKE_EDITOR\"" &&
+		export GIT_EDITOR &&
+		echo e | git commit -p -m commit2 file &&
+		git diff HEAD^ HEAD >actual &&
+		test_cmp expect actual
+	)
 '
 
 test_done
Benoit Pierre· Mar 15, 2014, 21:42 UTC · re: Benoit Pierre · lore

[PATCH 1/7] merge hook tests: fix missing '&&' in test

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 t/t7505-prepare-commit-msg-hook.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t7505-prepare-commit-msg-hook.sh +1 −1
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 3573751..1c95652 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -174,7 +174,7 @@ test_expect_success 'with failing hook (merge)' '
 	git add file &&
 	rm -f "$HOOK" &&
 	git commit -m other &&
-	write_script "$HOOK" <<-EOF
+	write_script "$HOOK" <<-EOF &&
 	exit 1
 	EOF
 	git checkout - &&

-- 
1.9.0
Benoit Pierre· Mar 15, 2014, 21:42 UTC · re: Benoit Pierre · lore

[PATCH 2/7] merge hook tests: use 'test_must_fail' instead of '!'

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 t/t7505-prepare-commit-msg-hook.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to t/t7505-prepare-commit-msg-hook.sh +2 −2
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 1c95652..5531abb 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -154,7 +154,7 @@ test_expect_success 'with failing hook' '
 	head=`git rev-parse HEAD` &&
 	echo "more" >> file &&
 	git add file &&
-	! GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit -c $head
+	test_must_fail env GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit -c $head
 
 '
 
@@ -163,7 +163,7 @@ test_expect_success 'with failing hook (--no-verify)' '
 	head=`git rev-parse HEAD` &&
 	echo "more" >> file &&
 	git add file &&
-	! GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit --no-verify -c $head
+	test_must_fail env GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit --no-verify -c $head
 
 '
 

-- 
1.9.0
Benoit Pierre· Mar 15, 2014, 21:42 UTC · re: Benoit Pierre · lore

[PATCH 3/7] test patch hunk editing with "commit -p -m"

Add (failing) tests: with commit changing the environment to let hooks know that no editor will be used (by setting GIT_EDITOR to ":"), the "edit hunk" functionality does not work (no editor is launched and the whole hunk is committed).

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 t/t7513-commit-patch.sh | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100755 t/t7513-commit-patch.sh
Show changes to t/t7513-commit-patch.sh +32 −0
diff --git a/t/t7513-commit-patch.sh b/t/t7513-commit-patch.sh
new file mode 100755
index 0000000..9311b0c
--- /dev/null
+++ b/t/t7513-commit-patch.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+
+test_description='hunk edit with "commit -p -m"'
+. ./test-lib.sh
+
+if ! test_have_prereq PERL
+then
+	skip_all="skipping '$test_description' tests, perl not available"
+	test_done
+fi
+
+test_expect_success 'setup (initial)' '
+	echo line1 >file &&
+	git add file &&
+	git commit -m commit1
+'
+
+test_expect_failure 'edit hunk "commit -p -m message"' '
+	test_when_finished "rm -f editor_was_started" &&
+	echo more >>file &&
+	echo e | env GIT_EDITOR="touch editor_was_started" git commit -p -m commit2 file &&
+	test -r editor_was_started
+'
+
+test_expect_failure 'edit hunk "commit --dry-run -p -m message"' '
+	test_when_finished "rm -f editor_was_started" &&
+	echo more >>file &&
+	echo e | env GIT_EDITOR="touch editor_was_started" git commit -p -m commit3 file &&
+	test -r editor_was_started
+'
+
+test_done

-- 
1.9.0
Junio C Hamano· Mar 17, 2014, 18:49 UTC · re: Benoit Pierre · lore

Re: [PATCH 3/7] test patch hunk editing with "commit -p -m"

Benoit Pierre <benoit.pierre@gmail.com> writes:
Show 12 quoted lines
> Add (failing) tests: with commit changing the environment to let hooks
> know that no editor will be used (by setting GIT_EDITOR to ":"), the
> "edit hunk" functionality does not work (no editor is launched and the
> whole hunk is committed).
>
> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
> ---
>  t/t7513-commit-patch.sh | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>  create mode 100755 t/t7513-commit-patch.sh
>
> diff --git a/t/t7513-commit-patch.sh b/t/t7513-commit-patch.sh

Again, as I said, I'll rename this to t7514-commit.patch.sh while I queue this.

Thanks.
Show 37 quoted lines
> new file mode 100755
> index 0000000..9311b0c
> --- /dev/null
> +++ b/t/t7513-commit-patch.sh
> @@ -0,0 +1,32 @@
> +#!/bin/sh
> +
> +test_description='hunk edit with "commit -p -m"'
> +. ./test-lib.sh
> +
> +if ! test_have_prereq PERL
> +then
> +	skip_all="skipping '$test_description' tests, perl not available"
> +	test_done
> +fi
> +
> +test_expect_success 'setup (initial)' '
> +	echo line1 >file &&
> +	git add file &&
> +	git commit -m commit1
> +'
> +
> +test_expect_failure 'edit hunk "commit -p -m message"' '
> +	test_when_finished "rm -f editor_was_started" &&
> +	echo more >>file &&
> +	echo e | env GIT_EDITOR="touch editor_was_started" git commit -p -m commit2 file &&
> +	test -r editor_was_started
> +'
> +
> +test_expect_failure 'edit hunk "commit --dry-run -p -m message"' '
> +	test_when_finished "rm -f editor_was_started" &&
> +	echo more >>file &&
> +	echo e | env GIT_EDITOR="touch editor_was_started" git commit -p -m commit3 file &&
> +	test -r editor_was_started
> +'
> +
> +test_done
Benoit Pierre· Mar 17, 2014, 19:46 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/7] test patch hunk editing with "commit -p -m"

On Mon, Mar 17, 2014 at 7:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
> Benoit Pierre <benoit.pierre@gmail.com> writes:
>
>> Add (failing) tests: with commit changing the environment to let hooks
>> know that no editor will be used (by setting GIT_EDITOR to ":"), the
>> "edit hunk" functionality does not work (no editor is launched and the
>> whole hunk is committed).
>>
>> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
>> ---
>>  t/t7513-commit-patch.sh | 32 ++++++++++++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>>  create mode 100755 t/t7513-commit-patch.sh
>>
>> diff --git a/t/t7513-commit-patch.sh b/t/t7513-commit-patch.sh
>
> Again, as I said, I'll rename this to t7514-commit.patch.sh while I
> queue this.

I assumed the "14" was a typo, will rename, but to t7514-commit-patch.sh right? (with 2 '-').

-- 
A: Because it destroys the flow of conversation.
Q: Why is top posting dumb?
Eric Sunshine· Mar 17, 2014, 19:51 UTC · re: Benoit Pierre · lore

Re: [PATCH 3/7] test patch hunk editing with "commit -p -m"

On Mon, Mar 17, 2014 at 3:46 PM, Benoit Pierre <benoit.pierre@gmail.com> wrote:
Show 21 quoted lines
> On Mon, Mar 17, 2014 at 7:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Benoit Pierre <benoit.pierre@gmail.com> writes:
>>
>>> Add (failing) tests: with commit changing the environment to let hooks
>>> know that no editor will be used (by setting GIT_EDITOR to ":"), the
>>> "edit hunk" functionality does not work (no editor is launched and the
>>> whole hunk is committed).
>>>
>>> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
>>> ---
>>>  t/t7513-commit-patch.sh | 32 ++++++++++++++++++++++++++++++++
>>>  1 file changed, 32 insertions(+)
>>>  create mode 100755 t/t7513-commit-patch.sh
>>>
>>> diff --git a/t/t7513-commit-patch.sh b/t/t7513-commit-patch.sh
>>
>> Again, as I said, I'll rename this to t7514-commit.patch.sh while I
>> queue this.
>
> I assumed the "14" was a typo, will rename, but to
> t7514-commit-patch.sh right? (with 2 '-').
Yes, two '-'.
In the 'pu' branch, there is a new t7513-interpret-trailers.sh.
Benoit Pierre· Mar 17, 2014, 19:53 UTC · re: Eric Sunshine · lore

Re: [PATCH 3/7] test patch hunk editing with "commit -p -m"

On Mon, Mar 17, 2014 at 8:51 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 17 quoted lines
> On Mon, Mar 17, 2014 at 3:46 PM, Benoit Pierre <benoit.pierre@gmail.com> wrote:
>> On Mon, Mar 17, 2014 at 7:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Benoit Pierre <benoit.pierre@gmail.com> writes:
>>>
>>>> [...]
>>>>
>>>> diff --git a/t/t7513-commit-patch.sh b/t/t7513-commit-patch.sh
>>>
>>> Again, as I said, I'll rename this to t7514-commit.patch.sh while I
>>> queue this.
>>
>> I assumed the "14" was a typo, will rename, but to
>> t7514-commit-patch.sh right? (with 2 '-').
>
> Yes, two '-'.
>
> In the 'pu' branch, there is a new t7513-interpret-trailers.sh.
OK.
-- 
A: Because it destroys the flow of conversation.
Q: Why is top posting dumb?
Junio C Hamano· Mar 17, 2014, 21:33 UTC · re: Benoit Pierre · lore

Re: [PATCH 3/7] test patch hunk editing with "commit -p -m"

Benoit Pierre <benoit.pierre@gmail.com> writes:
Show 21 quoted lines
> On Mon, Mar 17, 2014 at 7:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Benoit Pierre <benoit.pierre@gmail.com> writes:
>>
>>> Add (failing) tests: with commit changing the environment to let hooks
>>> know that no editor will be used (by setting GIT_EDITOR to ":"), the
>>> "edit hunk" functionality does not work (no editor is launched and the
>>> whole hunk is committed).
>>>
>>> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
>>> ---
>>>  t/t7513-commit-patch.sh | 32 ++++++++++++++++++++++++++++++++
>>>  1 file changed, 32 insertions(+)
>>>  create mode 100755 t/t7513-commit-patch.sh
>>>
>>> diff --git a/t/t7513-commit-patch.sh b/t/t7513-commit-patch.sh
>>
>> Again, as I said, I'll rename this to t7514-commit.patch.sh while I
>> queue this.
>
> I assumed the "14" was a typo, will rename, but to
> t7514-commit-patch.sh right? (with 2 '-').

Thanks, yes. That is how I queued the previous one on 'pu', I think.

Junio C Hamano· Mar 17, 2014, 18:53 UTC · re: Benoit Pierre · lore

Re: [PATCH 3/7] test patch hunk editing with "commit -p -m"

Benoit Pierre <benoit.pierre@gmail.com> writes:
> +test_expect_failure 'edit hunk "commit -p -m message"' '
> +	test_when_finished "rm -f editor_was_started" &&

Not just "when finished", run "rm -f" here to make sure that the file does not exist. Later other people may add new tests before this test piece and affect the state of your throw-away working tree, and you would want to protect yourself from their changes.

> +	echo more >>file &&
> +	echo e | env GIT_EDITOR="touch editor_was_started" git commit -p -m commit2 file &&

Avoid "touch" unless you are interested in the timestamp to be updated. Use ": >editor_was_started" or something like that when you are only interested in "it was not there, now it is".

The same comment applies to the next one.
Thanks.
Show 11 quoted lines
> +	test -r editor_was_started
> +'
> +
> +test_expect_failure 'edit hunk "commit --dry-run -p -m message"' '
> +	test_when_finished "rm -f editor_was_started" &&
> +	echo more >>file &&
> +	echo e | env GIT_EDITOR="touch editor_was_started" git commit -p -m commit3 file &&
> +	test -r editor_was_started
> +'
> +
> +test_done
Benoit Pierre· Mar 17, 2014, 19:52 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/7] test patch hunk editing with "commit -p -m"

On Mon, Mar 17, 2014 at 7:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
> Benoit Pierre <benoit.pierre@gmail.com> writes:
>
>> +test_expect_failure 'edit hunk "commit -p -m message"' '
>> +     test_when_finished "rm -f editor_was_started" &&
>
> Not just "when finished", run "rm -f" here to make sure that the
> file does not exist.  Later other people may add new tests before
> this test piece and affect the state of your throw-away working
> tree, and you would want to protect yourself from their changes.
>
>> +     echo more >>file &&
>> +     echo e | env GIT_EDITOR="touch editor_was_started" git commit -p -m commit2 file &&
>
> Avoid "touch" unless you are interested in the timestamp to be
> updated.  Use ": >editor_was_started" or something like that when
> you are only interested in "it was not there, now it is".
>
> The same comment applies to the next one.

Isn't the point of using "when finished" to have each test leave the tree clean after execution, to avoid "bleeding" onto the next test(s)? Instead of having each test clean after the previous test(s). It seems to me using both is kind of redundant, no?

-- 
A: Because it destroys the flow of conversation.
Q: Why is top posting dumb?
Junio C Hamano· Mar 17, 2014, 21:35 UTC · re: Benoit Pierre · lore

Re: [PATCH 3/7] test patch hunk editing with "commit -p -m"

Benoit Pierre <benoit.pierre@gmail.com> writes:
> Isn't the point of using "when finished" to have each test leave the
> tree clean after execution, to avoid "bleeding" onto the next test(s)?

But you cannot anticipate what other people will do in the future before you have a chance to run this piece. Your using when-finished is courtesy for others. Your explicitly making sure what you do not want to be stray behind is protecting yourself from others.

They serve different purposes, and you need both.
Benoit Pierre· Mar 18, 2014, 10:00 UTC · re: Junio C Hamano · lore

[PATCH 1/7] merge hook tests: fix missing '&&' in test

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 t/t7505-prepare-commit-msg-hook.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t7505-prepare-commit-msg-hook.sh +1 −1
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 3573751..1c95652 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -174,7 +174,7 @@ test_expect_success 'with failing hook (merge)' '
 	git add file &&
 	rm -f "$HOOK" &&
 	git commit -m other &&
-	write_script "$HOOK" <<-EOF
+	write_script "$HOOK" <<-EOF &&
 	exit 1
 	EOF
 	git checkout - &&

-- 
1.9.0
Benoit Pierre· Mar 18, 2014, 10:00 UTC · re: Benoit Pierre · lore

[PATCH 2/7] merge hook tests: use 'test_must_fail' instead of '!'

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 t/t7505-prepare-commit-msg-hook.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to t/t7505-prepare-commit-msg-hook.sh +2 −2
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 1c95652..5531abb 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -154,7 +154,7 @@ test_expect_success 'with failing hook' '
 	head=`git rev-parse HEAD` &&
 	echo "more" >> file &&
 	git add file &&
-	! GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit -c $head
+	test_must_fail env GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit -c $head
 
 '
 
@@ -163,7 +163,7 @@ test_expect_success 'with failing hook (--no-verify)' '
 	head=`git rev-parse HEAD` &&
 	echo "more" >> file &&
 	git add file &&
-	! GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit --no-verify -c $head
+	test_must_fail env GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit --no-verify -c $head
 
 '
 

-- 
1.9.0
Benoit Pierre· Mar 18, 2014, 10:00 UTC · re: Benoit Pierre · lore

[PATCH 3/7] test patch hunk editing with "commit -p -m"

Add (failing) tests: with commit changing the environment to let hooks know that no editor will be used (by setting GIT_EDITOR to ":"), the "edit hunk" functionality does not work (no editor is launched and the whole hunk is committed).

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 t/t7514-commit-patch.sh | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100755 t/t7514-commit-patch.sh
Show changes to t/t7514-commit-patch.sh +34 −0
diff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh
new file mode 100755
index 0000000..41dd37a
--- /dev/null
+++ b/t/t7514-commit-patch.sh
@@ -0,0 +1,34 @@
+#!/bin/sh
+
+test_description='hunk edit with "commit -p -m"'
+. ./test-lib.sh
+
+if ! test_have_prereq PERL
+then
+	skip_all="skipping '$test_description' tests, perl not available"
+	test_done
+fi
+
+test_expect_success 'setup (initial)' '
+	echo line1 >file &&
+	git add file &&
+	git commit -m commit1
+'
+
+test_expect_failure 'edit hunk "commit -p -m message"' '
+	test_when_finished "rm -f editor_was_started" &&
+	rm -f editor_was_started &&
+	echo more >>file &&
+	echo e | env GIT_EDITOR=": >editor_was_started" git commit -p -m commit2 file &&
+	test -r editor_was_started
+'
+
+test_expect_failure 'edit hunk "commit --dry-run -p -m message"' '
+	test_when_finished "rm -f editor_was_started" &&
+	rm -f editor_was_started &&
+	echo more >>file &&
+	echo e | env GIT_EDITOR=": >editor_was_started" git commit -p -m commit3 file &&
+	test -r editor_was_started
+'
+
+test_done

-- 
1.9.0
Benoit Pierre· Mar 18, 2014, 10:00 UTC · re: Benoit Pierre · lore

[PATCH 4/7] commit: fix patch hunk editing with "commit -p -m"

Don't change git environment: move the GIT_EDITOR=":" override to the hook command subprocess, like it's already done for GIT_INDEX_FILE.

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 builtin/checkout.c      |  8 ++++----
 builtin/clone.c         |  4 ++--
 builtin/commit.c        | 35 ++++++++++++++++++++++++++++-------
 builtin/gc.c            |  2 +-
 builtin/merge.c         |  6 +++---
 commit.h                |  3 +++
 run-command.c           | 44 ++++++++++++++++++++++++++++++++------------
 run-command.h           |  6 +++++-
 t/t7514-commit-patch.sh |  4 ++--
 9 files changed, 80 insertions(+), 32 deletions(-)
Show changes to 9 files +80 −32

builtin/checkout.c, builtin/clone.c, builtin/commit.c, builtin/gc.c, builtin/merge.c, commit.h, run-command.c, run-command.h, t/t7514-commit-patch.sh

diff --git a/builtin/checkout.c b/builtin/checkout.c
index ada51fa..1b86d9c 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -53,10 +53,10 @@ struct checkout_opts {
 static int post_checkout_hook(struct commit *old, struct commit *new,
 			      int changed)
 {
-	return run_hook(NULL, "post-checkout",
-			sha1_to_hex(old ? old->object.sha1 : null_sha1),
-			sha1_to_hex(new ? new->object.sha1 : null_sha1),
-			changed ? "1" : "0", NULL);
+	return run_hook_le(NULL, "post-checkout",
+			   sha1_to_hex(old ? old->object.sha1 : null_sha1),
+			   sha1_to_hex(new ? new->object.sha1 : null_sha1),
+			   changed ? "1" : "0", NULL);
 	/* "new" can be NULL when checking out from the index before
 	   a commit exists. */
 
diff --git a/builtin/clone.c b/builtin/clone.c
index 43e772c..9b3c04d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -660,8 +660,8 @@ static int checkout(void)
 	    commit_locked_index(lock_file))
 		die(_("unable to write new index file"));
 
-	err |= run_hook(NULL, "post-checkout", sha1_to_hex(null_sha1),
-			sha1_to_hex(sha1), "1", NULL);
+	err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
+			   sha1_to_hex(sha1), "1", NULL);
 
 	if (!err && option_recursive)
 		err = run_command_v_opt(argv_submodule, RUN_GIT_CMD);
diff --git a/builtin/commit.c b/builtin/commit.c
index 3783bca..68a90b3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -610,7 +610,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	/* This checks and barfs if author is badly specified */
 	determine_author_info(author_ident);
 
-	if (!no_verify && run_hook(index_file, "pre-commit", NULL))
+	if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit", NULL))
 		return 0;
 
 	if (squash_message) {
@@ -867,8 +867,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		return 0;
 	}
 
-	if (run_hook(index_file, "prepare-commit-msg",
-		     git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
+	if (run_commit_hook(use_editor, index_file, "prepare-commit-msg",
+			    git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
 		return 0;
 
 	if (use_editor) {
@@ -884,7 +884,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	}
 
 	if (!no_verify &&
-	    run_hook(index_file, "commit-msg", git_path(commit_editmsg), NULL)) {
+	    run_commit_hook(use_editor, index_file, "commit-msg", git_path(commit_editmsg), NULL)) {
 		return 0;
 	}
 
@@ -1068,8 +1068,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		use_editor = 0;
 	if (0 <= edit_flag)
 		use_editor = edit_flag;
-	if (!use_editor)
-		setenv("GIT_EDITOR", ":", 1);
 
 	/* Sanity check options */
 	if (amend && !current_head)
@@ -1450,6 +1448,29 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
 	return finish_command(&proc);
 }
 
+int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...)
+{
+	const char *hook_env[3] =  { NULL };
+	char index[PATH_MAX];
+	va_list args;
+	int ret;
+
+	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+	hook_env[0] = index;
+
+	/*
+	 * Let the hook know that no editor will be launched.
+	 */
+	if (!editor_is_used)
+		hook_env[1] = "GIT_EDITOR=:";
+
+	va_start(args, name);
+	ret = run_hook_ve(hook_env, name, args);
+	va_end(args);
+
+	return ret;
+}
+
 int cmd_commit(int argc, const char **argv, const char *prefix)
 {
 	static struct wt_status s;
@@ -1674,7 +1695,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		     "not exceeded, and then \"git reset HEAD\" to recover."));
 
 	rerere(0);
-	run_hook(get_index_file(), "post-commit", NULL);
+	run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
 	if (amend && !no_post_rewrite) {
 		struct notes_rewrite_cfg *cfg;
 		cfg = init_copy_notes_for_rewrite("amend");
diff --git a/builtin/gc.c b/builtin/gc.c
index 63d400b..11cf295 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -184,7 +184,7 @@ static int need_to_gc(void)
 	else if (!too_many_loose_objects())
 		return 0;
 
-	if (run_hook(NULL, "pre-auto-gc", NULL))
+	if (run_hook_le(NULL, "pre-auto-gc", NULL))
 		return 0;
 	return 1;
 }
diff --git a/builtin/merge.c b/builtin/merge.c
index f0cf120..bdf6655 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -421,7 +421,7 @@ static void finish(struct commit *head_commit,
 	}
 
 	/* Run a post-merge hook */
-	run_hook(NULL, "post-merge", squash ? "1" : "0", NULL);
+	run_hook_le(NULL, "post-merge", squash ? "1" : "0", NULL);
 
 	strbuf_release(&reflog_message);
 }
@@ -824,8 +824,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	if (0 < option_edit)
 		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
 	write_merge_msg(&msg);
-	if (run_hook(get_index_file(), "prepare-commit-msg",
-		     git_path("MERGE_MSG"), "merge", NULL, NULL))
+	if (run_commit_hook(1, get_index_file(), "prepare-commit-msg",
+			    git_path("MERGE_MSG"), "merge", NULL))
 		abort_commit(remoteheads, NULL);
 	if (0 < option_edit) {
 		if (launch_editor(git_path("MERGE_MSG"), NULL, NULL))
diff --git a/commit.h b/commit.h
index 16d9c43..8d97a5c 100644
--- a/commit.h
+++ b/commit.h
@@ -304,4 +304,7 @@ extern void check_commit_signature(const struct commit* commit, struct signature
 
 int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused);
 
+LAST_ARG_MUST_BE_NULL
+extern int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...);
+
 #endif /* COMMIT_H */
diff --git a/run-command.c b/run-command.c
index 3914d9c..75abc47 100644
--- a/run-command.c
+++ b/run-command.c
@@ -760,13 +760,11 @@ char *find_hook(const char *name)
 	return path;
 }
 
-int run_hook(const char *index_file, const char *name, ...)
+int run_hook_ve(const char *const *env, const char *name, va_list args)
 {
 	struct child_process hook;
 	struct argv_array argv = ARGV_ARRAY_INIT;
-	const char *p, *env[2];
-	char index[PATH_MAX];
-	va_list args;
+	const char *p;
 	int ret;
 
 	p = find_hook(name);
@@ -775,23 +773,45 @@ int run_hook(const char *index_file, const char *name, ...)
 
 	argv_array_push(&argv, p);
 
-	va_start(args, name);
 	while ((p = va_arg(args, const char *)))
 		argv_array_push(&argv, p);
-	va_end(args);
 
 	memset(&hook, 0, sizeof(hook));
 	hook.argv = argv.argv;
+	hook.env = env;
 	hook.no_stdin = 1;
 	hook.stdout_to_stderr = 1;
-	if (index_file) {
-		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
-		env[0] = index;
-		env[1] = NULL;
-		hook.env = env;
-	}
 
 	ret = run_command(&hook);
 	argv_array_clear(&argv);
 	return ret;
 }
+
+int run_hook_le(const char *const *env, const char *name, ...)
+{
+	va_list args;
+	int ret;
+
+	va_start(args, name);
+	ret = run_hook_ve(env, name, args);
+	va_end(args);
+
+	return ret;
+}
+
+int run_hook_with_custom_index(const char *index_file, const char *name, ...)
+{
+	const char *hook_env[3] =  { NULL };
+	char index[PATH_MAX];
+	va_list args;
+	int ret;
+
+	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+	hook_env[0] = index;
+
+	va_start(args, name);
+	ret = run_hook_ve(hook_env, name, args);
+	va_end(args);
+
+	return ret;
+}
diff --git a/run-command.h b/run-command.h
index 6b985af..88460f9 100644
--- a/run-command.h
+++ b/run-command.h
@@ -47,7 +47,11 @@ int run_command(struct child_process *);
 
 extern char *find_hook(const char *name);
 LAST_ARG_MUST_BE_NULL
-extern int run_hook(const char *index_file, const char *name, ...);
+extern int run_hook_le(const char *const *env, const char *name, ...);
+extern int run_hook_ve(const char *const *env, const char *name, va_list args);
+
+LAST_ARG_MUST_BE_NULL
+extern int run_hook_with_custom_index(const char *index_file, const char *name, ...);
 
 #define RUN_COMMAND_NO_STDIN 1
 #define RUN_GIT_CMD	     2	/*If this is to be git sub-command */
diff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh
index 41dd37a..998a210 100755
--- a/t/t7514-commit-patch.sh
+++ b/t/t7514-commit-patch.sh
@@ -15,7 +15,7 @@ test_expect_success 'setup (initial)' '
 	git commit -m commit1
 '
 
-test_expect_failure 'edit hunk "commit -p -m message"' '
+test_expect_success 'edit hunk "commit -p -m message"' '
 	test_when_finished "rm -f editor_was_started" &&
 	rm -f editor_was_started &&
 	echo more >>file &&
@@ -23,7 +23,7 @@ test_expect_failure 'edit hunk "commit -p -m message"' '
 	test -r editor_was_started
 '
 
-test_expect_failure 'edit hunk "commit --dry-run -p -m message"' '
+test_expect_success 'edit hunk "commit --dry-run -p -m message"' '
 	test_when_finished "rm -f editor_was_started" &&
 	rm -f editor_was_started &&
 	echo more >>file &&

-- 
1.9.0
Torsten Bögershausen· Mar 19, 2014, 07:32 UTC · re: Benoit Pierre · lore

Re: [PATCH 4/7] commit: fix patch hunk editing with "commit -p -m"

On 03/18/2014 11:00 AM, Benoit Pierre wrote:
Show 16 quoted lines
> Don't change git environment: move the GIT_EDITOR=":" override to the
> hook command subprocess, like it's already done for GIT_INDEX_FILE.
>
> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
> ---
>   builtin/checkout.c      |  8 ++++----
>   builtin/clone.c         |  4 ++--
>   builtin/commit.c        | 35 ++++++++++++++++++++++++++++-------
>   builtin/gc.c            |  2 +-
>   builtin/merge.c         |  6 +++---
>   commit.h                |  3 +++
>   run-command.c           | 44 ++++++++++++++++++++++++++++++++------------
>   run-command.h           |  6 +++++-
>   t/t7514-commit-patch.sh |  4 ++--
>   9 files changed, 80 insertions(+), 32 deletions(-)
>
[]
Show 14 quoted lines
> index 3914d9c..75abc47 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -760,13 +760,11 @@ char *find_hook(const char *name)
>   	return path;
>   }
>
> -int run_hook(const char *index_file, const char *name, ...)
> +int run_hook_ve(const char *const *env, const char *name, va_list args)
>   {
>   	struct child_process hook;
>   	struct argv_array argv = ARGV_ARRAY_INIT;
> -	const char *p, *env[2];
> -	char index[PATH_MAX];
(Please see below)
Show 47 quoted lines
> -	va_list args;
> +	const char *p;
>   	int ret;
>
>   	p = find_hook(name);
> @@ -775,23 +773,45 @@ int run_hook(const char *index_file, const char *name, ...)
>
>   	argv_array_push(&argv, p);
>
> -	va_start(args, name);
>   	while ((p = va_arg(args, const char *)))
>   		argv_array_push(&argv, p);
> -	va_end(args);
>
>   	memset(&hook, 0, sizeof(hook));
>   	hook.argv = argv.argv;
> +	hook.env = env;
>   	hook.no_stdin = 1;
>   	hook.stdout_to_stderr = 1;
> -	if (index_file) {
> -		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
> -		env[0] = index;
> -		env[1] = NULL;
> -		hook.env = env;
> -	}
>
>   	ret = run_command(&hook);
>   	argv_array_clear(&argv);
>   	return ret;
>   }
> +
> +int run_hook_le(const char *const *env, const char *name, ...)
> +{
> +	va_list args;
> +	int ret;
> +
> +	va_start(args, name);
> +	ret = run_hook_ve(env, name, args);
> +	va_end(args);
> +
> +	return ret;
> +}
> +
> +int run_hook_with_custom_index(const char *index_file, const char *name, ...)
> +{
> +	const char *hook_env[3] =  { NULL };
> +	char index[PATH_MAX];
Sorry being late with the review.
Recently some effort has been put to replace the
  "PATH_MAX/snprintf() combo" with code using strbuf.

So I think for new developed code it could make sense to avoid PATH_MAX from the start. To my knowledge mkpathdup() from path.c can be used, (or are there better ways ?) and the whole function will look like below (not tested)

Show 12 quoted lines
> +	va_list args;
> +	int ret;
> +
> +	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
> +	hook_env[0] = index;
> +
> +	va_start(args, name);
> +	ret = run_hook_ve(hook_env, name, args);
> +	va_end(args);
> +
> +	return ret;
> +}
int run_hook_with_custom_index(const char *index_file, const char *name, 
...)
{
	const char *hook_env[3] =  { NULL };
	char index = mkpathdup("GIT_INDEX_FILE=%s", index_file);
	va_list args;
	int ret;
	hook_env[0] = index;
	va_start(args, name);
	ret = run_hook_ve(hook_env, name, args);
	va_end(args);
	free(index);
	return ret;
}
Show 40 quoted lines
> diff --git a/run-command.h b/run-command.h
> index 6b985af..88460f9 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -47,7 +47,11 @@ int run_command(struct child_process *);
>
>   extern char *find_hook(const char *name);
>   LAST_ARG_MUST_BE_NULL
> -extern int run_hook(const char *index_file, const char *name, ...);
> +extern int run_hook_le(const char *const *env, const char *name, ...);
> +extern int run_hook_ve(const char *const *env, const char *name, va_list args);
> +
> +LAST_ARG_MUST_BE_NULL
> +extern int run_hook_with_custom_index(const char *index_file, const char *name, ...);
>
>   #define RUN_COMMAND_NO_STDIN 1
>   #define RUN_GIT_CMD	     2	/*If this is to be git sub-command */
> diff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh
> index 41dd37a..998a210 100755
> --- a/t/t7514-commit-patch.sh
> +++ b/t/t7514-commit-patch.sh
> @@ -15,7 +15,7 @@ test_expect_success 'setup (initial)' '
>   	git commit -m commit1
>   '
>
> -test_expect_failure 'edit hunk "commit -p -m message"' '
> +test_expect_success 'edit hunk "commit -p -m message"' '
>   	test_when_finished "rm -f editor_was_started" &&
>   	rm -f editor_was_started &&
>   	echo more >>file &&
> @@ -23,7 +23,7 @@ test_expect_failure 'edit hunk "commit -p -m message"' '
>   	test -r editor_was_started
>   '
>
> -test_expect_failure 'edit hunk "commit --dry-run -p -m message"' '
> +test_expect_success 'edit hunk "commit --dry-run -p -m message"' '
>   	test_when_finished "rm -f editor_was_started" &&
>   	rm -f editor_was_started &&
>   	echo more >>file &&
>
Junio C Hamano· Mar 19, 2014, 17:19 UTC · re: Torsten Bögershausen · lore

Re: [PATCH 4/7] commit: fix patch hunk editing with "commit -p -m"

Torsten Bögershausen <tboegi@web.de> writes:
Show 11 quoted lines
>> +int run_hook_with_custom_index(const char *index_file, const char *name, ...)
>> +{
>> +	const char *hook_env[3] =  { NULL };
>> +	char index[PATH_MAX];
> Sorry being late with the review.
>
> Recently some effort has been put to replace the
>  "PATH_MAX/snprintf() combo" with code using strbuf.
>
> So I think for new developed code it could make sense to avoid
> PATH_MAX from the start.

Yes but because this is a compatibility wrapper for an existing function that does the string[PATH_MAX] thing already, it would be clearer to have such a conversion as a separate step.

Benoit Pierre· Mar 18, 2014, 10:00 UTC · re: Benoit Pierre · lore

[PATCH 5/7] merge: fix GIT_EDITOR override for commit hook

Don't set GIT_EDITOR to ":" when calling prepare-commit-msg hook if the editor is going to be called (e.g. with "merge -e").

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 builtin/merge.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to builtin/merge.c +1 −1
diff --git a/builtin/merge.c b/builtin/merge.c
index bdf6655..e15d0e1 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -824,7 +824,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	if (0 < option_edit)
 		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
 	write_merge_msg(&msg);
-	if (run_commit_hook(1, get_index_file(), "prepare-commit-msg",
+	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
 			    git_path("MERGE_MSG"), "merge", NULL))
 		abort_commit(remoteheads, NULL);
 	if (0 < option_edit) {

-- 
1.9.0
Benoit Pierre· Mar 18, 2014, 10:00 UTC · re: Benoit Pierre · lore

[PATCH 6/7] merge hook tests: fix and update tests

- update 'no editor' hook test and add 'editor' hook test
- make sure the tree is reset to a clean state after running a test
  (using test_when_finished) so later tests are not impacted
Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 t/t7505-prepare-commit-msg-hook.sh | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)
Show changes to t/t7505-prepare-commit-msg-hook.sh +21 −6
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 5531abb..03dce09 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -134,14 +134,26 @@ test_expect_success 'with hook (-c)' '
 
 test_expect_success 'with hook (merge)' '
 
-	head=`git rev-parse HEAD` &&
-	git checkout -b other HEAD@{1} &&
-	echo "more" >> file &&
+	test_when_finished "git checkout -f master" &&
+	git checkout -B other HEAD@{1} &&
+	echo "more" >>file &&
+	git add file &&
+	git commit -m other &&
+	git checkout - &&
+	git merge --no-ff other &&
+	test "`git log -1 --pretty=format:%s`" = "merge (no editor)"
+'
+
+test_expect_success 'with hook and editor (merge)' '
+
+	test_when_finished "git checkout -f master" &&
+	git checkout -B other HEAD@{1} &&
+	echo "more" >>file &&
 	git add file &&
 	git commit -m other &&
 	git checkout - &&
-	git merge other &&
-	test "`git log -1 --pretty=format:%s`" = merge
+	env GIT_EDITOR="\"\$FAKE_EDITOR\"" git merge --no-ff -e other &&
+	test "`git log -1 --pretty=format:%s`" = "merge"
 '
 
 cat > "$HOOK" <<'EOF'
@@ -151,6 +163,7 @@ EOF
 
 test_expect_success 'with failing hook' '
 
+	test_when_finished "git checkout -f master" &&
 	head=`git rev-parse HEAD` &&
 	echo "more" >> file &&
 	git add file &&
@@ -160,6 +173,7 @@ test_expect_success 'with failing hook' '
 
 test_expect_success 'with failing hook (--no-verify)' '
 
+	test_when_finished "git checkout -f master" &&
 	head=`git rev-parse HEAD` &&
 	echo "more" >> file &&
 	git add file &&
@@ -169,6 +183,7 @@ test_expect_success 'with failing hook (--no-verify)' '
 
 test_expect_success 'with failing hook (merge)' '
 
+	test_when_finished "git checkout -f master" &&
 	git checkout -B other HEAD@{1} &&
 	echo "more" >> file &&
 	git add file &&
@@ -178,7 +193,7 @@ test_expect_success 'with failing hook (merge)' '
 	exit 1
 	EOF
 	git checkout - &&
-	test_must_fail git merge other
+	test_must_fail git merge --no-ff other
 
 '
 

-- 
1.9.0
Benoit Pierre· Mar 18, 2014, 10:00 UTC · re: Benoit Pierre · lore

[PATCH 7/7] run-command: mark run_hook_with_custom_index as deprecated

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 run-command.h | 1 +
 1 file changed, 1 insertion(+)
Show changes to run-command.h +1 −0
diff --git a/run-command.h b/run-command.h
index 88460f9..3653bfa 100644
--- a/run-command.h
+++ b/run-command.h
@@ -51,6 +51,7 @@ extern int run_hook_le(const char *const *env, const char *name, ...);
 extern int run_hook_ve(const char *const *env, const char *name, va_list args);
 
 LAST_ARG_MUST_BE_NULL
+__attribute__((deprecated))
 extern int run_hook_with_custom_index(const char *index_file, const char *name, ...);
 
 #define RUN_COMMAND_NO_STDIN 1

-- 
1.9.0
Benoit Pierre· Mar 15, 2014, 21:42 UTC · re: Benoit Pierre · lore

[PATCH 4/7] commit: fix patch hunk editing with "commit -p -m"

Don't change git environment: move the GIT_EDITOR=":" override to the hook command subprocess, like it's already done for GIT_INDEX_FILE.

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 builtin/checkout.c      |  8 ++++----
 builtin/clone.c         |  4 ++--
 builtin/commit.c        | 35 ++++++++++++++++++++++++++++-------
 builtin/gc.c            |  2 +-
 builtin/merge.c         |  6 +++---
 commit.h                |  3 +++
 run-command.c           | 44 ++++++++++++++++++++++++++++++++------------
 run-command.h           |  6 +++++-
 t/t7513-commit-patch.sh |  4 ++--
 9 files changed, 80 insertions(+), 32 deletions(-)
Show changes to 9 files +80 −32

builtin/checkout.c, builtin/clone.c, builtin/commit.c, builtin/gc.c, builtin/merge.c, commit.h, run-command.c, run-command.h, t/t7513-commit-patch.sh

diff --git a/builtin/checkout.c b/builtin/checkout.c
index ada51fa..1b86d9c 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -53,10 +53,10 @@ struct checkout_opts {
 static int post_checkout_hook(struct commit *old, struct commit *new,
 			      int changed)
 {
-	return run_hook(NULL, "post-checkout",
-			sha1_to_hex(old ? old->object.sha1 : null_sha1),
-			sha1_to_hex(new ? new->object.sha1 : null_sha1),
-			changed ? "1" : "0", NULL);
+	return run_hook_le(NULL, "post-checkout",
+			   sha1_to_hex(old ? old->object.sha1 : null_sha1),
+			   sha1_to_hex(new ? new->object.sha1 : null_sha1),
+			   changed ? "1" : "0", NULL);
 	/* "new" can be NULL when checking out from the index before
 	   a commit exists. */
 
diff --git a/builtin/clone.c b/builtin/clone.c
index 43e772c..9b3c04d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -660,8 +660,8 @@ static int checkout(void)
 	    commit_locked_index(lock_file))
 		die(_("unable to write new index file"));
 
-	err |= run_hook(NULL, "post-checkout", sha1_to_hex(null_sha1),
-			sha1_to_hex(sha1), "1", NULL);
+	err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
+			   sha1_to_hex(sha1), "1", NULL);
 
 	if (!err && option_recursive)
 		err = run_command_v_opt(argv_submodule, RUN_GIT_CMD);
diff --git a/builtin/commit.c b/builtin/commit.c
index 3783bca..68a90b3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -610,7 +610,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	/* This checks and barfs if author is badly specified */
 	determine_author_info(author_ident);
 
-	if (!no_verify && run_hook(index_file, "pre-commit", NULL))
+	if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit", NULL))
 		return 0;
 
 	if (squash_message) {
@@ -867,8 +867,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		return 0;
 	}
 
-	if (run_hook(index_file, "prepare-commit-msg",
-		     git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
+	if (run_commit_hook(use_editor, index_file, "prepare-commit-msg",
+			    git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
 		return 0;
 
 	if (use_editor) {
@@ -884,7 +884,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	}
 
 	if (!no_verify &&
-	    run_hook(index_file, "commit-msg", git_path(commit_editmsg), NULL)) {
+	    run_commit_hook(use_editor, index_file, "commit-msg", git_path(commit_editmsg), NULL)) {
 		return 0;
 	}
 
@@ -1068,8 +1068,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		use_editor = 0;
 	if (0 <= edit_flag)
 		use_editor = edit_flag;
-	if (!use_editor)
-		setenv("GIT_EDITOR", ":", 1);
 
 	/* Sanity check options */
 	if (amend && !current_head)
@@ -1450,6 +1448,29 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
 	return finish_command(&proc);
 }
 
+int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...)
+{
+	const char *hook_env[3] =  { NULL };
+	char index[PATH_MAX];
+	va_list args;
+	int ret;
+
+	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+	hook_env[0] = index;
+
+	/*
+	 * Let the hook know that no editor will be launched.
+	 */
+	if (!editor_is_used)
+		hook_env[1] = "GIT_EDITOR=:";
+
+	va_start(args, name);
+	ret = run_hook_ve(hook_env, name, args);
+	va_end(args);
+
+	return ret;
+}
+
 int cmd_commit(int argc, const char **argv, const char *prefix)
 {
 	static struct wt_status s;
@@ -1674,7 +1695,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		     "not exceeded, and then \"git reset HEAD\" to recover."));
 
 	rerere(0);
-	run_hook(get_index_file(), "post-commit", NULL);
+	run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
 	if (amend && !no_post_rewrite) {
 		struct notes_rewrite_cfg *cfg;
 		cfg = init_copy_notes_for_rewrite("amend");
diff --git a/builtin/gc.c b/builtin/gc.c
index 63d400b..11cf295 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -184,7 +184,7 @@ static int need_to_gc(void)
 	else if (!too_many_loose_objects())
 		return 0;
 
-	if (run_hook(NULL, "pre-auto-gc", NULL))
+	if (run_hook_le(NULL, "pre-auto-gc", NULL))
 		return 0;
 	return 1;
 }
diff --git a/builtin/merge.c b/builtin/merge.c
index f0cf120..bdf6655 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -421,7 +421,7 @@ static void finish(struct commit *head_commit,
 	}
 
 	/* Run a post-merge hook */
-	run_hook(NULL, "post-merge", squash ? "1" : "0", NULL);
+	run_hook_le(NULL, "post-merge", squash ? "1" : "0", NULL);
 
 	strbuf_release(&reflog_message);
 }
@@ -824,8 +824,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	if (0 < option_edit)
 		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
 	write_merge_msg(&msg);
-	if (run_hook(get_index_file(), "prepare-commit-msg",
-		     git_path("MERGE_MSG"), "merge", NULL, NULL))
+	if (run_commit_hook(1, get_index_file(), "prepare-commit-msg",
+			    git_path("MERGE_MSG"), "merge", NULL))
 		abort_commit(remoteheads, NULL);
 	if (0 < option_edit) {
 		if (launch_editor(git_path("MERGE_MSG"), NULL, NULL))
diff --git a/commit.h b/commit.h
index 16d9c43..8d97a5c 100644
--- a/commit.h
+++ b/commit.h
@@ -304,4 +304,7 @@ extern void check_commit_signature(const struct commit* commit, struct signature
 
 int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused);
 
+LAST_ARG_MUST_BE_NULL
+extern int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...);
+
 #endif /* COMMIT_H */
diff --git a/run-command.c b/run-command.c
index 3914d9c..75abc47 100644
--- a/run-command.c
+++ b/run-command.c
@@ -760,13 +760,11 @@ char *find_hook(const char *name)
 	return path;
 }
 
-int run_hook(const char *index_file, const char *name, ...)
+int run_hook_ve(const char *const *env, const char *name, va_list args)
 {
 	struct child_process hook;
 	struct argv_array argv = ARGV_ARRAY_INIT;
-	const char *p, *env[2];
-	char index[PATH_MAX];
-	va_list args;
+	const char *p;
 	int ret;
 
 	p = find_hook(name);
@@ -775,23 +773,45 @@ int run_hook(const char *index_file, const char *name, ...)
 
 	argv_array_push(&argv, p);
 
-	va_start(args, name);
 	while ((p = va_arg(args, const char *)))
 		argv_array_push(&argv, p);
-	va_end(args);
 
 	memset(&hook, 0, sizeof(hook));
 	hook.argv = argv.argv;
+	hook.env = env;
 	hook.no_stdin = 1;
 	hook.stdout_to_stderr = 1;
-	if (index_file) {
-		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
-		env[0] = index;
-		env[1] = NULL;
-		hook.env = env;
-	}
 
 	ret = run_command(&hook);
 	argv_array_clear(&argv);
 	return ret;
 }
+
+int run_hook_le(const char *const *env, const char *name, ...)
+{
+	va_list args;
+	int ret;
+
+	va_start(args, name);
+	ret = run_hook_ve(env, name, args);
+	va_end(args);
+
+	return ret;
+}
+
+int run_hook_with_custom_index(const char *index_file, const char *name, ...)
+{
+	const char *hook_env[3] =  { NULL };
+	char index[PATH_MAX];
+	va_list args;
+	int ret;
+
+	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+	hook_env[0] = index;
+
+	va_start(args, name);
+	ret = run_hook_ve(hook_env, name, args);
+	va_end(args);
+
+	return ret;
+}
diff --git a/run-command.h b/run-command.h
index 6b985af..88460f9 100644
--- a/run-command.h
+++ b/run-command.h
@@ -47,7 +47,11 @@ int run_command(struct child_process *);
 
 extern char *find_hook(const char *name);
 LAST_ARG_MUST_BE_NULL
-extern int run_hook(const char *index_file, const char *name, ...);
+extern int run_hook_le(const char *const *env, const char *name, ...);
+extern int run_hook_ve(const char *const *env, const char *name, va_list args);
+
+LAST_ARG_MUST_BE_NULL
+extern int run_hook_with_custom_index(const char *index_file, const char *name, ...);
 
 #define RUN_COMMAND_NO_STDIN 1
 #define RUN_GIT_CMD	     2	/*If this is to be git sub-command */
diff --git a/t/t7513-commit-patch.sh b/t/t7513-commit-patch.sh
index 9311b0c..67e4cde 100755
--- a/t/t7513-commit-patch.sh
+++ b/t/t7513-commit-patch.sh
@@ -15,14 +15,14 @@ test_expect_success 'setup (initial)' '
 	git commit -m commit1
 '
 
-test_expect_failure 'edit hunk "commit -p -m message"' '
+test_expect_success 'edit hunk "commit -p -m message"' '
 	test_when_finished "rm -f editor_was_started" &&
 	echo more >>file &&
 	echo e | env GIT_EDITOR="touch editor_was_started" git commit -p -m commit2 file &&
 	test -r editor_was_started
 '
 
-test_expect_failure 'edit hunk "commit --dry-run -p -m message"' '
+test_expect_success 'edit hunk "commit --dry-run -p -m message"' '
 	test_when_finished "rm -f editor_was_started" &&
 	echo more >>file &&
 	echo e | env GIT_EDITOR="touch editor_was_started" git commit -p -m commit3 file &&

-- 
1.9.0
Benoit Pierre· Mar 15, 2014, 21:42 UTC · re: Benoit Pierre · lore

[PATCH 5/7] merge: fix GIT_EDITOR override for commit hook

Don't set GIT_EDITOR to ":" when calling prepare-commit-msg hook if the editor is going to be called (e.g. with "merge -e").

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 builtin/merge.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to builtin/merge.c +1 −1
diff --git a/builtin/merge.c b/builtin/merge.c
index bdf6655..e15d0e1 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -824,7 +824,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	if (0 < option_edit)
 		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
 	write_merge_msg(&msg);
-	if (run_commit_hook(1, get_index_file(), "prepare-commit-msg",
+	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
 			    git_path("MERGE_MSG"), "merge", NULL))
 		abort_commit(remoteheads, NULL);
 	if (0 < option_edit) {

-- 
1.9.0
Benoit Pierre· Mar 15, 2014, 21:42 UTC · re: Benoit Pierre · lore

[PATCH 6/7] merge hook tests: fix and update tests

- update 'no editor' hook test and add 'editor' hook test
- make sure the tree is reset to a clean state after running a test
  (using test_when_finished) so later tests are not impacted
Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 t/t7505-prepare-commit-msg-hook.sh | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)
Show changes to t/t7505-prepare-commit-msg-hook.sh +21 −6
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 5531abb..03dce09 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -134,14 +134,26 @@ test_expect_success 'with hook (-c)' '
 
 test_expect_success 'with hook (merge)' '
 
-	head=`git rev-parse HEAD` &&
-	git checkout -b other HEAD@{1} &&
-	echo "more" >> file &&
+	test_when_finished "git checkout -f master" &&
+	git checkout -B other HEAD@{1} &&
+	echo "more" >>file &&
+	git add file &&
+	git commit -m other &&
+	git checkout - &&
+	git merge --no-ff other &&
+	test "`git log -1 --pretty=format:%s`" = "merge (no editor)"
+'
+
+test_expect_success 'with hook and editor (merge)' '
+
+	test_when_finished "git checkout -f master" &&
+	git checkout -B other HEAD@{1} &&
+	echo "more" >>file &&
 	git add file &&
 	git commit -m other &&
 	git checkout - &&
-	git merge other &&
-	test "`git log -1 --pretty=format:%s`" = merge
+	env GIT_EDITOR="\"\$FAKE_EDITOR\"" git merge --no-ff -e other &&
+	test "`git log -1 --pretty=format:%s`" = "merge"
 '
 
 cat > "$HOOK" <<'EOF'
@@ -151,6 +163,7 @@ EOF
 
 test_expect_success 'with failing hook' '
 
+	test_when_finished "git checkout -f master" &&
 	head=`git rev-parse HEAD` &&
 	echo "more" >> file &&
 	git add file &&
@@ -160,6 +173,7 @@ test_expect_success 'with failing hook' '
 
 test_expect_success 'with failing hook (--no-verify)' '
 
+	test_when_finished "git checkout -f master" &&
 	head=`git rev-parse HEAD` &&
 	echo "more" >> file &&
 	git add file &&
@@ -169,6 +183,7 @@ test_expect_success 'with failing hook (--no-verify)' '
 
 test_expect_success 'with failing hook (merge)' '
 
+	test_when_finished "git checkout -f master" &&
 	git checkout -B other HEAD@{1} &&
 	echo "more" >> file &&
 	git add file &&
@@ -178,7 +193,7 @@ test_expect_success 'with failing hook (merge)' '
 	exit 1
 	EOF
 	git checkout - &&
-	test_must_fail git merge other
+	test_must_fail git merge --no-ff other
 
 '
 

-- 
1.9.0
Benoit Pierre· Mar 15, 2014, 21:42 UTC · re: Benoit Pierre · lore

[PATCH 7/7] run-command: mark run_hook_with_custom_index as deprecated

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 run-command.h | 1 +
 1 file changed, 1 insertion(+)
Show changes to run-command.h +1 −0
diff --git a/run-command.h b/run-command.h
index 88460f9..3653bfa 100644
--- a/run-command.h
+++ b/run-command.h
@@ -51,6 +51,7 @@ extern int run_hook_le(const char *const *env, const char *name, ...);
 extern int run_hook_ve(const char *const *env, const char *name, va_list args);
 
 LAST_ARG_MUST_BE_NULL
+__attribute__((deprecated))
 extern int run_hook_with_custom_index(const char *index_file, const char *name, ...);
 
 #define RUN_COMMAND_NO_STDIN 1

-- 
1.9.0
Benoit Pierre· Mar 10, 2014, 18:49 UTC · re: Benoit Pierre · lore

[PATCH 4/7] commit: fix patch hunk editing with "commit -p -m"

Don't change git environment: move the GIT_EDITOR=":" override to the hook command subprocess, like it's already done for GIT_INDEX_FILE.

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 builtin/checkout.c                |  8 +++----
 builtin/clone.c                   |  4 ++--
 builtin/commit.c                  | 35 ++++++++++++++++++++++++-------
 builtin/gc.c                      |  2 +-
 builtin/merge.c                   |  6 +++---
 commit.h                          |  3 +++
 run-command.c                     | 44 ++++++++++++++++++++++++++++-----------
 run-command.h                     |  6 +++++-
 t/t7513-commit_-p_-m_hunk_edit.sh |  2 +-
 9 files changed, 79 insertions(+), 31 deletions(-)
Show changes to 9 files +79 −31

builtin/checkout.c, builtin/clone.c, builtin/commit.c, builtin/gc.c, builtin/merge.c, commit.h, run-command.c, run-command.h, t/t7513-commit_-p_-m_hunk_edit.sh

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5df3837..da423b2 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -53,10 +53,10 @@ struct checkout_opts {
 static int post_checkout_hook(struct commit *old, struct commit *new,
 			      int changed)
 {
-	return run_hook(NULL, "post-checkout",
-			sha1_to_hex(old ? old->object.sha1 : null_sha1),
-			sha1_to_hex(new ? new->object.sha1 : null_sha1),
-			changed ? "1" : "0", NULL);
+    return run_hook_le(NULL, "post-checkout",
+		       sha1_to_hex(old ? old->object.sha1 : null_sha1),
+		       sha1_to_hex(new ? new->object.sha1 : null_sha1),
+		       changed ? "1" : "0", NULL);
 	/* "new" can be NULL when checking out from the index before
 	   a commit exists. */
 
diff --git a/builtin/clone.c b/builtin/clone.c
index 43e772c..9b3c04d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -660,8 +660,8 @@ static int checkout(void)
 	    commit_locked_index(lock_file))
 		die(_("unable to write new index file"));
 
-	err |= run_hook(NULL, "post-checkout", sha1_to_hex(null_sha1),
-			sha1_to_hex(sha1), "1", NULL);
+	err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
+			   sha1_to_hex(sha1), "1", NULL);
 
 	if (!err && option_recursive)
 		err = run_command_v_opt(argv_submodule, RUN_GIT_CMD);
diff --git a/builtin/commit.c b/builtin/commit.c
index 3767478..baf1fc0 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -612,7 +612,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	/* This checks and barfs if author is badly specified */
 	determine_author_info(author_ident);
 
-	if (!no_verify && run_hook(index_file, "pre-commit", NULL))
+	if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit", NULL))
 		return 0;
 
 	if (squash_message) {
@@ -866,8 +866,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		return 0;
 	}
 
-	if (run_hook(index_file, "prepare-commit-msg",
-		     git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
+	if (run_commit_hook(use_editor, index_file, "prepare-commit-msg",
+			    git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
 		return 0;
 
 	if (use_editor) {
@@ -883,7 +883,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	}
 
 	if (!no_verify &&
-	    run_hook(index_file, "commit-msg", git_path(commit_editmsg), NULL)) {
+	    run_commit_hook(use_editor, index_file, "commit-msg", git_path(commit_editmsg), NULL)) {
 		return 0;
 	}
 
@@ -1067,8 +1067,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		use_editor = 0;
 	if (0 <= edit_flag)
 		use_editor = edit_flag;
-	if (!use_editor)
-		setenv("GIT_EDITOR", ":", 1);
 
 	/* Sanity check options */
 	if (amend && !current_head)
@@ -1445,6 +1443,29 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
 	return finish_command(&proc);
 }
 
+int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...)
+{
+	const char *hook_env[3] =  { NULL };
+	char index[PATH_MAX];
+	va_list args;
+	int ret;
+
+	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+	hook_env[0] = index;
+
+	/*
+	 * Let the hook know that no editor will be launched.
+	 */
+	if (!editor_is_used)
+		hook_env[1] = "GIT_EDITOR=:";
+
+	va_start(args, name);
+	ret = run_hook_ve(hook_env, name, args);
+	va_end(args);
+
+	return ret;
+}
+
 int cmd_commit(int argc, const char **argv, const char *prefix)
 {
 	static struct wt_status s;
@@ -1669,7 +1690,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		     "not exceeded, and then \"git reset HEAD\" to recover."));
 
 	rerere(0);
-	run_hook(get_index_file(), "post-commit", NULL);
+	run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
 	if (amend && !no_post_rewrite) {
 		struct notes_rewrite_cfg *cfg;
 		cfg = init_copy_notes_for_rewrite("amend");
diff --git a/builtin/gc.c b/builtin/gc.c
index c19545d..7fa717a 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -179,7 +179,7 @@ static int need_to_gc(void)
 	else if (!too_many_loose_objects())
 		return 0;
 
-	if (run_hook(NULL, "pre-auto-gc", NULL))
+	if (run_hook_le(NULL, "pre-auto-gc", NULL))
 		return 0;
 	return 1;
 }
diff --git a/builtin/merge.c b/builtin/merge.c
index e576a7f..67f312d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -421,7 +421,7 @@ static void finish(struct commit *head_commit,
 	}
 
 	/* Run a post-merge hook */
-	run_hook(NULL, "post-merge", squash ? "1" : "0", NULL);
+	run_hook_le(NULL, "post-merge", squash ? "1" : "0", NULL);
 
 	strbuf_release(&reflog_message);
 }
@@ -821,8 +821,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	if (0 < option_edit)
 		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
 	write_merge_msg(&msg);
-	if (run_hook(get_index_file(), "prepare-commit-msg",
-		     git_path("MERGE_MSG"), "merge", NULL, NULL))
+	if (run_commit_hook(1, get_index_file(), "prepare-commit-msg",
+			    git_path("MERGE_MSG"), "merge", NULL))
 		abort_commit(remoteheads, NULL);
 	if (0 < option_edit) {
 		if (launch_editor(git_path("MERGE_MSG"), NULL, NULL))
diff --git a/commit.h b/commit.h
index 16d9c43..8d97a5c 100644
--- a/commit.h
+++ b/commit.h
@@ -304,4 +304,7 @@ extern void check_commit_signature(const struct commit* commit, struct signature
 
 int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused);
 
+LAST_ARG_MUST_BE_NULL
+extern int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...);
+
 #endif /* COMMIT_H */
diff --git a/run-command.c b/run-command.c
index 3914d9c..75abc47 100644
--- a/run-command.c
+++ b/run-command.c
@@ -760,13 +760,11 @@ char *find_hook(const char *name)
 	return path;
 }
 
-int run_hook(const char *index_file, const char *name, ...)
+int run_hook_ve(const char *const *env, const char *name, va_list args)
 {
 	struct child_process hook;
 	struct argv_array argv = ARGV_ARRAY_INIT;
-	const char *p, *env[2];
-	char index[PATH_MAX];
-	va_list args;
+	const char *p;
 	int ret;
 
 	p = find_hook(name);
@@ -775,23 +773,45 @@ int run_hook(const char *index_file, const char *name, ...)
 
 	argv_array_push(&argv, p);
 
-	va_start(args, name);
 	while ((p = va_arg(args, const char *)))
 		argv_array_push(&argv, p);
-	va_end(args);
 
 	memset(&hook, 0, sizeof(hook));
 	hook.argv = argv.argv;
+	hook.env = env;
 	hook.no_stdin = 1;
 	hook.stdout_to_stderr = 1;
-	if (index_file) {
-		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
-		env[0] = index;
-		env[1] = NULL;
-		hook.env = env;
-	}
 
 	ret = run_command(&hook);
 	argv_array_clear(&argv);
 	return ret;
 }
+
+int run_hook_le(const char *const *env, const char *name, ...)
+{
+	va_list args;
+	int ret;
+
+	va_start(args, name);
+	ret = run_hook_ve(env, name, args);
+	va_end(args);
+
+	return ret;
+}
+
+int run_hook_with_custom_index(const char *index_file, const char *name, ...)
+{
+	const char *hook_env[3] =  { NULL };
+	char index[PATH_MAX];
+	va_list args;
+	int ret;
+
+	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+	hook_env[0] = index;
+
+	va_start(args, name);
+	ret = run_hook_ve(hook_env, name, args);
+	va_end(args);
+
+	return ret;
+}
diff --git a/run-command.h b/run-command.h
index 6b985af..88460f9 100644
--- a/run-command.h
+++ b/run-command.h
@@ -47,7 +47,11 @@ int run_command(struct child_process *);
 
 extern char *find_hook(const char *name);
 LAST_ARG_MUST_BE_NULL
-extern int run_hook(const char *index_file, const char *name, ...);
+extern int run_hook_le(const char *const *env, const char *name, ...);
+extern int run_hook_ve(const char *const *env, const char *name, va_list args);
+
+LAST_ARG_MUST_BE_NULL
+extern int run_hook_with_custom_index(const char *index_file, const char *name, ...);
 
 #define RUN_COMMAND_NO_STDIN 1
 #define RUN_GIT_CMD	     2	/*If this is to be git sub-command */
diff --git a/t/t7513-commit_-p_-m_hunk_edit.sh b/t/t7513-commit_-p_-m_hunk_edit.sh
index 994939a..1f05d32 100755
--- a/t/t7513-commit_-p_-m_hunk_edit.sh
+++ b/t/t7513-commit_-p_-m_hunk_edit.sh
@@ -25,7 +25,7 @@ test_expect_success 'setup (initial)' '
 	EOF
 '
 
-test_expect_failure 'edit hunk "commit -p -m message"' '
+test_expect_success 'edit hunk "commit -p -m message"' '
 	echo e | env GIT_EDITOR="sed s/+line3\$/+line2/ -i" git commit -p -m commit2 file &&
 	git diff HEAD^ HEAD >actual &&
 	test_cmp expect actual

-- 
1.9.0
Jeff King· Mar 10, 2014, 20:07 UTC · re: Benoit Pierre · lore

Re: [PATCH 4/7] commit: fix patch hunk editing with "commit -p -m"

On Mon, Mar 10, 2014 at 07:49:34PM +0100, Benoit Pierre wrote:
Show 15 quoted lines
> Don't change git environment: move the GIT_EDITOR=":" override to the
> hook command subprocess, like it's already done for GIT_INDEX_FILE.
> 
> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
> ---
>  builtin/checkout.c                |  8 +++----
>  builtin/clone.c                   |  4 ++--
>  builtin/commit.c                  | 35 ++++++++++++++++++++++++-------
>  builtin/gc.c                      |  2 +-
>  builtin/merge.c                   |  6 +++---
>  commit.h                          |  3 +++
>  run-command.c                     | 44 ++++++++++++++++++++++++++++-----------
>  run-command.h                     |  6 +++++-
>  t/t7513-commit_-p_-m_hunk_edit.sh |  2 +-
>  9 files changed, 79 insertions(+), 31 deletions(-)

This is a lot of change, and in some ways I think it is making things better overall. However, the simplest fix for this is basically to move the setting of GIT_EDITOR down to after we prepare the index.

Jun Hao (cc'd) has been preparing a series for this based on the Bloomberg git hackday a few weeks ago, but it hasn't been sent to the list yet.

Commits are here:
  https://github.com/bloomberg/git/compare/commit-patch-allow-hunk-editing

if you care to look. I'm not sure which solution is technically superior, but it's worth considering both.

I regret not encouraging Jun to post to the list sooner, as we might have avoided some duplicated effort. But that's a sunk cost, and we should pick up whichever is the best for the project.

-Peff
Jun Hao· Mar 11, 2014, 00:45 UTC · re: Jeff King · lore

Re: [PATCH 4/7] commit: fix patch hunk editing with "commit -p -m"

Jeff King <peff@peff.net> writes:
Show 38 quoted lines
> On Mon, Mar 10, 2014 at 07:49:34PM +0100, Benoit Pierre wrote:
>
>> Don't change git environment: move the GIT_EDITOR=":" override to the
>> hook command subprocess, like it's already done for GIT_INDEX_FILE.
>> 
>> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
>> ---
>>  builtin/checkout.c                |  8 +++----
>>  builtin/clone.c                   |  4 ++--
>>  builtin/commit.c                  | 35 ++++++++++++++++++++++++-------
>>  builtin/gc.c                      |  2 +-
>>  builtin/merge.c                   |  6 +++---
>>  commit.h                          |  3 +++
>>  run-command.c                     | 44 ++++++++++++++++++++++++++++-----------
>>  run-command.h                     |  6 +++++-
>>  t/t7513-commit_-p_-m_hunk_edit.sh |  2 +-
>>  9 files changed, 79 insertions(+), 31 deletions(-)
>
> This is a lot of change, and in some ways I think it is making things
> better overall. However, the simplest fix for this is basically to move
> the setting of GIT_EDITOR down to after we prepare the index.
>
> Jun Hao (cc'd) has been preparing a series for this based on the
> Bloomberg git hackday a few weeks ago, but it hasn't been sent to the
> list yet.
>
> Commits are here:
>
>   https://github.com/bloomberg/git/compare/commit-patch-allow-hunk-editing
>
> if you care to look. I'm not sure which solution is technically
> superior, but it's worth considering both.
>
> I regret not encouraging Jun to post to the list sooner, as we might
> have avoided some duplicated effort. But that's a sunk cost, and we
> should pick up whichever is the best for the project.
>
> -Peff

I like the idea that the environment setting should be done in one place. Just not sure run_hook is the right place tho. If user doesn't have any hook setup, will this kick in? One more question, will this work for dry run? Or dry run doesn't matter in this case?

Thanks - Jun
Benoit Pierre· Mar 11, 2014, 17:56 UTC · re: Jeff King · lore

Re: [PATCH 4/7] commit: fix patch hunk editing with "commit -p -m"

On Mon, Mar 10, 2014 at 9:07 PM, Jeff King <peff@peff.net> wrote:
Show 24 quoted lines
> On Mon, Mar 10, 2014 at 07:49:34PM +0100, Benoit Pierre wrote:
>
>> Don't change git environment: move the GIT_EDITOR=":" override to the
>> hook command subprocess, like it's already done for GIT_INDEX_FILE.
>> [...]
>
> This is a lot of change, and in some ways I think it is making things
> better overall. However, the simplest fix for this is basically to move
> the setting of GIT_EDITOR down to after we prepare the index.
>
> Jun Hao (cc'd) has been preparing a series for this based on the
> Bloomberg git hackday a few weeks ago, but it hasn't been sent to the
> list yet.
>
> Commits are here:
>
>   https://github.com/bloomberg/git/compare/commit-patch-allow-hunk-editing
>
> if you care to look. I'm not sure which solution is technically
> superior, but it's worth considering both.
>
> I regret not encouraging Jun to post to the list sooner, as we might
> have avoided some duplicated effort. But that's a sunk cost, and we
> should pick up whichever is the best for the project.

Replying here instead of to Jun Hao message (I'm not subscribed to the mailing list, so I did not received it):

Jun Hao wrote:
Show 5 quoted lines
> I like the idea that the environment setting should be done in one
> place. Just not sure run_hook is the right place tho. If user doesn't have
> any hook setup, will this kick in?
> One more question, will this work for dry run? Or dry run doesn't matter
> in this case?

According to the original commit, the change to GIT_EDITOR is only here for hooks:

commit 406400ce4f69e79b544dd3539a71b85d99331820
Author: Paolo Bonzini <bonzini@gnu.org>
Date:   Tue Feb 5 11:01:45 2008 +0100
    git-commit: set GIT_EDITOR=: if editor will not be launched
    This is a preparatory patch that provides a simple way for the future
    prepare-commit-msg hook to discover if the editor will be launched.
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

So there is really no reason to set it earlier, and not just in the hook subprocess environment.

Regarding dry run: the bug is present, and my patch fix it too (but is missing a test for this).

As to which patch is better: it's really not for me to decide! It's a question for the maintainer(s), Jun Hao patch is sure much smaller and simpler, mine is more involved and I believe cleaner in the long term: there is no risk of another part of the commit process to be impacted by the change of environment. Also note that my patch changes the merge builtin too: GIT_EDITOR will not be overriden if an editor will be launched (when used with --edit).

-- 
A: Because it destroys the flow of conversation.
Q: Why is top posting dumb?
Jeff King· Mar 11, 2014, 18:00 UTC · re: Benoit Pierre · lore

Re: [PATCH 4/7] commit: fix patch hunk editing with "commit -p -m"

On Tue, Mar 11, 2014 at 06:56:02PM +0100, Benoit Pierre wrote:
Show 16 quoted lines
> According to the original commit, the change to GIT_EDITOR is only
> here for hooks:
> 
> commit 406400ce4f69e79b544dd3539a71b85d99331820
> Author: Paolo Bonzini <bonzini@gnu.org>
> Date:   Tue Feb 5 11:01:45 2008 +0100
> 
>     git-commit: set GIT_EDITOR=: if editor will not be launched
> 
>     This is a preparatory patch that provides a simple way for the future
>     prepare-commit-msg hook to discover if the editor will be launched.
> 
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
> 
> So there is really no reason to set it earlier, and not just in the
> hook subprocess environment.

Ah, you're right. I was thinking that our invocation of launch_editor also respected it. It does, but we never get there at all because use_editor is set to 0. So yeah, it really is just needed for the hook.

Your patch, even though it is a bigger change, keeps the setting to the minimal area, which is cleaner and more maintainable in the long run.

-Peff
Jun Hao· Mar 11, 2014, 18:40 UTC · re: Jeff King · lore

Re: [PATCH 4/7] commit: fix patch hunk editing with

Jeff King <peff <at> peff.net> writes:
Show 10 quoted lines
> 
> Ah, you're right. I was thinking that our invocation of launch_editor
> also respected it. It does, but we never get there at all because
> use_editor is set to 0. So yeah, it really is just needed for the hook.
> 
> Your patch, even though it is a bigger change, keeps the setting to the
> minimal area, which is cleaner and more maintainable in the long run.
> 
> -Peff
>

Oh, didn't realize GIT_EDITOR change is only for hooks. Good catch. I agree Benoit's patch is better for the long term.

Thanks - Jun
Junio C Hamano· Mar 11, 2014, 21:02 UTC · re: Benoit Pierre · lore

Re: [PATCH 4/7] commit: fix patch hunk editing with "commit -p -m"

Benoit Pierre <benoit.pierre@gmail.com> writes:
Show 16 quoted lines
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 5df3837..da423b2 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -53,10 +53,10 @@ struct checkout_opts {
>  static int post_checkout_hook(struct commit *old, struct commit *new,
>  			      int changed)
>  {
> -	return run_hook(NULL, "post-checkout",
> -			sha1_to_hex(old ? old->object.sha1 : null_sha1),
> -			sha1_to_hex(new ? new->object.sha1 : null_sha1),
> -			changed ? "1" : "0", NULL);
> +    return run_hook_le(NULL, "post-checkout",
> +		       sha1_to_hex(old ? old->object.sha1 : null_sha1),
> +		       sha1_to_hex(new ? new->object.sha1 : null_sha1),
> +		       changed ? "1" : "0", NULL);
Funny indentation.
Benoit Pierre· Mar 10, 2014, 18:49 UTC · re: Benoit Pierre · lore

[PATCH 5/7] merge: fix GIT_EDITOR override for commit hook

Don't set GIT_EDITOR to ":" when calling prepare-commit-msg hook if the editor is going to be called (e.g. with "merge -e").

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 builtin/merge.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to builtin/merge.c +1 −1
diff --git a/builtin/merge.c b/builtin/merge.c
index 67f312d..b11a528 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -821,7 +821,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	if (0 < option_edit)
 		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
 	write_merge_msg(&msg);
-	if (run_commit_hook(1, get_index_file(), "prepare-commit-msg",
+	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
 			    git_path("MERGE_MSG"), "merge", NULL))
 		abort_commit(remoteheads, NULL);
 	if (0 < option_edit) {

-- 
1.9.0
Benoit Pierre· Mar 10, 2014, 18:49 UTC · re: Benoit Pierre · lore

[PATCH 6/7] merge hook tests: fix and update tests

- update 'no editor' hook test and add 'editor' hook test
- make sure the tree is reset to a clean state after running a test
  (using test_when_finished) so later tests are not impacted
Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 t/t7505-prepare-commit-msg-hook.sh | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)
Show changes to t/t7505-prepare-commit-msg-hook.sh +21 −6
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 5531abb..03dce09 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -134,14 +134,26 @@ test_expect_success 'with hook (-c)' '
 
 test_expect_success 'with hook (merge)' '
 
-	head=`git rev-parse HEAD` &&
-	git checkout -b other HEAD@{1} &&
-	echo "more" >> file &&
+	test_when_finished "git checkout -f master" &&
+	git checkout -B other HEAD@{1} &&
+	echo "more" >>file &&
+	git add file &&
+	git commit -m other &&
+	git checkout - &&
+	git merge --no-ff other &&
+	test "`git log -1 --pretty=format:%s`" = "merge (no editor)"
+'
+
+test_expect_success 'with hook and editor (merge)' '
+
+	test_when_finished "git checkout -f master" &&
+	git checkout -B other HEAD@{1} &&
+	echo "more" >>file &&
 	git add file &&
 	git commit -m other &&
 	git checkout - &&
-	git merge other &&
-	test "`git log -1 --pretty=format:%s`" = merge
+	env GIT_EDITOR="\"\$FAKE_EDITOR\"" git merge --no-ff -e other &&
+	test "`git log -1 --pretty=format:%s`" = "merge"
 '
 
 cat > "$HOOK" <<'EOF'
@@ -151,6 +163,7 @@ EOF
 
 test_expect_success 'with failing hook' '
 
+	test_when_finished "git checkout -f master" &&
 	head=`git rev-parse HEAD` &&
 	echo "more" >> file &&
 	git add file &&
@@ -160,6 +173,7 @@ test_expect_success 'with failing hook' '
 
 test_expect_success 'with failing hook (--no-verify)' '
 
+	test_when_finished "git checkout -f master" &&
 	head=`git rev-parse HEAD` &&
 	echo "more" >> file &&
 	git add file &&
@@ -169,6 +183,7 @@ test_expect_success 'with failing hook (--no-verify)' '
 
 test_expect_success 'with failing hook (merge)' '
 
+	test_when_finished "git checkout -f master" &&
 	git checkout -B other HEAD@{1} &&
 	echo "more" >> file &&
 	git add file &&
@@ -178,7 +193,7 @@ test_expect_success 'with failing hook (merge)' '
 	exit 1
 	EOF
 	git checkout - &&
-	test_must_fail git merge other
+	test_must_fail git merge --no-ff other
 
 '
 

-- 
1.9.0
Eric Sunshine· Mar 10, 2014, 20:27 UTC · re: Benoit Pierre · lore

Re: [PATCH 6/7] merge hook tests: fix and update tests

On Mon, Mar 10, 2014 at 2:49 PM, Benoit Pierre <benoit.pierre@gmail.com> wrote:
> - update 'no editor' hook test and add 'editor' hook test
> - make sure the tree is reset to a clean state after running a test
>   (using test_when_finished) so later tests are not impacted

As conceptually distinct changes, it might make sense to split these into two patches, one a purely cleanup patch adding test_when_finished, and the second introducing your new test. Doing so would ease the review process and make the intended changes more obvious.

Show 76 quoted lines
> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
> ---
>  t/t7505-prepare-commit-msg-hook.sh | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
> index 5531abb..03dce09 100755
> --- a/t/t7505-prepare-commit-msg-hook.sh
> +++ b/t/t7505-prepare-commit-msg-hook.sh
> @@ -134,14 +134,26 @@ test_expect_success 'with hook (-c)' '
>
>  test_expect_success 'with hook (merge)' '
>
> -       head=`git rev-parse HEAD` &&
> -       git checkout -b other HEAD@{1} &&
> -       echo "more" >> file &&
> +       test_when_finished "git checkout -f master" &&
> +       git checkout -B other HEAD@{1} &&
> +       echo "more" >>file &&
> +       git add file &&
> +       git commit -m other &&
> +       git checkout - &&
> +       git merge --no-ff other &&
> +       test "`git log -1 --pretty=format:%s`" = "merge (no editor)"
> +'
> +
> +test_expect_success 'with hook and editor (merge)' '
> +
> +       test_when_finished "git checkout -f master" &&
> +       git checkout -B other HEAD@{1} &&
> +       echo "more" >>file &&
>         git add file &&
>         git commit -m other &&
>         git checkout - &&
> -       git merge other &&
> -       test "`git log -1 --pretty=format:%s`" = merge
> +       env GIT_EDITOR="\"\$FAKE_EDITOR\"" git merge --no-ff -e other &&
> +       test "`git log -1 --pretty=format:%s`" = "merge"
>  '
>
>  cat > "$HOOK" <<'EOF'
> @@ -151,6 +163,7 @@ EOF
>
>  test_expect_success 'with failing hook' '
>
> +       test_when_finished "git checkout -f master" &&
>         head=`git rev-parse HEAD` &&
>         echo "more" >> file &&
>         git add file &&
> @@ -160,6 +173,7 @@ test_expect_success 'with failing hook' '
>
>  test_expect_success 'with failing hook (--no-verify)' '
>
> +       test_when_finished "git checkout -f master" &&
>         head=`git rev-parse HEAD` &&
>         echo "more" >> file &&
>         git add file &&
> @@ -169,6 +183,7 @@ test_expect_success 'with failing hook (--no-verify)' '
>
>  test_expect_success 'with failing hook (merge)' '
>
> +       test_when_finished "git checkout -f master" &&
>         git checkout -B other HEAD@{1} &&
>         echo "more" >> file &&
>         git add file &&
> @@ -178,7 +193,7 @@ test_expect_success 'with failing hook (merge)' '
>         exit 1
>         EOF
>         git checkout - &&
> -       test_must_fail git merge other
> +       test_must_fail git merge --no-ff other
>
>  '
>
> --
> 1.9.0
Benoit Pierre· Mar 10, 2014, 18:49 UTC · re: Benoit Pierre · lore

[PATCH 7/7] run-command: mark run_hook_with_custom_index as deprecated

---
 run-command.h | 1 +
 1 file changed, 1 insertion(+)
Show changes to run-command.h +1 −0
diff --git a/run-command.h b/run-command.h
index 88460f9..3653bfa 100644
--- a/run-command.h
+++ b/run-command.h
@@ -51,6 +51,7 @@ extern int run_hook_le(const char *const *env, const char *name, ...);
 extern int run_hook_ve(const char *const *env, const char *name, va_list args);
 
 LAST_ARG_MUST_BE_NULL
+__attribute__((deprecated))
 extern int run_hook_with_custom_index(const char *index_file, const char *name, ...);
 
 #define RUN_COMMAND_NO_STDIN 1

-- 
1.9.0
brian m. carlson· Mar 11, 2014, 01:00 UTC · re: Benoit Pierre · lore

Re: [PATCH 7/7] run-command: mark run_hook_with_custom_index as deprecated

On Mon, Mar 10, 2014 at 07:49:37PM +0100, Benoit Pierre wrote:
Show 13 quoted lines
> ---
>  run-command.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/run-command.h b/run-command.h
> index 88460f9..3653bfa 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -51,6 +51,7 @@ extern int run_hook_le(const char *const *env, const char *name, ...);
>  extern int run_hook_ve(const char *const *env, const char *name, va_list args);
>  
>  LAST_ARG_MUST_BE_NULL
> +__attribute__((deprecated))

It doesn't appear that we use the deprecated attribute anywhere else in the code. Wouldn't it just be better to change the places that use this and then remove the function altogether? I imagine your current patch might introduce a number of warnings that some people would rather avoid.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187
Benoit Pierre· Mar 11, 2014, 17:37 UTC · re: brian m. carlson · lore

Re: [PATCH 7/7] run-command: mark run_hook_with_custom_index as deprecated

On Tue, Mar 11, 2014 at 2:00 AM, brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 20 quoted lines
> On Mon, Mar 10, 2014 at 07:49:37PM +0100, Benoit Pierre wrote:
>> ---
>>  run-command.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/run-command.h b/run-command.h
>> index 88460f9..3653bfa 100644
>> --- a/run-command.h
>> +++ b/run-command.h
>> @@ -51,6 +51,7 @@ extern int run_hook_le(const char *const *env, const char *name, ...);
>>  extern int run_hook_ve(const char *const *env, const char *name, va_list args);
>>
>>  LAST_ARG_MUST_BE_NULL
>> +__attribute__((deprecated))
>
> It doesn't appear that we use the deprecated attribute anywhere else in
> the code.  Wouldn't it just be better to change the places that use this
> and then remove the function altogether?  I imagine your current patch
> might introduce a number of warnings that some people would rather
> avoid.

This last patch is optional. There are no callers to run_hook_with_custom_index, so no warnings for now. See discussion on first draft as to why I added it:

http://article.gmane.org/gmane.comp.version-control.git/243561
-- 
A: Because it destroys the flow of conversation.
Q: Why is top posting dumb?
Jonathan Nieder· Apr 3, 2014, 22:11 UTC · re: Benoit Pierre · lore

Re: [PATCH V2 0/7] fix hunk editing with 'commit -p -m'

Hi,
A quick note for the future:
Benoit Pierre wrote:
> This patch fixes the fact that hunk editing with 'commit -p -m' does not work:
> GIT_EDITOR is set to ':' to indicate to hooks that no editor will be launched,
> which result in the 'hunk edit' option not launching the editor (and selecting
> the whole hunk).

This information should have gone in the relevant patch's commit message itself. That way, people don't have to hunt down the relevant mailing list thread to understand the patch.

Generally a cover letter should say as little as possible (mostly "here is what patch you might want to look at first, and here is an overview of why the patches are in this particular order").

Thanks for a nice fix. Perhaps we'll see more in the future, hence this note. :) And if you have ideas for where an explanation of this could go in the documentation (somewhere in Documentation/SubmittingPatches?), that would be welcome too.

Thanks, Jonathan

← back to recent threads