{"thread":{"id":"56260","subject":"[PATCH] commit: restore --edit when combined with --fixup","startedAt":"2021-08-11T13:49:22Z","lastAt":"2021-08-17T10:07:40Z","messageCount":19,"participants":["Joel Klinghed via GitGitGadget","Jeff King","Joel Klinghed","brian m. carlson","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"432481","messageId":"pull.1014.git.1628689758413.gitgitgadget@gmail.com","threadId":"56260","inReplyTo":null,"subject":"[PATCH] commit: restore --edit when combined with --fixup","fromName":"Joel Klinghed via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-11T13:49:18Z","receivedAt":"2021-08-11T13:49:22Z","isPatch":true,"sender":{"key":"the_jk@spawned.biz","avatar":"https://avatars.githubusercontent.com/u/977644?v=4"},"body":"From: Joel Klinghed <the_jk@spawned.biz>\n\nRecent changes to --fixup, adding amend suboption, caused the\n--edit flag to be ignored as use_editor was always set to zero.\n\nRestore edit_flag having higher priority than fixup_message when\ndeciding the value of use_editor by only changing the default\nif edit_flag is not set.\n\nSigned-off-by: Joel Klinghed <the_jk@spawned.biz>\n---\n    commit: restore --edit when combined with --fixup\n    \n    Recent changes to --fixup, adding amend suboption, caused the --edit\n    flag to be ignored as use_editor was always set to zero.\n    \n    Restore edit_flag having higher priority than fixup_message when\n    deciding the value of use_editor by only changing the default if\n    edit_flag is not set.\n    \n    Signed-off-by: Joel Klinghed the_jk@spawned.biz\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1014%2Fthejk%2Ffixup_edit-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1014/thejk/fixup_edit-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1014\n\n builtin/commit.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 190d215d43b..4c5286840c5 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1333,7 +1333,8 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \t\t} else {\n \t\t\tfixup_commit = fixup_message;\n \t\t\tfixup_prefix = \"fixup\";\n-\t\t\tuse_editor = 0;\n+\t\t\tif (0 > edit_flag)\n+\t\t\t\tuse_editor = 0;\n \t\t}\n \t}\n \n\nbase-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb\n-- \ngitgitgadget\n"},{"id":"432514","messageId":"YRQx/jXdydxMGBmS@coredump.intra.peff.net","threadId":"56260","inReplyTo":"pull.1014.git.1628689758413.gitgitgadget@gmail.com","subject":"Re: [PATCH] commit: restore --edit when combined with --fixup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-11T20:24:30Z","receivedAt":"2021-08-11T20:24:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 11, 2021 at 01:49:18PM +0000, Joel Klinghed via GitGitGadget wrote:\n\n> From: Joel Klinghed <the_jk@spawned.biz>\n> \n> Recent changes to --fixup, adding amend suboption, caused the\n> --edit flag to be ignored as use_editor was always set to zero.\n> \n> Restore edit_flag having higher priority than fixup_message when\n> deciding the value of use_editor by only changing the default\n> if edit_flag is not set.\n\nThis is definitely a change in behavior due to 494d314a05 (commit: add\namend suboption to --fixup to create amend! commit, 2021-03-15). That\nwas in v2.32.0, so it's not a regression in the upcoming v2.33 which\nneeds to be handled in the next few days.\n\nMy inclination is to call it a regression and restore the original\nbehavior. But when I was going to suggest that you add a test, it made\nme wonder: what would we be testing for?\n\nIf the user says \"git commit --fixup HEAD --edit\", it seems reasonable\nfor them to expect that the editor is run, and that is easy to check.\nBut what are they planning to edit? If they modify the subject line of\nthe commit, it will wreck the \"fixup!\" mechanism. If they modify the\nbody (which starts blank), it's going to be discarded by the fixup\noperation.\n\nIs the goal that they might leave notes for themselves, which they can\nview in the meantime before they run \"rebase --autosquash\"?\n\n-Peff\n"},{"id":"432532","messageId":"127626db-302e-426c-a158-35f46205e1d6@www.fastmail.com","threadId":"56260","inReplyTo":"YRQx/jXdydxMGBmS@coredump.intra.peff.net","subject":"Re: [PATCH] commit: restore --edit when combined with --fixup","fromName":"Joel Klinghed","fromEmail":"the_jk@spawned.biz","sentAt":"2021-08-11T22:10:28Z","receivedAt":"2021-08-11T22:10:58Z","isPatch":true,"sender":{"key":"the_jk@spawned.biz","avatar":"https://avatars.githubusercontent.com/u/977644?v=4"},"body":"\n\nOn Wed, Aug 11, 2021, at 22:24, Jeff King wrote:\n> On Wed, Aug 11, 2021 at 01:49:18PM +0000, Joel Klinghed via GitGitGadget wrote:\n> \n> > From: Joel Klinghed <the_jk@spawned.biz>\n> > \n> > Recent changes to --fixup, adding amend suboption, caused the\n> > --edit flag to be ignored as use_editor was always set to zero.\n> > \n> > Restore edit_flag having higher priority than fixup_message when\n> > deciding the value of use_editor by only changing the default\n> > if edit_flag is not set.\n> \n> This is definitely a change in behavior due to 494d314a05 (commit: add\n> amend suboption to --fixup to create amend! commit, 2021-03-15). That\n> was in v2.32.0, so it's not a regression in the upcoming v2.33 which\n> needs to be handled in the next few days.\n> \n> My inclination is to call it a regression and restore the original\n> behavior. But when I was going to suggest that you add a test, it made\n> me wonder: what would we be testing for?\n> \n> If the user says \"git commit --fixup HEAD --edit\", it seems reasonable\n> for them to expect that the editor is run, and that is easy to check.\n> But what are they planning to edit? If they modify the subject line of\n> the commit, it will wreck the \"fixup!\" mechanism. If they modify the\n> body (which starts blank), it's going to be discarded by the fixup\n> operation.\n> \n> Is the goal that they might leave notes for themselves, which they can\n> view in the meantime before they run \"rebase --autosquash\"?\n> \n\nAt my work we use \"fixup!\" when pushing fixes to a review, using\nthe modified body to outline what issue the commit is addressing,\nhelping the reviewers to see intent and not just the end result.\nAll \"fixup!\" are then ofc. squashed before integration.\nSame can be achieved with -m but --edit is generally\neasier to work with in my experience.\n\nI've also once or twice used it to avoid a \"fixup!\" of a \"fixup!\" instead\nof looking up the original target commit hash but that's just me being\nlazy.\n\nI'll add a test to check for previous behavior.\n\n/JK\n"},{"id":"432533","messageId":"YRRNuQWkgWxQyfU0@coredump.intra.peff.net","threadId":"56260","inReplyTo":"127626db-302e-426c-a158-35f46205e1d6@www.fastmail.com","subject":"Re: [PATCH] commit: restore --edit when combined with --fixup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-11T22:22:49Z","receivedAt":"2021-08-11T22:22:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 12, 2021 at 12:10:28AM +0200, Joel Klinghed wrote:\n\n> > Is the goal that they might leave notes for themselves, which they can\n> > view in the meantime before they run \"rebase --autosquash\"?\n> > \n> \n> At my work we use \"fixup!\" when pushing fixes to a review, using\n> the modified body to outline what issue the commit is addressing,\n> helping the reviewers to see intent and not just the end result.\n> All \"fixup!\" are then ofc. squashed before integration.\n> Same can be achieved with -m but --edit is generally\n> easier to work with in my experience.\n> \n> I've also once or twice used it to avoid a \"fixup!\" of a \"fixup!\" instead\n> of looking up the original target commit hash but that's just me being\n> lazy.\n\nAh, thanks for explaining. That makes sense.\n\n> I'll add a test to check for previous behavior.\n\nThis is what I worked up, in case it helps.\n\ndiff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh\nindex 38a532d81c..3e4364066f 100755\n--- a/t/t7502-commit-porcelain.sh\n+++ b/t/t7502-commit-porcelain.sh\n@@ -940,4 +940,22 @@ EOF\n \t)\n '\n \n+test_expect_success 'commit --fixup respects --edit' '\n+\techo broken >foo.c &&\n+\tgit add foo.c &&\n+\tgit commit -m \"wip of foo.c\" &&\n+\techo fixed >foo.c &&\n+\t(\n+\t\ttest_set_editor \"$TEST_DIRECTORY/t7500/add-content\" &&\n+\t\tgit commit --fixup HEAD --edit foo.c\n+\t) &&\n+\tcat >expect <<-\\EOF &&\n+\tfixup! wip of foo.c\n+\n+\tcommit message\n+\tEOF\n+\tgit log -1 --pretty=format:%B HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\n-Peff\n"},{"id":"432543","messageId":"YRRc1PDpSOmd4sEY@camp.crustytoothpaste.net","threadId":"56260","inReplyTo":"127626db-302e-426c-a158-35f46205e1d6@www.fastmail.com","subject":"Re: [PATCH] commit: restore --edit when combined with --fixup","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-08-11T23:27:16Z","receivedAt":"2021-08-11T23:27:52Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-08-11 at 22:10:28, Joel Klinghed wrote:\n> At my work we use \"fixup!\" when pushing fixes to a review, using\n> the modified body to outline what issue the commit is addressing,\n> helping the reviewers to see intent and not just the end result.\n> All \"fixup!\" are then ofc. squashed before integration.\n> Same can be achieved with -m but --edit is generally\n> easier to work with in my experience.\n\nYeah, I do exactly the same thing.  I want to write a nice explanation\nof the change I made for the reviewer, but I don't want to preserve it\nfor the future.\n\nI recently had a series that was 33 commits after squashing but with 117\nbefore squashing, thanks to a series of very thorough and thoughtful\nreviews, so in this kind of scenario, it can really be kind to the\nreviewer to help them match up the change with their comment.\n\n> I've also once or twice used it to avoid a \"fixup!\" of a \"fixup!\" instead\n> of looking up the original target commit hash but that's just me being\n> lazy.\n> \n> I'll add a test to check for previous behavior.\n\nThanks for catching this.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"432546","messageId":"pull.1014.v2.git.1628725421868.gitgitgadget@gmail.com","threadId":"56260","inReplyTo":"pull.1014.git.1628689758413.gitgitgadget@gmail.com","subject":"[PATCH v2] commit: restore --edit when combined with --fixup","fromName":"Joel Klinghed via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-11T23:43:41Z","receivedAt":"2021-08-11T23:43:46Z","isPatch":true,"sender":{"key":"the_jk@spawned.biz","avatar":"https://avatars.githubusercontent.com/u/977644?v=4"},"body":"From: Joel Klinghed <the_jk@spawned.biz>\n\nRecent changes to --fixup, adding amend suboption, caused the\n--edit flag to be ignored as use_editor was always set to zero.\n\nRestore edit_flag having higher priority than fixup_message when\ndeciding the value of use_editor by only changing the default\nif edit_flag is not set.\n\nSigned-off-by: Joel Klinghed <the_jk@spawned.biz>\n---\n    commit: restore --edit when combined with --fixup\n    \n    Recent changes to --fixup, adding amend suboption, caused the --edit\n    flag to be ignored as use_editor was always set to zero.\n    \n    Restore edit_flag having higher priority than fixup_message when\n    deciding the value of use_editor by only changing the default if\n    edit_flag is not set.\n    \n    Changes since v1: Added test verifying that --fixup --edit brings up\n    editor.\n    \n    Signed-off-by: Joel Klinghed the_jk@spawned.biz\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1014%2Fthejk%2Ffixup_edit-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1014/thejk/fixup_edit-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1014\n\nRange-diff vs v1:\n\n 1:  b3e44c6ccde ! 1:  0ee926d4149 commit: restore --edit when combined with --fixup\n     @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *ar\n       \t\t}\n       \t}\n       \n     +\n     + ## t/t7500-commit-template-squash-signoff.sh ##\n     +@@ t/t7500-commit-template-squash-signoff.sh: test_expect_success 'commit --fixup -m\"something\" -m\"extra\"' '\n     + \n     + extra\"\n     + '\n     ++test_expect_success 'commit --fixup --edit' '\n     ++\tcommit_for_rebase_autosquash_setup &&\n     ++\tcat >e-append <<-\\EOF &&\n     ++\t#!/bin/sh\n     ++\tsed -e \"2a\\\\\n     ++something\\\\\n     ++extra\" <\"$1\" >\"$1-\"\n     ++\tmv \"$1-\" \"$1\"\n     ++\tEOF\n     ++\tchmod 755 e-append &&\n     ++\tEDITOR=\"./e-append\" git commit --fixup HEAD~1 --edit &&\n     ++\tcommit_msg_is \"fixup! target message subject linesomething\n     ++extra\"\n     ++'\n     ++\n     + get_commit_msg () {\n     + \trev=\"$1\" &&\n     + \tgit log -1 --pretty=format:\"%B\" \"$rev\"\n\n\n builtin/commit.c                          |  3 ++-\n t/t7500-commit-template-squash-signoff.sh | 15 +++++++++++++++\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 190d215d43b..4c5286840c5 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1333,7 +1333,8 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \t\t} else {\n \t\t\tfixup_commit = fixup_message;\n \t\t\tfixup_prefix = \"fixup\";\n-\t\t\tuse_editor = 0;\n+\t\t\tif (0 > edit_flag)\n+\t\t\t\tuse_editor = 0;\n \t\t}\n \t}\n \ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 7d02f79c0de..d71c7812180 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -281,6 +281,21 @@ test_expect_success 'commit --fixup -m\"something\" -m\"extra\"' '\n \n extra\"\n '\n+test_expect_success 'commit --fixup --edit' '\n+\tcommit_for_rebase_autosquash_setup &&\n+\tcat >e-append <<-\\EOF &&\n+\t#!/bin/sh\n+\tsed -e \"2a\\\\\n+something\\\\\n+extra\" <\"$1\" >\"$1-\"\n+\tmv \"$1-\" \"$1\"\n+\tEOF\n+\tchmod 755 e-append &&\n+\tEDITOR=\"./e-append\" git commit --fixup HEAD~1 --edit &&\n+\tcommit_msg_is \"fixup! target message subject linesomething\n+extra\"\n+'\n+\n get_commit_msg () {\n \trev=\"$1\" &&\n \tgit log -1 --pretty=format:\"%B\" \"$rev\"\n\nbase-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb\n-- \ngitgitgadget\n"},{"id":"432562","messageId":"xmqq1r6z6xwf.fsf@gitster.g","threadId":"56260","inReplyTo":"pull.1014.v2.git.1628725421868.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] commit: restore --edit when combined with --fixup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-12T05:21:36Z","receivedAt":"2021-08-12T05:21:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Joel Klinghed via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 190d215d43b..4c5286840c5 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1333,7 +1333,8 @@ static int parse_and_validate_options(int argc, const char *argv[],\n>  \t\t} else {\n>  \t\t\tfixup_commit = fixup_message;\n>  \t\t\tfixup_prefix = \"fixup\";\n> -\t\t\tuse_editor = 0;\n> +\t\t\tif (0 > edit_flag)\n\nWriting this as\n\n\t\t\tif (edit_flag < 0)\n\nmakes it far easier to immediately see that we are talking about a\nnagetive edit_flag.\n\n> +\t\t\t\tuse_editor = 0;\n>  \t\t}\n>  \t}\n>  \n> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\n> index 7d02f79c0de..d71c7812180 100755\n> --- a/t/t7500-commit-template-squash-signoff.sh\n> +++ b/t/t7500-commit-template-squash-signoff.sh\n> @@ -281,6 +281,21 @@ test_expect_success 'commit --fixup -m\"something\" -m\"extra\"' '\n>  \n>  extra\"\n>  '\n> +test_expect_success 'commit --fixup --edit' '\n> +\tcommit_for_rebase_autosquash_setup &&\n\n> +\tcat >e-append <<-\\EOF &&\n> +\t#!/bin/sh\n> +\tsed -e \"2a\\\\\n> +something\\\\\n> +extra\" <\"$1\" >\"$1-\"\n> +\tmv \"$1-\" \"$1\"\n> +\tEOF\n> +\tchmod 755 e-append &&\n\nUse write_script helper from test-lib-functions.sh here and lose the\nhardcoded reference to /bin/sh.\n\n> +\tEDITOR=\"./e-append\" git commit --fixup HEAD~1 --edit &&\n> +\tcommit_msg_is \"fixup! target message subject linesomething\n> +extra\"\n> +'\n\n\nThanks.\n"},{"id":"432564","messageId":"873ec1ab-c062-4306-b3fb-09d32c70cc7f@www.fastmail.com","threadId":"56260","inReplyTo":"xmqq1r6z6xwf.fsf@gitster.g","subject":"Re: [PATCH v2] commit: restore --edit when combined with --fixup","fromName":"Joel Klinghed","fromEmail":"the_jk@spawned.biz","sentAt":"2021-08-12T07:42:06Z","receivedAt":"2021-08-12T07:43:00Z","isPatch":true,"sender":{"key":"the_jk@spawned.biz","avatar":"https://avatars.githubusercontent.com/u/977644?v=4"},"body":"On Thu, Aug 12, 2021, at 07:21, Junio C Hamano wrote:\n> \"Joel Klinghed via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n> > diff --git a/builtin/commit.c b/builtin/commit.c\n> > index 190d215d43b..4c5286840c5 100644\n> > --- a/builtin/commit.c\n> > +++ b/builtin/commit.c\n> > @@ -1333,7 +1333,8 @@ static int parse_and_validate_options(int argc, const char *argv[],\n> >  \t\t} else {\n> >  \t\t\tfixup_commit = fixup_message;\n> >  \t\t\tfixup_prefix = \"fixup\";\n> > -\t\t\tuse_editor = 0;\n> > +\t\t\tif (0 > edit_flag)\n> \n> Writing this as\n> \n> \t\t\tif (edit_flag < 0)\n> \n> makes it far easier to immediately see that we are talking about a\n> nagetive edit_flag.\n> \n\nAgree, I'll change it.\nI was unsure of the style and copied from the earlier condition:\n\tif (0 <= edit_flag)\n\t\tuse_editor = edit_flag;\n\n> > +\t\t\t\tuse_editor = 0;\n> >  \t\t}\n> >  \t}\n> >  \n\n> \n> Use write_script helper from test-lib-functions.sh here and lose the\n> hardcoded reference to /bin/sh.\n> \n\nAh, missed that one.\n\nThanks.\n\n/JK\n"},{"id":"432566","messageId":"pull.1014.v3.git.1628755346354.gitgitgadget@gmail.com","threadId":"56260","inReplyTo":"pull.1014.v2.git.1628725421868.gitgitgadget@gmail.com","subject":"[PATCH v3] commit: restore --edit when combined with --fixup","fromName":"Joel Klinghed via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-12T08:02:26Z","receivedAt":"2021-08-12T08:03:01Z","isPatch":true,"sender":{"key":"the_jk@spawned.biz","avatar":"https://avatars.githubusercontent.com/u/977644?v=4"},"body":"From: Joel Klinghed <the_jk@spawned.biz>\n\nRecent changes to --fixup, adding amend suboption, caused the\n--edit flag to be ignored as use_editor was always set to zero.\n\nRestore edit_flag having higher priority than fixup_message when\ndeciding the value of use_editor by only changing the default\nif edit_flag is not set.\n\nSigned-off-by: Joel Klinghed <the_jk@spawned.biz>\n---\n    commit: restore --edit when combined with --fixup\n    \n    Recent changes to --fixup, adding amend suboption, caused the --edit\n    flag to be ignored as use_editor was always set to zero.\n    \n    Restore edit_flag having higher priority than fixup_message when\n    deciding the value of use_editor by only changing the default if\n    edit_flag is not set.\n    \n    Changes since v1: Added test verifying that --fixup --edit brings up\n    editor.\n    \n    Changes since v2: Clarify if condition and use write_script helper in\n    test.\n    \n    Signed-off-by: Joel Klinghed the_jk@spawned.biz\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1014%2Fthejk%2Ffixup_edit-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1014/thejk/fixup_edit-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1014\n\nRange-diff vs v2:\n\n 1:  0ee926d4149 ! 1:  6bc5d8bbe61 commit: restore --edit when combined with --fixup\n     @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *ar\n       \t\t\tfixup_commit = fixup_message;\n       \t\t\tfixup_prefix = \"fixup\";\n      -\t\t\tuse_editor = 0;\n     -+\t\t\tif (0 > edit_flag)\n     ++\t\t\tif (edit_flag < 0)\n      +\t\t\t\tuse_editor = 0;\n       \t\t}\n       \t}\n     @@ t/t7500-commit-template-squash-signoff.sh: test_expect_success 'commit --fixup -\n       '\n      +test_expect_success 'commit --fixup --edit' '\n      +\tcommit_for_rebase_autosquash_setup &&\n     -+\tcat >e-append <<-\\EOF &&\n     -+\t#!/bin/sh\n     ++\twrite_script e-append <<-\\EOF &&\n      +\tsed -e \"2a\\\\\n      +something\\\\\n      +extra\" <\"$1\" >\"$1-\"\n      +\tmv \"$1-\" \"$1\"\n      +\tEOF\n     -+\tchmod 755 e-append &&\n      +\tEDITOR=\"./e-append\" git commit --fixup HEAD~1 --edit &&\n      +\tcommit_msg_is \"fixup! target message subject linesomething\n      +extra\"\n\n\n builtin/commit.c                          |  3 ++-\n t/t7500-commit-template-squash-signoff.sh | 13 +++++++++++++\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 190d215d43b..560aecd21b1 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1333,7 +1333,8 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \t\t} else {\n \t\t\tfixup_commit = fixup_message;\n \t\t\tfixup_prefix = \"fixup\";\n-\t\t\tuse_editor = 0;\n+\t\t\tif (edit_flag < 0)\n+\t\t\t\tuse_editor = 0;\n \t\t}\n \t}\n \ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 7d02f79c0de..a48fe859235 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -281,6 +281,19 @@ test_expect_success 'commit --fixup -m\"something\" -m\"extra\"' '\n \n extra\"\n '\n+test_expect_success 'commit --fixup --edit' '\n+\tcommit_for_rebase_autosquash_setup &&\n+\twrite_script e-append <<-\\EOF &&\n+\tsed -e \"2a\\\\\n+something\\\\\n+extra\" <\"$1\" >\"$1-\"\n+\tmv \"$1-\" \"$1\"\n+\tEOF\n+\tEDITOR=\"./e-append\" git commit --fixup HEAD~1 --edit &&\n+\tcommit_msg_is \"fixup! target message subject linesomething\n+extra\"\n+'\n+\n get_commit_msg () {\n \trev=\"$1\" &&\n \tgit log -1 --pretty=format:\"%B\" \"$rev\"\n\nbase-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb\n-- \ngitgitgadget\n"},{"id":"432573","messageId":"e3a24819-9953-0245-7f64-472def4d180a@gmail.com","threadId":"56260","inReplyTo":"pull.1014.v3.git.1628755346354.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] commit: restore --edit when combined with --fixup","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-08-12T09:32:59Z","receivedAt":"2021-08-12T09:33:06Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Joel\n\nOn 12/08/2021 09:02, Joel Klinghed via GitGitGadget wrote:\n> From: Joel Klinghed <the_jk@spawned.biz>\n> \n> Recent changes to --fixup, adding amend suboption, caused the\n> --edit flag to be ignored as use_editor was always set to zero.\n> \n> Restore edit_flag having higher priority than fixup_message when\n> deciding the value of use_editor by only changing the default\n> if edit_flag is not set.\n\nThanks for fixing this\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 190d215d43b..560aecd21b1 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1333,7 +1333,8 @@ static int parse_and_validate_options(int argc, const char *argv[],\n>   \t\t} else {\n>   \t\t\tfixup_commit = fixup_message;\n>   \t\t\tfixup_prefix = \"fixup\";\n> -\t\t\tuse_editor = 0;\n> +\t\t\tif (edit_flag < 0)\n> +\t\t\t\tuse_editor = 0;\n>   \t\t}\n>   \t}\n>   \n\nCommit 494d314a05 (\"commit: add amend suboption to --fixup to create \namend! commit\", 2021-03-15) that broke this has the following hunk above \nthis change\n\n@@ -1170,7 +1206,7 @@ static int parse_and_validate_options(int argc, \nconst char *argv[],\n         if (force_author && renew_authorship)\n                 die(_(\"Using both --reset-author and --author does not \nmake sense\"));\n\n-       if (logfile || have_option_m || use_message || fixup_message)\n+       if (logfile || have_option_m || use_message)\n                 use_editor = 0;\n         if (0 <= edit_flag)\n                 use_editor = edit_flag;\n\nI think it should have moved those last two context lines that set \n`use_editor` to below the part that you are fixing. Then the `use_editor \n= 0` line that you are changing continues to work as before. (As you see \nthere are quite a few legacy Yoda conditions in this file, nowadays we \navoid adding new ones). I'm not sure if it is worth re working this \npatch to do that, but it does have the advantage of only testing \nedit_flag once.\n\n> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\n> index 7d02f79c0de..a48fe859235 100755\n> --- a/t/t7500-commit-template-squash-signoff.sh\n> +++ b/t/t7500-commit-template-squash-signoff.sh\n> @@ -281,6 +281,19 @@ test_expect_success 'commit --fixup -m\"something\" -m\"extra\"' '\n>   \n>   extra\"\n>   '\n> +test_expect_success 'commit --fixup --edit' '\n> +\tcommit_for_rebase_autosquash_setup &&\n> +\twrite_script e-append <<-\\EOF &&\n> +\tsed -e \"2a\\\\\n> +something\\\\\n> +extra\" <\"$1\" >\"$1-\"\n> +\tmv \"$1-\" \"$1\"\n> +\tEOF\n\nAgain I'm not sure it is worth changing it now but for future reference \nthis is a rather complicated way of appending to the commit message. The \ntest file has an example using set_fake_editor() together with \nFAKE_COMMIT_AMEND just below where you have added this test or you can \njust have\n\n     EDITOR=\"echo something extra >>\" git commit --fixup=HEAD~1 --edit\n\nBest Wishes\n\nPhillip\n\n> +\tEDITOR=\"./e-append\" git commit --fixup HEAD~1 --edit &&\n> +\tcommit_msg_is \"fixup! target message subject linesomething\n> +extra\"\n> +'\n> +\n>   get_commit_msg () {\n>   \trev=\"$1\" &&\n>   \tgit log -1 --pretty=format:\"%B\" \"$rev\"\n> \n> base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb\n> \n"},{"id":"432574","messageId":"0576a44a-c726-4550-ad29-52f09982de98@www.fastmail.com","threadId":"56260","inReplyTo":"e3a24819-9953-0245-7f64-472def4d180a@gmail.com","subject":"Re: [PATCH v3] commit: restore --edit when combined with --fixup","fromName":"Joel Klinghed","fromEmail":"the_jk@spawned.biz","sentAt":"2021-08-12T10:01:10Z","receivedAt":"2021-08-12T10:01:34Z","isPatch":true,"sender":{"key":"the_jk@spawned.biz","avatar":"https://avatars.githubusercontent.com/u/977644?v=4"},"body":"\n\nOn Thu, Aug 12, 2021, at 11:32, Phillip Wood wrote:\n> Hi Joel\n> \n> @@ -1170,7 +1206,7 @@ static int parse_and_validate_options(int argc, \n> const char *argv[],\n>          if (force_author && renew_authorship)\n>                  die(_(\"Using both --reset-author and --author does not \n> make sense\"));\n> \n> -       if (logfile || have_option_m || use_message || fixup_message)\n> +       if (logfile || have_option_m || use_message)\n>                  use_editor = 0;\n>          if (0 <= edit_flag)\n>                  use_editor = edit_flag;\n> \n> I think it should have moved those last two context lines that set \n> `use_editor` to below the part that you are fixing. Then the `use_editor \n> = 0` line that you are changing continues to work as before. (As you see \n> there are quite a few legacy Yoda conditions in this file, nowadays we \n> avoid adding new ones). I'm not sure if it is worth re working this \n> patch to do that, but it does have the advantage of only testing \n> edit_flag once.\n\nI looked at moving the condition to one place but as use_editor = 0\nis only set for --fixup if there isn't a suboption specified I didn't want\nto have to duplicate the check for a suboption when deciding if\nuse_editor should default to zero.\n \n> > diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\n> > index 7d02f79c0de..a48fe859235 100755\n> > --- a/t/t7500-commit-template-squash-signoff.sh\n> > +++ b/t/t7500-commit-template-squash-signoff.sh\n> > @@ -281,6 +281,19 @@ test_expect_success 'commit --fixup -m\"something\" -m\"extra\"' '\n> >   \n> >   extra\"\n> >   '\n> > +test_expect_success 'commit --fixup --edit' '\n> > +\tcommit_for_rebase_autosquash_setup &&\n> > +\twrite_script e-append <<-\\EOF &&\n> > +\tsed -e \"2a\\\\\n> > +something\\\\\n> > +extra\" <\"$1\" >\"$1-\"\n> > +\tmv \"$1-\" \"$1\"\n> > +\tEOF\n> \n> Again I'm not sure it is worth changing it now but for future reference \n> this is a rather complicated way of appending to the commit message. The \n> test file has an example using set_fake_editor() together with \n> FAKE_COMMIT_AMEND just below where you have added this test or you can \n> just have\n> \n>      EDITOR=\"echo something extra >>\" git commit --fixup=HEAD~1 --edit\n> \n> Best Wishes\n> \n> Phillip\n> \n\nYeah, especially getting sed in a POSIX and BSD compatible mode took some\ndoing. I wanted to have a similar output to the test above this one, with a line break \nbetween something and extra, and frankly, my shell-foo lacked for\ngetting either FAKE_COMMIT_AMEND or EDITOR=\"... >>\" to include a newline.\nBut looking at it again, I realize that EDITOR=\"printf \\\"something\\nextra\\\" >>\" \nworks just fine.\n\n/JK\n"},{"id":"432578","messageId":"pull.1014.v4.git.1628769334197.gitgitgadget@gmail.com","threadId":"56260","inReplyTo":"pull.1014.v3.git.1628755346354.gitgitgadget@gmail.com","subject":"[PATCH v4] commit: restore --edit when combined with --fixup","fromName":"Joel Klinghed via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-12T11:55:33Z","receivedAt":"2021-08-12T11:55:38Z","isPatch":true,"sender":{"key":"the_jk@spawned.biz","avatar":"https://avatars.githubusercontent.com/u/977644?v=4"},"body":"From: Joel Klinghed <the_jk@spawned.biz>\n\nRecent changes to --fixup, adding amend suboption, caused the\n--edit flag to be ignored as use_editor was always set to zero.\n\nRestore edit_flag having higher priority than fixup_message when\ndeciding the value of use_editor by only changing the default\nif edit_flag is not set.\n\nSigned-off-by: Joel Klinghed <the_jk@spawned.biz>\n---\n    commit: restore --edit when combined with --fixup\n    \n    Recent changes to --fixup, adding amend suboption, caused the --edit\n    flag to be ignored as use_editor was always set to zero.\n    \n    Restore edit_flag having higher priority than fixup_message when\n    deciding the value of use_editor by only changing the default if\n    edit_flag is not set.\n    \n    Changes since v1: Added test verifying that --fixup --edit brings up\n    editor.\n    \n    Changes since v2: Clarify if condition and use write_script helper in\n    test.\n    \n    Changes since v3: Simplify test.\n    \n    Signed-off-by: Joel Klinghed the_jk@spawned.biz\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1014%2Fthejk%2Ffixup_edit-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1014/thejk/fixup_edit-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1014\n\nRange-diff vs v3:\n\n 1:  6bc5d8bbe61 ! 1:  0c0cb647e03 commit: restore --edit when combined with --fixup\n     @@ t/t7500-commit-template-squash-signoff.sh: test_expect_success 'commit --fixup -\n       '\n      +test_expect_success 'commit --fixup --edit' '\n      +\tcommit_for_rebase_autosquash_setup &&\n     -+\twrite_script e-append <<-\\EOF &&\n     -+\tsed -e \"2a\\\\\n     -+something\\\\\n     -+extra\" <\"$1\" >\"$1-\"\n     -+\tmv \"$1-\" \"$1\"\n     -+\tEOF\n     -+\tEDITOR=\"./e-append\" git commit --fixup HEAD~1 --edit &&\n     ++\tEDITOR=\"printf \\\"something\\nextra\\\" >>\" git commit --fixup HEAD~1 --edit &&\n      +\tcommit_msg_is \"fixup! target message subject linesomething\n      +extra\"\n      +'\n\n\n builtin/commit.c                          | 3 ++-\n t/t7500-commit-template-squash-signoff.sh | 7 +++++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 190d215d43b..560aecd21b1 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1333,7 +1333,8 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \t\t} else {\n \t\t\tfixup_commit = fixup_message;\n \t\t\tfixup_prefix = \"fixup\";\n-\t\t\tuse_editor = 0;\n+\t\t\tif (edit_flag < 0)\n+\t\t\t\tuse_editor = 0;\n \t\t}\n \t}\n \ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 7d02f79c0de..811566bf847 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -281,6 +281,13 @@ test_expect_success 'commit --fixup -m\"something\" -m\"extra\"' '\n \n extra\"\n '\n+test_expect_success 'commit --fixup --edit' '\n+\tcommit_for_rebase_autosquash_setup &&\n+\tEDITOR=\"printf \\\"something\\nextra\\\" >>\" git commit --fixup HEAD~1 --edit &&\n+\tcommit_msg_is \"fixup! target message subject linesomething\n+extra\"\n+'\n+\n get_commit_msg () {\n \trev=\"$1\" &&\n \tgit log -1 --pretty=format:\"%B\" \"$rev\"\n\nbase-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb\n-- \ngitgitgadget\n"},{"id":"432595","messageId":"xmqqbl6230i1.fsf@gitster.g","threadId":"56260","inReplyTo":"873ec1ab-c062-4306-b3fb-09d32c70cc7f@www.fastmail.com","subject":"Re: [PATCH v2] commit: restore --edit when combined with --fixup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-12T19:51:18Z","receivedAt":"2021-08-12T19:51:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Joel Klinghed\" <the_jk@spawned.biz> writes:\n\n>> Writing this as\n>> \n>> \t\t\tif (edit_flag < 0)\n>> \n>> makes it far easier to immediately see that we are talking about a\n>> nagetive edit_flag.\n>> \n>\n> Agree, I'll change it.\n> I was unsure of the style and copied from the earlier condition:\n> \tif (0 <= edit_flag)\n> \t\tuse_editor = edit_flag;\n\nThere are two valid schools of thought when it comes to comparison.\n\nSome folks consider that a comparison between a variable and a\nconstant is a statement about the variable, hence the expression\nshould be\n\n\t\tif (VARIRABLE comparison-operator CONSTANT)\n\nThey will write things like:\n\n\t\tif (edit_flag >= 0)\n\t\tif (edit_flag < 0)\n\nOther folks consider that textual order of the comparison should\nmatch the actual order of the things being compared, as if they are\narranged on a number line, hence the expression should be\n\n\t\tif (SMALLER < LARGER)\n\nno matter which one is variable and which one is constant.\n\nThey will write:\n\n\t\tif (0 <= edit_flag)\n\t\tif (edit_flag < 0)\n\nThe case in question, asserting that edit_flag is negative, is what\nboth camps agree how to write.\n"},{"id":"432669","messageId":"0151003c-d544-1fab-18e9-34eb84842555@gmail.com","threadId":"56260","inReplyTo":"0576a44a-c726-4550-ad29-52f09982de98@www.fastmail.com","subject":"Re: [PATCH v3] commit: restore --edit when combined with --fixup","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-08-13T13:06:20Z","receivedAt":"2021-08-13T13:06:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 12/08/2021 11:01, Joel Klinghed wrote:\n> On Thu, Aug 12, 2021, at 11:32, Phillip Wood wrote:\n>> Hi Joel\n>>\n>> @@ -1170,7 +1206,7 @@ static int parse_and_validate_options(int argc,\n>> const char *argv[],\n>>           if (force_author && renew_authorship)\n>>                   die(_(\"Using both --reset-author and --author does not\n>> make sense\"));\n>>\n>> -       if (logfile || have_option_m || use_message || fixup_message)\n>> +       if (logfile || have_option_m || use_message)\n>>                   use_editor = 0;\n>>           if (0 <= edit_flag)\n>>                   use_editor = edit_flag;\n>>\n>> I think it should have moved those last two context lines that set\n>> `use_editor` to below the part that you are fixing. Then the `use_editor\n>> = 0` line that you are changing continues to work as before. (As you see\n>> there are quite a few legacy Yoda conditions in this file, nowadays we\n>> avoid adding new ones). I'm not sure if it is worth re working this\n>> patch to do that, but it does have the advantage of only testing\n>> edit_flag once.\n> \n> I looked at moving the condition to one place but as use_editor = 0\n> is only set for --fixup if there isn't a suboption specified I didn't want\n> to have to duplicate the check for a suboption when deciding if\n> use_editor should default to zero.\n\nI don't think you need to duplicate the check for a suboption, can't you \njust do this on top of master (i.e without you patch applied)?\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 243c626307..67a84ff6e4 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1251,11 +1251,6 @@ static int parse_and_validate_options(int argc, \nconst char *argv[],\n         if (force_author && renew_authorship)\n                 die(_(\"Using both --reset-author and --author does not \nmake sense\"));\n\n-       if (logfile || have_option_m || use_message)\n-               use_editor = 0;\n-       if (0 <= edit_flag)\n-               use_editor = edit_flag;\n-\n         /* Sanity check options */\n         if (amend && !current_head)\n                 die(_(\"You have nothing to amend.\"));\n@@ -1344,6 +1339,11 @@ static int parse_and_validate_options(int argc, \nconst char *argv[],\n                 }\n         }\n\n+       if (logfile || have_option_m || use_message)\n+               use_editor = 0;\n+       if (0 <= edit_flag)\n+               use_editor = edit_flag;\n+\n         cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);\n\n         handle_untracked_files_arg(s);\n\nI chose to move the other clause that sets use_editor as well so they \nstay together.\n\nBest wishes\n\nPhillip\n>   \n>>> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\n>>> index 7d02f79c0de..a48fe859235 100755\n>>> --- a/t/t7500-commit-template-squash-signoff.sh\n>>> +++ b/t/t7500-commit-template-squash-signoff.sh\n>>> @@ -281,6 +281,19 @@ test_expect_success 'commit --fixup -m\"something\" -m\"extra\"' '\n>>>    \n>>>    extra\"\n>>>    '\n>>> +test_expect_success 'commit --fixup --edit' '\n>>> +\tcommit_for_rebase_autosquash_setup &&\n>>> +\twrite_script e-append <<-\\EOF &&\n>>> +\tsed -e \"2a\\\\\n>>> +something\\\\\n>>> +extra\" <\"$1\" >\"$1-\"\n>>> +\tmv \"$1-\" \"$1\"\n>>> +\tEOF\n>>\n>> Again I'm not sure it is worth changing it now but for future reference\n>> this is a rather complicated way of appending to the commit message. The\n>> test file has an example using set_fake_editor() together with\n>> FAKE_COMMIT_AMEND just below where you have added this test or you can\n>> just have\n>>\n>>       EDITOR=\"echo something extra >>\" git commit --fixup=HEAD~1 --edit\n>>\n>> Best Wishes\n>>\n>> Phillip\n>>\n> \n> Yeah, especially getting sed in a POSIX and BSD compatible mode took some\n> doing. I wanted to have a similar output to the test above this one, with a line break\n> between something and extra, and frankly, my shell-foo lacked for\n> getting either FAKE_COMMIT_AMEND or EDITOR=\"... >>\" to include a newline.\n> But looking at it again, I realize that EDITOR=\"printf \\\"something\\nextra\\\" >>\"\n> works just fine.\n> \n> /JK\n> \n"},{"id":"432679","messageId":"1fc066c5-a085-4865-9eb9-853dfcbe33c2@www.fastmail.com","threadId":"56260","inReplyTo":"0151003c-d544-1fab-18e9-34eb84842555@gmail.com","subject":"Re: [PATCH v3] commit: restore --edit when combined with --fixup","fromName":"Joel Klinghed","fromEmail":"the_jk@spawned.biz","sentAt":"2021-08-13T15:35:43Z","receivedAt":"2021-08-13T15:35:59Z","isPatch":true,"sender":{"key":"the_jk@spawned.biz","avatar":"https://avatars.githubusercontent.com/u/977644?v=4"},"body":"\n\nOn Fri, Aug 13, 2021, at 15:06, Phillip Wood wrote:\n> On 12/08/2021 11:01, Joel Klinghed wrote:\n> > I looked at moving the condition to one place but as use_editor = 0\n> > is only set for --fixup if there isn't a suboption specified I didn't want\n> > to have to duplicate the check for a suboption when deciding if\n> > use_editor should default to zero.\n> \n> I don't think you need to duplicate the check for a suboption, can't you \n> just do this on top of master (i.e without you patch applied)?\n> \n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 243c626307..67a84ff6e4 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1251,11 +1251,6 @@ static int parse_and_validate_options(int argc, \n> const char *argv[],\n>          if (force_author && renew_authorship)\n>                  die(_(\"Using both --reset-author and --author does not \n> make sense\"));\n> \n> -       if (logfile || have_option_m || use_message)\n> -               use_editor = 0;\n> -       if (0 <= edit_flag)\n> -               use_editor = edit_flag;\n> -\n>          /* Sanity check options */\n>          if (amend && !current_head)\n>                  die(_(\"You have nothing to amend.\"));\n> @@ -1344,6 +1339,11 @@ static int parse_and_validate_options(int argc, \n> const char *argv[],\n>                  }\n>          }\n> \n> +       if (logfile || have_option_m || use_message)\n> +               use_editor = 0;\n> +       if (0 <= edit_flag)\n> +               use_editor = edit_flag;\n> +\n>          cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);\n> \n>          handle_untracked_files_arg(s);\n> \n> I chose to move the other clause that sets use_editor as well so they \n> stay together.\n> \n\nWith the above change use_editor no longer defaults to 0 for --fixup as\nit used to do.\nMy expected behavior (based on old versions):\ngit commit --fixup <hash>  /// No editor\ngit commit --fixup <hash> --edit  /// Editor\nAs far as I can see your change would display an editor in both cases.\n\nAn alternative would be:\n+       if (logfile || have_option_m || use_message || fixup_message)\n+               use_editor = 0;\n+       if (0 <= edit_flag)\n+               use_editor = edit_flag;\n\nThat would fix the above cases but in \n\"commit: add amend suboption to --fixup to create amend! commit\"\nthe implementation left\ngit commit --fixup amend:<hash>  // Editor\nand I didn't want to change that. But if the default should be no editor\nhere as well then the above would be a better patch.\n\n/JK\n"},{"id":"432736","messageId":"ec5f6698-46e9-c8c8-057d-b04851cb9265@gmail.com","threadId":"56260","inReplyTo":"1fc066c5-a085-4865-9eb9-853dfcbe33c2@www.fastmail.com","subject":"Re: [PATCH v3] commit: restore --edit when combined with --fixup","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-08-14T15:20:37Z","receivedAt":"2021-08-14T15:20:49Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Joel\n\nOn 13/08/2021 16:35, Joel Klinghed wrote:\n> \n> \n> On Fri, Aug 13, 2021, at 15:06, Phillip Wood wrote:\n>> On 12/08/2021 11:01, Joel Klinghed wrote:\n>>> I looked at moving the condition to one place but as use_editor = 0\n>>> is only set for --fixup if there isn't a suboption specified I didn't want\n>>> to have to duplicate the check for a suboption when deciding if\n>>> use_editor should default to zero.\n>>\n>> I don't think you need to duplicate the check for a suboption, can't you\n>> just do this on top of master (i.e without you patch applied)?\n>>\n>> diff --git a/builtin/commit.c b/builtin/commit.c\n>> index 243c626307..67a84ff6e4 100644\n>> --- a/builtin/commit.c\n>> +++ b/builtin/commit.c\n>> @@ -1251,11 +1251,6 @@ static int parse_and_validate_options(int argc,\n>> const char *argv[],\n>>           if (force_author && renew_authorship)\n>>                   die(_(\"Using both --reset-author and --author does not\n>> make sense\"));\n>>\n>> -       if (logfile || have_option_m || use_message)\n>> -               use_editor = 0;\n>> -       if (0 <= edit_flag)\n>> -               use_editor = edit_flag;\n>> -\n>>           /* Sanity check options */\n>>           if (amend && !current_head)\n>>                   die(_(\"You have nothing to amend.\"));\n>> @@ -1344,6 +1339,11 @@ static int parse_and_validate_options(int argc,\n>> const char *argv[],\n>>                   }\n>>           }\n>>\n>> +       if (logfile || have_option_m || use_message)\n>> +               use_editor = 0;\n>> +       if (0 <= edit_flag)\n>> +               use_editor = edit_flag;\n>> +\n>>           cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);\n>>\n>>           handle_untracked_files_arg(s);\n>>\n>> I chose to move the other clause that sets use_editor as well so they\n>> stay together.\n>>\n> \n> With the above change use_editor no longer defaults to 0 for --fixup as\n> it used to do.\n> My expected behavior (based on old versions):\n> git commit --fixup <hash>  /// No editor\n> git commit --fixup <hash> --edit  /// Editor\n> As far as I can see your change would display an editor in both cases.\n\nI've just tested it and it works as expected. However moving the\n'if (logfile...)' breaks the test \"commit --squash works with -c\" so we\nneed to just move the second if clause. This is what I have on top of\nmaster (i.e. without your patch so a plain fixup is still setting\nuse_editor=0)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 243c626307..7c9b1e7be3 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1253,8 +1253,6 @@ static int parse_and_validate_options(int argc, const char *argv[],\n\n         if (logfile || have_option_m || use_message)\n                 use_editor = 0;\n-       if (0 <= edit_flag)\n-               use_editor = edit_flag;\n\n         /* Sanity check options */\n         if (amend && !current_head)\n@@ -1344,6 +1342,9 @@ static int parse_and_validate_options(int argc, const char *argv[],\n                 }\n         }\n\n+       if (0 <= edit_flag)\n+               use_editor = edit_flag;\n+\n         cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);\n\n         handle_untracked_files_arg(s);\ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 54c2082acb..3fa674e52d 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -270,7 +270,7 @@ EOF\n  \n  test_expect_success 'commit --fixup provides correct one-line commit message' '\n         commit_for_rebase_autosquash_setup &&\n-       git commit --fixup HEAD~1 &&\n+       EDITOR=\"printf \\\"something\\nextra\\\" >>\" git commit --fixup HEAD~1 &&\n         commit_msg_is \"fixup! target message subject line\"\n  '\n  \n@@ -281,6 +281,14 @@ test_expect_success 'commit --fixup -m\"something\" -m\"extra\"' '\n\n  extra\"\n  '\n+\n+test_expect_success 'commit --fixup --edit' '\n+       commit_for_rebase_autosquash_setup &&\n+       EDITOR=\"printf \\\"something\\nextra\\\" >>\" git commit --fixup HEAD~1 --edit &&\n+       commit_msg_is \"fixup! target message subject linesomething\n+extra\"\n+'\n+\n  get_commit_msg () {\n         rev=\"$1\" &&\n         git log -1 --pretty=format:\"%B\" \"$rev\"\n"},{"id":"432746","messageId":"d268b6ef-89c7-4649-828e-759ed269e9c6@www.fastmail.com","threadId":"56260","inReplyTo":"ec5f6698-46e9-c8c8-057d-b04851cb9265@gmail.com","subject":"Re: [PATCH v3] commit: restore --edit when combined with --fixup","fromName":"Joel Klinghed","fromEmail":"the_jk@spawned.biz","sentAt":"2021-08-14T21:19:22Z","receivedAt":"2021-08-14T21:20:33Z","isPatch":true,"sender":{"key":"the_jk@spawned.biz","avatar":"https://avatars.githubusercontent.com/u/977644?v=4"},"body":"On Sat, Aug 14, 2021, at 17:20, Phillip Wood wrote:\n> On 13/08/2021 16:35, Joel Klinghed wrote:\n> > \n> > With the above change use_editor no longer defaults to 0 for --fixup as\n> > it used to do.\n> > My expected behavior (based on old versions):\n> > git commit --fixup <hash>  /// No editor\n> > git commit --fixup <hash> --edit  /// Editor\n> > As far as I can see your change would display an editor in both cases.\n> \n> I've just tested it and it works as expected. However moving the\n> 'if (logfile...)' breaks the test \"commit --squash works with -c\" so we\n> need to just move the second if clause. This is what I have on top of\n> master (i.e. without your patch so a plain fixup is still setting\n> use_editor=0)\n> \n\nAh, my bad. I misunderstood and thought your first patch was to\nbe applied without my fixes. This way is cleaner, no doubt.\n\n> diff --git a/t/t7500-commit-template-squash-signoff.sh \n> b/t/t7500-commit-template-squash-signoff.sh\n> index 54c2082acb..3fa674e52d 100755\n> --- a/t/t7500-commit-template-squash-signoff.sh\n> +++ b/t/t7500-commit-template-squash-signoff.sh\n> @@ -270,7 +270,7 @@ EOF\n>   \n>   test_expect_success 'commit --fixup provides correct one-line commit \n> message' '\n>          commit_for_rebase_autosquash_setup &&\n> -       git commit --fixup HEAD~1 &&\n> +       EDITOR=\"printf \\\"something\\nextra\\\" >>\" git commit --fixup \n> HEAD~1 &&\n>          commit_msg_is \"fixup! target message subject line\"\n>   '\n\nGood idea to make --fixup without edit behavior verification explicit.\n\n/JK\n"},{"id":"432747","messageId":"pull.1014.v5.git.1628977230247.gitgitgadget@gmail.com","threadId":"56260","inReplyTo":"pull.1014.v4.git.1628769334197.gitgitgadget@gmail.com","subject":"[PATCH v5] commit: restore --edit when combined with --fixup","fromName":"Joel Klinghed via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-14T21:40:30Z","receivedAt":"2021-08-14T21:56:03Z","isPatch":true,"sender":{"key":"the_jk@spawned.biz","avatar":"https://avatars.githubusercontent.com/u/977644?v=4"},"body":"From: Joel Klinghed <the_jk@spawned.biz>\n\nRecent changes to --fixup, adding amend suboption, caused the\n--edit flag to be ignored as use_editor was always set to zero.\n\nRestore edit_flag having higher priority than fixup_message when\ndeciding the value of use_editor by moving the edit flag condition\nlater in the method.\n\nSigned-off-by: Joel Klinghed <the_jk@spawned.biz>\n---\n    commit: restore --edit when combined with --fixup\n    \n    Recent changes to --fixup, adding amend suboption, caused the --edit\n    flag to be ignored as use_editor was always set to zero.\n    \n    Restore edit_flag having higher priority than fixup_message when\n    deciding the value of use_editor by only changing the default if\n    edit_flag is not set.\n    \n    Changes since v1: Added test verifying that --fixup --edit brings up\n    editor.\n    \n    Changes since v2: Clarify if condition and use write_script helper in\n    test.\n    \n    Changes since v3: Simplify test.\n    \n    Changes since v4: Using cleaner fix by Phillip Wood instead and added an\n    explicit verification to a test for --fixup without --edit.\n    \n    Signed-off-by: Joel Klinghed the_jk@spawned.biz\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1014%2Fthejk%2Ffixup_edit-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1014/thejk/fixup_edit-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/1014\n\nRange-diff vs v4:\n\n 1:  0c0cb647e03 ! 1:  1c608daf0cd commit: restore --edit when combined with --fixup\n     @@ Commit message\n          --edit flag to be ignored as use_editor was always set to zero.\n      \n          Restore edit_flag having higher priority than fixup_message when\n     -    deciding the value of use_editor by only changing the default\n     -    if edit_flag is not set.\n     +    deciding the value of use_editor by moving the edit flag condition\n     +    later in the method.\n      \n          Signed-off-by: Joel Klinghed <the_jk@spawned.biz>\n      \n       ## builtin/commit.c ##\n      @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *argv[],\n     - \t\t} else {\n     - \t\t\tfixup_commit = fixup_message;\n     - \t\t\tfixup_prefix = \"fixup\";\n     --\t\t\tuse_editor = 0;\n     -+\t\t\tif (edit_flag < 0)\n     -+\t\t\t\tuse_editor = 0;\n     + \n     + \tif (logfile || have_option_m || use_message)\n     + \t\tuse_editor = 0;\n     +-\tif (0 <= edit_flag)\n     +-\t\tuse_editor = edit_flag;\n     + \n     + \t/* Sanity check options */\n     + \tif (amend && !current_head)\n     +@@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *argv[],\n       \t\t}\n       \t}\n       \n     ++\tif (0 <= edit_flag)\n     ++\t\tuse_editor = edit_flag;\n     ++\n     + \tcleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);\n     + \n     + \thandle_untracked_files_arg(s);\n      \n       ## t/t7500-commit-template-squash-signoff.sh ##\n     +@@ t/t7500-commit-template-squash-signoff.sh: EOF\n     + \n     + test_expect_success 'commit --fixup provides correct one-line commit message' '\n     + \tcommit_for_rebase_autosquash_setup &&\n     +-\tgit commit --fixup HEAD~1 &&\n     ++\tEDITOR=\"echo ignored >>\" git commit --fixup HEAD~1 &&\n     + \tcommit_msg_is \"fixup! target message subject line\"\n     + '\n     + \n      @@ t/t7500-commit-template-squash-signoff.sh: test_expect_success 'commit --fixup -m\"something\" -m\"extra\"' '\n       \n       extra\"\n\n\n builtin/commit.c                          | 5 +++--\n t/t7500-commit-template-squash-signoff.sh | 9 ++++++++-\n 2 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 190d215d43b..854903ad113 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1246,8 +1246,6 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \n \tif (logfile || have_option_m || use_message)\n \t\tuse_editor = 0;\n-\tif (0 <= edit_flag)\n-\t\tuse_editor = edit_flag;\n \n \t/* Sanity check options */\n \tif (amend && !current_head)\n@@ -1337,6 +1335,9 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \t\t}\n \t}\n \n+\tif (0 <= edit_flag)\n+\t\tuse_editor = edit_flag;\n+\n \tcleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);\n \n \thandle_untracked_files_arg(s);\ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 7d02f79c0de..8515736003a 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -270,7 +270,7 @@ EOF\n \n test_expect_success 'commit --fixup provides correct one-line commit message' '\n \tcommit_for_rebase_autosquash_setup &&\n-\tgit commit --fixup HEAD~1 &&\n+\tEDITOR=\"echo ignored >>\" git commit --fixup HEAD~1 &&\n \tcommit_msg_is \"fixup! target message subject line\"\n '\n \n@@ -281,6 +281,13 @@ test_expect_success 'commit --fixup -m\"something\" -m\"extra\"' '\n \n extra\"\n '\n+test_expect_success 'commit --fixup --edit' '\n+\tcommit_for_rebase_autosquash_setup &&\n+\tEDITOR=\"printf \\\"something\\nextra\\\" >>\" git commit --fixup HEAD~1 --edit &&\n+\tcommit_msg_is \"fixup! target message subject linesomething\n+extra\"\n+'\n+\n get_commit_msg () {\n \trev=\"$1\" &&\n \tgit log -1 --pretty=format:\"%B\" \"$rev\"\n\nbase-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb\n-- \ngitgitgadget\n"},{"id":"432934","messageId":"24356b20-45e2-5d60-434c-4f04d1b2055c@gmail.com","threadId":"56260","inReplyTo":"pull.1014.v5.git.1628977230247.gitgitgadget@gmail.com","subject":"Re: [PATCH v5] commit: restore --edit when combined with --fixup","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-08-17T10:06:58Z","receivedAt":"2021-08-17T10:07:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Joel\n\nOn 14/08/2021 22:40, Joel Klinghed via GitGitGadget wrote:\n> From: Joel Klinghed <the_jk@spawned.biz>\n> \n> Recent changes to --fixup, adding amend suboption, caused the\n> --edit flag to be ignored as use_editor was always set to zero.\n> \n> Restore edit_flag having higher priority than fixup_message when\n> deciding the value of use_editor by moving the edit flag condition\n> later in the method.\n\nThis version looks good, thanks for revising it\n\nBest Wishes\n\nPhillip\n\n> \n> Signed-off-by: Joel Klinghed <the_jk@spawned.biz>\n> ---\n>      commit: restore --edit when combined with --fixup\n>      \n>      Recent changes to --fixup, adding amend suboption, caused the --edit\n>      flag to be ignored as use_editor was always set to zero.\n>      \n>      Restore edit_flag having higher priority than fixup_message when\n>      deciding the value of use_editor by only changing the default if\n>      edit_flag is not set.\n>      \n>      Changes since v1: Added test verifying that --fixup --edit brings up\n>      editor.\n>      \n>      Changes since v2: Clarify if condition and use write_script helper in\n>      test.\n>      \n>      Changes since v3: Simplify test.\n>      \n>      Changes since v4: Using cleaner fix by Phillip Wood instead and added an\n>      explicit verification to a test for --fixup without --edit.\n>      \n>      Signed-off-by: Joel Klinghed the_jk@spawned.biz\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1014%2Fthejk%2Ffixup_edit-v5\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1014/thejk/fixup_edit-v5\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1014\n> \n> Range-diff vs v4:\n> \n>   1:  0c0cb647e03 ! 1:  1c608daf0cd commit: restore --edit when combined with --fixup\n>       @@ Commit message\n>            --edit flag to be ignored as use_editor was always set to zero.\n>        \n>            Restore edit_flag having higher priority than fixup_message when\n>       -    deciding the value of use_editor by only changing the default\n>       -    if edit_flag is not set.\n>       +    deciding the value of use_editor by moving the edit flag condition\n>       +    later in the method.\n>        \n>            Signed-off-by: Joel Klinghed <the_jk@spawned.biz>\n>        \n>         ## builtin/commit.c ##\n>        @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *argv[],\n>       - \t\t} else {\n>       - \t\t\tfixup_commit = fixup_message;\n>       - \t\t\tfixup_prefix = \"fixup\";\n>       --\t\t\tuse_editor = 0;\n>       -+\t\t\tif (edit_flag < 0)\n>       -+\t\t\t\tuse_editor = 0;\n>       +\n>       + \tif (logfile || have_option_m || use_message)\n>       + \t\tuse_editor = 0;\n>       +-\tif (0 <= edit_flag)\n>       +-\t\tuse_editor = edit_flag;\n>       +\n>       + \t/* Sanity check options */\n>       + \tif (amend && !current_head)\n>       +@@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *argv[],\n>         \t\t}\n>         \t}\n>         \n>       ++\tif (0 <= edit_flag)\n>       ++\t\tuse_editor = edit_flag;\n>       ++\n>       + \tcleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);\n>       +\n>       + \thandle_untracked_files_arg(s);\n>        \n>         ## t/t7500-commit-template-squash-signoff.sh ##\n>       +@@ t/t7500-commit-template-squash-signoff.sh: EOF\n>       +\n>       + test_expect_success 'commit --fixup provides correct one-line commit message' '\n>       + \tcommit_for_rebase_autosquash_setup &&\n>       +-\tgit commit --fixup HEAD~1 &&\n>       ++\tEDITOR=\"echo ignored >>\" git commit --fixup HEAD~1 &&\n>       + \tcommit_msg_is \"fixup! target message subject line\"\n>       + '\n>       +\n>        @@ t/t7500-commit-template-squash-signoff.sh: test_expect_success 'commit --fixup -m\"something\" -m\"extra\"' '\n>         \n>         extra\"\n> \n> \n>   builtin/commit.c                          | 5 +++--\n>   t/t7500-commit-template-squash-signoff.sh | 9 ++++++++-\n>   2 files changed, 11 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 190d215d43b..854903ad113 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1246,8 +1246,6 @@ static int parse_and_validate_options(int argc, const char *argv[],\n>   \n>   \tif (logfile || have_option_m || use_message)\n>   \t\tuse_editor = 0;\n> -\tif (0 <= edit_flag)\n> -\t\tuse_editor = edit_flag;\n>   \n>   \t/* Sanity check options */\n>   \tif (amend && !current_head)\n> @@ -1337,6 +1335,9 @@ static int parse_and_validate_options(int argc, const char *argv[],\n>   \t\t}\n>   \t}\n>   \n> +\tif (0 <= edit_flag)\n> +\t\tuse_editor = edit_flag;\n> +\n>   \tcleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);\n>   \n>   \thandle_untracked_files_arg(s);\n> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\n> index 7d02f79c0de..8515736003a 100755\n> --- a/t/t7500-commit-template-squash-signoff.sh\n> +++ b/t/t7500-commit-template-squash-signoff.sh\n> @@ -270,7 +270,7 @@ EOF\n>   \n>   test_expect_success 'commit --fixup provides correct one-line commit message' '\n>   \tcommit_for_rebase_autosquash_setup &&\n> -\tgit commit --fixup HEAD~1 &&\n> +\tEDITOR=\"echo ignored >>\" git commit --fixup HEAD~1 &&\n>   \tcommit_msg_is \"fixup! target message subject line\"\n>   '\n>   \n> @@ -281,6 +281,13 @@ test_expect_success 'commit --fixup -m\"something\" -m\"extra\"' '\n>   \n>   extra\"\n>   '\n> +test_expect_success 'commit --fixup --edit' '\n> +\tcommit_for_rebase_autosquash_setup &&\n> +\tEDITOR=\"printf \\\"something\\nextra\\\" >>\" git commit --fixup HEAD~1 --edit &&\n> +\tcommit_msg_is \"fixup! target message subject linesomething\n> +extra\"\n> +'\n> +\n>   get_commit_msg () {\n>   \trev=\"$1\" &&\n>   \tgit log -1 --pretty=format:\"%B\" \"$rev\"\n> \n> base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb\n> \n\n"}]}