{"thread":{"id":"36074","subject":"[PATCH 1/6] test patch hunk editing with \"commit -p -m\"","startedAt":"2014-03-06T14:50:18Z","lastAt":"2014-03-07T00:35:47Z","messageCount":16,"participants":["Benoit Pierre","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"236162","messageId":"1394117424-29780-1-git-send-email-benoit.pierre@gmail.com","threadId":"36074","inReplyTo":null,"subject":"[PATCH 0/6] fix hunk editing with 'commit -p -m'","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-06T14:50:18Z","receivedAt":"2014-03-06T14:50:18Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"This patch fixes the fact that hunk editing with 'commit -p -m' does not work:\nGIT_EDITOR is set to ':' to indicate to hooks that no editor will be launched,\nwhich result in the 'hunk edit' option not launching the editor (and selecting\nthe whole hunk).\n\nThe fix consists in deferring the GIT_EDITOR override to the hook subprocess,\nlike it's already done for GIT_INDEX_FILE:\n- modify 'run_hook' so the first parameter is the environment to set\n- add a 'run_hook_v' variant that take a va_list\n- add a new 'run_commit_hook' helper (to set both GIT_EDITOR and GIT_INDEX_FILE)\n\nN.B.: the merge builtin 'prepare-commit-msg' hook handling has also been updated\nto be consistent; i.e. GIT_EDITOR will not be set to ':' if the '--edit' option\nis used.\n\nBenoit Pierre (6):\n  test patch hunk editing with \"commit -p -m\"\n  commit: fix patch hunk editing with \"commit -p -m\"\n  merge: fix GIT_EDITOR override for commit hook\n  merge hook tests: fix and update tests\n  merge hook tests: fix missing '&&' in test\n  merge hook tests: use 'test_must_fail' instead of '!'\n\n builtin/commit.c                   | 35 ++++++++++++++++++++++++++++-------\n builtin/merge.c                    |  4 ++--\n commit.h                           |  3 +++\n run-command.c                      | 27 +++++++++++++++------------\n run-command.h                      |  3 ++-\n t/t7505-prepare-commit-msg-hook.sh | 23 +++++++++++++++++++----\n t/t7513-commit_-p_-m_hunk_edit.sh  | 37 +++++++++++++++++++++++++++++++++++++\n 7 files changed, 106 insertions(+), 26 deletions(-)\n create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh\n\n-- \n1.9.0\n"},{"id":"236159","messageId":"1394117424-29780-2-git-send-email-benoit.pierre@gmail.com","threadId":"36074","inReplyTo":"1394117424-29780-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 1/6] test patch hunk editing with \"commit -p -m\"","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-06T14:50:19Z","receivedAt":"2014-03-06T14:50:19Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"Add (failing) test: with commit changing the environment to let hooks\nnow that no editor will be used (by setting GIT_EDITOR to \":\"), the\n\"edit hunk\" functionality does not work (no editor is launched and the\nwhole hunk is committed).\n\nSigned-off-by: Benoit Pierre <benoit.pierre@gmail.com>\n---\n t/t7513-commit_-p_-m_hunk_edit.sh | 37 +++++++++++++++++++++++++++++++++++++\n 1 file changed, 37 insertions(+)\n create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh\n\ndiff --git a/t/t7513-commit_-p_-m_hunk_edit.sh b/t/t7513-commit_-p_-m_hunk_edit.sh\nnew file mode 100755\nindex 0000000..e0ad905\n--- /dev/null\n+++ b/t/t7513-commit_-p_-m_hunk_edit.sh\n@@ -0,0 +1,37 @@\n+#!/bin/sh\n+\n+test_description='hunk edit with \"commit -p -m\"'\n+. ./test-lib.sh\n+\n+if ! test_have_prereq PERL\n+then\n+\tskip_all=\"skipping '$test_description' tests, perl not available\"\n+\ttest_done\n+fi\n+\n+test_expect_success 'setup (initial)' '\n+\techo line1 >file &&\n+\tgit add file &&\n+\tgit commit -m commit1\n+\techo line3 >>file\n+'\n+\n+test_expect_success 'setup expected' '\n+cat >expected <<EOF\n+diff --git a/file b/file\n+index a29bdeb..c0d0fb4 100644\n+--- a/file\n++++ b/file\n+@@ -1 +1,2 @@\n+ line1\n++line2\n+EOF\n+'\n+\n+test_expect_failure 'edit hunk \"commit -p -m message\"' '\n+\techo e | env GIT_EDITOR=\"sed s/+line3\\$/+line2/ -i\" git commit -p -m commit2 file &&\n+\tgit diff HEAD^ HEAD >diff &&\n+\ttest_cmp expected diff\n+'\n+\n+test_done\n-- \n1.9.0\n"},{"id":"236163","messageId":"1394117424-29780-3-git-send-email-benoit.pierre@gmail.com","threadId":"36074","inReplyTo":"1394117424-29780-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 2/6] commit: fix patch hunk editing with \"commit -p -m\"","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-06T14:50:20Z","receivedAt":"2014-03-06T14:50:20Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"Don't change git environment: move the GIT_EDITOR=\":\" override to the\nhook command subprocess, like it's already done for GIT_INDEX_FILE.\n\nSigned-off-by: Benoit Pierre <benoit.pierre@gmail.com>\n---\n builtin/commit.c                  | 35 ++++++++++++++++++++++++++++-------\n builtin/merge.c                   |  4 ++--\n commit.h                          |  3 +++\n run-command.c                     | 27 +++++++++++++++------------\n run-command.h                     |  3 ++-\n t/t7513-commit_-p_-m_hunk_edit.sh |  2 +-\n 6 files changed, 51 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 3767478..2f5a44f 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -612,7 +612,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t/* This checks and barfs if author is badly specified */\n \tdetermine_author_info(author_ident);\n \n-\tif (!no_verify && run_hook(index_file, \"pre-commit\", NULL))\n+\tif (!no_verify && run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n \t\treturn 0;\n \n \tif (squash_message) {\n@@ -866,8 +866,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\treturn 0;\n \t}\n \n-\tif (run_hook(index_file, \"prepare-commit-msg\",\n-\t\t     git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))\n+\tif (run_commit_hook(use_editor, index_file, \"prepare-commit-msg\",\n+\t\t\t    git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))\n \t\treturn 0;\n \n \tif (use_editor) {\n@@ -883,7 +883,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t}\n \n \tif (!no_verify &&\n-\t    run_hook(index_file, \"commit-msg\", git_path(commit_editmsg), NULL)) {\n+\t    run_commit_hook(use_editor, index_file, \"commit-msg\", git_path(commit_editmsg), NULL)) {\n \t\treturn 0;\n \t}\n \n@@ -1067,8 +1067,6 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \t\tuse_editor = 0;\n \tif (0 <= edit_flag)\n \t\tuse_editor = edit_flag;\n-\tif (!use_editor)\n-\t\tsetenv(\"GIT_EDITOR\", \":\", 1);\n \n \t/* Sanity check options */\n \tif (amend && !current_head)\n@@ -1445,6 +1443,29 @@ static int run_rewrite_hook(const unsigned char *oldsha1,\n \treturn finish_command(&proc);\n }\n \n+int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...)\n+{\n+\tconst char *hook_env[3] =  { NULL };\n+\tchar index[PATH_MAX];\n+\tva_list args;\n+\tint ret;\n+\n+\tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", index_file);\n+\thook_env[0] = index;\n+\n+\t/*\n+\t * Let the hook know that no editor will be launched.\n+\t */\n+\tif (!editor_is_used)\n+\t\thook_env[1] = \"GIT_EDITOR=:\";\n+\n+\tva_start(args, name);\n+\tret = run_hook_v(hook_env, name, args);\n+\tva_end(args);\n+\n+\treturn ret;\n+}\n+\n int cmd_commit(int argc, const char **argv, const char *prefix)\n {\n \tstatic struct wt_status s;\n@@ -1669,7 +1690,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t     \"not exceeded, and then \\\"git reset HEAD\\\" to recover.\"));\n \n \trerere(0);\n-\trun_hook(get_index_file(), \"post-commit\", NULL);\n+\trun_commit_hook(use_editor, get_index_file(), \"post-commit\", NULL);\n \tif (amend && !no_post_rewrite) {\n \t\tstruct notes_rewrite_cfg *cfg;\n \t\tcfg = init_copy_notes_for_rewrite(\"amend\");\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex e576a7f..d2a1bfe 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -821,8 +821,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \tif (0 < option_edit)\n \t\tstrbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);\n \twrite_merge_msg(&msg);\n-\tif (run_hook(get_index_file(), \"prepare-commit-msg\",\n-\t\t     git_path(\"MERGE_MSG\"), \"merge\", NULL, NULL))\n+\tif (run_commit_hook(1, get_index_file(), \"prepare-commit-msg\",\n+\t\t\t    git_path(\"MERGE_MSG\"), \"merge\", NULL))\n \t\tabort_commit(remoteheads, NULL);\n \tif (0 < option_edit) {\n \t\tif (launch_editor(git_path(\"MERGE_MSG\"), NULL, NULL))\ndiff --git a/commit.h b/commit.h\nindex 16d9c43..8d97a5c 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -304,4 +304,7 @@ extern void check_commit_signature(const struct commit* commit, struct signature\n \n int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused);\n \n+LAST_ARG_MUST_BE_NULL\n+extern int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...);\n+\n #endif /* COMMIT_H */\ndiff --git a/run-command.c b/run-command.c\nindex 3914d9c..4e9be12 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -760,13 +760,11 @@ char *find_hook(const char *name)\n \treturn path;\n }\n \n-int run_hook(const char *index_file, const char *name, ...)\n+int run_hook_v(const char *const *env, const char *name, va_list args)\n {\n \tstruct child_process hook;\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n-\tconst char *p, *env[2];\n-\tchar index[PATH_MAX];\n-\tva_list args;\n+\tconst char *p;\n \tint ret;\n \n \tp = find_hook(name);\n@@ -775,23 +773,28 @@ int run_hook(const char *index_file, const char *name, ...)\n \n \targv_array_push(&argv, p);\n \n-\tva_start(args, name);\n \twhile ((p = va_arg(args, const char *)))\n \t\targv_array_push(&argv, p);\n-\tva_end(args);\n \n \tmemset(&hook, 0, sizeof(hook));\n \thook.argv = argv.argv;\n+\thook.env = env;\n \thook.no_stdin = 1;\n \thook.stdout_to_stderr = 1;\n-\tif (index_file) {\n-\t\tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", index_file);\n-\t\tenv[0] = index;\n-\t\tenv[1] = NULL;\n-\t\thook.env = env;\n-\t}\n \n \tret = run_command(&hook);\n \targv_array_clear(&argv);\n \treturn ret;\n }\n+\n+int run_hook(const char *const *env, const char *name, ...)\n+{\n+\tva_list args;\n+\tint ret;\n+\n+\tva_start(args, name);\n+\tret = run_hook_v(env, name, args);\n+\tva_end(args);\n+\n+\treturn ret;\n+}\ndiff --git a/run-command.h b/run-command.h\nindex 6b985af..9f89e9f 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -47,7 +47,8 @@ int run_command(struct child_process *);\n \n extern char *find_hook(const char *name);\n LAST_ARG_MUST_BE_NULL\n-extern int run_hook(const char *index_file, const char *name, ...);\n+extern int run_hook(const char *const *env, const char *name, ...);\n+extern int run_hook_v(const char *const *env, const char *name, va_list args);\n \n #define RUN_COMMAND_NO_STDIN 1\n #define RUN_GIT_CMD\t     2\t/*If this is to be git sub-command */\ndiff --git a/t/t7513-commit_-p_-m_hunk_edit.sh b/t/t7513-commit_-p_-m_hunk_edit.sh\nindex e0ad905..ffc4120 100755\n--- a/t/t7513-commit_-p_-m_hunk_edit.sh\n+++ b/t/t7513-commit_-p_-m_hunk_edit.sh\n@@ -28,7 +28,7 @@ index a29bdeb..c0d0fb4 100644\n EOF\n '\n \n-test_expect_failure 'edit hunk \"commit -p -m message\"' '\n+test_expect_success 'edit hunk \"commit -p -m message\"' '\n \techo e | env GIT_EDITOR=\"sed s/+line3\\$/+line2/ -i\" git commit -p -m commit2 file &&\n \tgit diff HEAD^ HEAD >diff &&\n \ttest_cmp expected diff\n-- \n1.9.0\n"},{"id":"236160","messageId":"1394117424-29780-4-git-send-email-benoit.pierre@gmail.com","threadId":"36074","inReplyTo":"1394117424-29780-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 3/6] merge: fix GIT_EDITOR override for commit hook","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-06T14:50:21Z","receivedAt":"2014-03-06T14:50:21Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"Don't set GIT_EDITOR to \":\" when calling prepare-commit-msg hook if the\neditor is going to be called (e.g. with \"merge -e\").\n\nSigned-off-by: Benoit Pierre <benoit.pierre@gmail.com>\n---\n builtin/merge.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex d2a1bfe..da7cafe 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -821,7 +821,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \tif (0 < option_edit)\n \t\tstrbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);\n \twrite_merge_msg(&msg);\n-\tif (run_commit_hook(1, get_index_file(), \"prepare-commit-msg\",\n+\tif (run_commit_hook(0 < option_edit, get_index_file(), \"prepare-commit-msg\",\n \t\t\t    git_path(\"MERGE_MSG\"), \"merge\", NULL))\n \t\tabort_commit(remoteheads, NULL);\n \tif (0 < option_edit) {\n-- \n1.9.0\n"},{"id":"236161","messageId":"1394117424-29780-5-git-send-email-benoit.pierre@gmail.com","threadId":"36074","inReplyTo":"1394117424-29780-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 4/6] merge hook tests: fix and update tests","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-06T14:50:22Z","receivedAt":"2014-03-06T14:50:22Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"- update 'no editor' hook test and add 'editor' hook test\n- reset tree at the beginning of failing hook tests to avoid failures\n  due to an unclean tree after running a previous test\n\nSigned-off-by: Benoit Pierre <benoit.pierre@gmail.com>\n---\n t/t7505-prepare-commit-msg-hook.sh | 17 ++++++++++++++++-\n 1 file changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\nindex 3573751..ae7b2db 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -141,7 +141,19 @@ test_expect_success 'with hook (merge)' '\n \tgit commit -m other &&\n \tgit checkout - &&\n \tgit merge other &&\n-\ttest \"`git log -1 --pretty=format:%s`\" = merge\n+\ttest \"`git log -1 --pretty=format:%s`\" = \"merge (no editor)\"\n+'\n+\n+test_expect_success 'with hook and editor (merge)' '\n+\n+\thead=`git rev-parse HEAD` &&\n+\tgit checkout -B other HEAD@{1} &&\n+\techo \"more\" >> file &&\n+\tgit add file &&\n+\tgit commit -m other &&\n+\tgit checkout - &&\n+\tenv GIT_EDITOR=\"\\\"\\$FAKE_EDITOR\\\"\" git merge -e other &&\n+\ttest \"`git log -1 --pretty=format:%s`\" = \"merge\"\n '\n \n cat > \"$HOOK\" <<'EOF'\n@@ -151,6 +163,7 @@ EOF\n \n test_expect_success 'with failing hook' '\n \n+\tgit reset --hard &&\n \thead=`git rev-parse HEAD` &&\n \techo \"more\" >> file &&\n \tgit add file &&\n@@ -160,6 +173,7 @@ test_expect_success 'with failing hook' '\n \n test_expect_success 'with failing hook (--no-verify)' '\n \n+\tgit reset --hard &&\n \thead=`git rev-parse HEAD` &&\n \techo \"more\" >> file &&\n \tgit add file &&\n@@ -169,6 +183,7 @@ test_expect_success 'with failing hook (--no-verify)' '\n \n test_expect_success 'with failing hook (merge)' '\n \n+\tgit reset --hard &&\n \tgit checkout -B other HEAD@{1} &&\n \techo \"more\" >> file &&\n \tgit add file &&\n-- \n1.9.0\n"},{"id":"236165","messageId":"1394117424-29780-6-git-send-email-benoit.pierre@gmail.com","threadId":"36074","inReplyTo":"1394117424-29780-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 5/6] merge hook tests: fix missing '&&' in test","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-06T14:50:23Z","receivedAt":"2014-03-06T14:50:23Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>\n---\n t/t7505-prepare-commit-msg-hook.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\nindex ae7b2db..604c06e 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -189,7 +189,7 @@ test_expect_success 'with failing hook (merge)' '\n \tgit add file &&\n \trm -f \"$HOOK\" &&\n \tgit commit -m other &&\n-\twrite_script \"$HOOK\" <<-EOF\n+\twrite_script \"$HOOK\" <<-EOF &&\n \texit 1\n \tEOF\n \tgit checkout - &&\n-- \n1.9.0\n"},{"id":"236164","messageId":"1394117424-29780-7-git-send-email-benoit.pierre@gmail.com","threadId":"36074","inReplyTo":"1394117424-29780-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 6/6] merge hook tests: use 'test_must_fail' instead of '!'","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-06T14:50:24Z","receivedAt":"2014-03-06T14:50:24Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>\n---\n t/t7505-prepare-commit-msg-hook.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\nindex 604c06e..1be6cec 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -167,7 +167,7 @@ test_expect_success 'with failing hook' '\n \thead=`git rev-parse HEAD` &&\n \techo \"more\" >> file &&\n \tgit add file &&\n-\t! GIT_EDITOR=\"\\\"\\$FAKE_EDITOR\\\"\" git commit -c $head\n+\ttest_must_fail env GIT_EDITOR=\"\\\"\\$FAKE_EDITOR\\\"\" git commit -c $head\n \n '\n \n@@ -177,7 +177,7 @@ test_expect_success 'with failing hook (--no-verify)' '\n \thead=`git rev-parse HEAD` &&\n \techo \"more\" >> file &&\n \tgit add file &&\n-\t! GIT_EDITOR=\"\\\"\\$FAKE_EDITOR\\\"\" git commit --no-verify -c $head\n+\ttest_must_fail env GIT_EDITOR=\"\\\"\\$FAKE_EDITOR\\\"\" git commit --no-verify -c $head\n \n '\n \n-- \n1.9.0\n"},{"id":"236199","messageId":"xmqqvbvrvxk8.fsf@gitster.dls.corp.google.com","threadId":"36074","inReplyTo":"1394117424-29780-1-git-send-email-benoit.pierre@gmail.com","subject":"Re: [PATCH 0/6] fix hunk editing with 'commit -p -m'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-06T21:15:02Z","receivedAt":"2014-03-06T21:15:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Benoit Pierre <benoit.pierre@gmail.com> writes:\n\n> This patch fixes the fact that hunk editing with 'commit -p -m' does not work:\n> GIT_EDITOR is set to ':' to indicate to hooks that no editor will be launched,\n> which result in the 'hunk edit' option not launching the editor (and selecting\n> the whole hunk).\n>\n> The fix consists in deferring the GIT_EDITOR override to the hook subprocess,\n> like it's already done for GIT_INDEX_FILE:\n> - modify 'run_hook' so the first parameter is the environment to set\n> - add a 'run_hook_v' variant that take a va_list\n> - add a new 'run_commit_hook' helper (to set both GIT_EDITOR and GIT_INDEX_FILE)\n\nI sense that this is in line with one of the \"leftover bits\" items I\nkeep in http://git-blame.blogspot.com/p/leftover-bits.html,\nespecially http://thread.gmane.org/gmane.comp.version-control.git/192669/focus=192806\n;-)\n"},{"id":"236202","messageId":"xmqqr46fvx39.fsf@gitster.dls.corp.google.com","threadId":"36074","inReplyTo":"1394117424-29780-3-git-send-email-benoit.pierre@gmail.com","subject":"Re: [PATCH 2/6] commit: fix patch hunk editing with \"commit -p -m\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-06T21:25:14Z","receivedAt":"2014-03-06T21:25:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Benoit Pierre <benoit.pierre@gmail.com> writes:\n\n> +int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...)\n> +{\n> +\tconst char *hook_env[3] =  { NULL };\n> +\tchar index[PATH_MAX];\n> +\tva_list args;\n> +\tint ret;\n> +\n> +\tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", index_file);\n> +\thook_env[0] = index;\n> +\n> +\t/*\n> +\t * Let the hook know that no editor will be launched.\n> +\t */\n> +\tif (!editor_is_used)\n> +\t\thook_env[1] = \"GIT_EDITOR=:\";\n> +\n> +\tva_start(args, name);\n> +\tret = run_hook_v(hook_env, name, args);\n\n> diff --git a/run-command.c b/run-command.c\n> index 3914d9c..4e9be12 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -760,13 +760,11 @@ char *find_hook(const char *name)\n>  \treturn path;\n>  }\n>  \n> -int run_hook(const char *index_file, const char *name, ...)\n> +int run_hook_v(const char *const *env, const char *name, va_list args)\n>  {\n\nI think you named it as \"foo_v()\" for \"this takes va_list\" in a way\nsimilar to the \"v\" in \"execv()\", but this also takes environment, so\nperhaps we want to say \"ve\" instead?\n\nOther than that, I like it---I admit that I am biased that I\nessentially did the same earlier but with a _le variant ;-)\n\n> +int run_hook(const char *const *env, const char *name, ...)\n> +{\n\nI'd rather not to see this changed in the same commit, so that any\nother topic in-flight that adds a new call to run_hook() that expects\nto pass the index file as its first parameter will not be broken.\n\nName it run_hook_le() (name modelled after execle()), and call it in\nyour change where you add new calls to this function, and add a thin\nwrapper run_hook() that preserves the traditional \"We can pass only\nthe index-file\" for new callers we do not even know about on the\ntopics in flight.\n\nLater we can eradicate callers of run_hook() that treats the index-file\nspecially, which was a grave mistake in a public API.\n"},{"id":"236203","messageId":"xmqqmwh3vx08.fsf@gitster.dls.corp.google.com","threadId":"36074","inReplyTo":"1394117424-29780-6-git-send-email-benoit.pierre@gmail.com","subject":"Re: [PATCH 5/6] merge hook tests: fix missing '&&' in test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-06T21:27:03Z","receivedAt":"2014-03-06T21:27:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Benoit Pierre <benoit.pierre@gmail.com> writes:\n\n> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>\n> ---\n\nPlease have these preparatory fix-ups (i.e. the changes that would\nbe immediately useful even if it turns out that the main body of the\nseries were not ready for inclusion) early in the series, not late\nas 5th patch of a 6 patch series.\n\nThanks.\n\n>  t/t7505-prepare-commit-msg-hook.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\n> index ae7b2db..604c06e 100755\n> --- a/t/t7505-prepare-commit-msg-hook.sh\n> +++ b/t/t7505-prepare-commit-msg-hook.sh\n> @@ -189,7 +189,7 @@ test_expect_success 'with failing hook (merge)' '\n>  \tgit add file &&\n>  \trm -f \"$HOOK\" &&\n>  \tgit commit -m other &&\n> -\twrite_script \"$HOOK\" <<-EOF\n> +\twrite_script \"$HOOK\" <<-EOF &&\n>  \texit 1\n>  \tEOF\n>  \tgit checkout - &&\n"},{"id":"236205","messageId":"xmqqiorrvwvh.fsf@gitster.dls.corp.google.com","threadId":"36074","inReplyTo":"1394117424-29780-7-git-send-email-benoit.pierre@gmail.com","subject":"Re: [PATCH 6/6] merge hook tests: use 'test_must_fail' instead of '!'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-06T21:29:54Z","receivedAt":"2014-03-06T21:29:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Benoit Pierre <benoit.pierre@gmail.com> writes:\n\n> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>\n> ---\n>  t/t7505-prepare-commit-msg-hook.sh | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\n> index 604c06e..1be6cec 100755\n> --- a/t/t7505-prepare-commit-msg-hook.sh\n> +++ b/t/t7505-prepare-commit-msg-hook.sh\n> @@ -167,7 +167,7 @@ test_expect_success 'with failing hook' '\n>  \thead=`git rev-parse HEAD` &&\n>  \techo \"more\" >> file &&\n>  \tgit add file &&\n> -\t! GIT_EDITOR=\"\\\"\\$FAKE_EDITOR\\\"\" git commit -c $head\n> +\ttest_must_fail env GIT_EDITOR=\"\\\"\\$FAKE_EDITOR\\\"\" git commit -c $head\n\nThanks. It is good that you avoided the common pitfall of attempting\n\n\tGIT_EDITOR=... test_must_fail git commit -c $head    ;# WRONG\n\nbut do we assume everybody has \"env\"?\n\nTo be portable, we can do this instead.\n\n\t(\n\t\tGIT_EDITOR=... &&\n        \texport GIT_EDITOR &&\n                test_must_fail git commit -c $head\n\t)\n"},{"id":"236208","messageId":"xmqqeh2fvw1h.fsf@gitster.dls.corp.google.com","threadId":"36074","inReplyTo":"xmqqr46fvx39.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/6] commit: fix patch hunk editing with \"commit -p -m\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-06T21:47:54Z","receivedAt":"2014-03-06T21:47:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Name it run_hook_le() (name modelled after execle()), and call it in\n> your change where you add new calls to this function, and add a thin\n> wrapper run_hook() that preserves the traditional \"We can pass only\n> the index-file\" for new callers we do not even know about on the\n> topics in flight.\n>\n> Later we can eradicate callers of run_hook() that treats the index-file\n> specially, which was a grave mistake in a public API.\n\nI am also OK if the patch _removed_ run_hook() and renamed the one\nwith the current semantics to run_hook_with_custom_index() or\nsomething.\n\nIt would allow us to catch any in-flight topic we do not know about\nthat adds a call to run_hook() expecting that it would take a custom\nindex file.  We will see a link failure, and then we can evil-merge\nto update such a callsite to call run_hook_with_custom_index().\n\nAn updated run_hook() with different function signature (which is\nin this patch) will also let us notice, but the evil-merge to fix\nthe resulting mess will have to be larger than necessary, which is\nnot what we want.\n\nThanks.\n"},{"id":"236211","messageId":"CAPig+cTTCsMDJ_B73+cbedrxoEqbM_txV6JYWmCq-LyX12TpNw@mail.gmail.com","threadId":"36074","inReplyTo":"1394117424-29780-2-git-send-email-benoit.pierre@gmail.com","subject":"Re: [PATCH 1/6] test patch hunk editing with \"commit -p -m\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-06T22:07:13Z","receivedAt":"2014-03-06T22:07:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 6, 2014 at 9:50 AM, Benoit Pierre <benoit.pierre@gmail.com> wrote:\n> Add (failing) test: with commit changing the environment to let hooks\n> now that no editor will be used (by setting GIT_EDITOR to \":\"), the\n> \"edit hunk\" functionality does not work (no editor is launched and the\n> whole hunk is committed).\n>\n> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>\n> ---\n>  t/t7513-commit_-p_-m_hunk_edit.sh | 37 +++++++++++++++++++++++++++++++++++++\n>  1 file changed, 37 insertions(+)\n>  create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh\n\nA rather unusual filename.\n\n> diff --git a/t/t7513-commit_-p_-m_hunk_edit.sh b/t/t7513-commit_-p_-m_hunk_edit.sh\n> new file mode 100755\n> index 0000000..e0ad905\n> --- /dev/null\n> +++ b/t/t7513-commit_-p_-m_hunk_edit.sh\n> @@ -0,0 +1,37 @@\n> +#!/bin/sh\n> +\n> +test_description='hunk edit with \"commit -p -m\"'\n> +. ./test-lib.sh\n> +\n> +if ! test_have_prereq PERL\n> +then\n> +       skip_all=\"skipping '$test_description' tests, perl not available\"\n> +       test_done\n> +fi\n> +\n> +test_expect_success 'setup (initial)' '\n> +       echo line1 >file &&\n> +       git add file &&\n> +       git commit -m commit1\n\nBroken &&-chain.\n\n> +       echo line3 >>file\n> +'\n> +\n> +test_expect_success 'setup expected' '\n> +cat >expected <<EOF\n> +diff --git a/file b/file\n> +index a29bdeb..c0d0fb4 100644\n> +--- a/file\n> ++++ b/file\n> +@@ -1 +1,2 @@\n> + line1\n> ++line2\n> +EOF\n> +'\n\nIf you use <<-EOF, you can indent the content you're dumping into\n'expected', which can enhance readability. Even better, use <<-\\EOF to\nindicate that you don't want interpolation done within the content.\n\n> +test_expect_failure 'edit hunk \"commit -p -m message\"' '\n> +       echo e | env GIT_EDITOR=\"sed s/+line3\\$/+line2/ -i\" git commit -p -m commit2 file &&\n> +       git diff HEAD^ HEAD >diff &&\n> +       test_cmp expected diff\n> +'\n\nIf you ever add more tests, is it likely that they will be using the\nsame 'expected' file used by this test? If not, perhaps it makes sense\nto move creation of 'expected' into the test itself.\n\n> +test_done\n> --\n> 1.9.0\n"},{"id":"236212","messageId":"CAPig+cRXLQVcZFbawc=TzBAMqb3XwBFGZH1viMnTwtoqz6p-zA@mail.gmail.com","threadId":"36074","inReplyTo":"1394117424-29780-5-git-send-email-benoit.pierre@gmail.com","subject":"Re: [PATCH 4/6] merge hook tests: fix and update tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-06T22:11:47Z","receivedAt":"2014-03-06T22:11:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 6, 2014 at 9:50 AM, Benoit Pierre <benoit.pierre@gmail.com> wrote:\n> - update 'no editor' hook test and add 'editor' hook test\n> - reset tree at the beginning of failing hook tests to avoid failures\n>   due to an unclean tree after running a previous test\n>\n> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>\n> ---\n>  t/t7505-prepare-commit-msg-hook.sh | 17 ++++++++++++++++-\n>  1 file changed, 16 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\n> index 3573751..ae7b2db 100755\n> --- a/t/t7505-prepare-commit-msg-hook.sh\n> +++ b/t/t7505-prepare-commit-msg-hook.sh\n> @@ -141,7 +141,19 @@ test_expect_success 'with hook (merge)' '\n>         git commit -m other &&\n>         git checkout - &&\n>         git merge other &&\n> -       test \"`git log -1 --pretty=format:%s`\" = merge\n> +       test \"`git log -1 --pretty=format:%s`\" = \"merge (no editor)\"\n> +'\n> +\n> +test_expect_success 'with hook and editor (merge)' '\n> +\n> +       head=`git rev-parse HEAD` &&\n> +       git checkout -B other HEAD@{1} &&\n> +       echo \"more\" >> file &&\n\nDrop space after >>.\n\n> +       git add file &&\n> +       git commit -m other &&\n> +       git checkout - &&\n\nIf one of the commands above this point fails, then \"git checkout -\"\nwill never be invoked, and the working tree may be left in a state\ninconsistent with what following tests expect. Instead, perhaps use\ntest_when_finished to perform the restorative \"git checkout ...\".\n\n> +       env GIT_EDITOR=\"\\\"\\$FAKE_EDITOR\\\"\" git merge -e other &&\n> +       test \"`git log -1 --pretty=format:%s`\" = \"merge\"\n>  '\n>\n>  cat > \"$HOOK\" <<'EOF'\n> @@ -151,6 +163,7 @@ EOF\n>\n>  test_expect_success 'with failing hook' '\n>\n> +       git reset --hard &&\n>         head=`git rev-parse HEAD` &&\n>         echo \"more\" >> file &&\n>         git add file &&\n> @@ -160,6 +173,7 @@ test_expect_success 'with failing hook' '\n>\n>  test_expect_success 'with failing hook (--no-verify)' '\n>\n> +       git reset --hard &&\n>         head=`git rev-parse HEAD` &&\n>         echo \"more\" >> file &&\n>         git add file &&\n> @@ -169,6 +183,7 @@ test_expect_success 'with failing hook (--no-verify)' '\n>\n>  test_expect_success 'with failing hook (merge)' '\n>\n> +       git reset --hard &&\n>         git checkout -B other HEAD@{1} &&\n>         echo \"more\" >> file &&\n>         git add file &&\n> --\n> 1.9.0\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"236216","messageId":"xmqq61nqx6qj.fsf@gitster.dls.corp.google.com","threadId":"36074","inReplyTo":"CAPig+cTTCsMDJ_B73+cbedrxoEqbM_txV6JYWmCq-LyX12TpNw@mail.gmail.com","subject":"Re: [PATCH 1/6] test patch hunk editing with \"commit -p -m\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-06T23:11:32Z","receivedAt":"2014-03-06T23:11:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n>> +test_expect_failure 'edit hunk \"commit -p -m message\"' '\n>> +       echo e | env GIT_EDITOR=\"sed s/+line3\\$/+line2/ -i\" git commit -p -m commit2 file &&\n>> +       git diff HEAD^ HEAD >diff &&\n>> +       test_cmp expected diff\n>> +'\n>\n> If you ever add more tests, is it likely that they will be using the\n> same 'expected' file used by this test? If not, perhaps it makes sense\n> to move creation of 'expected' into the test itself.\n\nAll good points.  Also, I think we try to use \"expect\" (not\n\"expected\") vs \"actual\" (not \"diff\") in most of the tests.\n"},{"id":"236225","messageId":"xmqqk3c6vo9o.fsf@gitster.dls.corp.google.com","threadId":"36074","inReplyTo":"xmqqiorrvwvh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 6/6] merge hook tests: use 'test_must_fail' instead of '!'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-07T00:35:47Z","receivedAt":"2014-03-07T00:35:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Benoit Pierre <benoit.pierre@gmail.com> writes:\n>\n>> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>\n>> ---\n>>  t/t7505-prepare-commit-msg-hook.sh | 4 ++--\n>>  1 file changed, 2 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\n>> index 604c06e..1be6cec 100755\n>> --- a/t/t7505-prepare-commit-msg-hook.sh\n>> +++ b/t/t7505-prepare-commit-msg-hook.sh\n>> @@ -167,7 +167,7 @@ test_expect_success 'with failing hook' '\n>>  \thead=`git rev-parse HEAD` &&\n>>  \techo \"more\" >> file &&\n>>  \tgit add file &&\n>> -\t! GIT_EDITOR=\"\\\"\\$FAKE_EDITOR\\\"\" git commit -c $head\n>> +\ttest_must_fail env GIT_EDITOR=\"\\\"\\$FAKE_EDITOR\\\"\" git commit -c $head\n>\n> Thanks. It is good that you avoided the common pitfall of attempting\n>\n> \tGIT_EDITOR=... test_must_fail git commit -c $head    ;# WRONG\n>\n> but do we assume everybody has \"env\"?\n\nIt turns out that the answer to this question seems to be \"yes, we\nalready do.\"; so the patch is probably OK as-is.\n\nThanks.\n\n> To be portable, we can do this instead.\n>\n> \t(\n> \t\tGIT_EDITOR=... &&\n>         \texport GIT_EDITOR &&\n>                 test_must_fail git commit -c $head\n> \t)\n"}]}