threads / patch / 36074

patch, 6 partstest patch hunk editing with "commit -p -m"

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

## tl;dr

16 messages between Mar 6, 2014 and Mar 7, 2014. Diffs are folded; open one to read it.

replies: 15people: 3as markdown or json

Benoit Pierre· Mar 6, 2014, 14:50 UTC · lore

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

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
Benoit Pierre· Mar 6, 2014, 14:50 UTC · re: Benoit Pierre · lore

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
Show changes to t/t7513-commit_-p_-m_hunk_edit.sh +36 −0
diff --git a/t/t7513-commit_-p_-m_hunk_edit.sh b/t/t7513-commit_-p_-m_hunk_edit.sh
new file mode 100755
index 0000000..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
Eric Sunshine· Mar 6, 2014, 22:07 UTC · re: Benoit Pierre · lore

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

On Thu, Mar 6, 2014 at 9:50 AM, Benoit Pierre <benoit.pierre@gmail.com> wrote:
Show 10 quoted lines
> Add (failing) test: with commit changing the environment to let hooks
> now that no editor will be used (by setting GIT_EDITOR to ":"), the
> "edit hunk" functionality does not work (no editor is launched and the
> whole hunk is committed).
>
> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
> ---
>  t/t7513-commit_-p_-m_hunk_edit.sh | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh
A rather unusual filename.
Show 21 quoted lines
> diff --git a/t/t7513-commit_-p_-m_hunk_edit.sh b/t/t7513-commit_-p_-m_hunk_edit.sh
> new file mode 100755
> index 0000000..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.
Show 14 quoted lines
> +       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.

Show 5 quoted lines
> +test_expect_failure 'edit hunk "commit -p -m message"' '
> +       echo e | env GIT_EDITOR="sed s/+line3\$/+line2/ -i" git commit -p -m commit2 file &&
> +       git diff HEAD^ HEAD >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
Junio C Hamano· Mar 6, 2014, 23:11 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 9 quoted lines
>> +test_expect_failure 'edit hunk "commit -p -m message"' '
>> +       echo e | env GIT_EDITOR="sed s/+line3\$/+line2/ -i" git commit -p -m commit2 file &&
>> +       git diff HEAD^ HEAD >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.

Benoit Pierre· Mar 6, 2014, 14:50 UTC · re: Benoit Pierre · lore

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

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

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 builtin/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(-)
Show changes to 6 files +51 −23

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

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
Junio C Hamano· Mar 6, 2014, 21:25 UTC · re: Benoit Pierre · lore

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

Benoit Pierre <benoit.pierre@gmail.com> writes:
Show 18 quoted lines
> +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);
Show 11 quoted lines
> 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.

Junio C Hamano· Mar 6, 2014, 21:47 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 8 quoted lines
> 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.
Benoit Pierre· Mar 6, 2014, 14:50 UTC · re: Benoit Pierre · lore

[PATCH 3/6] merge: fix GIT_EDITOR override for commit hook

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

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 builtin/merge.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to builtin/merge.c +1 −1
diff --git a/builtin/merge.c b/builtin/merge.c
index 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
Benoit Pierre· Mar 6, 2014, 14:50 UTC · re: Benoit Pierre · lore

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

- 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(-)
Show changes to t/t7505-prepare-commit-msg-hook.sh +16 −1
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
Eric Sunshine· Mar 6, 2014, 22:11 UTC · re: Benoit Pierre · lore

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

On Thu, Mar 6, 2014 at 9:50 AM, Benoit Pierre <benoit.pierre@gmail.com> wrote:
Show 26 quoted lines
> - 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 ...".

Show 36 quoted lines
> +       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
Benoit Pierre· Mar 6, 2014, 14:50 UTC · re: Benoit Pierre · lore

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

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 t/t7505-prepare-commit-msg-hook.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t7505-prepare-commit-msg-hook.sh +1 −1
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 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
Junio C Hamano· Mar 6, 2014, 21:27 UTC · re: Benoit Pierre · lore

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

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.
Show 16 quoted lines
>  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 - &&
Benoit Pierre· Mar 6, 2014, 14:50 UTC · re: Benoit Pierre · lore

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

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 t/t7505-prepare-commit-msg-hook.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to t/t7505-prepare-commit-msg-hook.sh +2 −2
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 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
Junio C Hamano· Mar 6, 2014, 21:29 UTC · re: Benoit Pierre · lore

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

Benoit Pierre <benoit.pierre@gmail.com> writes:
Show 15 quoted lines
> 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
	)
Junio C Hamano· Mar 7, 2014, 00:35 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 23 quoted lines
> 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.
Show 7 quoted lines
> To be portable, we can do this instead.
>
> 	(
> 		GIT_EDITOR=... &&
>         	export GIT_EDITOR &&
>                 test_must_fail git commit -c $head
> 	)
Junio C Hamano· Mar 6, 2014, 21:15 UTC · re: Benoit Pierre · lore

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

Benoit Pierre <benoit.pierre@gmail.com> writes:
Show 10 quoted lines
> 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 ;-)

← back to recent threads