{"thread":{"id":"35627","subject":"[PATCH] merge: make merge state available to prepare-commit-msg hook","startedAt":"2014-01-08T19:00:36Z","lastAt":"2014-01-11T00:20:39Z","messageCount":18,"participants":["Ryan Biesemeyer","Matthieu Moy","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"232920","messageId":"6B177FFA-1797-45FE-9EF1-2C9E6EE8A234@yaauie.com","threadId":"35627","inReplyTo":null,"subject":"[PATCH] merge: make merge state available to prepare-commit-msg hook","fromName":"Ryan Biesemeyer","fromEmail":"ryan@yaauie.com","sentAt":"2014-01-08T19:00:36Z","receivedAt":"2014-01-08T19:00:36Z","isPatch":true,"sender":{"key":"ryan@yaauie.com","avatar":"https://gravatar.com/avatar/783155ca56a7f827d64b5849022f5bdc9300adb5ba1d927a599a0b7777d80b6c?d=mp&s=160"},"body":"From a1f898fdf560e719d447a544569d5d1457307855 Mon Sep 17 00:00:00 2001\nFrom: Ryan Biesemeyer \nDate: Wed, 8 Jan 2014 04:22:12 +0000\nSubject: [PATCH 0/2] merge make merge state available to prepare-commit-msg hook\n\nSince prepare-commit-msg hook is given 'merge' as an argument when a merge is\ntaking place, it was surprising that the merge state (MERGE_HEAD, etc.) was not\npresent for the hook's execution.\n\nMake sure that the merge state is written before the prepare-commit-msg hook is\ncalled.\n\nRyan Biesemeyer (2):\n merge: make prepare_to_commit responsible for write_merge_state\n merge: drop unused arg from abort_commit method signature\n\n builtin/merge.c | 13 +++++++------\n t/t7505-prepare-commit-msg-hook.sh | 22 ++++++++++++++++++++++\n 2 files changed, 29 insertions(+), 6 deletions(-)\n\n-- \n1.8.5\n\n\n"},{"id":"232921","messageId":"DA9E6645-82DC-4714-845F-423726F96A40@yaauie.com","threadId":"35627","inReplyTo":"6B177FFA-1797-45FE-9EF1-2C9E6EE8A234@yaauie.com","subject":"Re: [PATCH] merge: make merge state available to prepare-commit-msg hook","fromName":"Ryan Biesemeyer","fromEmail":"ryan@yaauie.com","sentAt":"2014-01-08T19:02:57Z","receivedAt":"2014-01-08T19:02:57Z","isPatch":true,"sender":{"key":"ryan@yaauie.com","avatar":"https://gravatar.com/avatar/783155ca56a7f827d64b5849022f5bdc9300adb5ba1d927a599a0b7777d80b6c?d=mp&s=160"},"body":"From 9b431e5206652cf62ebb09dad4607989976e7748 Mon Sep 17 00:00:00 2001\nFrom: Ryan Biesemeyer \nDate: Wed, 8 Jan 2014 00:46:41 +0000\nSubject: [PATCH 1/2] merge: make prepare_to_commit responsible for\n write_merge_state\n\nWhen merging, make the prepare-commit-msg hook have access to the merge\nstate in order to make decisions about the commit message it is preparing.\n\nSince `abort_commit` is *only* called after `write_merge_state`, and a\nsuccessful commit *always* calls `drop_save` to clear the saved state, this\nchange effectively ensures that the merge state is also available to the\nprepare-commit-msg hook and while the message is being edited.\n\nSigned-off-by: Ryan Biesemeyer \n---\n builtin/merge.c | 3 ++-\n t/t7505-prepare-commit-msg-hook.sh | 22 ++++++++++++++++++++++\n 2 files changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 4941a6c..b7bfc9c 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -802,7 +802,6 @@ static void abort_commit(struct commit_list *remoteheads, const char *err_msg)\n \t\terror(\"%s\", err_msg);\n \tfprintf(stderr,\n \t\t_(\"Not committing merge; use 'git commit' to complete the merge.\\n\"));\n-\twrite_merge_state(remoteheads);\n \texit(1);\n }\n \n@@ -816,6 +815,8 @@ N_(\"Please enter a commit message to explain why this merge is necessary,\\n\"\n static void prepare_to_commit(struct commit_list *remoteheads)\n {\n \tstruct strbuf msg = STRBUF_INIT;\n+\n+\twrite_merge_state(remoteheads);\n \tstrbuf_addbuf(&msg, &merge_msg);\n \tstrbuf_addch(&msg, '\\n');\n \tif (0 < option_edit)\ndiff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\nindex 3573751..89cdfe8 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -181,5 +181,27 @@ test_expect_success 'with failing hook (merge)' '\n \ttest_must_fail git merge other\n \n '\n+git merge --abort # cleanup, since the merge failed.\n+\n+test_expect_success 'should have MERGE_HEAD (merge)' '\n+\n+\tgit checkout -B other HEAD@{1} &&\n+\techo \"more\" >> file &&\n+\tgit add file &&\n+\trm -f \"$HOOK\" &&\n+\tgit commit -m other &&\n+\tgit checkout - &&\n+\twrite_script \"$HOOK\" <<-EOF\n+\tif [ -s \"$(git rev-parse --git-dir)/MERGE_HEAD\" ]; then\n+\t\texit 0\n+\telse\n+\t\texit 1\n+\tfi\n+\tEOF\n+\tgit merge other &&\n+\ttest \"`git log -1 --pretty=format:%s`\" = \"Merge branch '\"'\"'other'\"'\"'\" &&\n+\ttest ! -s \"$(git rev-parse --git-dir)/MERGE_HEAD\"\n+\n+# '\n \n test_done\n-- \n1.8.5\n\n"},{"id":"232922","messageId":"4EE4E1D9-6805-4CA8-8368-6F0EA5EA9F4A@simplymeasured.com","threadId":"35627","inReplyTo":"6B177FFA-1797-45FE-9EF1-2C9E6EE8A234@yaauie.com","subject":"Re: [PATCH] merge: make merge state available to prepare-commit-msg hook","fromName":"Ryan Biesemeyer","fromEmail":"ryan@simplymeasured.com","sentAt":"2014-01-08T19:03:56Z","receivedAt":"2014-01-08T19:03:56Z","isPatch":true,"sender":{"key":"ryan@simplymeasured.com","avatar":null},"body":"From a1f898fdf560e719d447a544569d5d1457307855 Mon Sep 17 00:00:00 2001\nFrom: Ryan Biesemeyer \nDate: Wed, 8 Jan 2014 00:47:41 +0000\nSubject: [PATCH 2/2] merge: drop unused arg from abort_commit method signature\n\nSince abort_commit is no longer responsible for writing merge state, remove\nthe unused argument that was originally needed solely for writing merge state.\n\nSigned-off-by: Ryan Biesemeyer \n---\n builtin/merge.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex b7bfc9c..c3108cf 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -795,8 +795,7 @@ static void read_merge_msg(struct strbuf *msg)\n \t\tdie_errno(_(\"Could not read from '%s'\"), filename);\n }\n \n-static void write_merge_state(struct commit_list *);\n-static void abort_commit(struct commit_list *remoteheads, const char *err_msg)\n+static void abort_commit(const char *err_msg)\n {\n \tif (err_msg)\n \t\terror(\"%s\", err_msg);\n@@ -812,6 +811,7 @@ N_(\"Please enter a commit message to explain why this merge is necessary,\\n\"\n \"Lines starting with '%c' will be ignored, and an empty message aborts\\n\"\n \"the commit.\\n\");\n \n+static void write_merge_state(struct commit_list *);\n static void prepare_to_commit(struct commit_list *remoteheads)\n {\n \tstruct strbuf msg = STRBUF_INIT;\n@@ -824,15 +824,15 @@ static void prepare_to_commit(struct commit_list *remoteheads)\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-\t\tabort_commit(remoteheads, NULL);\n+\t\tabort_commit(NULL);\n \tif (0 < option_edit) {\n \t\tif (launch_editor(git_path(\"MERGE_MSG\"), NULL, NULL))\n-\t\t\tabort_commit(remoteheads, NULL);\n+\t\t\tabort_commit(NULL);\n \t}\n \tread_merge_msg(&msg);\n \tstripspace(&msg, 0 < option_edit);\n \tif (!msg.len)\n-\t\tabort_commit(remoteheads, _(\"Empty commit message.\"));\n+\t\tabort_commit(_(\"Empty commit message.\"));\n \tstrbuf_release(&merge_msg);\n \tstrbuf_addbuf(&merge_msg, &msg);\n \tstrbuf_release(&msg);\n-- \n1.8.5\n\n\n"},{"id":"232923","messageId":"vpqr48ip7bm.fsf@anie.imag.fr","threadId":"35627","inReplyTo":"DA9E6645-82DC-4714-845F-423726F96A40@yaauie.com","subject":"Re: [PATCH] merge: make merge state available to prepare-commit-msg hook","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-01-08T20:06:05Z","receivedAt":"2014-01-08T20:06:05Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Ryan Biesemeyer writes:\n\n> index 3573751..89cdfe8 100755\n> --- a/t/t7505-prepare-commit-msg-hook.sh\n> +++ b/t/t7505-prepare-commit-msg-hook.sh\n> @@ -181,5 +181,27 @@ test_expect_success 'with failing hook (merge)' '\n> \ttest_must_fail git merge other\n> \n> '\n> +git merge --abort # cleanup, since the merge failed.\n\nPlease, avoid having code outside a test_expect_* (see t/README, \" - Put\nall code inside test_expect_success and other assertions.\").\n\n> +test_expect_success 'should have MERGE_HEAD (merge)' '\n> +\n> +\tgit checkout -B other HEAD@{1} &&\n> +\techo \"more\" >> file &&\n> +\tgit add file &&\n> +\trm -f \"$HOOK\" &&\n> +\tgit commit -m other &&\n> +\tgit checkout - &&\n> +\twrite_script \"$HOOK\" <<-EOF\n> +\tif [ -s \"$(git rev-parse --git-dir)/MERGE_HEAD\" ]; then\n> +\t\texit 0\n> +\telse\n> +\t\texit 1\n> +\tfi\n> +\tEOF\n\nI think you lack one && for the write_script line.\n\nThere's another instance in the same file (probably where you got it\nfrom), you should add this to your patch series:\n\n>From c3d754a2a16d98b31d43a7e45973821ae8adc043 Mon Sep 17 00:00:00 2001\nFrom: Matthieu Moy \nDate: Wed, 8 Jan 2014 21:03:27 +0100\nSubject: [PATCH] t7505: add missing &&\n\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.8.5.rc3.4.g8bd3721\n\n(a quick \"git grep write_script\" seems to show a lot of other instances,\nbut no time to dig this now sorry)\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"232924","messageId":"B19C65C1-C2D9-486F-AEA7-1497A3B5C5B4@yaauie.com","threadId":"35627","inReplyTo":"vpqr48ip7bm.fsf@anie.imag.fr","subject":"Re: [PATCH] merge: make merge state available to prepare-commit-msg hook","fromName":"Ryan Biesemeyer","fromEmail":"ryan@yaauie.com","sentAt":"2014-01-08T20:21:25Z","receivedAt":"2014-01-08T20:21:25Z","isPatch":true,"sender":{"key":"ryan@yaauie.com","avatar":"https://gravatar.com/avatar/783155ca56a7f827d64b5849022f5bdc9300adb5ba1d927a599a0b7777d80b6c?d=mp&s=160"},"body":"\nOn 2014-01-08, at 20:06Z, Matthieu Moy wrote:\n\n> Ryan Biesemeyer writes:\n> \n>> index 3573751..89cdfe8 100755\n>> --- a/t/t7505-prepare-commit-msg-hook.sh\n>> +++ b/t/t7505-prepare-commit-msg-hook.sh\n>> @@ -181,5 +181,27 @@ test_expect_success 'with failing hook (merge)' '\n>> \ttest_must_fail git merge other\n>> \n>> '\n>> +git merge --abort # cleanup, since the merge failed.\n> \n> Please, avoid having code outside a test_expect_* (see t/README, \" - Put\n> all code inside test_expect_success and other assertions.\").\n\nIn this case it was not immediately clear to me how to add cleanup to an existing\ntest that dirtied the state of the test repository by leaving behind an in-progress\nmerge. I see `test_cleanup` defined in the test lib & related functions, but see no\nexamples of its use in the test suites. Could you advise? \n\n> \n>> +test_expect_success 'should have MERGE_HEAD (merge)' '\n>> +\n>> +\tgit checkout -B other HEAD@{1} &&\n>> +\techo \"more\" >> file &&\n>> +\tgit add file &&\n>> +\trm -f \"$HOOK\" &&\n>> +\tgit commit -m other &&\n>> +\tgit checkout - &&\n>> +\twrite_script \"$HOOK\" <<-EOF\n>> +\tif [ -s \"$(git rev-parse --git-dir)/MERGE_HEAD\" ]; then\n>> +\t\texit 0\n>> +\telse\n>> +\t\texit 1\n>> +\tfi\n>> +\tEOF\n> \n> I think you lack one && for the write_script line.\n\nGood catch.\n\n> There's another instance in the same file (probably where you got it\n> from), you should add this to your patch series:\n\nI'm new to the mailing-list patch submission process; how would I go about adding it?\nSubmit the cover-letter & patches again? Squash your commit into the relevant one of\nmine?\n\n> From c3d754a2a16d98b31d43a7e45973821ae8adc043 Mon Sep 17 00:00:00 2001\n> From: Matthieu Moy \n> Date: Wed, 8 Jan 2014 21:03:27 +0100\n> Subject: [PATCH] t7505: add missing &&\n> \n> ---\n> t/t7505-prepare-commit-msg-hook.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\n> index 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> -- \n> 1.8.5.rc3.4.g8bd3721\n> \n> (a quick \"git grep write_script\" seems to show a lot of other instances,\n> but no time to dig this now sorry)\n> \n> -- \n> Matthieu Moy\n> http://www-verimag.imag.fr/~moy/\n\nThanks for the review.\n\n--\nRyan Biesemeyer (@yaauie)\n\n"},{"id":"232925","messageId":"20140108202942.GM3881@google.com","threadId":"35627","inReplyTo":"B19C65C1-C2D9-486F-AEA7-1497A3B5C5B4@yaauie.com","subject":"Re: [PATCH] merge: make merge state available to prepare-commit-msg hook","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-01-08T20:29:42Z","receivedAt":"2014-01-08T20:29:42Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRyan Biesemeyer wrote:\n\n> In this case it was not immediately clear to me how to add cleanup to an existing\n> test that dirtied the state of the test repository by leaving behind an in-progress\n> merge. I see `test_cleanup` defined in the test lib & related functions, but see no\n> examples of its use in the test suites. Could you advise? \n\ntest_when_finished pushes a command to be run unconditionally\nwhen the current test assertion finishes.\n\nThanks,\nJonathan\n"},{"id":"232926","messageId":"vpqtxdenoug.fsf@anie.imag.fr","threadId":"35627","inReplyTo":"B19C65C1-C2D9-486F-AEA7-1497A3B5C5B4@yaauie.com","subject":"Re: [PATCH] merge: make merge state available to prepare-commit-msg hook","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-01-08T21:30:31Z","receivedAt":"2014-01-08T21:30:31Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Ryan Biesemeyer writes:\n\n> In this case it was not immediately clear to me how to add cleanup to an existing\n> test that dirtied the state of the test repository by leaving behind an in-progress\n> merge.\n\nJonathan's answer is an option. Another one is\n\ntest_expect_success 'cleanup' '\n\tgit reset ...\n'\n\nSo if the cleanup goes wrong, one can notice.\n\n> I'm new to the mailing-list patch submission process; how would I go\n> about adding it?\n\nYou can apply my patch with \"git am\" in your tree (or at worse, do it by\nhand and steal authorship, I don't mind for a 2 characters patch ;-) ),\nfix your patch to add the missing &&, and then resend with stg like \"git\nsend-email -v2 --in-reply-to=\"\n\n> Submit the cover-letter & patches again?\n\nDefinitely submit patches again. Usually, the cover letter for a resend\nemphasizes on changes compared to previous version.\n\n> Squash your commit into the relevant one of mine?\n\nPreferably not, as my fix is unrelated from yours (mine can come before,\nas a cleanup).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"232927","messageId":"20140108220119.GN3881@google.com","threadId":"35627","inReplyTo":"vpqtxdenoug.fsf@anie.imag.fr","subject":"Re: [PATCH] merge: make merge state available to prepare-commit-msg hook","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-01-08T22:01:19Z","receivedAt":"2014-01-08T22:01:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Matthieu Moy wrote:\n\n> Jonathan's answer is an option. Another one is\n[...]\n> So if the cleanup goes wrong, one can notice.\n\ntest_when_finished also makes the test fail if the cleanup failed.\n\nAnother common strategy is\n\n\ttest_expect_success 'my exciting test' '\n\t\t# this test will rely on these files being absent\n\t\trm -f a b c etc &&\n\n\t\t... rest of the test goes here ...\n\t'\n\nwhich can be a handy way for an especially picky test to protect\nitself (for example with 'git clean -fdx') regardless of the state\nother test assertions create for it.\n\nThis particular example (merge --abort) seems like a good use for\ntest_when_finished because it is about a specific test having made a\nmess and needing to clean up after itself to restore sanity.\n\nHoping that clarifies,\nJonathan\n"},{"id":"232933","messageId":"1389228344-38813-1-git-send-email-ryan@yaauie.com","threadId":"35627","inReplyTo":"6B177FFA-1797-45FE-9EF1-2C9E6EE8A234@yaauie.com","subject":"[PATCH v2 0/4] merge: make merge state available to prepare-commit-msg hook","fromName":"Ryan Biesemeyer","fromEmail":"ryan@yaauie.com","sentAt":"2014-01-09T00:45:40Z","receivedAt":"2014-01-09T00:45:40Z","isPatch":true,"sender":{"key":"ryan@yaauie.com","avatar":"https://gravatar.com/avatar/783155ca56a7f827d64b5849022f5bdc9300adb5ba1d927a599a0b7777d80b6c?d=mp&s=160"},"body":"Ensure merge state is available to the prepare-commit-msg hook.\n\nv2 patchset incorporates early feedback from the list\n\nMatthieu Moy (1):\n t7505: add missing &&\n\nRyan Biesemeyer (3):\n t7505: ensure cleanup after hook blocks merge\n merge: make prepare_to_commit responsible for write_merge_state\n merge: drop unused arg from abort_commit method signature\n\n builtin/merge.c | 13 +++++++------\n t/t7505-prepare-commit-msg-hook.sh | 30 ++++++++++++++++++++++++++----\n 2 files changed, 33 insertions(+), 10 deletions(-)\n\n-- \n1.8.5\n"},{"id":"232934","messageId":"1389228344-38813-2-git-send-email-ryan@yaauie.com","threadId":"35627","inReplyTo":"1389228344-38813-1-git-send-email-ryan@yaauie.com","subject":"[PATCH v2 1/4] t7505: add missing &&","fromName":"Ryan Biesemeyer","fromEmail":"ryan@yaauie.com","sentAt":"2014-01-09T00:45:41Z","receivedAt":"2014-01-09T00:45:41Z","isPatch":true,"sender":{"key":"ryan@yaauie.com","avatar":"https://gravatar.com/avatar/783155ca56a7f827d64b5849022f5bdc9300adb5ba1d927a599a0b7777d80b6c?d=mp&s=160"},"body":"From: Matthieu Moy \n\nSigned-off-by: Ryan Biesemeyer \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.8.5\n"},{"id":"232935","messageId":"1389228344-38813-3-git-send-email-ryan@yaauie.com","threadId":"35627","inReplyTo":"1389228344-38813-1-git-send-email-ryan@yaauie.com","subject":"[PATCH v2 2/4] t7505: ensure cleanup after hook blocks merge","fromName":"Ryan Biesemeyer","fromEmail":"ryan@yaauie.com","sentAt":"2014-01-09T00:45:42Z","receivedAt":"2014-01-09T00:45:42Z","isPatch":true,"sender":{"key":"ryan@yaauie.com","avatar":"https://gravatar.com/avatar/783155ca56a7f827d64b5849022f5bdc9300adb5ba1d927a599a0b7777d80b6c?d=mp&s=160"},"body":"Signed-off-by: Ryan Biesemeyer \n---\n t/t7505-prepare-commit-msg-hook.sh | 25 +++++++++++++------------\n 1 file changed, 13 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\nindex 1c95652..697ecc0 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -168,18 +168,19 @@ test_expect_success 'with failing hook (--no-verify)' '\n '\n \n test_expect_success 'with failing hook (merge)' '\n-\n-\tgit checkout -B other HEAD@{1} &&\n-\techo \"more\" >> file &&\n-\tgit add file &&\n-\trm -f \"$HOOK\" &&\n-\tgit commit -m other &&\n-\twrite_script \"$HOOK\" <<-EOF &&\n-\texit 1\n-\tEOF\n-\tgit checkout - &&\n-\ttest_must_fail git merge other\n-\n+ test_when_finished \"git merge --abort\" &&\n+ (\n+\t\tgit checkout -B other HEAD@{1} &&\n+\t\techo \"more\" >> file &&\n+\t\tgit add file &&\n+\t\trm -f \"$HOOK\" &&\n+\t\tgit commit -m other &&\n+\t\twrite_script \"$HOOK\" <<-EOF &&\n+\t\texit 1\n+\t\tEOF\n+\t\tgit checkout - &&\n+\t\ttest_must_fail git merge other\n+ )\n '\n \n test_done\n-- \n1.8.5\n"},{"id":"232936","messageId":"1389228344-38813-4-git-send-email-ryan@yaauie.com","threadId":"35627","inReplyTo":"1389228344-38813-1-git-send-email-ryan@yaauie.com","subject":"[PATCH v2 3/4] merge: make prepare_to_commit responsible for write_merge_state","fromName":"Ryan Biesemeyer","fromEmail":"ryan@yaauie.com","sentAt":"2014-01-09T00:45:43Z","receivedAt":"2014-01-09T00:45:43Z","isPatch":true,"sender":{"key":"ryan@yaauie.com","avatar":"https://gravatar.com/avatar/783155ca56a7f827d64b5849022f5bdc9300adb5ba1d927a599a0b7777d80b6c?d=mp&s=160"},"body":"When merging, make the prepare-commit-msg hook have access to the merge\nstate in order to make decisions about the commit message it is preparing.\n\nSince `abort_commit` is *only* called after `write_merge_state`, and a\nsuccessful commit *always* calls `drop_save` to clear the saved state, this\nchange effectively ensures that the merge state is also available to the\nprepare-commit-msg hook and while the message is being edited.\n\nSigned-off-by: Ryan Biesemeyer \n---\n builtin/merge.c | 3 ++-\n t/t7505-prepare-commit-msg-hook.sh | 21 +++++++++++++++++++++\n 2 files changed, 23 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 4941a6c..b7bfc9c 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -802,7 +802,6 @@ static void abort_commit(struct commit_list *remoteheads, const char *err_msg)\n \t\terror(\"%s\", err_msg);\n \tfprintf(stderr,\n \t\t_(\"Not committing merge; use 'git commit' to complete the merge.\\n\"));\n-\twrite_merge_state(remoteheads);\n \texit(1);\n }\n \n@@ -816,6 +815,8 @@ N_(\"Please enter a commit message to explain why this merge is necessary,\\n\"\n static void prepare_to_commit(struct commit_list *remoteheads)\n {\n \tstruct strbuf msg = STRBUF_INIT;\n+\n+\twrite_merge_state(remoteheads);\n \tstrbuf_addbuf(&msg, &merge_msg);\n \tstrbuf_addch(&msg, '\\n');\n \tif (0 < option_edit)\ndiff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\nindex 697ecc0..7ccf870 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -183,4 +183,25 @@ test_expect_success 'with failing hook (merge)' '\n )\n '\n \n+test_expect_success 'should have MERGE_HEAD (merge)' '\n+\n+\tgit checkout -B other HEAD@{1} &&\n+\techo \"more\" >> file &&\n+\tgit add file &&\n+\trm -f \"$HOOK\" &&\n+\tgit commit -m other &&\n+\tgit checkout - &&\n+\twrite_script \"$HOOK\" <<-EOF &&\n+\tif [ -s \"$(git rev-parse --git-dir)/MERGE_HEAD\" ]; then\n+\t\texit 0\n+\telse\n+\t\texit 1\n+\tfi\n+\tEOF\n+\tgit merge other &&\n+\ttest \"`git log -1 --pretty=format:%s`\" = \"Merge branch '\"'\"'other'\"'\"'\" &&\n+\ttest ! -s \"$(git rev-parse --git-dir)/MERGE_HEAD\"\n+\n+'\n+\n test_done\n-- \n1.8.5\n"},{"id":"232937","messageId":"1389228344-38813-5-git-send-email-ryan@yaauie.com","threadId":"35627","inReplyTo":"1389228344-38813-1-git-send-email-ryan@yaauie.com","subject":"[PATCH v2 4/4] merge: drop unused arg from abort_commit method signature","fromName":"Ryan Biesemeyer","fromEmail":"ryan@yaauie.com","sentAt":"2014-01-09T00:45:44Z","receivedAt":"2014-01-09T00:45:44Z","isPatch":true,"sender":{"key":"ryan@yaauie.com","avatar":"https://gravatar.com/avatar/783155ca56a7f827d64b5849022f5bdc9300adb5ba1d927a599a0b7777d80b6c?d=mp&s=160"},"body":"Since abort_commit is no longer responsible for writing merge state, remove\nthe unused argument that was originally needed solely for writing merge state.\n\nSigned-off-by: Ryan Biesemeyer \n---\n builtin/merge.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex b7bfc9c..c3108cf 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -795,8 +795,7 @@ static void read_merge_msg(struct strbuf *msg)\n \t\tdie_errno(_(\"Could not read from '%s'\"), filename);\n }\n \n-static void write_merge_state(struct commit_list *);\n-static void abort_commit(struct commit_list *remoteheads, const char *err_msg)\n+static void abort_commit(const char *err_msg)\n {\n \tif (err_msg)\n \t\terror(\"%s\", err_msg);\n@@ -812,6 +811,7 @@ N_(\"Please enter a commit message to explain why this merge is necessary,\\n\"\n \"Lines starting with '%c' will be ignored, and an empty message aborts\\n\"\n \"the commit.\\n\");\n \n+static void write_merge_state(struct commit_list *);\n static void prepare_to_commit(struct commit_list *remoteheads)\n {\n \tstruct strbuf msg = STRBUF_INIT;\n@@ -824,15 +824,15 @@ static void prepare_to_commit(struct commit_list *remoteheads)\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-\t\tabort_commit(remoteheads, NULL);\n+\t\tabort_commit(NULL);\n \tif (0 < option_edit) {\n \t\tif (launch_editor(git_path(\"MERGE_MSG\"), NULL, NULL))\n-\t\t\tabort_commit(remoteheads, NULL);\n+\t\t\tabort_commit(NULL);\n \t}\n \tread_merge_msg(&msg);\n \tstripspace(&msg, 0 < option_edit);\n \tif (!msg.len)\n-\t\tabort_commit(remoteheads, _(\"Empty commit message.\"));\n+\t\tabort_commit(_(\"Empty commit message.\"));\n \tstrbuf_release(&merge_msg);\n \tstrbuf_addbuf(&merge_msg, &msg);\n \tstrbuf_release(&msg);\n-- \n1.8.5\n"},{"id":"232949","messageId":"vpq61ptnwdl.fsf@anie.imag.fr","threadId":"35627","inReplyTo":"1389228344-38813-3-git-send-email-ryan@yaauie.com","subject":"Re: [PATCH v2 2/4] t7505: ensure cleanup after hook blocks merge","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-01-09T13:00:06Z","receivedAt":"2014-01-09T13:00:06Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Ryan Biesemeyer writes:\n\n> + test_when_finished \"git merge --abort\" &&\n> + (\n> +\t\tgit checkout -B other HEAD@{1} &&\n\nWeird indentation (space/tab mix).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"232950","messageId":"vpq4n5dmgnh.fsf@anie.imag.fr","threadId":"35627","inReplyTo":"20140108220119.GN3881@google.com","subject":"Re: [PATCH] merge: make merge state available to prepare-commit-msg hook","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-01-09T13:25:06Z","receivedAt":"2014-01-09T13:25:06Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jonathan Nieder writes:\n\n> Matthieu Moy wrote:\n>\n>> Jonathan's answer is an option. Another one is\n> [...]\n>> So if the cleanup goes wrong, one can notice.\n>\n> test_when_finished also makes the test fail if the cleanup failed.\n\nYes, I was mentionning it as opposed to \"throwing the code at the\ntoplevel of the shell\", not as opposed to test_when_finished.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"233016","messageId":"xmqqppnz76d5.fsf@gitster.dls.corp.google.com","threadId":"35627","inReplyTo":"vpq61ptnwdl.fsf@anie.imag.fr","subject":"Re: [PATCH v2 2/4] t7505: ensure cleanup after hook blocks merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-10T23:40:54Z","receivedAt":"2014-01-10T23:40:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Ryan Biesemeyer writes:\n>\n>> + test_when_finished \"git merge --abort\" &&\n>> + (\n>> +\t\tgit checkout -B other HEAD@{1} &&\n>\n> Weird indentation (space/tab mix).\n\nAlso I do not quite see why the body has to be in a subshell.\n"},{"id":"233017","messageId":"xmqqlhyn74yo.fsf@gitster.dls.corp.google.com","threadId":"35627","inReplyTo":"1389228344-38813-4-git-send-email-ryan@yaauie.com","subject":"Re: [PATCH v2 3/4] merge: make prepare_to_commit responsible for write_merge_state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-11T00:11:11Z","receivedAt":"2014-01-11T00:11:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ryan Biesemeyer writes:\n\n> When merging, make the prepare-commit-msg hook have access to the merge\n> state in order to make decisions about the commit message it is preparing.\n\nWhat information is currently not available, and if available how\nwould that help the hook to formulate a better message?\n\n\tI am not asking you to answer the question to me in an\n\te-mail response. The above is an example of a natural\n\tquestion a reader of the above statement would have and\n\twould wish the log message already answered when the reader\n\tread it.\n\n> Since `abort_commit` is *only* called after `write_merge_state`, and a\n> successful commit *always* calls `drop_save` to clear the saved state, this\n> change effectively ensures that the merge state is also available to the\n> prepare-commit-msg hook and while the message is being edited.\n>\n> Signed-off-by: Ryan Biesemeyer \n> ---\n\nOK. The most important part is that this makes sure that these\nintermediate state files never remains after the normal codepath\nfinishes what it does.\n\nYou seem to be only interested in prepare-commit-msg hook, but\nbecause of your calling write_merge_state() early, these state files\nwill persist while we call finish() and they are also visible while\nthe post-merge hook is run. While I think it may be a good thing\nthat the post-merge hook too can view that information, the log\nmessage makes it sound as if it is an unintended side effect of the\nchange.\n\n> builtin/merge.c | 3 ++-\n> t/t7505-prepare-commit-msg-hook.sh | 21 +++++++++++++++++++++\n> 2 files changed, 23 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 4941a6c..b7bfc9c 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -802,7 +802,6 @@ static void abort_commit(struct commit_list *remoteheads, const char *err_msg)\n> \t\terror(\"%s\", err_msg);\n> \tfprintf(stderr,\n> \t\t_(\"Not committing merge; use 'git commit' to complete the merge.\\n\"));\n> -\twrite_merge_state(remoteheads);\n> \texit(1);\n> }\n> \n> @@ -816,6 +815,8 @@ N_(\"Please enter a commit message to explain why this merge is necessary,\\n\"\n> static void prepare_to_commit(struct commit_list *remoteheads)\n> {\n> \tstruct strbuf msg = STRBUF_INIT;\n> +\n> +\twrite_merge_state(remoteheads);\n> \tstrbuf_addbuf(&msg, &merge_msg);\n> \tstrbuf_addch(&msg, '\\n');\n> \tif (0 < option_edit)\n> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\n> index 697ecc0..7ccf870 100755\n> --- a/t/t7505-prepare-commit-msg-hook.sh\n> +++ b/t/t7505-prepare-commit-msg-hook.sh\n> @@ -183,4 +183,25 @@ test_expect_success 'with failing hook (merge)' '\n> )\n> '\n> \n> +test_expect_success 'should have MERGE_HEAD (merge)' '\n> +\n> +\tgit checkout -B other HEAD@{1} &&\n> +\techo \"more\" >> file &&\n\nStyle: no SP between the redirection operator and its target, i.e.\n\n\techo more >>file &&\n\n> +\tgit add file &&\n> +\trm -f \"$HOOK\" &&\n> +\tgit commit -m other &&\n> +\tgit checkout - &&\n> +\twrite_script \"$HOOK\" <<-EOF &&\n> +\tif [ -s \"$(git rev-parse --git-dir)/MERGE_HEAD\" ]; then\n\nStyle: no [], and no semicolon to join two lines of control\nstatement into one, i.e.\n\n\tif test -s ...\n\tthen\n\n> +\t\texit 0\n> +\telse\n> +\t\texit 1\n> +\tfi\n> +\tEOF\n> +\tgit merge other &&\n> +\ttest \"`git log -1 --pretty=format:%s`\" = \"Merge branch '\"'\"'other'\"'\"'\" &&\n\nStyle:\n\n - After \"sh t7505-*.sh v -i\" fails for whatever reason, being\n able to inspect the trash directory to see what actually was\n produced is much easier way to debug than having to re-run the\n command with \"sh -x\" to peek into what the \"test\" statement is\n getting.\n\n - $(...) is much easier to read than `...`, but you do not have\n to use either if you follow the above.\n\ni.e.\n\n\tgit log -1 --format=%s >actual &&\n echo \"Merge branch '\\''other'\\''\" >expect &&\n test_cmp expect actual &&\n\n> +\ttest ! -s \"$(git rev-parse --git-dir)/MERGE_HEAD\"\n> +\n> +'\n> +\n> test_done\n"},{"id":"233018","messageId":"xmqqha9b74iw.fsf@gitster.dls.corp.google.com","threadId":"35627","inReplyTo":"1389228344-38813-4-git-send-email-ryan@yaauie.com","subject":"Re: [PATCH v2 3/4] merge: make prepare_to_commit responsible for write_merge_state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-11T00:20:39Z","receivedAt":"2014-01-11T00:20:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ryan Biesemeyer writes:\n\n> +\twrite_script \"$HOOK\" <<-EOF &&\n> +\tif [ -s \"$(git rev-parse --git-dir)/MERGE_HEAD\" ]; then\n> +\t\texit 0\n> +\telse\n> +\t\texit 1\n> +\tfi\n> +\tEOF\n\nThe script can be a one-liner\n\n\twrite_scirpt \"$HOOK\" <<-\\EOF &&\n test -s \"$(git rev-parse --git-dir)/MERGE_HEAD\"\n\tEOF\n\ncan't it? I also do not think you want to have the rev-parse run\nwhile writing the script (rather, you would want it run inside the\nscript, no?)\n\n> +\tgit merge other &&\n> +\ttest \"`git log -1 --pretty=format:%s`\" = \"Merge branch '\"'\"'other'\"'\"'\" &&\n> +\ttest ! -s \"$(git rev-parse --git-dir)/MERGE_HEAD\"\n> +\n> +'\n> +\n> test_done\n"}]}