{"thread":{"id":"36111","subject":"[PATCH V2 0/7] fix hunk editing with 'commit -p -m'","startedAt":"2014-03-10T18:49:30Z","lastAt":"2014-04-03T22:11:15Z","messageCount":50,"participants":["Benoit Pierre","Jeff King","Eric Sunshine","Philip Oakley","Jun Hao","brian m. carlson","Junio C Hamano","Torsten Bögershausen","Jonathan Nieder"],"isPatch":true,"patchVersion":2,"patchTotal":7},"messages":[{"id":"236429","messageId":"1394477377-10994-1-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":null,"subject":"[PATCH V2 0/7] fix hunk editing with 'commit -p -m'","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-10T18:49:30Z","receivedAt":"2014-03-10T18:49:30Z","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- rename 'run_hook' to 'run_hook_le' and change the first parameter to the environment to set\n- add a 'run_hook_ve' variant that take a va_list\n- add a new 'run_commit_hook' helper (to set both GIT_EDITOR and GIT_INDEX_FILE)\n- the old 'run_hook' functionality is available as 'run_hook_with_custom_index'\n (and marked as deprecated in the last optional patch of this series)\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 (7):\n merge hook tests: fix missing '&&' in test\n merge hook tests: use 'test_must_fail' instead of '!'\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 run-command: mark run_hook_with_custom_index as deprecated\n\n builtin/checkout.c | 8 +++----\n builtin/clone.c | 4 ++--\n builtin/commit.c | 35 ++++++++++++++++++++++++------\n builtin/gc.c | 2 +-\n builtin/merge.c | 6 +++---\n commit.h | 3 +++\n run-command.c | 44 +++++++++++++++++++++++++++-----------\n run-command.h | 7 +++++-\n t/t7505-prepare-commit-msg-hook.sh | 33 ++++++++++++++++++++--------\n t/t7513-commit_-p_-m_hunk_edit.sh | 34 +++++++++++++++++++++++++++++\n 10 files changed, 137 insertions(+), 39 deletions(-)\n create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh\n\n-- \n1.9.0\n"},{"id":"236430","messageId":"1394477377-10994-2-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":"1394477377-10994-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 1/7] merge hook tests: fix missing '&&' in test","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-10T18:49:31Z","receivedAt":"2014-03-10T18:49:31Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"Signed-off-by: Benoit Pierre \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 3573751..1c95652 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -174,7 +174,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":"236432","messageId":"1394477377-10994-3-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":"1394477377-10994-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 2/7] merge hook tests: use 'test_must_fail' instead of '!'","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-10T18:49:32Z","receivedAt":"2014-03-10T18:49:32Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"Signed-off-by: Benoit Pierre \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 1c95652..5531abb 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -154,7 +154,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@@ -163,7 +163,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":"236431","messageId":"1394477377-10994-4-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":"1394477377-10994-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 3/7] test patch hunk editing with \"commit -p -m\"","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-10T18:49:33Z","receivedAt":"2014-03-10T18:49:33Z","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 \n---\n t/t7513-commit_-p_-m_hunk_edit.sh | 34 ++++++++++++++++++++++++++++++++++\n 1 file changed, 34 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..994939a\n--- /dev/null\n+++ b/t/t7513-commit_-p_-m_hunk_edit.sh\n@@ -0,0 +1,34 @@\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+\tcat >expect <<-\\EOF\n+\tdiff --git a/file b/file\n+\tindex a29bdeb..c0d0fb4 100644\n+\t--- a/file\n+\t+++ b/file\n+\t@@ -1 +1,2 @@\n+\t line1\n+\t+line2\n+\tEOF\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 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.9.0\n"},{"id":"236435","messageId":"1394477377-10994-5-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":"1394477377-10994-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 4/7] commit: fix patch hunk editing with \"commit -p -m\"","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-10T18:49:34Z","receivedAt":"2014-03-10T18:49:34Z","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 \n---\n builtin/checkout.c | 8 +++----\n builtin/clone.c | 4 ++--\n builtin/commit.c | 35 ++++++++++++++++++++++++-------\n builtin/gc.c | 2 +-\n builtin/merge.c | 6 +++---\n commit.h | 3 +++\n run-command.c | 44 ++++++++++++++++++++++++++++-----------\n run-command.h | 6 +++++-\n t/t7513-commit_-p_-m_hunk_edit.sh | 2 +-\n 9 files changed, 79 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 5df3837..da423b2 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -53,10 +53,10 @@ struct checkout_opts {\n static int post_checkout_hook(struct commit *old, struct commit *new,\n \t\t\t int changed)\n {\n-\treturn run_hook(NULL, \"post-checkout\",\n-\t\t\tsha1_to_hex(old ? old->object.sha1 : null_sha1),\n-\t\t\tsha1_to_hex(new ? new->object.sha1 : null_sha1),\n-\t\t\tchanged ? \"1\" : \"0\", NULL);\n+ return run_hook_le(NULL, \"post-checkout\",\n+\t\t sha1_to_hex(old ? old->object.sha1 : null_sha1),\n+\t\t sha1_to_hex(new ? new->object.sha1 : null_sha1),\n+\t\t changed ? \"1\" : \"0\", NULL);\n \t/* \"new\" can be NULL when checking out from the index before\n \t a commit exists. */\n \ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 43e772c..9b3c04d 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -660,8 +660,8 @@ static int checkout(void)\n \t commit_locked_index(lock_file))\n \t\tdie(_(\"unable to write new index file\"));\n \n-\terr |= run_hook(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\n-\t\t\tsha1_to_hex(sha1), \"1\", NULL);\n+\terr |= run_hook_le(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\n+\t\t\t sha1_to_hex(sha1), \"1\", NULL);\n \n \tif (!err && option_recursive)\n \t\terr = run_command_v_opt(argv_submodule, RUN_GIT_CMD);\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 3767478..baf1fc0 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_ve(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/gc.c b/builtin/gc.c\nindex c19545d..7fa717a 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -179,7 +179,7 @@ static int need_to_gc(void)\n \telse if (!too_many_loose_objects())\n \t\treturn 0;\n \n-\tif (run_hook(NULL, \"pre-auto-gc\", NULL))\n+\tif (run_hook_le(NULL, \"pre-auto-gc\", NULL))\n \t\treturn 0;\n \treturn 1;\n }\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex e576a7f..67f312d 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -421,7 +421,7 @@ static void finish(struct commit *head_commit,\n \t}\n \n \t/* Run a post-merge hook */\n-\trun_hook(NULL, \"post-merge\", squash ? \"1\" : \"0\", NULL);\n+\trun_hook_le(NULL, \"post-merge\", squash ? \"1\" : \"0\", NULL);\n \n \tstrbuf_release(&reflog_message);\n }\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..75abc47 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_ve(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,45 @@ 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_le(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_ve(env, name, args);\n+\tva_end(args);\n+\n+\treturn ret;\n+}\n+\n+int run_hook_with_custom_index(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+\tva_start(args, name);\n+\tret = run_hook_ve(hook_env, name, args);\n+\tva_end(args);\n+\n+\treturn ret;\n+}\ndiff --git a/run-command.h b/run-command.h\nindex 6b985af..88460f9 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -47,7 +47,11 @@ 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_le(const char *const *env, const char *name, ...);\n+extern int run_hook_ve(const char *const *env, const char *name, va_list args);\n+\n+LAST_ARG_MUST_BE_NULL\n+extern int run_hook_with_custom_index(const char *index_file, const char *name, ...);\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 994939a..1f05d32 100755\n--- a/t/t7513-commit_-p_-m_hunk_edit.sh\n+++ b/t/t7513-commit_-p_-m_hunk_edit.sh\n@@ -25,7 +25,7 @@ test_expect_success 'setup (initial)' '\n \tEOF\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 >actual &&\n \ttest_cmp expect actual\n-- \n1.9.0\n"},{"id":"236433","messageId":"1394477377-10994-6-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":"1394477377-10994-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 5/7] merge: fix GIT_EDITOR override for commit hook","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-10T18:49:35Z","receivedAt":"2014-03-10T18:49:35Z","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 \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 67f312d..b11a528 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":"236434","messageId":"1394477377-10994-7-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":"1394477377-10994-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 6/7] merge hook tests: fix and update tests","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-10T18:49:36Z","receivedAt":"2014-03-10T18:49:36Z","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- make sure the tree is reset to a clean state after running a test\n (using test_when_finished) so later tests are not impacted\n\nSigned-off-by: Benoit Pierre \n---\n t/t7505-prepare-commit-msg-hook.sh | 27 +++++++++++++++++++++------\n 1 file changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\nindex 5531abb..03dce09 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -134,14 +134,26 @@ test_expect_success 'with hook (-c)' '\n \n test_expect_success 'with hook (merge)' '\n \n-\thead=`git rev-parse HEAD` &&\n-\tgit checkout -b other HEAD@{1} &&\n-\techo \"more\" >> file &&\n+\ttest_when_finished \"git checkout -f master\" &&\n+\tgit checkout -B other HEAD@{1} &&\n+\techo \"more\" >>file &&\n+\tgit add file &&\n+\tgit commit -m other &&\n+\tgit checkout - &&\n+\tgit merge --no-ff other &&\n+\ttest \"`git log -1 --pretty=format:%s`\" = \"merge (no editor)\"\n+'\n+\n+test_expect_success 'with hook and editor (merge)' '\n+\n+\ttest_when_finished \"git checkout -f master\" &&\n+\tgit checkout -B other HEAD@{1} &&\n+\techo \"more\" >>file &&\n \tgit add file &&\n \tgit commit -m other &&\n \tgit checkout - &&\n-\tgit merge other &&\n-\ttest \"`git log -1 --pretty=format:%s`\" = merge\n+\tenv GIT_EDITOR=\"\\\"\\$FAKE_EDITOR\\\"\" git merge --no-ff -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+\ttest_when_finished \"git checkout -f master\" &&\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+\ttest_when_finished \"git checkout -f master\" &&\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+\ttest_when_finished \"git checkout -f master\" &&\n \tgit checkout -B other HEAD@{1} &&\n \techo \"more\" >> file &&\n \tgit add file &&\n@@ -178,7 +193,7 @@ test_expect_success 'with failing hook (merge)' '\n \texit 1\n \tEOF\n \tgit checkout - &&\n-\ttest_must_fail git merge other\n+\ttest_must_fail git merge --no-ff other\n \n '\n \n-- \n1.9.0\n"},{"id":"236436","messageId":"1394477377-10994-8-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":"1394477377-10994-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 7/7] run-command: mark run_hook_with_custom_index as deprecated","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-10T18:49:37Z","receivedAt":"2014-03-10T18:49:37Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"---\n run-command.h | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/run-command.h b/run-command.h\nindex 88460f9..3653bfa 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -51,6 +51,7 @@ extern int run_hook_le(const char *const *env, const char *name, ...);\n extern int run_hook_ve(const char *const *env, const char *name, va_list args);\n \n LAST_ARG_MUST_BE_NULL\n+__attribute__((deprecated))\n extern int run_hook_with_custom_index(const char *index_file, const char *name, ...);\n \n #define RUN_COMMAND_NO_STDIN 1\n-- \n1.9.0\n"},{"id":"236450","messageId":"20140310200756.GC24568@sigill.intra.peff.net","threadId":"36111","inReplyTo":"1394477377-10994-5-git-send-email-benoit.pierre@gmail.com","subject":"Re: [PATCH 4/7] commit: fix patch hunk editing with \"commit -p -m\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-10T20:07:56Z","receivedAt":"2014-03-10T20:07:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 10, 2014 at 07:49:34PM +0100, Benoit Pierre wrote:\n\n> Don't change git environment: move the GIT_EDITOR=\":\" override to the\n> hook command subprocess, like it's already done for GIT_INDEX_FILE.\n> \n> Signed-off-by: Benoit Pierre \n> ---\n> builtin/checkout.c | 8 +++----\n> builtin/clone.c | 4 ++--\n> builtin/commit.c | 35 ++++++++++++++++++++++++-------\n> builtin/gc.c | 2 +-\n> builtin/merge.c | 6 +++---\n> commit.h | 3 +++\n> run-command.c | 44 ++++++++++++++++++++++++++++-----------\n> run-command.h | 6 +++++-\n> t/t7513-commit_-p_-m_hunk_edit.sh | 2 +-\n> 9 files changed, 79 insertions(+), 31 deletions(-)\n\nThis is a lot of change, and in some ways I think it is making things\nbetter overall. However, the simplest fix for this is basically to move\nthe setting of GIT_EDITOR down to after we prepare the index.\n\nJun Hao (cc'd) has been preparing a series for this based on the\nBloomberg git hackday a few weeks ago, but it hasn't been sent to the\nlist yet.\n\nCommits are here:\n\n https://github.com/bloomberg/git/compare/commit-patch-allow-hunk-editing\n\nif you care to look. I'm not sure which solution is technically\nsuperior, but it's worth considering both.\n\nI regret not encouraging Jun to post to the list sooner, as we might\nhave avoided some duplicated effort. But that's a sunk cost, and we\nshould pick up whichever is the best for the project.\n\n-Peff\n"},{"id":"236452","messageId":"CAPig+cQoUW5dqSzs-wAVOD1xrbSzuzGoHCDKdmgT_BprngMbvQ@mail.gmail.com","threadId":"36111","inReplyTo":"1394477377-10994-4-git-send-email-benoit.pierre@gmail.com","subject":"Re: [PATCH 3/7] test patch hunk editing with \"commit -p -m\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-10T20:20:59Z","receivedAt":"2014-03-10T20:20:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 10, 2014 at 2:49 PM, Benoit Pierre 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 \n> ---\n> t/t7513-commit_-p_-m_hunk_edit.sh | 34 ++++++++++++++++++++++++++++++++++\n> 1 file changed, 34 insertions(+)\n> create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh\n\nIs it possible to give this file a name less unusual and more\nconsistent with other test scripts? Perhaps choose a more generic name\nwhich may allow other similar tests to be added to the file in the\nfuture (if needed)?\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..994939a\n> --- /dev/null\n> +++ b/t/t7513-commit_-p_-m_hunk_edit.sh\n> @@ -0,0 +1,34 @@\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> + echo line3 >>file &&\n> + cat >expect <<-\\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\nIn the previous review, the suggestion was that creation of 'expect'\nshould be moved to the test below where it is actually used rather\nthan into the 'setup' phase above.\n\n> +'\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 >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> +test_done\n> --\n> 1.9.0\n"},{"id":"236454","messageId":"CAPig+cRsFTNAAzAD99v+A1KCVRNTUOaSyuf_gqPZa8-MLMyoBQ@mail.gmail.com","threadId":"36111","inReplyTo":"1394477377-10994-7-git-send-email-benoit.pierre@gmail.com","subject":"Re: [PATCH 6/7] merge hook tests: fix and update tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-10T20:27:40Z","receivedAt":"2014-03-10T20:27:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 10, 2014 at 2:49 PM, Benoit Pierre wrote:\n> - update 'no editor' hook test and add 'editor' hook test\n> - make sure the tree is reset to a clean state after running a test\n> (using test_when_finished) so later tests are not impacted\n\nAs conceptually distinct changes, it might make sense to split these\ninto two patches, one a purely cleanup patch adding\ntest_when_finished, and the second introducing your new test. Doing so\nwould ease the review process and make the intended changes more\nobvious.\n\n> Signed-off-by: Benoit Pierre \n> ---\n> t/t7505-prepare-commit-msg-hook.sh | 27 +++++++++++++++++++++------\n> 1 file changed, 21 insertions(+), 6 deletions(-)\n>\n> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\n> index 5531abb..03dce09 100755\n> --- a/t/t7505-prepare-commit-msg-hook.sh\n> +++ b/t/t7505-prepare-commit-msg-hook.sh\n> @@ -134,14 +134,26 @@ test_expect_success 'with hook (-c)' '\n>\n> test_expect_success 'with hook (merge)' '\n>\n> - head=`git rev-parse HEAD` &&\n> - git checkout -b other HEAD@{1} &&\n> - echo \"more\" >> file &&\n> + test_when_finished \"git checkout -f master\" &&\n> + git checkout -B other HEAD@{1} &&\n> + echo \"more\" >>file &&\n> + git add file &&\n> + git commit -m other &&\n> + git checkout - &&\n> + git merge --no-ff other &&\n> + test \"`git log -1 --pretty=format:%s`\" = \"merge (no editor)\"\n> +'\n> +\n> +test_expect_success 'with hook and editor (merge)' '\n> +\n> + test_when_finished \"git checkout -f master\" &&\n> + git checkout -B other HEAD@{1} &&\n> + echo \"more\" >>file &&\n> git add file &&\n> git commit -m other &&\n> git checkout - &&\n> - git merge other &&\n> - test \"`git log -1 --pretty=format:%s`\" = merge\n> + env GIT_EDITOR=\"\\\"\\$FAKE_EDITOR\\\"\" git merge --no-ff -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> + test_when_finished \"git checkout -f master\" &&\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> + test_when_finished \"git checkout -f master\" &&\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> + test_when_finished \"git checkout -f master\" &&\n> git checkout -B other HEAD@{1} &&\n> echo \"more\" >> file &&\n> git add file &&\n> @@ -178,7 +193,7 @@ test_expect_success 'with failing hook (merge)' '\n> exit 1\n> EOF\n> git checkout - &&\n> - test_must_fail git merge other\n> + test_must_fail git merge --no-ff other\n>\n> '\n>\n> --\n> 1.9.0\n"},{"id":"236455","messageId":"E509041DA7394C9893F4F3EBE99B6E43@PhilipOakley","threadId":"36111","inReplyTo":"1394477377-10994-4-git-send-email-benoit.pierre@gmail.com","subject":"Re: [PATCH 3/7] test patch hunk editing with \"commit -p -m\"","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2014-03-10T20:27:40Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"mincro nit.\nFrom: \"Benoit Pierre\" \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\ns/now/know/\n\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 \n> ---\n> t/t7513-commit_-p_-m_hunk_edit.sh | 34 \n> ++++++++++++++++++++++++++++++++++\n> 1 file changed, 34 insertions(+)\n> create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh\n>\n> diff --git a/t/t7513-commit_-p_-m_hunk_edit.sh \n> b/t/t7513-commit_-p_-m_hunk_edit.sh\n> new file mode 100755\n> index 0000000..994939a\n> --- /dev/null\n> +++ b/t/t7513-commit_-p_-m_hunk_edit.sh\n> @@ -0,0 +1,34 @@\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> + echo line3 >>file &&\n> + cat >expect <<-\\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> + echo e | env GIT_EDITOR=\"sed s/+line3\\$/+line2/ -i\" git commit -p -m \n> commit2 file &&\n> + git diff HEAD^ HEAD >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> +test_done\n> -- \n> 1.9.0\n>\n> --\n> \n"},{"id":"236472","messageId":"m2mwgx7ecz.fsf@jh.home","threadId":"36111","inReplyTo":"20140310200756.GC24568@sigill.intra.peff.net","subject":"Re: [PATCH 4/7] commit: fix patch hunk editing with \"commit -p -m\"","fromName":"Jun Hao","fromEmail":"achilles.hao@gmail.com","sentAt":"2014-03-11T00:45:00Z","receivedAt":"2014-03-11T00:45:00Z","isPatch":true,"sender":{"key":"achilles.hao@gmail.com","avatar":null},"body":"Jeff King writes:\n\n> On Mon, Mar 10, 2014 at 07:49:34PM +0100, Benoit Pierre wrote:\n>\n>> Don't change git environment: move the GIT_EDITOR=\":\" override to the\n>> hook command subprocess, like it's already done for GIT_INDEX_FILE.\n>> \n>> Signed-off-by: Benoit Pierre \n>> ---\n>> builtin/checkout.c | 8 +++----\n>> builtin/clone.c | 4 ++--\n>> builtin/commit.c | 35 ++++++++++++++++++++++++-------\n>> builtin/gc.c | 2 +-\n>> builtin/merge.c | 6 +++---\n>> commit.h | 3 +++\n>> run-command.c | 44 ++++++++++++++++++++++++++++-----------\n>> run-command.h | 6 +++++-\n>> t/t7513-commit_-p_-m_hunk_edit.sh | 2 +-\n>> 9 files changed, 79 insertions(+), 31 deletions(-)\n>\n> This is a lot of change, and in some ways I think it is making things\n> better overall. However, the simplest fix for this is basically to move\n> the setting of GIT_EDITOR down to after we prepare the index.\n>\n> Jun Hao (cc'd) has been preparing a series for this based on the\n> Bloomberg git hackday a few weeks ago, but it hasn't been sent to the\n> list yet.\n>\n> Commits are here:\n>\n> https://github.com/bloomberg/git/compare/commit-patch-allow-hunk-editing\n>\n> if you care to look. I'm not sure which solution is technically\n> superior, but it's worth considering both.\n>\n> I regret not encouraging Jun to post to the list sooner, as we might\n> have avoided some duplicated effort. But that's a sunk cost, and we\n> should pick up whichever is the best for the project.\n>\n> -Peff\n\nI like the idea that the environment setting should be done in one\nplace. Just not sure run_hook is the right place tho. If user doesn't have\nany hook setup, will this kick in? \nOne more question, will this work for dry run? Or dry run doesn't matter\nin this case?\n\nThanks - Jun\n"},{"id":"236473","messageId":"20140311010039.GD4271@vauxhall.crustytoothpaste.net","threadId":"36111","inReplyTo":"1394477377-10994-8-git-send-email-benoit.pierre@gmail.com","subject":"Re: [PATCH 7/7] run-command: mark run_hook_with_custom_index as deprecated","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-03-11T01:00:39Z","receivedAt":"2014-03-11T01:00:39Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Mon, Mar 10, 2014 at 07:49:37PM +0100, Benoit Pierre wrote:\n> ---\n> run-command.h | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/run-command.h b/run-command.h\n> index 88460f9..3653bfa 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -51,6 +51,7 @@ extern int run_hook_le(const char *const *env, const char *name, ...);\n> extern int run_hook_ve(const char *const *env, const char *name, va_list args);\n> \n> LAST_ARG_MUST_BE_NULL\n> +__attribute__((deprecated))\n\nIt doesn't appear that we use the deprecated attribute anywhere else in\nthe code. Wouldn't it just be better to change the places that use this\nand then remove the function altogether? I imagine your current patch\nmight introduce a number of warnings that some people would rather\navoid.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"236511","messageId":"CA+SSzV3z5zQaEgJX_LC6j7FmfNYi-eC_7_UA=KWQBBooOpkgBQ@mail.gmail.com","threadId":"36111","inReplyTo":"20140311010039.GD4271@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH 7/7] run-command: mark run_hook_with_custom_index as deprecated","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-11T17:37:38Z","receivedAt":"2014-03-11T17:37:38Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"On Tue, Mar 11, 2014 at 2:00 AM, brian m. carlson\n wrote:\n> On Mon, Mar 10, 2014 at 07:49:37PM +0100, Benoit Pierre wrote:\n>> ---\n>> run-command.h | 1 +\n>> 1 file changed, 1 insertion(+)\n>>\n>> diff --git a/run-command.h b/run-command.h\n>> index 88460f9..3653bfa 100644\n>> --- a/run-command.h\n>> +++ b/run-command.h\n>> @@ -51,6 +51,7 @@ extern int run_hook_le(const char *const *env, const char *name, ...);\n>> extern int run_hook_ve(const char *const *env, const char *name, va_list args);\n>>\n>> LAST_ARG_MUST_BE_NULL\n>> +__attribute__((deprecated))\n>\n> It doesn't appear that we use the deprecated attribute anywhere else in\n> the code. Wouldn't it just be better to change the places that use this\n> and then remove the function altogether? I imagine your current patch\n> might introduce a number of warnings that some people would rather\n> avoid.\n\nThis last patch is optional. There are no callers to\nrun_hook_with_custom_index, so no warnings for now. See discussion on\nfirst draft as to why I added it:\n\nhttp://article.gmane.org/gmane.comp.version-control.git/243561\n\n-- \nA: Because it destroys the flow of conversation.\nQ: Why is top posting dumb?\n"},{"id":"236512","messageId":"CA+SSzV04KCGyTkFTtSx_9sYAZyh=UzHOPV6tf9JT82w_DsSLHg@mail.gmail.com","threadId":"36111","inReplyTo":"20140310200756.GC24568@sigill.intra.peff.net","subject":"Re: [PATCH 4/7] commit: fix patch hunk editing with \"commit -p -m\"","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-11T17:56:02Z","receivedAt":"2014-03-11T17:56:02Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"On Mon, Mar 10, 2014 at 9:07 PM, Jeff King wrote:\n> On Mon, Mar 10, 2014 at 07:49:34PM +0100, Benoit Pierre wrote:\n>\n>> Don't change git environment: move the GIT_EDITOR=\":\" override to the\n>> hook command subprocess, like it's already done for GIT_INDEX_FILE.\n>> [...]\n>\n> This is a lot of change, and in some ways I think it is making things\n> better overall. However, the simplest fix for this is basically to move\n> the setting of GIT_EDITOR down to after we prepare the index.\n>\n> Jun Hao (cc'd) has been preparing a series for this based on the\n> Bloomberg git hackday a few weeks ago, but it hasn't been sent to the\n> list yet.\n>\n> Commits are here:\n>\n> https://github.com/bloomberg/git/compare/commit-patch-allow-hunk-editing\n>\n> if you care to look. I'm not sure which solution is technically\n> superior, but it's worth considering both.\n>\n> I regret not encouraging Jun to post to the list sooner, as we might\n> have avoided some duplicated effort. But that's a sunk cost, and we\n> should pick up whichever is the best for the project.\n\nReplying here instead of to Jun Hao message (I'm not subscribed to the\nmailing list, so I did not received it):\n\nJun Hao wrote:\n> I like the idea that the environment setting should be done in one\n> place. Just not sure run_hook is the right place tho. If user doesn't have\n> any hook setup, will this kick in?\n> One more question, will this work for dry run? Or dry run doesn't matter\n> in this case?\n\nAccording to the original commit, the change to GIT_EDITOR is only\nhere for hooks:\n\ncommit 406400ce4f69e79b544dd3539a71b85d99331820\nAuthor: Paolo Bonzini \nDate: Tue Feb 5 11:01:45 2008 +0100\n\n git-commit: set GIT_EDITOR=: if editor will not be launched\n\n This is a preparatory patch that provides a simple way for the future\n prepare-commit-msg hook to discover if the editor will be launched.\n\n Signed-off-by: Junio C Hamano \n\nSo there is really no reason to set it earlier, and not just in the\nhook subprocess environment.\n\nRegarding dry run: the bug is present, and my patch fix it too (but is\nmissing a test for this).\n\nAs to which patch is better: it's really not for me to decide! It's a\nquestion for the maintainer(s), Jun Hao patch is sure much smaller and\nsimpler, mine is more involved and I believe cleaner in the long term:\nthere is no risk of another part of the commit process to be impacted\nby the change of environment. Also note that my patch changes the\nmerge builtin too: GIT_EDITOR will not be overriden if an editor will\nbe launched (when used with --edit).\n\n-- \nA: Because it destroys the flow of conversation.\nQ: Why is top posting dumb?\n"},{"id":"236513","messageId":"20140311180020.GA28154@sigill.intra.peff.net","threadId":"36111","inReplyTo":"CA+SSzV04KCGyTkFTtSx_9sYAZyh=UzHOPV6tf9JT82w_DsSLHg@mail.gmail.com","subject":"Re: [PATCH 4/7] commit: fix patch hunk editing with \"commit -p -m\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-11T18:00:20Z","receivedAt":"2014-03-11T18:00:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 11, 2014 at 06:56:02PM +0100, Benoit Pierre wrote:\n\n> According to the original commit, the change to GIT_EDITOR is only\n> here for hooks:\n> \n> commit 406400ce4f69e79b544dd3539a71b85d99331820\n> Author: Paolo Bonzini \n> Date: Tue Feb 5 11:01:45 2008 +0100\n> \n> git-commit: set GIT_EDITOR=: if editor will not be launched\n> \n> This is a preparatory patch that provides a simple way for the future\n> prepare-commit-msg hook to discover if the editor will be launched.\n> \n> Signed-off-by: Junio C Hamano \n> \n> So there is really no reason to set it earlier, and not just in the\n> hook subprocess environment.\n\nAh, you're right. I was thinking that our invocation of launch_editor\nalso respected it. It does, but we never get there at all because\nuse_editor is set to 0. So yeah, it really is just needed for the hook.\n\nYour patch, even though it is a bigger change, keeps the setting to the\nminimal area, which is cleaner and more maintainable in the long run.\n\n-Peff\n"},{"id":"236515","messageId":"xmqqbnxcr4bl.fsf@gitster.dls.corp.google.com","threadId":"36111","inReplyTo":"CAPig+cQoUW5dqSzs-wAVOD1xrbSzuzGoHCDKdmgT_BprngMbvQ@mail.gmail.com","subject":"Re: [PATCH 3/7] test patch hunk editing with \"commit -p -m\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-11T18:13:50Z","receivedAt":"2014-03-11T18:13:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Mon, Mar 10, 2014 at 2:49 PM, Benoit Pierre 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 \n>> ---\n>> t/t7513-commit_-p_-m_hunk_edit.sh | 34 ++++++++++++++++++++++++++++++++++\n>> 1 file changed, 34 insertions(+)\n>> create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh\n>\n> Is it possible to give this file a name less unusual and more\n> consistent with other test scripts? Perhaps choose a more generic name\n> which may allow other similar tests to be added to the file in the\n> future (if needed)?\n\nSurely. There are \"reset-patch\" and \"checkout-patch\" tests, and if\nwe were to add something like this, I'd imagine \"commit-patch\" would\nbe a logical name for the new test.\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..994939a\n>> --- /dev/null\n>> +++ b/t/t7513-commit_-p_-m_hunk_edit.sh\n>> @@ -0,0 +1,34 @@\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>> + echo line3 >>file &&\n>> + cat >expect <<-\\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> In the previous review, the suggestion was that creation of 'expect'\n> should be moved to the test below where it is actually used rather\n> than into the 'setup' phase above.\n>\n>> +'\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 >actual &&\n>> + test_cmp expect actual\n>> +'\n>> +\n>> +test_done\n>> --\n>> 1.9.0\n"},{"id":"236517","messageId":"loom.20140311T194024-418@post.gmane.org","threadId":"36111","inReplyTo":"20140311180020.GA28154@sigill.intra.peff.net","subject":"Re: [PATCH 4/7] commit: fix patch hunk editing with","fromName":"Jun Hao","fromEmail":"achilles.hao@gmail.com","sentAt":"2014-03-11T18:40:55Z","receivedAt":"2014-03-11T18:40:55Z","isPatch":true,"sender":{"key":"achilles.hao@gmail.com","avatar":null},"body":"Jeff King peff.net> writes:\n\n> \n> Ah, you're right. I was thinking that our invocation of launch_editor\n> also respected it. It does, but we never get there at all because\n> use_editor is set to 0. So yeah, it really is just needed for the hook.\n> \n> Your patch, even though it is a bigger change, keeps the setting to the\n> minimal area, which is cleaner and more maintainable in the long run.\n> \n> -Peff\n> \n\nOh, didn't realize GIT_EDITOR change is only for hooks. Good catch. I agree \nBenoit's patch is better for the long term.\n\nThanks - Jun\n"},{"id":"236536","messageId":"xmqqob1co3dm.fsf@gitster.dls.corp.google.com","threadId":"36111","inReplyTo":"1394477377-10994-5-git-send-email-benoit.pierre@gmail.com","subject":"Re: [PATCH 4/7] commit: fix patch hunk editing with \"commit -p -m\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-11T21:02:28Z","receivedAt":"2014-03-11T21:02:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Benoit Pierre writes:\n\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 5df3837..da423b2 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -53,10 +53,10 @@ struct checkout_opts {\n> static int post_checkout_hook(struct commit *old, struct commit *new,\n> \t\t\t int changed)\n> {\n> -\treturn run_hook(NULL, \"post-checkout\",\n> -\t\t\tsha1_to_hex(old ? old->object.sha1 : null_sha1),\n> -\t\t\tsha1_to_hex(new ? new->object.sha1 : null_sha1),\n> -\t\t\tchanged ? \"1\" : \"0\", NULL);\n> + return run_hook_le(NULL, \"post-checkout\",\n> +\t\t sha1_to_hex(old ? old->object.sha1 : null_sha1),\n> +\t\t sha1_to_hex(new ? new->object.sha1 : null_sha1),\n> +\t\t changed ? \"1\" : \"0\", NULL);\n\nFunny indentation.\n"},{"id":"236537","messageId":"xmqqk3c0o3cj.fsf@gitster.dls.corp.google.com","threadId":"36111","inReplyTo":"1394477377-10994-4-git-send-email-benoit.pierre@gmail.com","subject":"Re: [PATCH 3/7] test patch hunk editing with \"commit -p -m\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-11T21:03:08Z","receivedAt":"2014-03-11T21:03:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Benoit Pierre writes:\n\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 \n> ---\n> t/t7513-commit_-p_-m_hunk_edit.sh | 34 ++++++++++++++++++++++++++++++++++\n> 1 file changed, 34 insertions(+)\n> create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh\n>\n> diff --git a/t/t7513-commit_-p_-m_hunk_edit.sh b/t/t7513-commit_-p_-m_hunk_edit.sh\n\nI'll move this to t/t7514-commit-patch.sh for now while queuing.\n"},{"id":"236789","messageId":"53244777.2040302@web.de","threadId":"36111","inReplyTo":"xmqqk3c0o3cj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/7] test patch hunk editing with \"commit -p -m\"","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-03-15T12:28:39Z","receivedAt":"2014-03-15T12:28:39Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-03-11 22.03, Junio C Hamano wrote:\n> Benoit Pierre writes:\n> \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 \n>> ---\n>> t/t7513-commit_-p_-m_hunk_edit.sh | 34 ++++++++++++++++++++++++++++++++++\n>> 1 file changed, 34 insertions(+)\n>> create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh\n>>\n>> diff --git a/t/t7513-commit_-p_-m_hunk_edit.sh b/t/t7513-commit_-p_-m_hunk_edit.sh\n> \n> I'll move this to t/t7514-commit-patch.sh for now while queuing.\n\nThis line is problematic:\n\techo e | env GIT_EDITOR=\"sed s/+line3\\$/+line2/ -i\" git commit -p -m commit2 f\n\n(sed -i is not portable:\nhttp://pubs.opengroup.org/onlinepubs/007908799/xcu/sed.html)\n\nThe whole test hangs in a forever loop loop under MacOS:\ndebug=t verbose=t ./t7514-commit-patch.sh \nStage this hunk [y,n,q,a,d,/,e,?]? @@ -1 +1,2 @@\n line1\n+line3\n\nI think perl can be used instead of sed (but I haven't found the exact syntax yet)\n"},{"id":"236793","messageId":"CA+SSzV1LcuTWMGrJrto3cJ13-MxgFsJP6z3zTYjHp=qZGPoraw@mail.gmail.com","threadId":"36111","inReplyTo":"53244777.2040302@web.de","subject":"Re: [PATCH 3/7] test patch hunk editing with \"commit -p -m\"","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-15T16:11:08Z","receivedAt":"2014-03-15T16:11:08Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"On Sat, Mar 15, 2014 at 1:28 PM, Torsten Bögershausen wrote:\n> On 2014-03-11 22.03, Junio C Hamano wrote:\n>> Benoit Pierre writes:\n>>\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 \n>>> ---\n>>> t/t7513-commit_-p_-m_hunk_edit.sh | 34 ++++++++++++++++++++++++++++++++++\n>>> 1 file changed, 34 insertions(+)\n>>> create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh\n>>>\n>>> diff --git a/t/t7513-commit_-p_-m_hunk_edit.sh b/t/t7513-commit_-p_-m_hunk_edit.sh\n>>\n>> I'll move this to t/t7514-commit-patch.sh for now while queuing.\n>\n> This line is problematic:\n> echo e | env GIT_EDITOR=\"sed s/+line3\\$/+line2/ -i\" git commit -p -m commit2 f\n>\n> (sed -i is not portable:\n> http://pubs.opengroup.org/onlinepubs/007908799/xcu/sed.html)\n>\n> The whole test hangs in a forever loop loop under MacOS:\n> debug=t verbose=t ./t7514-commit-patch.sh\n> Stage this hunk [y,n,q,a,d,/,e,?]? @@ -1 +1,2 @@\n> line1\n> +line3\n>\n> I think perl can be used instead of sed (but I haven't found the exact syntax yet)\n\nOr maybe change the test to just 'touch' a temporary file or change\nits content like Jun Hao did with for its version of the tests:\n\nhttps://github.com/bloomberg/git/compare/commit-patch-allow-hunk-editing\n\nShould I make a third version? I'll simplify and move the tests to\nt/t7514-commit-patch.sh and add a test for the '--dry-run' case. And\nalso:\n- 'now' => 'know' in one of the commit message\n- sign off the last patch (which I forgot to do)\n- fix the indentation in one of the patch\n\n-- \nA: Because it destroys the flow of conversation.\nQ: Why is top posting dumb?\n"},{"id":"236802","messageId":"5324BF64.7020900@web.de","threadId":"36111","inReplyTo":"CA+SSzV1LcuTWMGrJrto3cJ13-MxgFsJP6z3zTYjHp=qZGPoraw@mail.gmail.com","subject":"Re: [PATCH 3/7] test patch hunk editing with \"commit -p -m\"","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-03-15T21:00:20Z","receivedAt":"2014-03-15T21:00:20Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-03-15 17.11, Benoit Pierre wrote:\n> On Sat, Mar 15, 2014 at 1:28 PM, Torsten Bögershausen wrote:\n>> On 2014-03-11 22.03, Junio C Hamano wrote:\n>>> Benoit Pierre writes:\n>>>\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 \n>>>> ---\n>>>> t/t7513-commit_-p_-m_hunk_edit.sh | 34 ++++++++++++++++++++++++++++++++++\n>>>> 1 file changed, 34 insertions(+)\n>>>> create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh\n>>>>\n>>>> diff --git a/t/t7513-commit_-p_-m_hunk_edit.sh b/t/t7513-commit_-p_-m_hunk_edit.sh\n>>>\n>>> I'll move this to t/t7514-commit-patch.sh for now while queuing.\n>>\n>> This line is problematic:\n>> echo e | env GIT_EDITOR=\"sed s/+line3\\$/+line2/ -i\" git commit -p -m commit2 f\n>>\n>> (sed -i is not portable:\n>> http://pubs.opengroup.org/onlinepubs/007908799/xcu/sed.html)\n>>\n>> The whole test hangs in a forever loop loop under MacOS:\n>> debug=t verbose=t ./t7514-commit-patch.sh\n>> Stage this hunk [y,n,q,a,d,/,e,?]? @@ -1 +1,2 @@\n>> line1\n>> +line3\n>>\n>> I think perl can be used instead of sed (but I haven't found the exact syntax yet)\n> \n> Or maybe change the test to just 'touch' a temporary file or change\n> its content like Jun Hao did with for its version of the tests:\n> \n> https://github.com/bloomberg/git/compare/commit-patch-allow-hunk-editing\n> \n> Should I make a third version? I'll simplify and move the tests to\n> t/t7514-commit-patch.sh and add a test for the '--dry-run' case. And\n> also:\n> - 'now' => 'know' in one of the commit message\n> - sign off the last patch (which I forgot to do)\n> - fix the indentation in one of the patch\n> \nThe following works for me, (diff against pu)\n(and if you want to send a 3rd version, please do so :-)\n\ndiff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh\nindex 1f05d32..da92669 100755\n--- a/t/t7514-commit-patch.sh\n+++ b/t/t7514-commit-patch.sh\n@@ -25,10 +25,20 @@ test_expect_success 'setup (initial)' '\n \tEOF\n '\n \n+write_script .git/FAKE_EDITOR <<\\EOF\n+\tsed -e \"s/+line3\\$/+line2/\" <\"$1\" >tmp &&\n+\tmv -f tmp \"$1\"\n+\texit 0\n+EOF\n+\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 >actual &&\n-\ttest_cmp expect actual\n+\t(\n+\t\tGIT_EDITOR=\"\\\"$(pwd)/.git/FAKE_EDITOR\\\"\" &&\n+\t\texport GIT_EDITOR &&\n+\t\techo e | git commit -p -m commit2 file &&\n+\t\tgit diff HEAD^ HEAD >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n '\n \n test_done\n"},{"id":"236806","messageId":"1394919750-28432-1-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":"CA+SSzV1LcuTWMGrJrto3cJ13-MxgFsJP6z3zTYjHp=qZGPoraw@mail.gmail.com","subject":"[PATCH 1/7] merge hook tests: fix missing '&&' in test","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-15T21:42:24Z","receivedAt":"2014-03-15T21:42:24Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"Signed-off-by: Benoit Pierre \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 3573751..1c95652 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -174,7 +174,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":"236807","messageId":"1394919750-28432-2-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":"1394919750-28432-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 2/7] merge hook tests: use 'test_must_fail' instead of '!'","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-15T21:42:25Z","receivedAt":"2014-03-15T21:42:25Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"Signed-off-by: Benoit Pierre \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 1c95652..5531abb 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -154,7 +154,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@@ -163,7 +163,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":"236808","messageId":"1394919750-28432-3-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":"1394919750-28432-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 3/7] test patch hunk editing with \"commit -p -m\"","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-15T21:42:26Z","receivedAt":"2014-03-15T21:42:26Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"Add (failing) tests: with commit changing the environment to let hooks\nknow 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 \n---\n t/t7513-commit-patch.sh | 32 ++++++++++++++++++++++++++++++++\n 1 file changed, 32 insertions(+)\n create mode 100755 t/t7513-commit-patch.sh\n\ndiff --git a/t/t7513-commit-patch.sh b/t/t7513-commit-patch.sh\nnew file mode 100755\nindex 0000000..9311b0c\n--- /dev/null\n+++ b/t/t7513-commit-patch.sh\n@@ -0,0 +1,32 @@\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+'\n+\n+test_expect_failure 'edit hunk \"commit -p -m message\"' '\n+\ttest_when_finished \"rm -f editor_was_started\" &&\n+\techo more >>file &&\n+\techo e | env GIT_EDITOR=\"touch editor_was_started\" git commit -p -m commit2 file &&\n+\ttest -r editor_was_started\n+'\n+\n+test_expect_failure 'edit hunk \"commit --dry-run -p -m message\"' '\n+\ttest_when_finished \"rm -f editor_was_started\" &&\n+\techo more >>file &&\n+\techo e | env GIT_EDITOR=\"touch editor_was_started\" git commit -p -m commit3 file &&\n+\ttest -r editor_was_started\n+'\n+\n+test_done\n-- \n1.9.0\n"},{"id":"236809","messageId":"1394919750-28432-4-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":"1394919750-28432-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 4/7] commit: fix patch hunk editing with \"commit -p -m\"","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-15T21:42:27Z","receivedAt":"2014-03-15T21:42:27Z","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 \n---\n builtin/checkout.c | 8 ++++----\n builtin/clone.c | 4 ++--\n builtin/commit.c | 35 ++++++++++++++++++++++++++++-------\n builtin/gc.c | 2 +-\n builtin/merge.c | 6 +++---\n commit.h | 3 +++\n run-command.c | 44 ++++++++++++++++++++++++++++++++------------\n run-command.h | 6 +++++-\n t/t7513-commit-patch.sh | 4 ++--\n 9 files changed, 80 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex ada51fa..1b86d9c 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -53,10 +53,10 @@ struct checkout_opts {\n static int post_checkout_hook(struct commit *old, struct commit *new,\n \t\t\t int changed)\n {\n-\treturn run_hook(NULL, \"post-checkout\",\n-\t\t\tsha1_to_hex(old ? old->object.sha1 : null_sha1),\n-\t\t\tsha1_to_hex(new ? new->object.sha1 : null_sha1),\n-\t\t\tchanged ? \"1\" : \"0\", NULL);\n+\treturn run_hook_le(NULL, \"post-checkout\",\n+\t\t\t sha1_to_hex(old ? old->object.sha1 : null_sha1),\n+\t\t\t sha1_to_hex(new ? new->object.sha1 : null_sha1),\n+\t\t\t changed ? \"1\" : \"0\", NULL);\n \t/* \"new\" can be NULL when checking out from the index before\n \t a commit exists. */\n \ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 43e772c..9b3c04d 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -660,8 +660,8 @@ static int checkout(void)\n \t commit_locked_index(lock_file))\n \t\tdie(_(\"unable to write new index file\"));\n \n-\terr |= run_hook(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\n-\t\t\tsha1_to_hex(sha1), \"1\", NULL);\n+\terr |= run_hook_le(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\n+\t\t\t sha1_to_hex(sha1), \"1\", NULL);\n \n \tif (!err && option_recursive)\n \t\terr = run_command_v_opt(argv_submodule, RUN_GIT_CMD);\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 3783bca..68a90b3 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -610,7 +610,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@@ -867,8 +867,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@@ -884,7 +884,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@@ -1068,8 +1068,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@@ -1450,6 +1448,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_ve(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@@ -1674,7 +1695,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/gc.c b/builtin/gc.c\nindex 63d400b..11cf295 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -184,7 +184,7 @@ static int need_to_gc(void)\n \telse if (!too_many_loose_objects())\n \t\treturn 0;\n \n-\tif (run_hook(NULL, \"pre-auto-gc\", NULL))\n+\tif (run_hook_le(NULL, \"pre-auto-gc\", NULL))\n \t\treturn 0;\n \treturn 1;\n }\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex f0cf120..bdf6655 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -421,7 +421,7 @@ static void finish(struct commit *head_commit,\n \t}\n \n \t/* Run a post-merge hook */\n-\trun_hook(NULL, \"post-merge\", squash ? \"1\" : \"0\", NULL);\n+\trun_hook_le(NULL, \"post-merge\", squash ? \"1\" : \"0\", NULL);\n \n \tstrbuf_release(&reflog_message);\n }\n@@ -824,8 +824,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..75abc47 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_ve(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,45 @@ 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_le(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_ve(env, name, args);\n+\tva_end(args);\n+\n+\treturn ret;\n+}\n+\n+int run_hook_with_custom_index(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+\tva_start(args, name);\n+\tret = run_hook_ve(hook_env, name, args);\n+\tva_end(args);\n+\n+\treturn ret;\n+}\ndiff --git a/run-command.h b/run-command.h\nindex 6b985af..88460f9 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -47,7 +47,11 @@ 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_le(const char *const *env, const char *name, ...);\n+extern int run_hook_ve(const char *const *env, const char *name, va_list args);\n+\n+LAST_ARG_MUST_BE_NULL\n+extern int run_hook_with_custom_index(const char *index_file, const char *name, ...);\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-patch.sh b/t/t7513-commit-patch.sh\nindex 9311b0c..67e4cde 100755\n--- a/t/t7513-commit-patch.sh\n+++ b/t/t7513-commit-patch.sh\n@@ -15,14 +15,14 @@ test_expect_success 'setup (initial)' '\n \tgit commit -m commit1\n '\n \n-test_expect_failure 'edit hunk \"commit -p -m message\"' '\n+test_expect_success 'edit hunk \"commit -p -m message\"' '\n \ttest_when_finished \"rm -f editor_was_started\" &&\n \techo more >>file &&\n \techo e | env GIT_EDITOR=\"touch editor_was_started\" git commit -p -m commit2 file &&\n \ttest -r editor_was_started\n '\n \n-test_expect_failure 'edit hunk \"commit --dry-run -p -m message\"' '\n+test_expect_success 'edit hunk \"commit --dry-run -p -m message\"' '\n \ttest_when_finished \"rm -f editor_was_started\" &&\n \techo more >>file &&\n \techo e | env GIT_EDITOR=\"touch editor_was_started\" git commit -p -m commit3 file &&\n-- \n1.9.0\n"},{"id":"236810","messageId":"1394919750-28432-5-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":"1394919750-28432-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 5/7] merge: fix GIT_EDITOR override for commit hook","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-15T21:42:28Z","receivedAt":"2014-03-15T21:42:28Z","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 \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 bdf6655..e15d0e1 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -824,7 +824,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":"236811","messageId":"1394919750-28432-6-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":"1394919750-28432-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 6/7] merge hook tests: fix and update tests","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-15T21:42:29Z","receivedAt":"2014-03-15T21:42:29Z","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- make sure the tree is reset to a clean state after running a test\n (using test_when_finished) so later tests are not impacted\n\nSigned-off-by: Benoit Pierre \n---\n t/t7505-prepare-commit-msg-hook.sh | 27 +++++++++++++++++++++------\n 1 file changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\nindex 5531abb..03dce09 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -134,14 +134,26 @@ test_expect_success 'with hook (-c)' '\n \n test_expect_success 'with hook (merge)' '\n \n-\thead=`git rev-parse HEAD` &&\n-\tgit checkout -b other HEAD@{1} &&\n-\techo \"more\" >> file &&\n+\ttest_when_finished \"git checkout -f master\" &&\n+\tgit checkout -B other HEAD@{1} &&\n+\techo \"more\" >>file &&\n+\tgit add file &&\n+\tgit commit -m other &&\n+\tgit checkout - &&\n+\tgit merge --no-ff other &&\n+\ttest \"`git log -1 --pretty=format:%s`\" = \"merge (no editor)\"\n+'\n+\n+test_expect_success 'with hook and editor (merge)' '\n+\n+\ttest_when_finished \"git checkout -f master\" &&\n+\tgit checkout -B other HEAD@{1} &&\n+\techo \"more\" >>file &&\n \tgit add file &&\n \tgit commit -m other &&\n \tgit checkout - &&\n-\tgit merge other &&\n-\ttest \"`git log -1 --pretty=format:%s`\" = merge\n+\tenv GIT_EDITOR=\"\\\"\\$FAKE_EDITOR\\\"\" git merge --no-ff -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+\ttest_when_finished \"git checkout -f master\" &&\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+\ttest_when_finished \"git checkout -f master\" &&\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+\ttest_when_finished \"git checkout -f master\" &&\n \tgit checkout -B other HEAD@{1} &&\n \techo \"more\" >> file &&\n \tgit add file &&\n@@ -178,7 +193,7 @@ test_expect_success 'with failing hook (merge)' '\n \texit 1\n \tEOF\n \tgit checkout - &&\n-\ttest_must_fail git merge other\n+\ttest_must_fail git merge --no-ff other\n \n '\n \n-- \n1.9.0\n"},{"id":"236812","messageId":"1394919750-28432-7-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":"1394919750-28432-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 7/7] run-command: mark run_hook_with_custom_index as deprecated","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-15T21:42:30Z","receivedAt":"2014-03-15T21:42:30Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"Signed-off-by: Benoit Pierre \n---\n run-command.h | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/run-command.h b/run-command.h\nindex 88460f9..3653bfa 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -51,6 +51,7 @@ extern int run_hook_le(const char *const *env, const char *name, ...);\n extern int run_hook_ve(const char *const *env, const char *name, va_list args);\n \n LAST_ARG_MUST_BE_NULL\n+__attribute__((deprecated))\n extern int run_hook_with_custom_index(const char *index_file, const char *name, ...);\n \n #define RUN_COMMAND_NO_STDIN 1\n-- \n1.9.0\n"},{"id":"236895","messageId":"xmqqr460ek3k.fsf@gitster.dls.corp.google.com","threadId":"36111","inReplyTo":"1394919750-28432-3-git-send-email-benoit.pierre@gmail.com","subject":"Re: [PATCH 3/7] test patch hunk editing with \"commit -p -m\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-17T18:49:35Z","receivedAt":"2014-03-17T18:49:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Benoit Pierre writes:\n\n> Add (failing) tests: with commit changing the environment to let hooks\n> know 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 \n> ---\n> t/t7513-commit-patch.sh | 32 ++++++++++++++++++++++++++++++++\n> 1 file changed, 32 insertions(+)\n> create mode 100755 t/t7513-commit-patch.sh\n>\n> diff --git a/t/t7513-commit-patch.sh b/t/t7513-commit-patch.sh\n\nAgain, as I said, I'll rename this to t7514-commit.patch.sh while I\nqueue this.\n\nThanks.\n\n> new file mode 100755\n> index 0000000..9311b0c\n> --- /dev/null\n> +++ b/t/t7513-commit-patch.sh\n> @@ -0,0 +1,32 @@\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> +'\n> +\n> +test_expect_failure 'edit hunk \"commit -p -m message\"' '\n> +\ttest_when_finished \"rm -f editor_was_started\" &&\n> +\techo more >>file &&\n> +\techo e | env GIT_EDITOR=\"touch editor_was_started\" git commit -p -m commit2 file &&\n> +\ttest -r editor_was_started\n> +'\n> +\n> +test_expect_failure 'edit hunk \"commit --dry-run -p -m message\"' '\n> +\ttest_when_finished \"rm -f editor_was_started\" &&\n> +\techo more >>file &&\n> +\techo e | env GIT_EDITOR=\"touch editor_was_started\" git commit -p -m commit3 file &&\n> +\ttest -r editor_was_started\n> +'\n> +\n> +test_done\n"},{"id":"236896","messageId":"xmqqmwgoejwq.fsf@gitster.dls.corp.google.com","threadId":"36111","inReplyTo":"1394919750-28432-3-git-send-email-benoit.pierre@gmail.com","subject":"Re: [PATCH 3/7] test patch hunk editing with \"commit -p -m\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-17T18:53:41Z","receivedAt":"2014-03-17T18:53:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Benoit Pierre writes:\n\n> +test_expect_failure 'edit hunk \"commit -p -m message\"' '\n> +\ttest_when_finished \"rm -f editor_was_started\" &&\n\nNot just \"when finished\", run \"rm -f\" here to make sure that the\nfile does not exist. Later other people may add new tests before\nthis test piece and affect the state of your throw-away working\ntree, and you would want to protect yourself from their changes.\n\n> +\techo more >>file &&\n> +\techo e | env GIT_EDITOR=\"touch editor_was_started\" git commit -p -m commit2 file &&\n\nAvoid \"touch\" unless you are interested in the timestamp to be\nupdated. Use \": >editor_was_started\" or something like that when\nyou are only interested in \"it was not there, now it is\".\n\nThe same comment applies to the next one.\n\nThanks.\n\n> +\ttest -r editor_was_started\n> +'\n> +\n> +test_expect_failure 'edit hunk \"commit --dry-run -p -m message\"' '\n> +\ttest_when_finished \"rm -f editor_was_started\" &&\n> +\techo more >>file &&\n> +\techo e | env GIT_EDITOR=\"touch editor_was_started\" git commit -p -m commit3 file &&\n> +\ttest -r editor_was_started\n> +'\n> +\n> +test_done\n"},{"id":"236904","messageId":"CA+SSzV3CZjpNaFuf1U1DcPXiKy=rpJH8M2q1xeMvyh9uEHxvOA@mail.gmail.com","threadId":"36111","inReplyTo":"xmqqr460ek3k.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/7] test patch hunk editing with \"commit -p -m\"","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-17T19:46:18Z","receivedAt":"2014-03-17T19:46:18Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"On Mon, Mar 17, 2014 at 7:49 PM, Junio C Hamano wrote:\n> Benoit Pierre writes:\n>\n>> Add (failing) tests: with commit changing the environment to let hooks\n>> know 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 \n>> ---\n>> t/t7513-commit-patch.sh | 32 ++++++++++++++++++++++++++++++++\n>> 1 file changed, 32 insertions(+)\n>> create mode 100755 t/t7513-commit-patch.sh\n>>\n>> diff --git a/t/t7513-commit-patch.sh b/t/t7513-commit-patch.sh\n>\n> Again, as I said, I'll rename this to t7514-commit.patch.sh while I\n> queue this.\n\nI assumed the \"14\" was a typo, will rename, but to\nt7514-commit-patch.sh right? (with 2 '-').\n\n-- \nA: Because it destroys the flow of conversation.\nQ: Why is top posting dumb?\n"},{"id":"236905","messageId":"CAPig+cTJz30TKmnNqQUiubFKTnjH8FHc5_v1mvt9AB5eMDBHag@mail.gmail.com","threadId":"36111","inReplyTo":"CA+SSzV3CZjpNaFuf1U1DcPXiKy=rpJH8M2q1xeMvyh9uEHxvOA@mail.gmail.com","subject":"Re: [PATCH 3/7] test patch hunk editing with \"commit -p -m\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-17T19:51:40Z","receivedAt":"2014-03-17T19:51:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 17, 2014 at 3:46 PM, Benoit Pierre wrote:\n> On Mon, Mar 17, 2014 at 7:49 PM, Junio C Hamano wrote:\n>> Benoit Pierre writes:\n>>\n>>> Add (failing) tests: with commit changing the environment to let hooks\n>>> know 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 \n>>> ---\n>>> t/t7513-commit-patch.sh | 32 ++++++++++++++++++++++++++++++++\n>>> 1 file changed, 32 insertions(+)\n>>> create mode 100755 t/t7513-commit-patch.sh\n>>>\n>>> diff --git a/t/t7513-commit-patch.sh b/t/t7513-commit-patch.sh\n>>\n>> Again, as I said, I'll rename this to t7514-commit.patch.sh while I\n>> queue this.\n>\n> I assumed the \"14\" was a typo, will rename, but to\n> t7514-commit-patch.sh right? (with 2 '-').\n\nYes, two '-'.\n\nIn the 'pu' branch, there is a new t7513-interpret-trailers.sh.\n"},{"id":"236906","messageId":"CA+SSzV2qpQ+O0Ee7hrv+TM-j03BAvC1s_qJqOnyoBtVbCgxjtw@mail.gmail.com","threadId":"36111","inReplyTo":"xmqqmwgoejwq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/7] test patch hunk editing with \"commit -p -m\"","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-17T19:52:00Z","receivedAt":"2014-03-17T19:52:00Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"On Mon, Mar 17, 2014 at 7:53 PM, Junio C Hamano wrote:\n> Benoit Pierre writes:\n>\n>> +test_expect_failure 'edit hunk \"commit -p -m message\"' '\n>> + test_when_finished \"rm -f editor_was_started\" &&\n>\n> Not just \"when finished\", run \"rm -f\" here to make sure that the\n> file does not exist. Later other people may add new tests before\n> this test piece and affect the state of your throw-away working\n> tree, and you would want to protect yourself from their changes.\n>\n>> + echo more >>file &&\n>> + echo e | env GIT_EDITOR=\"touch editor_was_started\" git commit -p -m commit2 file &&\n>\n> Avoid \"touch\" unless you are interested in the timestamp to be\n> updated. Use \": >editor_was_started\" or something like that when\n> you are only interested in \"it was not there, now it is\".\n>\n> The same comment applies to the next one.\n\nIsn't the point of using \"when finished\" to have each test leave the\ntree clean after execution, to avoid \"bleeding\" onto the next test(s)?\nInstead of having each test clean after the previous test(s). It seems\nto me using both is kind of redundant, no?\n\n-- \nA: Because it destroys the flow of conversation.\nQ: Why is top posting dumb?\n"},{"id":"236907","messageId":"CA+SSzV0+ijF=ws2OfNDsrBj1SFT--O=DbNMK47gfcxmyMdhK4g@mail.gmail.com","threadId":"36111","inReplyTo":"CAPig+cTJz30TKmnNqQUiubFKTnjH8FHc5_v1mvt9AB5eMDBHag@mail.gmail.com","subject":"Re: [PATCH 3/7] test patch hunk editing with \"commit -p -m\"","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-17T19:53:37Z","receivedAt":"2014-03-17T19:53:37Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"On Mon, Mar 17, 2014 at 8:51 PM, Eric Sunshine wrote:\n> On Mon, Mar 17, 2014 at 3:46 PM, Benoit Pierre wrote:\n>> On Mon, Mar 17, 2014 at 7:49 PM, Junio C Hamano wrote:\n>>> Benoit Pierre writes:\n>>>\n>>>> [...]\n>>>>\n>>>> diff --git a/t/t7513-commit-patch.sh b/t/t7513-commit-patch.sh\n>>>\n>>> Again, as I said, I'll rename this to t7514-commit.patch.sh while I\n>>> queue this.\n>>\n>> I assumed the \"14\" was a typo, will rename, but to\n>> t7514-commit-patch.sh right? (with 2 '-').\n>\n> Yes, two '-'.\n>\n> In the 'pu' branch, there is a new t7513-interpret-trailers.sh.\n\nOK.\n\n-- \nA: Because it destroys the flow of conversation.\nQ: Why is top posting dumb?\n"},{"id":"236919","messageId":"xmqqa9cocxyb.fsf@gitster.dls.corp.google.com","threadId":"36111","inReplyTo":"CA+SSzV3CZjpNaFuf1U1DcPXiKy=rpJH8M2q1xeMvyh9uEHxvOA@mail.gmail.com","subject":"Re: [PATCH 3/7] test patch hunk editing with \"commit -p -m\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-17T21:33:16Z","receivedAt":"2014-03-17T21:33:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Benoit Pierre writes:\n\n> On Mon, Mar 17, 2014 at 7:49 PM, Junio C Hamano wrote:\n>> Benoit Pierre writes:\n>>\n>>> Add (failing) tests: with commit changing the environment to let hooks\n>>> know 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 \n>>> ---\n>>> t/t7513-commit-patch.sh | 32 ++++++++++++++++++++++++++++++++\n>>> 1 file changed, 32 insertions(+)\n>>> create mode 100755 t/t7513-commit-patch.sh\n>>>\n>>> diff --git a/t/t7513-commit-patch.sh b/t/t7513-commit-patch.sh\n>>\n>> Again, as I said, I'll rename this to t7514-commit.patch.sh while I\n>> queue this.\n>\n> I assumed the \"14\" was a typo, will rename, but to\n> t7514-commit-patch.sh right? (with 2 '-').\n\nThanks, yes. That is how I queued the previous one on 'pu', I\nthink.\n"},{"id":"236920","messageId":"xmqq61nccxvc.fsf@gitster.dls.corp.google.com","threadId":"36111","inReplyTo":"CA+SSzV2qpQ+O0Ee7hrv+TM-j03BAvC1s_qJqOnyoBtVbCgxjtw@mail.gmail.com","subject":"Re: [PATCH 3/7] test patch hunk editing with \"commit -p -m\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-17T21:35:03Z","receivedAt":"2014-03-17T21:35:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Benoit Pierre writes:\n\n> Isn't the point of using \"when finished\" to have each test leave the\n> tree clean after execution, to avoid \"bleeding\" onto the next test(s)?\n\nBut you cannot anticipate what other people will do in the future\nbefore you have a chance to run this piece. Your using when-finished\nis courtesy for others. Your explicitly making sure what you do not\nwant to be stray behind is protecting yourself from others.\n\nThey serve different purposes, and you need both.\n"},{"id":"236971","messageId":"1395136856-17225-1-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":"xmqqmwgoejwq.fsf@gitster.dls.corp.google.com","subject":"[PATCH 1/7] merge hook tests: fix missing '&&' in test","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-18T10:00:50Z","receivedAt":"2014-03-18T10:00:50Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"Signed-off-by: Benoit Pierre \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 3573751..1c95652 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -174,7 +174,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":"236976","messageId":"1395136856-17225-2-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":"1395136856-17225-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 2/7] merge hook tests: use 'test_must_fail' instead of '!'","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-18T10:00:51Z","receivedAt":"2014-03-18T10:00:51Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"Signed-off-by: Benoit Pierre \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 1c95652..5531abb 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -154,7 +154,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@@ -163,7 +163,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":"236972","messageId":"1395136856-17225-3-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":"1395136856-17225-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 3/7] test patch hunk editing with \"commit -p -m\"","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-18T10:00:52Z","receivedAt":"2014-03-18T10:00:52Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"Add (failing) tests: with commit changing the environment to let hooks\nknow 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 \n---\n t/t7514-commit-patch.sh | 34 ++++++++++++++++++++++++++++++++++\n 1 file changed, 34 insertions(+)\n create mode 100755 t/t7514-commit-patch.sh\n\ndiff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh\nnew file mode 100755\nindex 0000000..41dd37a\n--- /dev/null\n+++ b/t/t7514-commit-patch.sh\n@@ -0,0 +1,34 @@\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+'\n+\n+test_expect_failure 'edit hunk \"commit -p -m message\"' '\n+\ttest_when_finished \"rm -f editor_was_started\" &&\n+\trm -f editor_was_started &&\n+\techo more >>file &&\n+\techo e | env GIT_EDITOR=\": >editor_was_started\" git commit -p -m commit2 file &&\n+\ttest -r editor_was_started\n+'\n+\n+test_expect_failure 'edit hunk \"commit --dry-run -p -m message\"' '\n+\ttest_when_finished \"rm -f editor_was_started\" &&\n+\trm -f editor_was_started &&\n+\techo more >>file &&\n+\techo e | env GIT_EDITOR=\": >editor_was_started\" git commit -p -m commit3 file &&\n+\ttest -r editor_was_started\n+'\n+\n+test_done\n-- \n1.9.0\n"},{"id":"236973","messageId":"1395136856-17225-4-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":"1395136856-17225-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 4/7] commit: fix patch hunk editing with \"commit -p -m\"","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-18T10:00:53Z","receivedAt":"2014-03-18T10:00:53Z","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 \n---\n builtin/checkout.c | 8 ++++----\n builtin/clone.c | 4 ++--\n builtin/commit.c | 35 ++++++++++++++++++++++++++++-------\n builtin/gc.c | 2 +-\n builtin/merge.c | 6 +++---\n commit.h | 3 +++\n run-command.c | 44 ++++++++++++++++++++++++++++++++------------\n run-command.h | 6 +++++-\n t/t7514-commit-patch.sh | 4 ++--\n 9 files changed, 80 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex ada51fa..1b86d9c 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -53,10 +53,10 @@ struct checkout_opts {\n static int post_checkout_hook(struct commit *old, struct commit *new,\n \t\t\t int changed)\n {\n-\treturn run_hook(NULL, \"post-checkout\",\n-\t\t\tsha1_to_hex(old ? old->object.sha1 : null_sha1),\n-\t\t\tsha1_to_hex(new ? new->object.sha1 : null_sha1),\n-\t\t\tchanged ? \"1\" : \"0\", NULL);\n+\treturn run_hook_le(NULL, \"post-checkout\",\n+\t\t\t sha1_to_hex(old ? old->object.sha1 : null_sha1),\n+\t\t\t sha1_to_hex(new ? new->object.sha1 : null_sha1),\n+\t\t\t changed ? \"1\" : \"0\", NULL);\n \t/* \"new\" can be NULL when checking out from the index before\n \t a commit exists. */\n \ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 43e772c..9b3c04d 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -660,8 +660,8 @@ static int checkout(void)\n \t commit_locked_index(lock_file))\n \t\tdie(_(\"unable to write new index file\"));\n \n-\terr |= run_hook(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\n-\t\t\tsha1_to_hex(sha1), \"1\", NULL);\n+\terr |= run_hook_le(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\n+\t\t\t sha1_to_hex(sha1), \"1\", NULL);\n \n \tif (!err && option_recursive)\n \t\terr = run_command_v_opt(argv_submodule, RUN_GIT_CMD);\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 3783bca..68a90b3 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -610,7 +610,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@@ -867,8 +867,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@@ -884,7 +884,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@@ -1068,8 +1068,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@@ -1450,6 +1448,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_ve(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@@ -1674,7 +1695,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/gc.c b/builtin/gc.c\nindex 63d400b..11cf295 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -184,7 +184,7 @@ static int need_to_gc(void)\n \telse if (!too_many_loose_objects())\n \t\treturn 0;\n \n-\tif (run_hook(NULL, \"pre-auto-gc\", NULL))\n+\tif (run_hook_le(NULL, \"pre-auto-gc\", NULL))\n \t\treturn 0;\n \treturn 1;\n }\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex f0cf120..bdf6655 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -421,7 +421,7 @@ static void finish(struct commit *head_commit,\n \t}\n \n \t/* Run a post-merge hook */\n-\trun_hook(NULL, \"post-merge\", squash ? \"1\" : \"0\", NULL);\n+\trun_hook_le(NULL, \"post-merge\", squash ? \"1\" : \"0\", NULL);\n \n \tstrbuf_release(&reflog_message);\n }\n@@ -824,8 +824,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..75abc47 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_ve(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,45 @@ 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_le(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_ve(env, name, args);\n+\tva_end(args);\n+\n+\treturn ret;\n+}\n+\n+int run_hook_with_custom_index(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+\tva_start(args, name);\n+\tret = run_hook_ve(hook_env, name, args);\n+\tva_end(args);\n+\n+\treturn ret;\n+}\ndiff --git a/run-command.h b/run-command.h\nindex 6b985af..88460f9 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -47,7 +47,11 @@ 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_le(const char *const *env, const char *name, ...);\n+extern int run_hook_ve(const char *const *env, const char *name, va_list args);\n+\n+LAST_ARG_MUST_BE_NULL\n+extern int run_hook_with_custom_index(const char *index_file, const char *name, ...);\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/t7514-commit-patch.sh b/t/t7514-commit-patch.sh\nindex 41dd37a..998a210 100755\n--- a/t/t7514-commit-patch.sh\n+++ b/t/t7514-commit-patch.sh\n@@ -15,7 +15,7 @@ test_expect_success 'setup (initial)' '\n \tgit commit -m commit1\n '\n \n-test_expect_failure 'edit hunk \"commit -p -m message\"' '\n+test_expect_success 'edit hunk \"commit -p -m message\"' '\n \ttest_when_finished \"rm -f editor_was_started\" &&\n \trm -f editor_was_started &&\n \techo more >>file &&\n@@ -23,7 +23,7 @@ test_expect_failure 'edit hunk \"commit -p -m message\"' '\n \ttest -r editor_was_started\n '\n \n-test_expect_failure 'edit hunk \"commit --dry-run -p -m message\"' '\n+test_expect_success 'edit hunk \"commit --dry-run -p -m message\"' '\n \ttest_when_finished \"rm -f editor_was_started\" &&\n \trm -f editor_was_started &&\n \techo more >>file &&\n-- \n1.9.0\n"},{"id":"236974","messageId":"1395136856-17225-5-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":"1395136856-17225-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 5/7] merge: fix GIT_EDITOR override for commit hook","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-18T10:00:54Z","receivedAt":"2014-03-18T10:00:54Z","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 \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 bdf6655..e15d0e1 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -824,7 +824,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":"236975","messageId":"1395136856-17225-6-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":"1395136856-17225-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 6/7] merge hook tests: fix and update tests","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-18T10:00:55Z","receivedAt":"2014-03-18T10:00:55Z","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- make sure the tree is reset to a clean state after running a test\n (using test_when_finished) so later tests are not impacted\n\nSigned-off-by: Benoit Pierre \n---\n t/t7505-prepare-commit-msg-hook.sh | 27 +++++++++++++++++++++------\n 1 file changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\nindex 5531abb..03dce09 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -134,14 +134,26 @@ test_expect_success 'with hook (-c)' '\n \n test_expect_success 'with hook (merge)' '\n \n-\thead=`git rev-parse HEAD` &&\n-\tgit checkout -b other HEAD@{1} &&\n-\techo \"more\" >> file &&\n+\ttest_when_finished \"git checkout -f master\" &&\n+\tgit checkout -B other HEAD@{1} &&\n+\techo \"more\" >>file &&\n+\tgit add file &&\n+\tgit commit -m other &&\n+\tgit checkout - &&\n+\tgit merge --no-ff other &&\n+\ttest \"`git log -1 --pretty=format:%s`\" = \"merge (no editor)\"\n+'\n+\n+test_expect_success 'with hook and editor (merge)' '\n+\n+\ttest_when_finished \"git checkout -f master\" &&\n+\tgit checkout -B other HEAD@{1} &&\n+\techo \"more\" >>file &&\n \tgit add file &&\n \tgit commit -m other &&\n \tgit checkout - &&\n-\tgit merge other &&\n-\ttest \"`git log -1 --pretty=format:%s`\" = merge\n+\tenv GIT_EDITOR=\"\\\"\\$FAKE_EDITOR\\\"\" git merge --no-ff -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+\ttest_when_finished \"git checkout -f master\" &&\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+\ttest_when_finished \"git checkout -f master\" &&\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+\ttest_when_finished \"git checkout -f master\" &&\n \tgit checkout -B other HEAD@{1} &&\n \techo \"more\" >> file &&\n \tgit add file &&\n@@ -178,7 +193,7 @@ test_expect_success 'with failing hook (merge)' '\n \texit 1\n \tEOF\n \tgit checkout - &&\n-\ttest_must_fail git merge other\n+\ttest_must_fail git merge --no-ff other\n \n '\n \n-- \n1.9.0\n"},{"id":"236977","messageId":"1395136856-17225-7-git-send-email-benoit.pierre@gmail.com","threadId":"36111","inReplyTo":"1395136856-17225-1-git-send-email-benoit.pierre@gmail.com","subject":"[PATCH 7/7] run-command: mark run_hook_with_custom_index as deprecated","fromName":"Benoit Pierre","fromEmail":"benoit.pierre@gmail.com","sentAt":"2014-03-18T10:00:56Z","receivedAt":"2014-03-18T10:00:56Z","isPatch":true,"sender":{"key":"benoit.pierre@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5104286?v=4"},"body":"Signed-off-by: Benoit Pierre \n---\n run-command.h | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/run-command.h b/run-command.h\nindex 88460f9..3653bfa 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -51,6 +51,7 @@ extern int run_hook_le(const char *const *env, const char *name, ...);\n extern int run_hook_ve(const char *const *env, const char *name, va_list args);\n \n LAST_ARG_MUST_BE_NULL\n+__attribute__((deprecated))\n extern int run_hook_with_custom_index(const char *index_file, const char *name, ...);\n \n #define RUN_COMMAND_NO_STDIN 1\n-- \n1.9.0\n"},{"id":"236994","messageId":"xmqq4n2v9xbt.fsf@gitster.dls.corp.google.com","threadId":"36111","inReplyTo":"1395136856-17225-1-git-send-email-benoit.pierre@gmail.com","subject":"Re: [PATCH 1/7] merge hook tests: fix missing '&&' in test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-18T18:27:18Z","receivedAt":"2014-03-18T18:27:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks; will replace what has been on 'pu' with the updated series.\n"},{"id":"237043","messageId":"53294808.8030106@web.de","threadId":"36111","inReplyTo":"1395136856-17225-4-git-send-email-benoit.pierre@gmail.com","subject":"Re: [PATCH 4/7] commit: fix patch hunk editing with \"commit -p -m\"","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-03-19T07:32:24Z","receivedAt":"2014-03-19T07:32:24Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 03/18/2014 11:00 AM, Benoit Pierre wrote:\n> Don't change git environment: move the GIT_EDITOR=\":\" override to the\n> hook command subprocess, like it's already done for GIT_INDEX_FILE.\n>\n> Signed-off-by: Benoit Pierre \n> ---\n> builtin/checkout.c | 8 ++++----\n> builtin/clone.c | 4 ++--\n> builtin/commit.c | 35 ++++++++++++++++++++++++++++-------\n> builtin/gc.c | 2 +-\n> builtin/merge.c | 6 +++---\n> commit.h | 3 +++\n> run-command.c | 44 ++++++++++++++++++++++++++++++++------------\n> run-command.h | 6 +++++-\n> t/t7514-commit-patch.sh | 4 ++--\n> 9 files changed, 80 insertions(+), 32 deletions(-)\n>\n[]\n> index 3914d9c..75abc47 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_ve(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(Please see below)\n> -\tva_list args;\n> +\tconst char *p;\n> \tint ret;\n>\n> \tp = find_hook(name);\n> @@ -775,23 +773,45 @@ 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_le(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_ve(env, name, args);\n> +\tva_end(args);\n> +\n> +\treturn ret;\n> +}\n> +\n> +int run_hook_with_custom_index(const char *index_file, const char *name, ...)\n> +{\n> +\tconst char *hook_env[3] = { NULL };\n> +\tchar index[PATH_MAX];\nSorry being late with the review.\n\nRecently some effort has been put to replace the\n \"PATH_MAX/snprintf() combo\" with code using strbuf.\n\nSo I think for new developed code it could make sense to avoid PATH_MAX \nfrom the start.\nTo my knowledge mkpathdup() from path.c can be used,\n(or are there better ways ?)\nand the whole function will look like below (not tested)\n\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> +\tva_start(args, name);\n> +\tret = run_hook_ve(hook_env, name, args);\n> +\tva_end(args);\n> +\n> +\treturn ret;\n> +}\n\nint run_hook_with_custom_index(const char *index_file, const char *name, \n...)\n{\n\tconst char *hook_env[3] = { NULL };\n\tchar index = mkpathdup(\"GIT_INDEX_FILE=%s\", index_file);\n\tva_list args;\n\tint ret;\n\n\thook_env[0] = index;\n\n\tva_start(args, name);\n\tret = run_hook_ve(hook_env, name, args);\n\tva_end(args);\n\n\tfree(index);\n\treturn ret;\n}\n\n\n\n\n> diff --git a/run-command.h b/run-command.h\n> index 6b985af..88460f9 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -47,7 +47,11 @@ 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_le(const char *const *env, const char *name, ...);\n> +extern int run_hook_ve(const char *const *env, const char *name, va_list args);\n> +\n> +LAST_ARG_MUST_BE_NULL\n> +extern int run_hook_with_custom_index(const char *index_file, const char *name, ...);\n>\n> #define RUN_COMMAND_NO_STDIN 1\n> #define RUN_GIT_CMD\t 2\t/*If this is to be git sub-command */\n> diff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh\n> index 41dd37a..998a210 100755\n> --- a/t/t7514-commit-patch.sh\n> +++ b/t/t7514-commit-patch.sh\n> @@ -15,7 +15,7 @@ test_expect_success 'setup (initial)' '\n> \tgit commit -m commit1\n> '\n>\n> -test_expect_failure 'edit hunk \"commit -p -m message\"' '\n> +test_expect_success 'edit hunk \"commit -p -m message\"' '\n> \ttest_when_finished \"rm -f editor_was_started\" &&\n> \trm -f editor_was_started &&\n> \techo more >>file &&\n> @@ -23,7 +23,7 @@ test_expect_failure 'edit hunk \"commit -p -m message\"' '\n> \ttest -r editor_was_started\n> '\n>\n> -test_expect_failure 'edit hunk \"commit --dry-run -p -m message\"' '\n> +test_expect_success 'edit hunk \"commit --dry-run -p -m message\"' '\n> \ttest_when_finished \"rm -f editor_was_started\" &&\n> \trm -f editor_was_started &&\n> \techo more >>file &&\n>\n"},{"id":"237086","messageId":"xmqqbnx25cnp.fsf@gitster.dls.corp.google.com","threadId":"36111","inReplyTo":"53294808.8030106@web.de","subject":"Re: [PATCH 4/7] commit: fix patch hunk editing with \"commit -p -m\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-19T17:19:38Z","receivedAt":"2014-03-19T17:19:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n>> +int run_hook_with_custom_index(const char *index_file, const char *name, ...)\n>> +{\n>> +\tconst char *hook_env[3] = { NULL };\n>> +\tchar index[PATH_MAX];\n> Sorry being late with the review.\n>\n> Recently some effort has been put to replace the\n> \"PATH_MAX/snprintf() combo\" with code using strbuf.\n>\n> So I think for new developed code it could make sense to avoid\n> PATH_MAX from the start.\n\nYes but because this is a compatibility wrapper for an existing\nfunction that does the string[PATH_MAX] thing already, it would be\nclearer to have such a conversion as a separate step.\n"},{"id":"238367","messageId":"20140403221115.GA23467@google.com","threadId":"36111","inReplyTo":"1394477377-10994-1-git-send-email-benoit.pierre@gmail.com","subject":"Re: [PATCH V2 0/7] fix hunk editing with 'commit -p -m'","fromName":"Jonathan Nieder","fromEmail":"jrn@google.com","sentAt":"2014-04-03T22:11:15Z","receivedAt":"2014-04-03T22:11:15Z","isPatch":true,"sender":{"key":"jrn@google.com","avatar":null},"body":"Hi,\n\nA quick note for the future:\n\nBenoit Pierre wrote:\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\nThis information should have gone in the relevant patch's commit\nmessage itself. That way, people don't have to hunt down the relevant\nmailing list thread to understand the patch.\n\nGenerally a cover letter should say as little as possible (mostly\n\"here is what patch you might want to look at first, and here is an\noverview of why the patches are in this particular order\").\n\nThanks for a nice fix. Perhaps we'll see more in the future, hence\nthis note. :) And if you have ideas for where an explanation of this\ncould go in the documentation (somewhere in\nDocumentation/SubmittingPatches?), that would be welcome too.\n\nThanks,\nJonathan\n"}]}