{"thread":{"id":"60060","subject":"[PATCH] rebase --skip: fix commit message clean up when skipping squash","startedAt":"2023-08-03T13:12:26Z","lastAt":"2023-08-07T16:16:43Z","messageCount":6,"participants":["Phillip Wood via GitGitGadget","Junio C Hamano","Marc Branchaud","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"480121","messageId":"pull.1558.git.git.1691068176051.gitgitgadget@gmail.com","threadId":"60060","inReplyTo":null,"subject":"[PATCH] rebase --skip: fix commit message clean up when skipping squash","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-03T13:09:35Z","receivedAt":"2023-08-03T13:12:26Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nDuring a series of \"fixup\" and/or \"squash\" commands, the interactive\nrebase accumulates a commit message from all the commits that are being\nsquashed together. If one of the commits has conflicts when it is picked\nand the user chooses to skip that commit then we need to remove that\ncommit's message from accumulated messages.  To do this 15ef69314d5\n(rebase --skip: clean up commit message after a failed fixup/squash,\n2018-04-27) updated commit_staged_changes() to reset the accumulated\nmessage to the commit message of HEAD (which does not contain the\nmessage from the skipped commit) when the last command was \"fixup\" or\n\"squash\" and there are no staged changes. Unfortunately the code to do\nthis contains two bugs.\n\n(1) If parse_head() fails we pass an invalid pointer to\n    unuse_commit_buffer().\n\n(2) The reconstructed message uses the entire commit buffer from HEAD\n    including the headers, rather than just the commit message.\n\nThe fist issue is fixed by splitting up the \"if\" condition into several\nstatements each with its own error handling. The second issue is fixed\nby finding the start of the commit message within the commit buffer\nusing find_commit_subject().\n\nThe existing test added by 15ef69314d5 is modified to show the effect of\nthis bug.  The bug is triggered when skipping the first command in the\nchain (as the test does before this commit) but the effect is hidden\nbecause opts->current_fixup_count is set to zero which leads\nupdate_squash_messages() to recreate the squash message file from\nscratch overwriting the bad message created by\ncommit_staged_changes(). The test is also updated to explicitly check\nthe commit messages rather than relying on grep to ensure they do not\ncontain any stay commit headers.\n\nTo check the commit message the function test_commit_message() is moved\nfrom t3437-rebase-fixup-options.sh to test-lib.sh. As the function is\nnow publicly available it is updated to provide better error detection\nand avoid overwriting the commonly used files \"actual\" and \"expect\".\nSupport for reading the expected commit message from stdin is also\nadded.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n    rebase --skip: fix commit message clean up when skipping squash\n    \n    This patch is based on maint.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1558%2Fphillipwood%2Fwip%2Frebase-skip-fix-message-cleanup-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1558/phillipwood/wip/rebase-skip-fix-message-cleanup-v1\nPull-Request: https://github.com/git/git/pull/1558\n\n sequencer.c                     | 26 +++++++++++----\n t/t3418-rebase-continue.sh      | 58 +++++++++++++++++++++++----------\n t/t3437-rebase-fixup-options.sh | 15 ---------\n t/test-lib-functions.sh         | 33 +++++++++++++++++++\n 4 files changed, 93 insertions(+), 39 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex bceb6abcb6c..af271ab6fbd 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5038,19 +5038,31 @@ static int commit_staged_changes(struct repository *r,\n \t\t\t\t * We need to update the squash message to skip\n \t\t\t\t * the latest commit message.\n \t\t\t\t */\n+\t\t\t\tint res = 0;\n \t\t\t\tstruct commit *commit;\n+\t\t\t\tconst char *msg;\n \t\t\t\tconst char *path = rebase_path_squash_msg();\n \t\t\t\tconst char *encoding = get_commit_output_encoding();\n \n-\t\t\t\tif (parse_head(r, &commit) ||\n-\t\t\t\t    !(p = repo_logmsg_reencode(r, commit, NULL, encoding)) ||\n-\t\t\t\t    write_message(p, strlen(p), path, 0)) {\n-\t\t\t\t\trepo_unuse_commit_buffer(r, commit, p);\n-\t\t\t\t\treturn error(_(\"could not write file: \"\n+\t\t\t\tif (parse_head(r, &commit))\n+\t\t\t\t\treturn error(_(\"could not parse HEAD\"));\n+\n+\t\t\t\tp = repo_logmsg_reencode(r, commit, NULL, encoding);\n+\t\t\t\tif (!p)  {\n+\t\t\t\t\tres = error(_(\"could not parse commit %s\"),\n+\t\t\t\t\t\t    oid_to_hex(&commit->object.oid));\n+\t\t\t\t\tgoto unuse_commit_buffer;\n+\t\t\t\t}\n+\t\t\t\tfind_commit_subject(p, &msg);\n+\t\t\t\tif (write_message(msg, strlen(msg), path, 0)) {\n+\t\t\t\t\tres = error(_(\"could not write file: \"\n \t\t\t\t\t\t       \"'%s'\"), path);\n+\t\t\t\t\tgoto unuse_commit_buffer;\n \t\t\t\t}\n-\t\t\t\trepo_unuse_commit_buffer(r,\n-\t\t\t\t\t\t\t commit, p);\n+\t\t\tunuse_commit_buffer:\n+\t\t\t\trepo_unuse_commit_buffer(r, commit, p);\n+\t\t\t\tif (res)\n+\t\t\t\t\treturn res;\n \t\t\t}\n \t\t}\n \ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex 2d0789e554b..fb7b68990cc 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -115,15 +115,23 @@ test_expect_success '--skip after failed fixup cleans commit message' '\n \ttest_when_finished \"test_might_fail git rebase --abort\" &&\n \tgit checkout -b with-conflicting-fixup &&\n \ttest_commit wants-fixup &&\n-\ttest_commit \"fixup! wants-fixup\" wants-fixup.t 1 wants-fixup-1 &&\n-\ttest_commit \"fixup! wants-fixup\" wants-fixup.t 2 wants-fixup-2 &&\n-\ttest_commit \"fixup! wants-fixup\" wants-fixup.t 3 wants-fixup-3 &&\n+\ttest_commit \"fixup 1\" wants-fixup.t 1 wants-fixup-1 &&\n+\ttest_commit \"fixup 2\" wants-fixup.t 2 wants-fixup-2 &&\n+\ttest_commit \"fixup 3\" wants-fixup.t 3 wants-fixup-3 &&\n \ttest_must_fail env FAKE_LINES=\"1 fixup 2 squash 4\" \\\n \t\tgit rebase -i HEAD~4 &&\n \n \t: now there is a conflict, and comments in the commit message &&\n-\tgit show HEAD >out &&\n-\tgrep \"fixup! wants-fixup\" out &&\n+\ttest_commit_message HEAD <<-\\EOF &&\n+\t# This is a combination of 2 commits.\n+\t# This is the 1st commit message:\n+\n+\twants-fixup\n+\n+\t# The commit message #2 will be skipped:\n+\n+\t# fixup 1\n+\tEOF\n \n \t: skip and continue &&\n \techo \"cp \\\"\\$1\\\" .git/copy.txt\" | write_script copy-editor.sh &&\n@@ -133,33 +141,49 @@ test_expect_success '--skip after failed fixup cleans commit message' '\n \ttest_path_is_missing .git/copy.txt &&\n \n \t: now the comments in the commit message should have been cleaned up &&\n-\tgit show HEAD >out &&\n-\t! grep \"fixup! wants-fixup\" out &&\n+\ttest_commit_message HEAD -m wants-fixup &&\n \n \t: now, let us ensure that \"squash\" is handled correctly &&\n \tgit reset --hard wants-fixup-3 &&\n-\ttest_must_fail env FAKE_LINES=\"1 squash 4 squash 2 squash 4\" \\\n+\ttest_must_fail env FAKE_LINES=\"1 squash 2 squash 1 squash 3 squash 1\" \\\n \t\tgit rebase -i HEAD~4 &&\n \n-\t: the first squash failed, but there are two more in the chain &&\n+\t: the second squash failed, but there are two more in the chain &&\n \t(test_set_editor \"$PWD/copy-editor.sh\" &&\n \t test_must_fail git rebase --skip) &&\n \n \t: not the final squash, no need to edit the commit message &&\n \ttest_path_is_missing .git/copy.txt &&\n \n-\t: The first squash was skipped, therefore: &&\n-\tgit show HEAD >out &&\n-\ttest_i18ngrep \"# This is a combination of 2 commits\" out &&\n-\ttest_i18ngrep \"# This is the commit message #2:\" out &&\n+\t: The first and third squashes succeeded, therefore: &&\n+\ttest_commit_message HEAD <<-\\EOF &&\n+\t# This is a combination of 3 commits.\n+\t# This is the 1st commit message:\n+\n+\twants-fixup\n+\n+\t# This is the commit message #2:\n+\n+\tfixup 1\n+\n+\t# This is the commit message #3:\n+\n+\tfixup 2\n+\tEOF\n \n \t(test_set_editor \"$PWD/copy-editor.sh\" && git rebase --skip) &&\n-\tgit show HEAD >out &&\n-\ttest_i18ngrep ! \"# This is a combination\" out &&\n+\ttest_commit_message HEAD <<-\\EOF &&\n+\twants-fixup\n+\n+\tfixup 1\n+\n+\tfixup 2\n+\tEOF\n \n \t: Final squash failed, but there was still a squash &&\n-\ttest_i18ngrep \"# This is a combination of 2 commits\" .git/copy.txt &&\n-\ttest_i18ngrep \"# This is the commit message #2:\" .git/copy.txt\n+\thead -n1 .git/copy.txt >first-line &&\n+\ttest_i18ngrep \"# This is a combination of 3 commits\" first-line &&\n+\ttest_i18ngrep \"# This is the commit message #3:\" .git/copy.txt\n '\n \n test_expect_success 'setup rerere database' '\ndiff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh\nindex dd3b301fa7a..7929e2e2e3a 100755\n--- a/t/t3437-rebase-fixup-options.sh\n+++ b/t/t3437-rebase-fixup-options.sh\n@@ -21,21 +21,6 @@ TEST_PASSES_SANITIZE_LEAK=true\n \n EMPTY=\"\"\n \n-# test_commit_message <rev> -m <msg>\n-# test_commit_message <rev> <path>\n-# Verify that the commit message of <rev> matches\n-# <msg> or the content of <path>.\n-test_commit_message () {\n-\tgit show --no-patch --pretty=format:%B \"$1\" >actual &&\n-\tcase \"$2\" in\n-\t-m)\n-\t\techo \"$3\" >expect &&\n-\t\ttest_cmp expect actual ;;\n-\t*)\n-\t\ttest_cmp \"$2\" actual ;;\n-\tesac\n-}\n-\n get_author () {\n \trev=\"$1\" &&\n \tgit log -1 --pretty=format:\"%an %ae %at\" \"$rev\"\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 6e19ebc922a..d8a52334eeb 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1273,6 +1273,39 @@ test_cmp_rev () {\n \tfi\n }\n \n+# Tests that a commit message matches the expected text\n+#\n+# Usage: test_commit_message <rev> [-m <msg> | <file>]\n+#\n+# When using \"-m\" <msg> will have a line feed appended. If the second\n+# argument is omitted then the expected message is read from stdin.\n+\n+test_commit_message () {\n+\tlocal msg_file=expect.msg\n+\n+\tcase $# in\n+\t3)\n+\t\tif test \"$2\" = \"-m\"\n+\t\tthen\n+\t\t\tprintf \"%s\\n\" \"$3\" >\"$msg_file\"\n+\t\telse\n+\t\t\tBUG \"Usage: test_commit_message <rev> [-m <message> | <file>]\"\n+\t\tfi\n+\t\t;;\n+\t2)\n+\t\tmsg_file=\"$2\"\n+\t\t;;\n+\t1)\n+\t\tcat >\"$msg_file\"\n+\t\t;;\n+\t*)\n+\t\tBUG \"Usage: test_commit_message <rev> [-m <message> | <file>]\"\n+\t\t;;\n+\tesac\n+\tgit show --no-patch --pretty=format:%B \"$1\" -- >actual.msg &&\n+\ttest_cmp \"$msg_file\" actual.msg\n+}\n+\n # Compare paths respecting core.ignoreCase\n test_cmp_fspath () {\n \tif test \"x$1\" = \"x$2\"\n\nbase-commit: fb7d80edcae482f4fa5d4be0227dc3054734e5f3\n-- \ngitgitgadget\n"},{"id":"480134","messageId":"xmqqedkj515a.fsf@gitster.g","threadId":"60060","inReplyTo":"pull.1558.git.git.1691068176051.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase --skip: fix commit message clean up when skipping squash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-03T20:20:01Z","receivedAt":"2023-08-03T20:20:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> ... The test is also updated to explicitly check\n> the commit messages rather than relying on grep to ensure they do not\n> contain any stay commit headers.\n\n\"stay\" -> \"stray\" presumably.\n\n> To check the commit message the function test_commit_message() is moved\n> from t3437-rebase-fixup-options.sh to test-lib.sh. As the function is\n> now publicly available it is updated to provide better error detection\n> and avoid overwriting the commonly used files \"actual\" and \"expect\".\n> Support for reading the expected commit message from stdin is also\n> added.\n\nIt may make it cleaner to do the refactoring as a separate\npreparatory patch, but the end-result is not so large from\nthe diffstat below, so it probably is OK.\n\nI am not sure if deviating from expect vs actual is such a good\nidea.  It is not like use of two temporary files are transparent to\nthe caller of the new test helper---indeed, expect and actual are\nlikely to be used by the caller in tests that comes before or after\nthe ones that use test_commit_message, and by using a pair of files\nthat are different, the caller will now see two extra untracked\nfiles left in the working tree.\n\nThe only case such a renaming could help callers is when they do\n\n\tcat >expect <<\\-EOF &&\n\there to prepare some outcome\n\tEOF\n\tgit do-something-to-make-commit &&\n\ttest_commit_message HEAD <<\\-EOF &&\n\there is what we expect to see in HEAD\n\tEOF\n\tgit some-other-thing >actual &&\n\ttest_cmp expect actual\n\t\nas use of files other than expect/actual in test_commit_message will\navoid stomping on the \"expect\" file that was already prepared.\n\nI suspect that it would be rare, and something we can fix when need\narises by allowing test_commit_message to accept an option to use\nnon-standard filenames for its temporaries.  The current callers,\nboth the existing ones in t3437 and the new ones added by this\npatch, would not benefit.  The only externally visible side effect\nis that the existing ones will have two extra untracked files in\ntheir working tree.\n\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>     rebase --skip: fix commit message clean up when skipping squash\n>     \n>     This patch is based on maint.\n> ...\n>  sequencer.c                     | 26 +++++++++++----\n>  t/t3418-rebase-continue.sh      | 58 +++++++++++++++++++++++----------\n>  t/t3437-rebase-fixup-options.sh | 15 ---------\n>  t/test-lib-functions.sh         | 33 +++++++++++++++++++\n>  4 files changed, 93 insertions(+), 39 deletions(-)\n\nOK.\n\n> diff --git a/sequencer.c b/sequencer.c\n> index bceb6abcb6c..af271ab6fbd 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -5038,19 +5038,31 @@ static int commit_staged_changes(struct repository *r,\n>  \t\t\t\t * We need to update the squash message to skip\n>  \t\t\t\t * the latest commit message.\n>  \t\t\t\t */\n> +\t\t\t\tint res = 0;\n>  \t\t\t\tstruct commit *commit;\n> +\t\t\t\tconst char *msg;\n>  \t\t\t\tconst char *path = rebase_path_squash_msg();\n>  \t\t\t\tconst char *encoding = get_commit_output_encoding();\n>  \n> -\t\t\t\tif (parse_head(r, &commit) ||\n> -\t\t\t\t    !(p = repo_logmsg_reencode(r, commit, NULL, encoding)) ||\n> -\t\t\t\t    write_message(p, strlen(p), path, 0)) {\n> -\t\t\t\t\trepo_unuse_commit_buffer(r, commit, p);\n> -\t\t\t\t\treturn error(_(\"could not write file: \"\n> +\t\t\t\tif (parse_head(r, &commit))\n> +\t\t\t\t\treturn error(_(\"could not parse HEAD\"));\n> +\n> +\t\t\t\tp = repo_logmsg_reencode(r, commit, NULL, encoding);\n> +\t\t\t\tif (!p)  {\n> +\t\t\t\t\tres = error(_(\"could not parse commit %s\"),\n> +\t\t\t\t\t\t    oid_to_hex(&commit->object.oid));\n> +\t\t\t\t\tgoto unuse_commit_buffer;\n> +\t\t\t\t}\n> +\t\t\t\tfind_commit_subject(p, &msg);\n> +\t\t\t\tif (write_message(msg, strlen(msg), path, 0)) {\n> +\t\t\t\t\tres = error(_(\"could not write file: \"\n>  \t\t\t\t\t\t       \"'%s'\"), path);\n> +\t\t\t\t\tgoto unuse_commit_buffer;\n>  \t\t\t\t}\n> -\t\t\t\trepo_unuse_commit_buffer(r,\n> -\t\t\t\t\t\t\t commit, p);\n> +\t\t\tunuse_commit_buffer:\n> +\t\t\t\trepo_unuse_commit_buffer(r, commit, p);\n> +\t\t\t\tif (res)\n> +\t\t\t\t\treturn res;\n>  \t\t\t}\n>  \t\t}\n\nJust as described in the proposed log message.  Looking good.\n\nWill queue.  Thanks.\n"},{"id":"480136","messageId":"32c8a181-76aa-5c90-427a-53b1ccdec8a3@xiplink.com","threadId":"60060","inReplyTo":"pull.1558.git.git.1691068176051.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase --skip: fix commit message clean up when skipping squash","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2023-08-03T20:38:57Z","receivedAt":"2023-08-03T20:39:06Z","isPatch":true,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"On 2023-08-03 09:09, Phillip Wood via GitGitGadget wrote:\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n> \n> During a series of \"fixup\" and/or \"squash\" commands, the interactive\n> rebase accumulates a commit message from all the commits that are being\n> squashed together. If one of the commits has conflicts when it is picked\n> and the user chooses to skip that commit then we need to remove that\n> commit's message from accumulated messages.  To do this 15ef69314d5\n> (rebase --skip: clean up commit message after a failed fixup/squash,\n> 2018-04-27) updated commit_staged_changes() to reset the accumulated\n> message to the commit message of HEAD (which does not contain the\n> message from the skipped commit) when the last command was \"fixup\" or\n> \"squash\" and there are no staged changes. Unfortunately the code to do\n> this contains two bugs.\n> \n> (1) If parse_head() fails we pass an invalid pointer to\n>      unuse_commit_buffer().\n> \n> (2) The reconstructed message uses the entire commit buffer from HEAD\n>      including the headers, rather than just the commit message.\n> \n> The fist issue is fixed by splitting up the \"if\" condition into several\n\ns/fist/first/\n\n(I'm not qualified to review anything else about this patch, but I like \nits intention...  :D )\n\n\t\tM.\n"},{"id":"480209","messageId":"b43a5b46-f391-bd9e-4bf2-bfeaa3548f3e@gmail.com","threadId":"60060","inReplyTo":"xmqqedkj515a.fsf@gitster.g","subject":"Re: [PATCH] rebase --skip: fix commit message clean up when skipping squash","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-08-07T09:59:41Z","receivedAt":"2023-08-07T09:59:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 03/08/2023 21:20, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> ... The test is also updated to explicitly check\n>> the commit messages rather than relying on grep to ensure they do not\n>> contain any stay commit headers.\n> \n> \"stay\" -> \"stray\" presumably.\n\nI see this is in next already, thanks for fixing that and the other typo \npointed out by Marc\n\n>> To check the commit message the function test_commit_message() is moved\n>> from t3437-rebase-fixup-options.sh to test-lib.sh. As the function is\n>> now publicly available it is updated to provide better error detection\n>> and avoid overwriting the commonly used files \"actual\" and \"expect\".\n>> Support for reading the expected commit message from stdin is also\n>> added.\n> \n> It may make it cleaner to do the refactoring as a separate\n> preparatory patch, but the end-result is not so large from\n> the diffstat below, so it probably is OK.\n\nI was in two minds about splitting out the refactoring, but in the end I \ndecided that as I was adding the ability to read the expected message \nfrom stdin for this commit it was easier include it all here rather than \nsplitting it out.\n\n> I am not sure if deviating from expect vs actual is such a good\n> idea.  It is not like use of two temporary files are transparent to\n> the caller of the new test helper---indeed, expect and actual are\n> likely to be used by the caller in tests that comes before or after\n> the ones that use test_commit_message, and by using a pair of files\n> that are different, the caller will now see two extra untracked\n> files left in the working tree.\n\nThat's true, looking at test-lib-functions.sh it seems I unwittingly \nfollowed the example of test_cmp_config() which uses \n{actual,expect}.config when it calls test_cmp(). Anyway as this is in \nnext I assume you're happy enough with the implementation as it stands.\n\nBest Wishes\n\nPhillip\n\n> The only case such a renaming could help callers is when they do\n> \n> \tcat >expect <<\\-EOF &&\n> \there to prepare some outcome\n> \tEOF\n> \tgit do-something-to-make-commit &&\n> \ttest_commit_message HEAD <<\\-EOF &&\n> \there is what we expect to see in HEAD\n> \tEOF\n> \tgit some-other-thing >actual &&\n> \ttest_cmp expect actual\n> \t\n> as use of files other than expect/actual in test_commit_message will\n> avoid stomping on the \"expect\" file that was already prepared.\n> \n> I suspect that it would be rare, and something we can fix when need\n> arises by allowing test_commit_message to accept an option to use\n> non-standard filenames for its temporaries.  The current callers,\n> both the existing ones in t3437 and the new ones added by this\n> patch, would not benefit.  The only externally visible side effect\n> is that the existing ones will have two extra untracked files in\n> their working tree.\n> \n>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>> ---\n>>      rebase --skip: fix commit message clean up when skipping squash\n>>      \n>>      This patch is based on maint.\n>> ...\n>>   sequencer.c                     | 26 +++++++++++----\n>>   t/t3418-rebase-continue.sh      | 58 +++++++++++++++++++++++----------\n>>   t/t3437-rebase-fixup-options.sh | 15 ---------\n>>   t/test-lib-functions.sh         | 33 +++++++++++++++++++\n>>   4 files changed, 93 insertions(+), 39 deletions(-)\n> \n> OK.\n> \n>> diff --git a/sequencer.c b/sequencer.c\n>> index bceb6abcb6c..af271ab6fbd 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -5038,19 +5038,31 @@ static int commit_staged_changes(struct repository *r,\n>>   \t\t\t\t * We need to update the squash message to skip\n>>   \t\t\t\t * the latest commit message.\n>>   \t\t\t\t */\n>> +\t\t\t\tint res = 0;\n>>   \t\t\t\tstruct commit *commit;\n>> +\t\t\t\tconst char *msg;\n>>   \t\t\t\tconst char *path = rebase_path_squash_msg();\n>>   \t\t\t\tconst char *encoding = get_commit_output_encoding();\n>>   \n>> -\t\t\t\tif (parse_head(r, &commit) ||\n>> -\t\t\t\t    !(p = repo_logmsg_reencode(r, commit, NULL, encoding)) ||\n>> -\t\t\t\t    write_message(p, strlen(p), path, 0)) {\n>> -\t\t\t\t\trepo_unuse_commit_buffer(r, commit, p);\n>> -\t\t\t\t\treturn error(_(\"could not write file: \"\n>> +\t\t\t\tif (parse_head(r, &commit))\n>> +\t\t\t\t\treturn error(_(\"could not parse HEAD\"));\n>> +\n>> +\t\t\t\tp = repo_logmsg_reencode(r, commit, NULL, encoding);\n>> +\t\t\t\tif (!p)  {\n>> +\t\t\t\t\tres = error(_(\"could not parse commit %s\"),\n>> +\t\t\t\t\t\t    oid_to_hex(&commit->object.oid));\n>> +\t\t\t\t\tgoto unuse_commit_buffer;\n>> +\t\t\t\t}\n>> +\t\t\t\tfind_commit_subject(p, &msg);\n>> +\t\t\t\tif (write_message(msg, strlen(msg), path, 0)) {\n>> +\t\t\t\t\tres = error(_(\"could not write file: \"\n>>   \t\t\t\t\t\t       \"'%s'\"), path);\n>> +\t\t\t\t\tgoto unuse_commit_buffer;\n>>   \t\t\t\t}\n>> -\t\t\t\trepo_unuse_commit_buffer(r,\n>> -\t\t\t\t\t\t\t commit, p);\n>> +\t\t\tunuse_commit_buffer:\n>> +\t\t\t\trepo_unuse_commit_buffer(r, commit, p);\n>> +\t\t\t\tif (res)\n>> +\t\t\t\t\treturn res;\n>>   \t\t\t}\n>>   \t\t}\n> \n> Just as described in the proposed log message.  Looking good.\n> \n> Will queue.  Thanks.\n\n"},{"id":"480210","messageId":"b6690531-c5e7-5fdd-107a-22c3d2435b5e@gmail.com","threadId":"60060","inReplyTo":"32c8a181-76aa-5c90-427a-53b1ccdec8a3@xiplink.com","subject":"Re: [PATCH] rebase --skip: fix commit message clean up when skipping squash","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-08-07T10:00:49Z","receivedAt":"2023-08-07T10:00:55Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 03/08/2023 21:38, Marc Branchaud wrote:\n>> The fist issue is fixed by splitting up the \"if\" condition into several\n> \n> s/fist/first/\n> \n> (I'm not qualified to review anything else about this patch, but I like \n> its intention...  :D )\n\nThanks Marc, it looks like Junio fixed that typo when he applied the patch.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"480217","messageId":"xmqqzg32hlpd.fsf@gitster.g","threadId":"60060","inReplyTo":"b43a5b46-f391-bd9e-4bf2-bfeaa3548f3e@gmail.com","subject":"Re: [PATCH] rebase --skip: fix commit message clean up when skipping squash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-07T16:16:30Z","receivedAt":"2023-08-07T16:16:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n>> I am not sure if deviating from expect vs actual is such a good\n>> idea.  It is not like use of two temporary files are transparent to\n>> the caller of the new test helper---indeed, expect and actual are\n>> likely to be used by the caller in tests that comes before or after\n>> the ones that use test_commit_message, and by using a pair of files\n>> that are different, the caller will now see two extra untracked\n>> files left in the working tree.\n>\n> That's true, looking at test-lib-functions.sh it seems I unwittingly\n> followed the example of test_cmp_config() which uses\n> {actual,expect}.config when it calls test_cmp(). Anyway as this is in\n> next I assume you're happy enough with the implementation as it\n> stands.\n\nYou assumed too much ;-).  The above is a bad move, but is minor\nenough and it is not a show-stopper.  If you are aware of other bad\nexamples, clean-up patches after the dust settles would be very much\nwelcomed.\n\nThanks.\n"}]}