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

[PATCH 0/6] 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:
- modify 'run_hook' so the first parameter is the environment to set
- add a 'run_hook_v' variant that take a va_list
- add a new 'run_commit_hook' helper (to set both GIT_EDITOR and GIT_INDEX_FILE)

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 (6):
  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
  merge hook tests: fix missing '&&' in test
  merge hook tests: use 'test_must_fail' instead of '!'

 builtin/commit.c                   | 35 ++++++++++++++++++++++++++++-------
 builtin/merge.c                    |  4 ++--
 commit.h                           |  3 +++
 run-command.c                      | 27 +++++++++++++++------------
 run-command.h                      |  3 ++-
 t/t7505-prepare-commit-msg-hook.sh | 23 +++++++++++++++++++----
 t/t7513-commit_-p_-m_hunk_edit.sh  | 37 +++++++++++++++++++++++++++++++++++++
 7 files changed, 106 insertions(+), 26 deletions(-)
 create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh

-- 
1.9.0

[PATCH 1/6] 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 | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 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..e0ad905
--- /dev/null
+++ b/t/t7513-commit_-p_-m_hunk_edit.sh
@@ -0,0 +1,37 @@
+#!/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
+'
+
+test_expect_success 'setup expected' '
+cat >expected <<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 >diff &&
+	test_cmp expected diff
+'
+
+test_done
-- 
1.9.0

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

[Eric Sunshine]

On Thu, Mar 6, 2014 at 9:50 AM, 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 | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh

A rather unusual filename.

> 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..e0ad905
> --- /dev/null
> +++ b/t/t7513-commit_-p_-m_hunk_edit.sh
> @@ -0,0 +1,37 @@
> +#!/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

Broken &&-chain.

> +       echo line3 >>file
> +'
> +
> +test_expect_success 'setup expected' '
> +cat >expected <<EOF
> +diff --git a/file b/file
> +index a29bdeb..c0d0fb4 100644
> +--- a/file
> ++++ b/file
> +@@ -1 +1,2 @@
> + line1
> ++line2
> +EOF
> +'

If you use <<-EOF, you can indent the content you're dumping into
'expected', which can enhance readability. Even better, use <<-\EOF to
indicate that you don't want interpolation done within the content.

> +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 >diff &&
> +       test_cmp expected diff
> +'

If you ever add more tests, is it likely that they will be using the
same 'expected' file used by this test? If not, perhaps it makes sense
to move creation of 'expected' into the test itself.

> +test_done
> --
> 1.9.0

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +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 >diff &&
>> +       test_cmp expected diff
>> +'
>
> If you ever add more tests, is it likely that they will be using the
> same 'expected' file used by this test? If not, perhaps it makes sense
> to move creation of 'expected' into the test itself.

All good points.  Also, I think we try to use "expect" (not
"expected") vs "actual" (not "diff") in most of the tests.

[PATCH 2/6] 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/commit.c                  | 35 ++++++++++++++++++++++++++++-------
 builtin/merge.c                   |  4 ++--
 commit.h                          |  3 +++
 run-command.c                     | 27 +++++++++++++++------------
 run-command.h                     |  3 ++-
 t/t7513-commit_-p_-m_hunk_edit.sh |  2 +-
 6 files changed, 51 insertions(+), 23 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 3767478..2f5a44f 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_v(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/merge.c b/builtin/merge.c
index e576a7f..d2a1bfe 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -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..4e9be12 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_v(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,28 @@ 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(const char *const *env, const char *name, ...)
+{
+	va_list args;
+	int ret;
+
+	va_start(args, name);
+	ret = run_hook_v(env, name, args);
+	va_end(args);
+
+	return ret;
+}
diff --git a/run-command.h b/run-command.h
index 6b985af..9f89e9f 100644
--- a/run-command.h
+++ b/run-command.h
@@ -47,7 +47,8 @@ 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(const char *const *env, const char *name, ...);
+extern int run_hook_v(const char *const *env, const char *name, va_list args);
 
 #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 e0ad905..ffc4120 100755
--- a/t/t7513-commit_-p_-m_hunk_edit.sh
+++ b/t/t7513-commit_-p_-m_hunk_edit.sh
@@ -28,7 +28,7 @@ index a29bdeb..c0d0fb4 100644
 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 >diff &&
 	test_cmp expected diff
-- 
1.9.0

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

[Junio C Hamano]

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

> +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_v(hook_env, name, args);

> diff --git a/run-command.c b/run-command.c
> index 3914d9c..4e9be12 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_v(const char *const *env, const char *name, va_list args)
>  {

I think you named it as "foo_v()" for "this takes va_list" in a way
similar to the "v" in "execv()", but this also takes environment, so
perhaps we want to say "ve" instead?

Other than that, I like it---I admit that I am biased that I
essentially did the same earlier but with a _le variant ;-)

> +int run_hook(const char *const *env, const char *name, ...)
> +{

I'd rather not to see this changed in the same commit, so that any
other topic in-flight that adds a new call to run_hook() that expects
to pass the index file as its first parameter will not be broken.

Name it run_hook_le() (name modelled after execle()), and call it in
your change where you add new calls to this function, and add a thin
wrapper run_hook() that preserves the traditional "We can pass only
the index-file" for new callers we do not even know about on the
topics in flight.

Later we can eradicate callers of run_hook() that treats the index-file
specially, which was a grave mistake in a public API.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Name it run_hook_le() (name modelled after execle()), and call it in
> your change where you add new calls to this function, and add a thin
> wrapper run_hook() that preserves the traditional "We can pass only
> the index-file" for new callers we do not even know about on the
> topics in flight.
>
> Later we can eradicate callers of run_hook() that treats the index-file
> specially, which was a grave mistake in a public API.

I am also OK if the patch _removed_ run_hook() and renamed the one
with the current semantics to run_hook_with_custom_index() or
something.

It would allow us to catch any in-flight topic we do not know about
that adds a call to run_hook() expecting that it would take a custom
index file.  We will see a link failure, and then we can evil-merge
to update such a callsite to call run_hook_with_custom_index().

An updated run_hook() with different function signature (which is
in this patch) will also let us notice, but the evil-merge to fix
the resulting mess will have to be larger than necessary, which is
not what we want.

Thanks.

[PATCH 3/6] 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 d2a1bfe..da7cafe 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 4/6] merge hook tests: fix and update tests

[Benoit Pierre]

- update 'no editor' hook test and add 'editor' hook test
- reset tree at the beginning of failing hook tests to avoid failures
  due to an unclean tree after running a previous test

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

diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 3573751..ae7b2db 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -141,7 +141,19 @@ test_expect_success 'with hook (merge)' '
 	git commit -m other &&
 	git checkout - &&
 	git merge other &&
-	test "`git log -1 --pretty=format:%s`" = merge
+	test "`git log -1 --pretty=format:%s`" = "merge (no editor)"
+'
+
+test_expect_success 'with hook and editor (merge)' '
+
+	head=`git rev-parse HEAD` &&
+	git checkout -B other HEAD@{1} &&
+	echo "more" >> file &&
+	git add file &&
+	git commit -m other &&
+	git checkout - &&
+	env GIT_EDITOR="\"\$FAKE_EDITOR\"" git merge -e other &&
+	test "`git log -1 --pretty=format:%s`" = "merge"
 '
 
 cat > "$HOOK" <<'EOF'
@@ -151,6 +163,7 @@ EOF
 
 test_expect_success 'with failing hook' '
 
+	git reset --hard &&
 	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)' '
 
+	git reset --hard &&
 	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)' '
 
+	git reset --hard &&
 	git checkout -B other HEAD@{1} &&
 	echo "more" >> file &&
 	git add file &&
-- 
1.9.0

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

[Eric Sunshine]

On Thu, Mar 6, 2014 at 9:50 AM, Benoit Pierre <benoit.pierre@gmail.com> wrote:
> - update 'no editor' hook test and add 'editor' hook test
> - reset tree at the beginning of failing hook tests to avoid failures
>   due to an unclean tree after running a previous test
>
> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
> ---
>  t/t7505-prepare-commit-msg-hook.sh | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
> index 3573751..ae7b2db 100755
> --- a/t/t7505-prepare-commit-msg-hook.sh
> +++ b/t/t7505-prepare-commit-msg-hook.sh
> @@ -141,7 +141,19 @@ test_expect_success 'with hook (merge)' '
>         git commit -m other &&
>         git checkout - &&
>         git merge other &&
> -       test "`git log -1 --pretty=format:%s`" = merge
> +       test "`git log -1 --pretty=format:%s`" = "merge (no editor)"
> +'
> +
> +test_expect_success 'with hook and editor (merge)' '
> +
> +       head=`git rev-parse HEAD` &&
> +       git checkout -B other HEAD@{1} &&
> +       echo "more" >> file &&

Drop space after >>.

> +       git add file &&
> +       git commit -m other &&
> +       git checkout - &&

If one of the commands above this point fails, then "git checkout -"
will never be invoked, and the working tree may be left in a state
inconsistent with what following tests expect. Instead, perhaps use
test_when_finished to perform the restorative "git checkout ...".

> +       env GIT_EDITOR="\"\$FAKE_EDITOR\"" git merge -e other &&
> +       test "`git log -1 --pretty=format:%s`" = "merge"
>  '
>
>  cat > "$HOOK" <<'EOF'
> @@ -151,6 +163,7 @@ EOF
>
>  test_expect_success 'with failing hook' '
>
> +       git reset --hard &&
>         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)' '
>
> +       git reset --hard &&
>         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)' '
>
> +       git reset --hard &&
>         git checkout -B other HEAD@{1} &&
>         echo "more" >> file &&
>         git add file &&
> --
> 1.9.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/6] 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 ae7b2db..604c06e 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -189,7 +189,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

Re: [PATCH 5/6] merge hook tests: fix missing '&&' in test

[Junio C Hamano]

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

> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
> ---

Please have these preparatory fix-ups (i.e. the changes that would
be immediately useful even if it turns out that the main body of the
series were not ready for inclusion) early in the series, not late
as 5th patch of a 6 patch series.

Thanks.

>  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 ae7b2db..604c06e 100755
> --- a/t/t7505-prepare-commit-msg-hook.sh
> +++ b/t/t7505-prepare-commit-msg-hook.sh
> @@ -189,7 +189,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 - &&

[PATCH 6/6] 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 604c06e..1be6cec 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -167,7 +167,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
 
 '
 
@@ -177,7 +177,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

Re: [PATCH 6/6] merge hook tests: use 'test_must_fail' instead of '!'

[Junio C Hamano]

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

> 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 604c06e..1be6cec 100755
> --- a/t/t7505-prepare-commit-msg-hook.sh
> +++ b/t/t7505-prepare-commit-msg-hook.sh
> @@ -167,7 +167,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

Thanks. It is good that you avoided the common pitfall of attempting

	GIT_EDITOR=... test_must_fail git commit -c $head    ;# WRONG

but do we assume everybody has "env"?

To be portable, we can do this instead.

	(
		GIT_EDITOR=... &&
        	export GIT_EDITOR &&
                test_must_fail git commit -c $head
	)

Re: [PATCH 6/6] merge hook tests: use 'test_must_fail' instead of '!'

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Benoit Pierre <benoit.pierre@gmail.com> writes:
>
>> 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 604c06e..1be6cec 100755
>> --- a/t/t7505-prepare-commit-msg-hook.sh
>> +++ b/t/t7505-prepare-commit-msg-hook.sh
>> @@ -167,7 +167,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
>
> Thanks. It is good that you avoided the common pitfall of attempting
>
> 	GIT_EDITOR=... test_must_fail git commit -c $head    ;# WRONG
>
> but do we assume everybody has "env"?

It turns out that the answer to this question seems to be "yes, we
already do."; so the patch is probably OK as-is.

Thanks.

> To be portable, we can do this instead.
>
> 	(
> 		GIT_EDITOR=... &&
>         	export GIT_EDITOR &&
>                 test_must_fail git commit -c $head
> 	)

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

[Junio C Hamano]

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

> 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:
> - modify 'run_hook' so the first parameter is the environment to set
> - add a 'run_hook_v' variant that take a va_list
> - add a new 'run_commit_hook' helper (to set both GIT_EDITOR and GIT_INDEX_FILE)

I sense that this is in line with one of the "leftover bits" items I
keep in http://git-blame.blogspot.com/p/leftover-bits.html,
especially http://thread.gmane.org/gmane.comp.version-control.git/192669/focus=192806
;-)