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

STALE3753d

Revision v1 of 2 in this series.

9 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Benoit Pierre <hidden>
Date: 2016-06-15 23:00:12

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"

From: Benoit Pierre <hidden>
Date: 2016-06-15 23:00:12

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

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

From: Benoit Pierre <hidden>
Date: 2016-06-15 23:00:12

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

From: Benoit Pierre <hidden>
Date: 2016-06-15 23:00:12

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

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

From: Benoit Pierre <hidden>
Date: 2016-06-15 23:00:12

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

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

From: Benoit Pierre <hidden>
Date: 2016-06-15 23:00:12

Signed-off-by: Benoit Pierre <redacted>
---
 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

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

From: Benoit Pierre <hidden>
Date: 2016-06-15 23:00:12

Signed-off-by: Benoit Pierre <redacted>
---
 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 1/6] test patch hunk editing with "commit -p -m"

From: Eric Sunshine <hidden>
Date: 2016-06-15 23:00:13

On Thu, Mar 6, 2014 at 9:50 AM, Benoit Pierre [off-list ref] 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 <redacted>
---
 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.
quoted hunk
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 4/6] merge hook tests: fix and update tests

From: Eric Sunshine <hidden>
Date: 2016-06-15 23:00:13

On Thu, Mar 6, 2014 at 9:50 AM, Benoit Pierre [off-list ref] wrote:
quoted hunk
- 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 <redacted>
---
 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 ...".
quoted hunk
+       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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help