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

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

[Benoit Pierre]

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

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

[Benoit Pierre]

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 t/t7505-prepare-commit-msg-hook.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

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

[Benoit Pierre]

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 t/t7505-prepare-commit-msg-hook.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

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

[Benoit Pierre]

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

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

[Eric Sunshine]

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

> 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

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

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

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

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

[Philip Oakley]

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/

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

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

[Junio C Hamano]

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.

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

[Torsten Bögershausen]

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)

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

[Benoit Pierre]

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

-- 
A: Because it destroys the flow of conversation.
Q: Why is top posting dumb?

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

[Torsten Bögershausen]

On 2014-03-15 17.11, Benoit Pierre wrote:
> 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 :-)

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

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

[Benoit Pierre]

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 t/t7505-prepare-commit-msg-hook.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

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

[Benoit Pierre]

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 t/t7505-prepare-commit-msg-hook.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

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

[Benoit Pierre]

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

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

[Junio C Hamano]

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.

Thanks.

> 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

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

[Benoit Pierre]

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

-- 
A: Because it destroys the flow of conversation.
Q: Why is top posting dumb?

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

[Eric Sunshine]

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:
>>
>>> 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.

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

[Benoit Pierre]

On Mon, Mar 17, 2014 at 8:51 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 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?

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

[Junio C Hamano]

Benoit Pierre <benoit.pierre@gmail.com> writes:

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

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

[Junio C Hamano]

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.

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

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

[Benoit Pierre]

On Mon, Mar 17, 2014 at 7:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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?

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

[Junio C Hamano]

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.

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

[Benoit Pierre]

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 t/t7505-prepare-commit-msg-hook.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

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

[Benoit Pierre]

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 t/t7505-prepare-commit-msg-hook.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

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

[Benoit Pierre]

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

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

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

[Benoit Pierre]

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

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

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

[Torsten Bögershausen]

On 03/18/2014 11:00 AM, 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/t7514-commit-patch.sh |  4 ++--
>   9 files changed, 80 insertions(+), 32 deletions(-)
>
[]
> 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)
> -	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)

> +	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;
}




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

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

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

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

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

[Benoit Pierre]

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

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

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

[Benoit Pierre]

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

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

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

[Benoit Pierre]

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 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))
 extern int run_hook_with_custom_index(const char *index_file, const char *name, ...);
 
 #define RUN_COMMAND_NO_STDIN 1
-- 
1.9.0

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

[Junio C Hamano]

Thanks; will replace what has been on 'pu' with the updated series.

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

[Benoit Pierre]

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

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

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

[Benoit Pierre]

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

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

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

[Benoit Pierre]

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

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

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

[Benoit Pierre]

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 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))
 extern int run_hook_with_custom_index(const char *index_file, const char *name, ...);
 
 #define RUN_COMMAND_NO_STDIN 1
-- 
1.9.0

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

[Benoit Pierre]

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

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

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

[Jeff King]

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

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

[Jun Hao]

Jeff King <peff@peff.net> writes:

> 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

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

[Benoit Pierre]

On Mon, Mar 10, 2014 at 9:07 PM, Jeff King <peff@peff.net> wrote:
> 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:
> 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?

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

[Jeff King]

On Tue, Mar 11, 2014 at 06:56:02PM +0100, Benoit Pierre wrote:

> 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

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

[Jun Hao]

Jeff King <peff <at> peff.net> writes:

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

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

[Junio C Hamano]

Benoit Pierre <benoit.pierre@gmail.com> writes:

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

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

[Benoit Pierre]

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

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

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

[Benoit Pierre]

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

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

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

[Eric Sunshine]

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.

> 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

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

[Benoit Pierre]

---
 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))
 extern int run_hook_with_custom_index(const char *index_file, const char *name, ...);
 
 #define RUN_COMMAND_NO_STDIN 1
-- 
1.9.0

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1009 bytes --]

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.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Benoit Pierre]

On Tue, Mar 11, 2014 at 2:00 AM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> 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?

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

[Jonathan Nieder]

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