{"thread":{"id":"45464","subject":"[PATCH 2/3] sequencer: make commit options more extensible","startedAt":"2017-03-22T15:02:08Z","lastAt":"2017-03-29T12:50:33Z","messageCount":27,"participants":["Johannes Schindelin","Sebastian Schuberth","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"314904","messageId":"c0c836e1f1739af25ad875419ee6125b53ac6a1a.1490194846.git.johannes.schindelin@gmx.de","threadId":"45464","inReplyTo":"cover.1490194846.git.johannes.schindelin@gmx.de","subject":"[PATCH 2/3] sequencer: make commit options more extensible","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-22T15:01:53Z","receivedAt":"2017-03-22T15:02:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It was pointed out during review of the sequencer-i patch series (which\ntaught the sequencer to execute an interactive rebase) that it may be\ncumbersome to keep extending the signature of the run_git_commit()\nfunction whenever a new commit option is needed.\n\nWhile that concern had merit, back then I was reluctant to change even\nmore than was already asked for (which typically introduces regressions,\nthis late in the review process, which is no fun for nobody).\n\nNow, with fresh eyes, and with an actual need, is a good time to change\nthe strategy from adding individual flag parameters to coalescing them\ninto a single flags parameter.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n sequencer.c | 48 ++++++++++++++++++++++++++----------------------\n 1 file changed, 26 insertions(+), 22 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 8183a83c1fa..1abe559fe86 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -602,6 +602,11 @@ N_(\"you have staged changes in your working tree\\n\"\n \"\\n\"\n \"  git rebase --continue\\n\");\n \n+#define ALLOW_EMPTY (1<<0)\n+#define EDIT_MSG    (1<<1)\n+#define AMEND_MSG   (1<<2)\n+#define CLEANUP_MSG (1<<3)\n+\n /*\n  * If we are cherry-pick, and if the merge did not result in\n  * hand-editing, we will hit this commit and inherit the original\n@@ -615,8 +620,7 @@ N_(\"you have staged changes in your working tree\\n\"\n  * author metadata.\n  */\n static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n-\t\t\t  int allow_empty, int edit, int amend,\n-\t\t\t  int cleanup_commit_message)\n+\t\t\t  int flags)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tconst char *value;\n@@ -624,7 +628,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \tcmd.git_cmd = 1;\n \n \tif (is_rebase_i(opts)) {\n-\t\tif (!edit) {\n+\t\tif (!(flags & EDIT_MSG)) {\n \t\t\tcmd.stdout_to_stderr = 1;\n \t\t\tcmd.err = -1;\n \t\t}\n@@ -640,7 +644,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \targv_array_push(&cmd.args, \"commit\");\n \targv_array_push(&cmd.args, \"-n\");\n \n-\tif (amend)\n+\tif ((flags & AMEND_MSG))\n \t\targv_array_push(&cmd.args, \"--amend\");\n \tif (opts->gpg_sign)\n \t\targv_array_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n@@ -648,16 +652,16 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \t\targv_array_push(&cmd.args, \"-s\");\n \tif (defmsg)\n \t\targv_array_pushl(&cmd.args, \"-F\", defmsg, NULL);\n-\tif (cleanup_commit_message)\n+\tif ((flags & CLEANUP_MSG))\n \t\targv_array_push(&cmd.args, \"--cleanup=strip\");\n-\tif (edit)\n+\tif ((flags & EDIT_MSG))\n \t\targv_array_push(&cmd.args, \"-e\");\n-\telse if (!cleanup_commit_message &&\n+\telse if (!(flags & CLEANUP_MSG) &&\n \t\t !opts->signoff && !opts->record_origin &&\n \t\t git_config_get_value(\"commit.cleanup\", &value))\n \t\targv_array_push(&cmd.args, \"--cleanup=verbatim\");\n \n-\tif (allow_empty)\n+\tif ((flags & ALLOW_EMPTY))\n \t\targv_array_push(&cmd.args, \"--allow-empty\");\n \n \tif (opts->allow_empty_message)\n@@ -926,14 +930,14 @@ static void record_in_rewritten(struct object_id *oid,\n static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\tstruct replay_opts *opts, int final_fixup)\n {\n-\tint edit = opts->edit, cleanup_commit_message = 0;\n-\tconst char *msg_file = edit ? NULL : git_path_merge_msg();\n+\tint flags = opts->edit ? EDIT_MSG : 0, allow = 0;\n+\tconst char *msg_file = opts->edit ? NULL : git_path_merge_msg();\n \tunsigned char head[20];\n \tstruct commit *base, *next, *parent;\n \tconst char *base_label, *next_label;\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL };\n \tstruct strbuf msgbuf = STRBUF_INIT;\n-\tint res, unborn = 0, amend = 0, allow = 0;\n+\tint res, unborn = 0;\n \n \tif (opts->no_commit) {\n \t\t/*\n@@ -991,7 +995,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\t\topts);\n \t\tif (res || command != TODO_REWORD)\n \t\t\tgoto leave;\n-\t\tedit = amend = 1;\n+\t\tflags |= EDIT_MSG | AMEND_MSG;\n \t\tmsg_file = NULL;\n \t\tgoto fast_forward_edit;\n \t}\n@@ -1046,15 +1050,15 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t}\n \n \tif (command == TODO_REWORD)\n-\t\tedit = 1;\n+\t\tflags |= EDIT_MSG;\n \telse if (is_fixup(command)) {\n \t\tif (update_squash_messages(command, commit, opts))\n \t\t\treturn -1;\n-\t\tamend = 1;\n+\t\tflags |= AMEND_MSG;\n \t\tif (!final_fixup)\n \t\t\tmsg_file = rebase_path_squash_msg();\n \t\telse if (file_exists(rebase_path_fixup_msg())) {\n-\t\t\tcleanup_commit_message = 1;\n+\t\t\tflags |= CLEANUP_MSG;\n \t\t\tmsg_file = rebase_path_fixup_msg();\n \t\t} else {\n \t\t\tconst char *dest = git_path(\"SQUASH_MSG\");\n@@ -1064,7 +1068,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\t\t\t\t     rebase_path_squash_msg(), dest);\n \t\t\tunlink(git_path(\"MERGE_MSG\"));\n \t\t\tmsg_file = dest;\n-\t\t\tedit = 1;\n+\t\t\tflags |= EDIT_MSG;\n \t\t}\n \t}\n \n@@ -1123,11 +1127,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \tif (allow < 0) {\n \t\tres = allow;\n \t\tgoto leave;\n-\t}\n+\t} else if (allow)\n+\t\tflags |= ALLOW_EMPTY;\n \tif (!opts->no_commit)\n fast_forward_edit:\n-\t\tres = run_git_commit(msg_file, opts, allow, edit, amend,\n-\t\t\t\t     cleanup_commit_message);\n+\t\tres = run_git_commit(msg_file, opts, flags);\n \n \tif (!res && final_fixup) {\n \t\tunlink(rebase_path_fixup_msg());\n@@ -2154,7 +2158,7 @@ static int continue_single_pick(void)\n \n static int commit_staged_changes(struct replay_opts *opts)\n {\n-\tint amend = 0;\n+\tint flags = ALLOW_EMPTY | EDIT_MSG;\n \n \tif (has_unstaged_changes(1))\n \t\treturn error(_(\"cannot rebase: You have unstaged changes.\"));\n@@ -2184,10 +2188,10 @@ static int commit_staged_changes(struct replay_opts *opts)\n \t\t\t\t       \"--continue' again.\"));\n \n \t\tstrbuf_release(&rev);\n-\t\tamend = 1;\n+\t\tflags |= AMEND_MSG;\n \t}\n \n-\tif (run_git_commit(rebase_path_message(), opts, 1, 1, amend, 0))\n+\tif (run_git_commit(rebase_path_message(), opts, flags))\n \t\treturn error(_(\"could not commit staged changes.\"));\n \tunlink(rebase_path_amend());\n \treturn 0;\n-- \n2.12.1.windows.1\n\n\n"},{"id":"314905","messageId":"773531a3892fb78e8f70e540fc000bceb2c1bb7b.1490194846.git.johannes.schindelin@gmx.de","threadId":"45464","inReplyTo":"cover.1490194846.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/3] t7504: document regression: reword no longer calls commit-msg","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-22T15:01:19Z","receivedAt":"2017-03-22T15:02:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The `reword` command of an interactive rebase used to call the\ncommit-msg hooks, but that regressed when we switched to the\nrebase--helper backed by the sequencer.\n\nNoticed by Sebastian Schuberth.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n t/t7504-commit-msg-hook.sh | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\nindex 8728db61d38..c3d9ab02a3b 100755\n--- a/t/t7504-commit-msg-hook.sh\n+++ b/t/t7504-commit-msg-hook.sh\n@@ -220,4 +220,21 @@ test_expect_success \"hook doesn't edit commit message (editor)\" '\n \n '\n \n+# set up fake editor to replace `pick` by `reword`\n+cat > reword-editor <<'EOF'\n+#!/bin/sh\n+mv \"$1\" \"$1\".bup &&\n+sed 's/^pick/reword/' <\"$1\".bup >\"$1\"\n+EOF\n+chmod +x reword-editor\n+REWORD_EDITOR=\"$(pwd)/reword-editor\"\n+export REWORD_EDITOR\n+\n+test_expect_failure 'hook is called for reword during `rebase -i`' '\n+\n+\tGIT_SEQUENCE_EDITOR=\"\\\"$REWORD_EDITOR\\\"\" git rebase -i HEAD^ &&\n+\tcommit_msg_is \"new message\"\n+\n+'\n+\n test_done\n-- \n2.12.1.windows.1\n\n\n"},{"id":"314906","messageId":"cover.1490194846.git.johannes.schindelin@gmx.de","threadId":"45464","inReplyTo":null,"subject":"[PATCH 0/3] rebase -i (reword): call the commit-msg hook again","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-22T15:01:14Z","receivedAt":"2017-03-22T15:02:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It is actually not only the commit-msg, but also the prepare-commit-msg\nhook...\n\n\nJohannes Schindelin (3):\n  t7504: document regression: reword no longer calls commit-msg\n  sequencer: make commit options more extensible\n  sequencer: allow the commit-msg hooks to run during a `reword`\n\n sequencer.c                | 58 +++++++++++++++++++++++++++-------------------\n t/t7504-commit-msg-hook.sh | 17 ++++++++++++++\n 2 files changed, 51 insertions(+), 24 deletions(-)\n\n\nbase-commit: afd6726309f57f532b4b989a75c1392359c611cc\nPublished-As: https://github.com/dscho/git/releases/tag/reword-commit-msg-hook-v1\nFetch-It-Via: git fetch https://github.com/dscho/git reword-commit-msg-hook-v1\n\n-- \n2.12.1.windows.1\n\n"},{"id":"314907","messageId":"9b83b6f7012368e970ec2b310f63b4daf66602e5.1490194846.git.johannes.schindelin@gmx.de","threadId":"45464","inReplyTo":"cover.1490194846.git.johannes.schindelin@gmx.de","subject":"[PATCH 3/3] sequencer: allow the commit-msg hooks to run during a `reword`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-22T15:02:01Z","receivedAt":"2017-03-22T15:02:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The `reword` command used to call `git commit` in a manner that asks for\nthe prepare-commit-msg and commit-msg hooks to do their thing.\n\nConverting that part of the interactive rebase to C code introduced the\nregression where those hooks were no longer run.\n\nLet's fix this.\n\nNote: the flag is called `VERIFY_MSG` instead of the more intuitive\n`RUN_COMMIT_MSG_HOOKS` to indicate that the flag suppresses the\n`--no-verify` flag (which may do other things in the future in addition\nto suppressing the commit message hooks, too).\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n sequencer.c                | 12 +++++++++---\n t/t7504-commit-msg-hook.sh |  2 +-\n 2 files changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 1abe559fe86..377af91c475 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -606,6 +606,7 @@ N_(\"you have staged changes in your working tree\\n\"\n #define EDIT_MSG    (1<<1)\n #define AMEND_MSG   (1<<2)\n #define CLEANUP_MSG (1<<3)\n+#define VERIFY_MSG  (1<<4)\n \n /*\n  * If we are cherry-pick, and if the merge did not result in\n@@ -642,8 +643,9 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \t}\n \n \targv_array_push(&cmd.args, \"commit\");\n-\targv_array_push(&cmd.args, \"-n\");\n \n+\tif (!(flags & VERIFY_MSG))\n+\t\targv_array_push(&cmd.args, \"-n\");\n \tif ((flags & AMEND_MSG))\n \t\targv_array_push(&cmd.args, \"--amend\");\n \tif (opts->gpg_sign)\n@@ -993,7 +995,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\t\twrite_author_script(msg.message);\n \t\tres = fast_forward_to(commit->object.oid.hash, head, unborn,\n \t\t\topts);\n-\t\tif (res || command != TODO_REWORD)\n+\t\tif (res)\n+\t\t\tgoto leave;\n+\t\telse if (command == TODO_REWORD)\n+\t\t\tflags |= VERIFY_MSG;\n+\t\telse\n \t\t\tgoto leave;\n \t\tflags |= EDIT_MSG | AMEND_MSG;\n \t\tmsg_file = NULL;\n@@ -1050,7 +1056,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t}\n \n \tif (command == TODO_REWORD)\n-\t\tflags |= EDIT_MSG;\n+\t\tflags |= EDIT_MSG | VERIFY_MSG;\n \telse if (is_fixup(command)) {\n \t\tif (update_squash_messages(command, commit, opts))\n \t\t\treturn -1;\ndiff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\nindex c3d9ab02a3b..88d4cda2992 100755\n--- a/t/t7504-commit-msg-hook.sh\n+++ b/t/t7504-commit-msg-hook.sh\n@@ -230,7 +230,7 @@ chmod +x reword-editor\n REWORD_EDITOR=\"$(pwd)/reword-editor\"\n export REWORD_EDITOR\n \n-test_expect_failure 'hook is called for reword during `rebase -i`' '\n+test_expect_success 'hook is called for reword during `rebase -i`' '\n \n \tGIT_SEQUENCE_EDITOR=\"\\\"$REWORD_EDITOR\\\"\" git rebase -i HEAD^ &&\n \tcommit_msg_is \"new message\"\n-- \n2.12.1.windows.1\n"},{"id":"314909","messageId":"CAHGBnuPL9CUincZkFR758KcZL3-Ra=n_fbsc1EQ=vio2scod5w@mail.gmail.com","threadId":"45464","inReplyTo":"773531a3892fb78e8f70e540fc000bceb2c1bb7b.1490194846.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 1/3] t7504: document regression: reword no longer calls commit-msg","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2017-03-22T15:18:54Z","receivedAt":"2017-03-22T15:19:17Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Wed, Mar 22, 2017 at 4:01 PM, Johannes Schindelin\n<johannes.schindelin@gmx.de> wrote:\n\n> Noticed by Sebastian Schuberth.\n\nThanks for working on the fix.\n\n> +# set up fake editor to replace `pick` by `reword`\n> +cat > reword-editor <<'EOF'\n> +#!/bin/sh\n> +mv \"$1\" \"$1\".bup &&\n> +sed 's/^pick/reword/' <\"$1\".bup >\"$1\"\n> +EOF\n\nMaybe use\n\nsed -i 's/^pick/reword/' \"$1\"\n\nhere to avoid renaming the input file? Not sure how portable -i for\nsed is, though. Otherwise, maybe remove the file \"$1\".bup afterwards\nto be clean?\n\n-- \nSebastian Schuberth\n"},{"id":"314911","messageId":"alpine.DEB.2.20.1703221704500.3767@virtualbox","threadId":"45464","inReplyTo":"CAHGBnuPL9CUincZkFR758KcZL3-Ra=n_fbsc1EQ=vio2scod5w@mail.gmail.com","subject":"Re: [PATCH 1/3] t7504: document regression: reword no longer calls commit-msg","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-22T16:09:38Z","receivedAt":"2017-03-22T16:10:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Sebastian,\n\nOn Wed, 22 Mar 2017, Sebastian Schuberth wrote:\n\n> On Wed, Mar 22, 2017 at 4:01 PM, Johannes Schindelin\n> <johannes.schindelin@gmx.de> wrote:\n> \n> > +# set up fake editor to replace `pick` by `reword`\n> > +cat > reword-editor <<'EOF'\n> > +#!/bin/sh\n> > +mv \"$1\" \"$1\".bup &&\n> > +sed 's/^pick/reword/' <\"$1\".bup >\"$1\"\n> > +EOF\n> \n> Maybe use\n> \n> sed -i 's/^pick/reword/' \"$1\"\n\nIt's not portable, otherwise I would have used it. GNU sed expects an\noptional extension to be used for a backup file, and that optional\nextension needs to be specified without whitespace between it and the -i\noption. BSD sed *requires* an extension to be specified, and it has to be\nseparated using white space (or is that orange space now?).\n\nSo even if we could make that call portable somehow (and remember, BSD sed\nis what OSX uses), there *would be a backup file*.\n\n> here to avoid renaming the input file? Not sure how portable -i for\n> sed is, though. Otherwise, maybe remove the file \"$1\".bup afterwards\n> to be clean?\n\nNo. The file in question is written to the .git/rebase-merge/\nsubdirectory, therefore cleaned up after the rebase.\n\nIn any case, this is *test* code. So I'd prefer to have the changes to the\nC code scrutinized a bit more, not the test code as long as it is obvious\nwhat it does.\n\nCiao,\nJohannes\n"},{"id":"314935","messageId":"xmqqinn1kwo6.fsf@gitster.mtv.corp.google.com","threadId":"45464","inReplyTo":"CAHGBnuPL9CUincZkFR758KcZL3-Ra=n_fbsc1EQ=vio2scod5w@mail.gmail.com","subject":"Re: [PATCH 1/3] t7504: document regression: reword no longer calls commit-msg","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-22T18:12:41Z","receivedAt":"2017-03-22T18:12:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth <sschuberth@gmail.com> writes:\n\n> On Wed, Mar 22, 2017 at 4:01 PM, Johannes Schindelin\n> <johannes.schindelin@gmx.de> wrote:\n>\n>> Noticed by Sebastian Schuberth.\n>\n> Thanks for working on the fix.\n>\n>> +# set up fake editor to replace `pick` by `reword`\n>> +cat > reword-editor <<'EOF'\n>> +#!/bin/sh\n>> +mv \"$1\" \"$1\".bup &&\n>> +sed 's/^pick/reword/' <\"$1\".bup >\"$1\"\n>> +EOF\n>\n> Maybe use\n>\n> sed -i 's/^pick/reword/' \"$1\"\n>\n> here to avoid renaming the input file? Not sure how portable -i for\n> sed is, though. Otherwise, maybe remove the file \"$1\".bup afterwards\n> to be clean?\n\n\"-i\" is GNUism and it is a good idea to avoid it, but cleaning after\nitself may be worth doing to avoid contaminating the working tree.\n\nThanks.\n"},{"id":"314936","messageId":"xmqqefxpkwjf.fsf@gitster.mtv.corp.google.com","threadId":"45464","inReplyTo":"alpine.DEB.2.20.1703221704500.3767@virtualbox","subject":"Re: [PATCH 1/3] t7504: document regression: reword no longer calls commit-msg","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-22T18:15:32Z","receivedAt":"2017-03-22T18:15:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> In any case, this is *test* code. So I'd prefer to have the changes to the\n> C code scrutinized a bit more, not the test code as long as it is obvious\n> what it does.\n\nI do not think the tone of this comment serves any productive\npurpose.  People have strengths in different areas, and those who\ncan spot issues in tests better than the C code should not be\ndiscouraged from suggesting improvements by getting scolded like\nthis.\n\nAlso remember what is \"obvious\" to you may not be obvious to others.\n\nThanks.\n"},{"id":"314937","messageId":"xmqqa88dkw9j.fsf@gitster.mtv.corp.google.com","threadId":"45464","inReplyTo":"c0c836e1f1739af25ad875419ee6125b53ac6a1a.1490194846.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 2/3] sequencer: make commit options more extensible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-22T18:21:28Z","receivedAt":"2017-03-22T18:21:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n> +#define ALLOW_EMPTY (1<<0)\n> +#define EDIT_MSG    (1<<1)\n> +#define AMEND_MSG   (1<<2)\n> +#define CLEANUP_MSG (1<<3)\n\nThese being bits makes it clear that they can be independently set\nand unset.\n\n> @@ -615,8 +620,7 @@ N_(\"you have staged changes in your working tree\\n\"\n>   * author metadata.\n>   */\n>  static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n> -\t\t\t  int allow_empty, int edit, int amend,\n> -\t\t\t  int cleanup_commit_message)\n> +\t\t\t  int flags)\n\nUse \"unsigned\" not signed integer for a collection of bits (see\ne.g. GSoC microproject ideas page).\n\n> @@ -1123,11 +1127,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n>  \tif (allow < 0) {\n>  \t\tres = allow;\n>  \t\tgoto leave;\n> -\t}\n> +\t} else if (allow)\n> +\t\tflags |= ALLOW_EMPTY;\n\n;-)  Much more descriptive than just \"allow\".\n\n>  static int commit_staged_changes(struct replay_opts *opts)\n>  {\n> -\tint amend = 0;\n> +\tint flags = ALLOW_EMPTY | EDIT_MSG;\n>  \n>  \tif (has_unstaged_changes(1))\n>  \t\treturn error(_(\"cannot rebase: You have unstaged changes.\"));\n> @@ -2184,10 +2188,10 @@ static int commit_staged_changes(struct replay_opts *opts)\n>  \t\t\t\t       \"--continue' again.\"));\n>  \n>  \t\tstrbuf_release(&rev);\n> -\t\tamend = 1;\n> +\t\tflags |= AMEND_MSG;\n>  \t}\n>  \n> -\tif (run_git_commit(rebase_path_message(), opts, 1, 1, amend, 0))\n> +\tif (run_git_commit(rebase_path_message(), opts, flags))\n\nOK, the initialization of \"flags\" corresponds to these \"1, 1\" in the\noriginal.\n\nOverall, much easier to understand (and to extend).  Good\nmaintaintability clean-up.\n\nThanks.\n"},{"id":"314941","messageId":"xmqq60j1kv8n.fsf@gitster.mtv.corp.google.com","threadId":"45464","inReplyTo":"9b83b6f7012368e970ec2b310f63b4daf66602e5.1490194846.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 3/3] sequencer: allow the commit-msg hooks to run during a `reword`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-22T18:43:36Z","receivedAt":"2017-03-22T18:43:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n> The `reword` command used to call `git commit` in a manner that asks for\n> the prepare-commit-msg and commit-msg hooks to do their thing.\n>\n> Converting that part of the interactive rebase to C code introduced the\n> regression where those hooks were no longer run.\n>\n> Let's fix this.\n>\n> Note: the flag is called `VERIFY_MSG` instead of the more intuitive\n> `RUN_COMMIT_MSG_HOOKS` to indicate that the flag suppresses the\n> `--no-verify` flag (which may do other things in the future in addition\n> to suppressing the commit message hooks, too).\n\nYup, this is a sound reasoning.  I would have made it not a \"Note\"\nbut just a regular description of the design decision, e.g.\n\n    Call the flag bit \"VERIFY_MSG\", because this is to suppress the\n    \"--no-verify\" flag (do not call it RUN_COMMIT_MSG_HOOKS, as the\n    purpose of the bit does not have to stay only to run the hooks).\n\nor somesuch, but I can go either way.\n\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>  sequencer.c                | 12 +++++++++---\n>  t/t7504-commit-msg-hook.sh |  2 +-\n>  2 files changed, 10 insertions(+), 4 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 1abe559fe86..377af91c475 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -606,6 +606,7 @@ N_(\"you have staged changes in your working tree\\n\"\n>  #define EDIT_MSG    (1<<1)\n>  #define AMEND_MSG   (1<<2)\n>  #define CLEANUP_MSG (1<<3)\n> +#define VERIFY_MSG  (1<<4)\n>  \n>  /*\n>   * If we are cherry-pick, and if the merge did not result in\n> @@ -642,8 +643,9 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n>  \t}\n>  \n>  \targv_array_push(&cmd.args, \"commit\");\n> -\targv_array_push(&cmd.args, \"-n\");\n>  \n> +\tif (!(flags & VERIFY_MSG))\n> +\t\targv_array_push(&cmd.args, \"-n\");\n\nOK, so by default we pass \"--no-verify\" but selected callers can\nset the bit in the flags word to disable it.  That sounds sensible,\nespecially given that the original callers, cherry-pick and revert, \ndid want \"--no-verify\".  \"reword\" in \"rebase -i\" is currently the\nonly one we want to supress \"--no-verify\" and the place that does so\nare all shown in this patch.\n\nJust to see if there are other callers that may want to do the same\nsuppressing of \"--no-verify\" as a follow-up, I looked at the whole\nfile after applying the patch, and I think everything looked good\nas-is.\n\n>  \tif ((flags & AMEND_MSG))\n>  \t\targv_array_push(&cmd.args, \"--amend\");\n>  \tif (opts->gpg_sign)\n> @@ -993,7 +995,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n>  \t\t\twrite_author_script(msg.message);\n>  \t\tres = fast_forward_to(commit->object.oid.hash, head, unborn,\n>  \t\t\topts);\n> -\t\tif (res || command != TODO_REWORD)\n> +\t\tif (res)\n> +\t\t\tgoto leave;\n> +\t\telse if (command == TODO_REWORD)\n> +\t\t\tflags |= VERIFY_MSG;\n> +\t\telse\n>  \t\t\tgoto leave;\n\nBoth before and after are your code, but wouldn't this:\n\n\tif (res || command != TODO_REWORD)\n\t\tgoto leave;\n+\tif (command == TODO_REWORD)\n+\t\tflags |= VERIFY_MSG\n\nresult in smaller changes relative to the original and still be much\neasier to understand than the above?\n\nThanks.\n"},{"id":"315074","messageId":"alpine.DEB.2.20.1703231539010.3767@virtualbox","threadId":"45464","inReplyTo":"xmqqa88dkw9j.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] sequencer: make commit options more extensible","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-23T14:39:26Z","receivedAt":"2017-03-23T14:39:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 22 Mar 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n> \n> > @@ -615,8 +620,7 @@ N_(\"you have staged changes in your working tree\\n\"\n> >   * author metadata.\n> >   */\n> >  static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n> > -\t\t\t  int allow_empty, int edit, int amend,\n> > -\t\t\t  int cleanup_commit_message)\n> > +\t\t\t  int flags)\n> \n> Use \"unsigned\" not signed integer for a collection of bits (see\n> e.g. GSoC microproject ideas page).\n\nTrue. Fixed in preparation for v2.\n\nCiao,\nDscho\n"},{"id":"315075","messageId":"alpine.DEB.2.20.1703231539410.3767@virtualbox","threadId":"45464","inReplyTo":"xmqqinn1kwo6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] t7504: document regression: reword no longer calls commit-msg","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-23T14:41:11Z","receivedAt":"2017-03-23T14:41:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 22 Mar 2017, Junio C Hamano wrote:\n\n> Sebastian Schuberth <sschuberth@gmail.com> writes:\n> \n> > On Wed, Mar 22, 2017 at 4:01 PM, Johannes Schindelin\n> > <johannes.schindelin@gmx.de> wrote:\n> >\n> >> Noticed by Sebastian Schuberth.\n> >\n> > Thanks for working on the fix.\n> >\n> >> +# set up fake editor to replace `pick` by `reword`\n> >> +cat > reword-editor <<'EOF'\n> >> +#!/bin/sh\n> >> +mv \"$1\" \"$1\".bup &&\n> >> +sed 's/^pick/reword/' <\"$1\".bup >\"$1\"\n> >> +EOF\n> >\n> > Maybe use\n> >\n> > sed -i 's/^pick/reword/' \"$1\"\n> >\n> > here to avoid renaming the input file? Not sure how portable -i for\n> > sed is, though. Otherwise, maybe remove the file \"$1\".bup afterwards\n> > to be clean?\n> \n> \"-i\" is GNUism and it is a good idea to avoid it, but cleaning after\n> itself may be worth doing to avoid contaminating the working tree.\n\nThis script is only used as *sequence* editor, i.e. for\n.git/merge-rebase/git-rebase-todo. As such, I find it valuable to have the\noriginal version in the same directory, and the file is cleaned up upon\nsuccess by the `git rebase -i` command anyway.\n\nIn that light, I am sure you agree that the current code is fine.\n\nCiao,\nDscho\n"},{"id":"315076","messageId":"alpine.DEB.2.20.1703231541180.3767@virtualbox","threadId":"45464","inReplyTo":"xmqqefxpkwjf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] t7504: document regression: reword no longer calls commit-msg","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-23T14:43:35Z","receivedAt":"2017-03-23T14:45:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 22 Mar 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > In any case, this is *test* code. So I'd prefer to have the changes to\n> > the C code scrutinized a bit more, not the test code as long as it is\n> > obvious what it does.\n> \n> [...] People have strengths in different areas, and those who can spot\n> issues in tests better than the C code should not be discouraged from\n> suggesting improvements by getting scolded like this.\n\nI know Sebastian well, and I would hope that he lends his substantial\ncompetence to making sure that the changes that affect end users are\ncorrect and that I do not introduce another regression.\n\nBesides, he is German, so I tried to spell it out clearly what I wish him\nto do in return for my addressing his problem.\n\nThat was the entire reasoning behind my \"scolding\". I am sorry that you\nunderstood it to be meant negatively. It was not.\n\nCiao,\nJohannes\n"},{"id":"315077","messageId":"CAHGBnuOJKopxq4s0e-mw0=siOERDcLXCEF4xzERpq7usbrZa6w@mail.gmail.com","threadId":"45464","inReplyTo":"alpine.DEB.2.20.1703231541180.3767@virtualbox","subject":"Re: [PATCH 1/3] t7504: document regression: reword no longer calls commit-msg","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2017-03-23T14:55:51Z","receivedAt":"2017-03-23T14:55:58Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Thu, Mar 23, 2017 at 3:43 PM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n\n> I know Sebastian well, and I would hope that he lends his substantial\n> competence to making sure that the changes that affect end users are\n> correct and that I do not introduce another regression.\n\nIf you'd really know me, you would also know that I only say something\n*if* I have to say something. I.e. me not making any remarks regarding\nthe C code means I did not find any issues.\n\n> Besides, he is German, so I tried to spell it out clearly what I wish him\n> to do in return for my addressing his problem.\n\nI fail to see how this is \"my\" problem just because I happened to\nnotice it first. While I'm grateful that you've addressed it timely, I\nbelieve this is a naturalness since you've introduced the regression.\n\n-- \nSebastian Schuberth\n"},{"id":"315101","messageId":"alpine.DEB.2.20.1703231646430.3767@virtualbox","threadId":"45464","inReplyTo":"xmqq60j1kv8n.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] sequencer: allow the commit-msg hooks to run during a `reword`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-23T15:49:24Z","receivedAt":"2017-03-23T15:49:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 22 Mar 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n> \n> > diff --git a/sequencer.c b/sequencer.c\n> > index 1abe559fe86..377af91c475 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -606,6 +606,7 @@ N_(\"you have staged changes in your working tree\\n\"\n> >  #define EDIT_MSG    (1<<1)\n> >  #define AMEND_MSG   (1<<2)\n> >  #define CLEANUP_MSG (1<<3)\n> > +#define VERIFY_MSG  (1<<4)\n> >  \n> >  /*\n> >   * If we are cherry-pick, and if the merge did not result in\n> > @@ -642,8 +643,9 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n> >  \t}\n> >  \n> >  \targv_array_push(&cmd.args, \"commit\");\n> > -\targv_array_push(&cmd.args, \"-n\");\n> >  \n> > +\tif (!(flags & VERIFY_MSG))\n> > +\t\targv_array_push(&cmd.args, \"-n\");\n> \n> OK, so by default we pass \"--no-verify\" but selected callers can\n> set the bit in the flags word to disable it.  That sounds sensible,\n> especially given that the original callers, cherry-pick and revert, \n> did want \"--no-verify\".  \"reword\" in \"rebase -i\" is currently the\n> only one we want to supress \"--no-verify\" and the place that does so\n> are all shown in this patch.\n\nIndeed, my reasoning was to keep the default to be the previous behavior.\n\n> Just to see if there are other callers that may want to do the same\n> suppressing of \"--no-verify\" as a follow-up, I looked at the whole\n> file after applying the patch, and I think everything looked good\n> as-is.\n\nThank you for being thorough; That is exactly the type of review I hoped\nfor. I did the same research myself, of course, but it is most reassuring\nif an independent reviewer comes to the same conclusion.\n\n> > @@ -993,7 +995,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n> >  \t\t\twrite_author_script(msg.message);\n> >  \t\tres = fast_forward_to(commit->object.oid.hash, head, unborn,\n> >  \t\t\topts);\n> > -\t\tif (res || command != TODO_REWORD)\n> > +\t\tif (res)\n> > +\t\t\tgoto leave;\n> > +\t\telse if (command == TODO_REWORD)\n> > +\t\t\tflags |= VERIFY_MSG;\n> > +\t\telse\n> >  \t\t\tgoto leave;\n> \n> Both before and after are your code, but wouldn't this:\n> \n> \tif (res || command != TODO_REWORD)\n> \t\tgoto leave;\n> +\tif (command == TODO_REWORD)\n> +\t\tflags |= VERIFY_MSG\n> \n> result in smaller changes relative to the original and still be much\n> easier to understand than the above?\n\nYes. I just did not like the repetition.\n\nBut thinking about it again, the previous logic was only concerned about\nan early exit, and the clause I added is all about the flags. Therefore, I\nagree with you that it should be untangled again.\n\nv2 coming,\nDscho\n"},{"id":"315103","messageId":"alpine.DEB.2.20.1703231650090.3767@virtualbox","threadId":"45464","inReplyTo":"CAHGBnuOJKopxq4s0e-mw0=siOERDcLXCEF4xzERpq7usbrZa6w@mail.gmail.com","subject":"Re: [PATCH 1/3] t7504: document regression: reword no longer calls commit-msg","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-23T15:52:03Z","receivedAt":"2017-03-23T15:52:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Sebastian,\n\nOn Thu, 23 Mar 2017, Sebastian Schuberth wrote:\n\n> On Thu, Mar 23, 2017 at 3:43 PM, Johannes Schindelin\n> <Johannes.Schindelin@gmx.de> wrote:\n> \n> > I know Sebastian well, and I would hope that he lends his substantial\n> > competence to making sure that the changes that affect end users are\n> > correct and that I do not introduce another regression.\n> \n> If you'd really know me, you would also know that I only say something\n> *if* I have to say something.\n\nOkay, then, I sit corrected: I do not know you well.\n\n> I.e. me not making any remarks regarding the C code means I did not find\n> any issues.\n\nSadly, this is indistinguishable from you having not bothered to read the\npatches to the C code at all.\n\nSo an explicit \"I have read the patch and found no fault\" or even better,\na review with some indication that you were thorough would have been a\nnice gesture.\n\nCiao,\nJohannes\n"},{"id":"315105","messageId":"xmqqa88cf08d.fsf@gitster.mtv.corp.google.com","threadId":"45464","inReplyTo":"c0c836e1f1739af25ad875419ee6125b53ac6a1a.1490194846.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 2/3] sequencer: make commit options more extensible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-23T16:04:34Z","receivedAt":"2017-03-23T16:04:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n> It was pointed out during review of the sequencer-i patch series (which\n> taught the sequencer to execute an interactive rebase) that it may be\n> cumbersome to keep extending the signature of the run_git_commit()\n> function whenever a new commit option is needed.\n>\n> While that concern had merit, back then I was reluctant to change even\n> more than was already asked for (which typically introduces regressions,\n> this late in the review process, which is no fun for nobody).\n>\n> Now, with fresh eyes, and with an actual need, is a good time to change\n> the strategy from adding individual flag parameters to coalescing them\n> into a single flags parameter.\n>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n\nI think the first two paragraphs are not about explaining this\ncommit.  You do not have to justify the past---some suggestions\ngiven during the review may end up not getting followed for\ndifferent reasons, which may be good or bad, but what happened has\nhappened, and the backstory is not useful for understanding why this\nis a good change to a reader of \"git log\".\n\nWhat belongs to the explanation / justification for this change is\nthat the interface to run_git_commit() that forces to add one\nboolean parameter whenever a new \"switch\" is required is cumbersome\nand it is better to use a flags word that is a collection of bits.\nWhen you reroll this for s/signed int/unsigned int/, please just\ndescribe that in the log. Having the backstory under three-dash\nlines may help reviewers who remember the original topic that\nintroduced the maintenance burden this commit fixes.\n\nThanks.\n"},{"id":"315106","messageId":"773531a3892fb78e8f70e540fc000bceb2c1bb7b.1490285210.git.johannes.schindelin@gmx.de","threadId":"45464","inReplyTo":"cover.1490285210.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 1/3] t7504: document regression: reword no longer calls commit-msg","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-23T16:07:07Z","receivedAt":"2017-03-23T16:07:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The `reword` command of an interactive rebase used to call the\ncommit-msg hooks, but that regressed when we switched to the\nrebase--helper backed by the sequencer.\n\nNoticed by Sebastian Schuberth.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n t/t7504-commit-msg-hook.sh | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\nindex 8728db61d38..c3d9ab02a3b 100755\n--- a/t/t7504-commit-msg-hook.sh\n+++ b/t/t7504-commit-msg-hook.sh\n@@ -220,4 +220,21 @@ test_expect_success \"hook doesn't edit commit message (editor)\" '\n \n '\n \n+# set up fake editor to replace `pick` by `reword`\n+cat > reword-editor <<'EOF'\n+#!/bin/sh\n+mv \"$1\" \"$1\".bup &&\n+sed 's/^pick/reword/' <\"$1\".bup >\"$1\"\n+EOF\n+chmod +x reword-editor\n+REWORD_EDITOR=\"$(pwd)/reword-editor\"\n+export REWORD_EDITOR\n+\n+test_expect_failure 'hook is called for reword during `rebase -i`' '\n+\n+\tGIT_SEQUENCE_EDITOR=\"\\\"$REWORD_EDITOR\\\"\" git rebase -i HEAD^ &&\n+\tcommit_msg_is \"new message\"\n+\n+'\n+\n test_done\n-- \n2.12.1.windows.1\n\n\n"},{"id":"315107","messageId":"75e2c4fe839602af19811ed3e251eb583ff6c8ad.1490285210.git.johannes.schindelin@gmx.de","threadId":"45464","inReplyTo":"cover.1490285210.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 2/3] sequencer: make commit options more extensible","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-23T16:07:11Z","receivedAt":"2017-03-23T16:07:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It was pointed out during review of the sequencer-i patch series (which\ntaught the sequencer to execute an interactive rebase) that it may be\ncumbersome to keep extending the signature of the run_git_commit()\nfunction whenever a new commit option is needed.\n\nWhile that concern had merit, back then I was reluctant to change even\nmore than was already asked for (which typically introduces regressions,\nthis late in the review process, which is no fun for nobody).\n\nNow, with fresh eyes, and with an actual need, is a good time to change\nthe strategy from adding individual flag parameters to coalescing them\ninto a single flags parameter.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n sequencer.c | 48 ++++++++++++++++++++++++++----------------------\n 1 file changed, 26 insertions(+), 22 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 8183a83c1fa..ce05d61a2ae 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -602,6 +602,11 @@ N_(\"you have staged changes in your working tree\\n\"\n \"\\n\"\n \"  git rebase --continue\\n\");\n \n+#define ALLOW_EMPTY (1<<0)\n+#define EDIT_MSG    (1<<1)\n+#define AMEND_MSG   (1<<2)\n+#define CLEANUP_MSG (1<<3)\n+\n /*\n  * If we are cherry-pick, and if the merge did not result in\n  * hand-editing, we will hit this commit and inherit the original\n@@ -615,8 +620,7 @@ N_(\"you have staged changes in your working tree\\n\"\n  * author metadata.\n  */\n static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n-\t\t\t  int allow_empty, int edit, int amend,\n-\t\t\t  int cleanup_commit_message)\n+\t\t\t  unsigned int flags)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tconst char *value;\n@@ -624,7 +628,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \tcmd.git_cmd = 1;\n \n \tif (is_rebase_i(opts)) {\n-\t\tif (!edit) {\n+\t\tif (!(flags & EDIT_MSG)) {\n \t\t\tcmd.stdout_to_stderr = 1;\n \t\t\tcmd.err = -1;\n \t\t}\n@@ -640,7 +644,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \targv_array_push(&cmd.args, \"commit\");\n \targv_array_push(&cmd.args, \"-n\");\n \n-\tif (amend)\n+\tif ((flags & AMEND_MSG))\n \t\targv_array_push(&cmd.args, \"--amend\");\n \tif (opts->gpg_sign)\n \t\targv_array_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n@@ -648,16 +652,16 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \t\targv_array_push(&cmd.args, \"-s\");\n \tif (defmsg)\n \t\targv_array_pushl(&cmd.args, \"-F\", defmsg, NULL);\n-\tif (cleanup_commit_message)\n+\tif ((flags & CLEANUP_MSG))\n \t\targv_array_push(&cmd.args, \"--cleanup=strip\");\n-\tif (edit)\n+\tif ((flags & EDIT_MSG))\n \t\targv_array_push(&cmd.args, \"-e\");\n-\telse if (!cleanup_commit_message &&\n+\telse if (!(flags & CLEANUP_MSG) &&\n \t\t !opts->signoff && !opts->record_origin &&\n \t\t git_config_get_value(\"commit.cleanup\", &value))\n \t\targv_array_push(&cmd.args, \"--cleanup=verbatim\");\n \n-\tif (allow_empty)\n+\tif ((flags & ALLOW_EMPTY))\n \t\targv_array_push(&cmd.args, \"--allow-empty\");\n \n \tif (opts->allow_empty_message)\n@@ -926,14 +930,14 @@ static void record_in_rewritten(struct object_id *oid,\n static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\tstruct replay_opts *opts, int final_fixup)\n {\n-\tint edit = opts->edit, cleanup_commit_message = 0;\n-\tconst char *msg_file = edit ? NULL : git_path_merge_msg();\n+\tunsigned int flags = opts->edit ? EDIT_MSG : 0, allow = 0;\n+\tconst char *msg_file = opts->edit ? NULL : git_path_merge_msg();\n \tunsigned char head[20];\n \tstruct commit *base, *next, *parent;\n \tconst char *base_label, *next_label;\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL };\n \tstruct strbuf msgbuf = STRBUF_INIT;\n-\tint res, unborn = 0, amend = 0, allow = 0;\n+\tint res, unborn = 0;\n \n \tif (opts->no_commit) {\n \t\t/*\n@@ -991,7 +995,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\t\topts);\n \t\tif (res || command != TODO_REWORD)\n \t\t\tgoto leave;\n-\t\tedit = amend = 1;\n+\t\tflags |= EDIT_MSG | AMEND_MSG;\n \t\tmsg_file = NULL;\n \t\tgoto fast_forward_edit;\n \t}\n@@ -1046,15 +1050,15 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t}\n \n \tif (command == TODO_REWORD)\n-\t\tedit = 1;\n+\t\tflags |= EDIT_MSG;\n \telse if (is_fixup(command)) {\n \t\tif (update_squash_messages(command, commit, opts))\n \t\t\treturn -1;\n-\t\tamend = 1;\n+\t\tflags |= AMEND_MSG;\n \t\tif (!final_fixup)\n \t\t\tmsg_file = rebase_path_squash_msg();\n \t\telse if (file_exists(rebase_path_fixup_msg())) {\n-\t\t\tcleanup_commit_message = 1;\n+\t\t\tflags |= CLEANUP_MSG;\n \t\t\tmsg_file = rebase_path_fixup_msg();\n \t\t} else {\n \t\t\tconst char *dest = git_path(\"SQUASH_MSG\");\n@@ -1064,7 +1068,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\t\t\t\t     rebase_path_squash_msg(), dest);\n \t\t\tunlink(git_path(\"MERGE_MSG\"));\n \t\t\tmsg_file = dest;\n-\t\t\tedit = 1;\n+\t\t\tflags |= EDIT_MSG;\n \t\t}\n \t}\n \n@@ -1123,11 +1127,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \tif (allow < 0) {\n \t\tres = allow;\n \t\tgoto leave;\n-\t}\n+\t} else if (allow)\n+\t\tflags |= ALLOW_EMPTY;\n \tif (!opts->no_commit)\n fast_forward_edit:\n-\t\tres = run_git_commit(msg_file, opts, allow, edit, amend,\n-\t\t\t\t     cleanup_commit_message);\n+\t\tres = run_git_commit(msg_file, opts, flags);\n \n \tif (!res && final_fixup) {\n \t\tunlink(rebase_path_fixup_msg());\n@@ -2154,7 +2158,7 @@ static int continue_single_pick(void)\n \n static int commit_staged_changes(struct replay_opts *opts)\n {\n-\tint amend = 0;\n+\tunsigned int flags = ALLOW_EMPTY | EDIT_MSG;\n \n \tif (has_unstaged_changes(1))\n \t\treturn error(_(\"cannot rebase: You have unstaged changes.\"));\n@@ -2184,10 +2188,10 @@ static int commit_staged_changes(struct replay_opts *opts)\n \t\t\t\t       \"--continue' again.\"));\n \n \t\tstrbuf_release(&rev);\n-\t\tamend = 1;\n+\t\tflags |= AMEND_MSG;\n \t}\n \n-\tif (run_git_commit(rebase_path_message(), opts, 1, 1, amend, 0))\n+\tif (run_git_commit(rebase_path_message(), opts, flags))\n \t\treturn error(_(\"could not commit staged changes.\"));\n \tunlink(rebase_path_amend());\n \treturn 0;\n-- \n2.12.1.windows.1\n\n\n"},{"id":"315108","messageId":"cover.1490285210.git.johannes.schindelin@gmx.de","threadId":"45464","inReplyTo":"cover.1490194846.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 0/3] rebase -i (reword): call the commit-msg hook again","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-23T16:07:03Z","receivedAt":"2017-03-23T16:07:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It is actually not only the commit-msg, but also the prepare-commit-msg\nhook...\n\nChanges since v1:\n\n- changed `int flags` to `unsigned int flags`\n\n- uncuddled the if...else if... construct to not mix the \"goto leave\"\n  logic with the one adding the VERIFY_MSG flag\n\n\nJohannes Schindelin (3):\n  t7504: document regression: reword no longer calls commit-msg\n  sequencer: make commit options more extensible\n  sequencer: allow the commit-msg hooks to run during a `reword`\n\n sequencer.c                | 54 ++++++++++++++++++++++++++--------------------\n t/t7504-commit-msg-hook.sh | 17 +++++++++++++++\n 2 files changed, 48 insertions(+), 23 deletions(-)\n\n\nbase-commit: afd6726309f57f532b4b989a75c1392359c611cc\nPublished-As: https://github.com/dscho/git/releases/tag/reword-commit-msg-hook-v2\nFetch-It-Via: git fetch https://github.com/dscho/git reword-commit-msg-hook-v2\n\nInterdiff vs v1:\n\n diff --git a/sequencer.c b/sequencer.c\n index 377af91c475..bc2fe48e65c 100644\n --- a/sequencer.c\n +++ b/sequencer.c\n @@ -621,7 +621,7 @@ N_(\"you have staged changes in your working tree\\n\"\n   * author metadata.\n   */\n  static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n -\t\t\t  int flags)\n +\t\t\t  unsigned int flags)\n  {\n  \tstruct child_process cmd = CHILD_PROCESS_INIT;\n  \tconst char *value;\n @@ -932,7 +932,7 @@ static void record_in_rewritten(struct object_id *oid,\n  static int do_pick_commit(enum todo_command command, struct commit *commit,\n  \t\tstruct replay_opts *opts, int final_fixup)\n  {\n -\tint flags = opts->edit ? EDIT_MSG : 0, allow = 0;\n +\tunsigned int flags = opts->edit ? EDIT_MSG : 0, allow = 0;\n  \tconst char *msg_file = opts->edit ? NULL : git_path_merge_msg();\n  \tunsigned char head[20];\n  \tstruct commit *base, *next, *parent;\n @@ -995,13 +995,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n  \t\t\twrite_author_script(msg.message);\n  \t\tres = fast_forward_to(commit->object.oid.hash, head, unborn,\n  \t\t\topts);\n -\t\tif (res)\n -\t\t\tgoto leave;\n -\t\telse if (command == TODO_REWORD)\n -\t\t\tflags |= VERIFY_MSG;\n -\t\telse\n +\t\tif (res || command != TODO_REWORD)\n  \t\t\tgoto leave;\n  \t\tflags |= EDIT_MSG | AMEND_MSG;\n +\t\tif (command == TODO_REWORD)\n +\t\t\tflags |= VERIFY_MSG;\n  \t\tmsg_file = NULL;\n  \t\tgoto fast_forward_edit;\n  \t}\n @@ -2164,7 +2162,7 @@ static int continue_single_pick(void)\n  \n  static int commit_staged_changes(struct replay_opts *opts)\n  {\n -\tint flags = ALLOW_EMPTY | EDIT_MSG;\n +\tunsigned int flags = ALLOW_EMPTY | EDIT_MSG;\n  \n  \tif (has_unstaged_changes(1))\n  \t\treturn error(_(\"cannot rebase: You have unstaged changes.\"));\n\n-- \n2.12.1.windows.1\n\n"},{"id":"315109","messageId":"315c237eb50572f8e109ea547a2010d9f81c3aff.1490285210.git.johannes.schindelin@gmx.de","threadId":"45464","inReplyTo":"cover.1490285210.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 3/3] sequencer: allow the commit-msg hooks to run during a `reword`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-23T16:07:17Z","receivedAt":"2017-03-23T16:07:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The `reword` command used to call `git commit` in a manner that asks for\nthe prepare-commit-msg and commit-msg hooks to do their thing.\n\nConverting that part of the interactive rebase to C code introduced the\nregression where those hooks were no longer run.\n\nLet's fix this.\n\nNote: the flag is called `VERIFY_MSG` instead of the more intuitive\n`RUN_COMMIT_MSG_HOOKS` to indicate that the flag suppresses the\n`--no-verify` flag (which may do other things in the future in addition\nto suppressing the commit message hooks, too).\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n sequencer.c                | 8 ++++++--\n t/t7504-commit-msg-hook.sh | 2 +-\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex ce05d61a2ae..bc2fe48e65c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -606,6 +606,7 @@ N_(\"you have staged changes in your working tree\\n\"\n #define EDIT_MSG    (1<<1)\n #define AMEND_MSG   (1<<2)\n #define CLEANUP_MSG (1<<3)\n+#define VERIFY_MSG  (1<<4)\n \n /*\n  * If we are cherry-pick, and if the merge did not result in\n@@ -642,8 +643,9 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \t}\n \n \targv_array_push(&cmd.args, \"commit\");\n-\targv_array_push(&cmd.args, \"-n\");\n \n+\tif (!(flags & VERIFY_MSG))\n+\t\targv_array_push(&cmd.args, \"-n\");\n \tif ((flags & AMEND_MSG))\n \t\targv_array_push(&cmd.args, \"--amend\");\n \tif (opts->gpg_sign)\n@@ -996,6 +998,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\tif (res || command != TODO_REWORD)\n \t\t\tgoto leave;\n \t\tflags |= EDIT_MSG | AMEND_MSG;\n+\t\tif (command == TODO_REWORD)\n+\t\t\tflags |= VERIFY_MSG;\n \t\tmsg_file = NULL;\n \t\tgoto fast_forward_edit;\n \t}\n@@ -1050,7 +1054,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t}\n \n \tif (command == TODO_REWORD)\n-\t\tflags |= EDIT_MSG;\n+\t\tflags |= EDIT_MSG | VERIFY_MSG;\n \telse if (is_fixup(command)) {\n \t\tif (update_squash_messages(command, commit, opts))\n \t\t\treturn -1;\ndiff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\nindex c3d9ab02a3b..88d4cda2992 100755\n--- a/t/t7504-commit-msg-hook.sh\n+++ b/t/t7504-commit-msg-hook.sh\n@@ -230,7 +230,7 @@ chmod +x reword-editor\n REWORD_EDITOR=\"$(pwd)/reword-editor\"\n export REWORD_EDITOR\n \n-test_expect_failure 'hook is called for reword during `rebase -i`' '\n+test_expect_success 'hook is called for reword during `rebase -i`' '\n \n \tGIT_SEQUENCE_EDITOR=\"\\\"$REWORD_EDITOR\\\"\" git rebase -i HEAD^ &&\n \tcommit_msg_is \"new message\"\n-- \n2.12.1.windows.1\n"},{"id":"315112","messageId":"xmqq60j0ey1k.fsf@gitster.mtv.corp.google.com","threadId":"45464","inReplyTo":"315c237eb50572f8e109ea547a2010d9f81c3aff.1490285210.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 3/3] sequencer: allow the commit-msg hooks to run during a `reword`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-23T16:51:51Z","receivedAt":"2017-03-23T16:52:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n> @@ -996,6 +998,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n>  \t\tif (res || command != TODO_REWORD)\n>  \t\t\tgoto leave;\n>  \t\tflags |= EDIT_MSG | AMEND_MSG;\n> +\t\tif (command == TODO_REWORD)\n> +\t\t\tflags |= VERIFY_MSG;\n\nGood.  This looks even cleaner than what we discussed during the\nreview of the previous round.  After deciding not to return early,\nwe set the default set of features in flags and then further add\nverify-msg in when needed.  Much easier to understand.\n\n"},{"id":"315219","messageId":"xmqq7f3fa3oz.fsf@gitster.mtv.corp.google.com","threadId":"45464","inReplyTo":"75e2c4fe839602af19811ed3e251eb583ff6c8ad.1490285210.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 2/3] sequencer: make commit options more extensible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-24T01:01:00Z","receivedAt":"2017-03-24T01:01:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n> @@ -926,14 +930,14 @@ static void record_in_rewritten(struct object_id *oid,\n>  static int do_pick_commit(enum todo_command command, struct commit *commit,\n>  \t\tstruct replay_opts *opts, int final_fixup)\n>  {\n> -\tint edit = opts->edit, cleanup_commit_message = 0;\n> -\tconst char *msg_file = edit ? NULL : git_path_merge_msg();\n> +\tunsigned int flags = opts->edit ? EDIT_MSG : 0, allow = 0;\n> +\tconst char *msg_file = opts->edit ? NULL : git_path_merge_msg();\n>  \tunsigned char head[20];\n>  \tstruct commit *base, *next, *parent;\n>  \tconst char *base_label, *next_label;\n>  \tstruct commit_message msg = { NULL, NULL, NULL, NULL };\n>  \tstruct strbuf msgbuf = STRBUF_INIT;\n> -\tint res, unborn = 0, amend = 0, allow = 0;\n> +\tint res, unborn = 0;\n> ... \n> @@ -1123,11 +1127,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n>  \tif (allow < 0) {\n>  \t\tres = allow;\n>  \t\tgoto leave;\n> -\t}\n> +\t} else if (allow)\n> +\t\tflags |= ALLOW_EMPTY;\n\nMaking \"flags\" unsigned was a correct change, but this is now wrong,\nas \"allow\" is made unsigned by accident.  It still needs to receive\nthe return value from allow_empty() that can signal an error by\nreturning a negative value.  It seems you did this deliberately (it\nwasn't like \"flags\" and \"allow\" were on the same line both \"int\" and\nthe change to make \"flags\" unsigned accidentally made \"allow\" also\nunsigned; \"allow\" was \"int\" and didn't have to move when \"flags\" was\nintroduced in the first place), but I failed to spot it because I\nwas only looking at s/int/unsigned/ on the line in the interdiff,\nand missed that \"allow\" was on the same line.  Compilers are better at\nfinding these bugs.\n\nPerhaps something like this needs to be squashed in?\n\n sequencer.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex bc2fe48e65..6c423566e6 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -932,14 +932,14 @@ static void record_in_rewritten(struct object_id *oid,\n static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\tstruct replay_opts *opts, int final_fixup)\n {\n-\tunsigned int flags = opts->edit ? EDIT_MSG : 0, allow = 0;\n+\tunsigned int flags = opts->edit ? EDIT_MSG : 0;\n \tconst char *msg_file = opts->edit ? NULL : git_path_merge_msg();\n \tunsigned char head[20];\n \tstruct commit *base, *next, *parent;\n \tconst char *base_label, *next_label;\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL };\n \tstruct strbuf msgbuf = STRBUF_INIT;\n-\tint res, unborn = 0;\n+\tint allow, res, unborn = 0;\n \n \tif (opts->no_commit) {\n \t\t/*\n-- \n2.12.1-432-gf364f02724\n\n\n\n"},{"id":"315360","messageId":"alpine.DEB.2.20.1703250100340.3767@virtualbox","threadId":"45464","inReplyTo":"xmqq7f3fa3oz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/3] sequencer: make commit options more extensible","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-25T00:01:55Z","receivedAt":"2017-03-25T00:02:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 23 Mar 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n> \n> > @@ -926,14 +930,14 @@ static void record_in_rewritten(struct object_id *oid,\n> >  static int do_pick_commit(enum todo_command command, struct commit *commit,\n> >  \t\tstruct replay_opts *opts, int final_fixup)\n> >  {\n> > -\tint edit = opts->edit, cleanup_commit_message = 0;\n> > -\tconst char *msg_file = edit ? NULL : git_path_merge_msg();\n> > +\tunsigned int flags = opts->edit ? EDIT_MSG : 0, allow = 0;\n> > +\tconst char *msg_file = opts->edit ? NULL : git_path_merge_msg();\n> >  \tunsigned char head[20];\n> >  \tstruct commit *base, *next, *parent;\n> >  \tconst char *base_label, *next_label;\n> >  \tstruct commit_message msg = { NULL, NULL, NULL, NULL };\n> >  \tstruct strbuf msgbuf = STRBUF_INIT;\n> > -\tint res, unborn = 0, amend = 0, allow = 0;\n> > +\tint res, unborn = 0;\n> > ... \n> > @@ -1123,11 +1127,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n> >  \tif (allow < 0) {\n> >  \t\tres = allow;\n> >  \t\tgoto leave;\n> > -\t}\n> > +\t} else if (allow)\n> > +\t\tflags |= ALLOW_EMPTY;\n> \n> Making \"flags\" unsigned was a correct change, but this is now wrong,\n> as \"allow\" is made unsigned by accident.\n\nRight. Bummer.\n\n> Perhaps something like this needs to be squashed in?\n> \n>  sequencer.c | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index bc2fe48e65..6c423566e6 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -932,14 +932,14 @@ static void record_in_rewritten(struct object_id *oid,\n>  static int do_pick_commit(enum todo_command command, struct commit *commit,\n>  \t\tstruct replay_opts *opts, int final_fixup)\n>  {\n> -\tunsigned int flags = opts->edit ? EDIT_MSG : 0, allow = 0;\n> +\tunsigned int flags = opts->edit ? EDIT_MSG : 0;\n>  \tconst char *msg_file = opts->edit ? NULL : git_path_merge_msg();\n>  \tunsigned char head[20];\n>  \tstruct commit *base, *next, *parent;\n>  \tconst char *base_label, *next_label;\n>  \tstruct commit_message msg = { NULL, NULL, NULL, NULL };\n>  \tstruct strbuf msgbuf = STRBUF_INIT;\n> -\tint res, unborn = 0;\n> +\tint allow, res, unborn = 0;\n\nI had originally moved it from there, to stay with the related flags, but\nI should just have left it there.\n\nYour patch looks good, you could do even better by reverting that move\n(IIRC it was at the end of the line, and it was set to 0 by default).\n\nCiao,\nDscho\n"},{"id":"315494","messageId":"xmqqwpbb35e3.fsf@gitster.mtv.corp.google.com","threadId":"45464","inReplyTo":"alpine.DEB.2.20.1703250100340.3767@virtualbox","subject":"Re: [PATCH v2 2/3] sequencer: make commit options more extensible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-27T00:55:16Z","receivedAt":"2017-03-27T00:55:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> Making \"flags\" unsigned was a correct change, but this is now wrong,\n>> as \"allow\" is made unsigned by accident.\n> ...\n>\n> Your patch looks good, you could do even better by reverting that move\n> (IIRC it was at the end of the line, and it was set to 0 by default).\n\nI do not think the variable needs to be initialized to anything (it\nis not looked at until it gets the result from allow_empty() call).\n\nAnyway, the series is not yet in 'next', so you can replace it to\nthe shape you would have made it into if you noticed that \"allow\"\nshouldn't be unsigned, while updating the log message to explain\nwhat the change is about (instead of only attempting to justify the\npast, which is not interesting in the context of understanding what\nthis change is about).\n\nAs this thing is about fixing a regression visible to end users, I\nmay get around to fixing things up myself, but I have other topics\nto attend to, so...\n\n\n\n"},{"id":"315532","messageId":"xmqqwpbazb34.fsf@gitster.mtv.corp.google.com","threadId":"45464","inReplyTo":"xmqqwpbb35e3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/3] sequencer: make commit options more extensible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-27T21:03:27Z","receivedAt":"2017-03-27T21:04:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> As this thing is about fixing a regression visible to end users, I\n> may get around to fixing things up myself, but I have other topics\n> to attend to, so...\n\nSo I ended up with this version before merging it to 'next'.  \n\nI moved 'allow' back on the line it is declared, but left it\nuninitialized because it is unconditionally assigned to before its\nvalue is looked at.\n\nAlso the updated log message stresses more about the reason why\npiling new parameters on top is a bad practice, and it is a good\nidea to do this clean-up now.  Compared to that, the reason why this\nclean-up was not done before and left as maintenance burden is much\nless important to the readers of the log.\n\n-- >8 --\nFrom: Johannes Schindelin <johannes.schindelin@gmx.de>\nDate: Thu, 23 Mar 2017 17:07:11 +0100\nSubject: [PATCH] sequencer: make commit options more extensible\n\nSo far every time we need to tweak the behaviour of run_git_commit()\nwe have been adding a \"int\" parameter to it.  As the function gains\nparameters and different callsites having different needs, this is\nbecoming a maintenance burden.  When a new knob needs to be added to\naddress a specific need for a single callsite, all the other callsites\nneed to add a \"no, I do not want anything special with respect to the\nnew knob\" argument.\n\nConsolidate the existing four parameters into a flag word to make it\nmore maintainable, as we will be adding a new one to the mix soon.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n sequencer.c | 48 ++++++++++++++++++++++++++----------------------\n 1 file changed, 26 insertions(+), 22 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 8183a83c1f..677e6ef764 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -602,6 +602,11 @@ N_(\"you have staged changes in your working tree\\n\"\n \"\\n\"\n \"  git rebase --continue\\n\");\n \n+#define ALLOW_EMPTY (1<<0)\n+#define EDIT_MSG    (1<<1)\n+#define AMEND_MSG   (1<<2)\n+#define CLEANUP_MSG (1<<3)\n+\n /*\n  * If we are cherry-pick, and if the merge did not result in\n  * hand-editing, we will hit this commit and inherit the original\n@@ -615,8 +620,7 @@ N_(\"you have staged changes in your working tree\\n\"\n  * author metadata.\n  */\n static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n-\t\t\t  int allow_empty, int edit, int amend,\n-\t\t\t  int cleanup_commit_message)\n+\t\t\t  unsigned int flags)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tconst char *value;\n@@ -624,7 +628,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \tcmd.git_cmd = 1;\n \n \tif (is_rebase_i(opts)) {\n-\t\tif (!edit) {\n+\t\tif (!(flags & EDIT_MSG)) {\n \t\t\tcmd.stdout_to_stderr = 1;\n \t\t\tcmd.err = -1;\n \t\t}\n@@ -640,7 +644,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \targv_array_push(&cmd.args, \"commit\");\n \targv_array_push(&cmd.args, \"-n\");\n \n-\tif (amend)\n+\tif ((flags & AMEND_MSG))\n \t\targv_array_push(&cmd.args, \"--amend\");\n \tif (opts->gpg_sign)\n \t\targv_array_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n@@ -648,16 +652,16 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \t\targv_array_push(&cmd.args, \"-s\");\n \tif (defmsg)\n \t\targv_array_pushl(&cmd.args, \"-F\", defmsg, NULL);\n-\tif (cleanup_commit_message)\n+\tif ((flags & CLEANUP_MSG))\n \t\targv_array_push(&cmd.args, \"--cleanup=strip\");\n-\tif (edit)\n+\tif ((flags & EDIT_MSG))\n \t\targv_array_push(&cmd.args, \"-e\");\n-\telse if (!cleanup_commit_message &&\n+\telse if (!(flags & CLEANUP_MSG) &&\n \t\t !opts->signoff && !opts->record_origin &&\n \t\t git_config_get_value(\"commit.cleanup\", &value))\n \t\targv_array_push(&cmd.args, \"--cleanup=verbatim\");\n \n-\tif (allow_empty)\n+\tif ((flags & ALLOW_EMPTY))\n \t\targv_array_push(&cmd.args, \"--allow-empty\");\n \n \tif (opts->allow_empty_message)\n@@ -926,14 +930,14 @@ static void record_in_rewritten(struct object_id *oid,\n static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\tstruct replay_opts *opts, int final_fixup)\n {\n-\tint edit = opts->edit, cleanup_commit_message = 0;\n-\tconst char *msg_file = edit ? NULL : git_path_merge_msg();\n+\tunsigned int flags = opts->edit ? EDIT_MSG : 0;\n+\tconst char *msg_file = opts->edit ? NULL : git_path_merge_msg();\n \tunsigned char head[20];\n \tstruct commit *base, *next, *parent;\n \tconst char *base_label, *next_label;\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL };\n \tstruct strbuf msgbuf = STRBUF_INIT;\n-\tint res, unborn = 0, amend = 0, allow = 0;\n+\tint res, unborn = 0, allow;\n \n \tif (opts->no_commit) {\n \t\t/*\n@@ -991,7 +995,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\t\topts);\n \t\tif (res || command != TODO_REWORD)\n \t\t\tgoto leave;\n-\t\tedit = amend = 1;\n+\t\tflags |= EDIT_MSG | AMEND_MSG;\n \t\tmsg_file = NULL;\n \t\tgoto fast_forward_edit;\n \t}\n@@ -1046,15 +1050,15 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t}\n \n \tif (command == TODO_REWORD)\n-\t\tedit = 1;\n+\t\tflags |= EDIT_MSG;\n \telse if (is_fixup(command)) {\n \t\tif (update_squash_messages(command, commit, opts))\n \t\t\treturn -1;\n-\t\tamend = 1;\n+\t\tflags |= AMEND_MSG;\n \t\tif (!final_fixup)\n \t\t\tmsg_file = rebase_path_squash_msg();\n \t\telse if (file_exists(rebase_path_fixup_msg())) {\n-\t\t\tcleanup_commit_message = 1;\n+\t\t\tflags |= CLEANUP_MSG;\n \t\t\tmsg_file = rebase_path_fixup_msg();\n \t\t} else {\n \t\t\tconst char *dest = git_path(\"SQUASH_MSG\");\n@@ -1064,7 +1068,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\t\t\t\t     rebase_path_squash_msg(), dest);\n \t\t\tunlink(git_path(\"MERGE_MSG\"));\n \t\t\tmsg_file = dest;\n-\t\t\tedit = 1;\n+\t\t\tflags |= EDIT_MSG;\n \t\t}\n \t}\n \n@@ -1123,11 +1127,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \tif (allow < 0) {\n \t\tres = allow;\n \t\tgoto leave;\n-\t}\n+\t} else if (allow)\n+\t\tflags |= ALLOW_EMPTY;\n \tif (!opts->no_commit)\n fast_forward_edit:\n-\t\tres = run_git_commit(msg_file, opts, allow, edit, amend,\n-\t\t\t\t     cleanup_commit_message);\n+\t\tres = run_git_commit(msg_file, opts, flags);\n \n \tif (!res && final_fixup) {\n \t\tunlink(rebase_path_fixup_msg());\n@@ -2154,7 +2158,7 @@ static int continue_single_pick(void)\n \n static int commit_staged_changes(struct replay_opts *opts)\n {\n-\tint amend = 0;\n+\tunsigned int flags = ALLOW_EMPTY | EDIT_MSG;\n \n \tif (has_unstaged_changes(1))\n \t\treturn error(_(\"cannot rebase: You have unstaged changes.\"));\n@@ -2184,10 +2188,10 @@ static int commit_staged_changes(struct replay_opts *opts)\n \t\t\t\t       \"--continue' again.\"));\n \n \t\tstrbuf_release(&rev);\n-\t\tamend = 1;\n+\t\tflags |= AMEND_MSG;\n \t}\n \n-\tif (run_git_commit(rebase_path_message(), opts, 1, 1, amend, 0))\n+\tif (run_git_commit(rebase_path_message(), opts, flags))\n \t\treturn error(_(\"could not commit staged changes.\"));\n \tunlink(rebase_path_amend());\n \treturn 0;\n-- \n2.12.2-430-ge0ef7fe78c\n\n"},{"id":"315662","messageId":"alpine.DEB.2.20.1703291449480.48202@virtualbox","threadId":"45464","inReplyTo":"xmqqwpbazb34.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/3] sequencer: make commit options more extensible","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-29T12:49:59Z","receivedAt":"2017-03-29T12:50:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 27 Mar 2017, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > As this thing is about fixing a regression visible to end users, I\n> > may get around to fixing things up myself, but I have other topics\n> > to attend to, so...\n> \n> So I ended up with this version before merging it to 'next'.  \n\nThanks,\nDscho\n"}]}