From: Phillip Wood Date: Mon, 07 Aug 2023 09:59:41 GMT Subject: Re: [PATCH] rebase --skip: fix commit message clean up when skipping squash Message-ID: In-Reply-To: On 03/08/2023 21:20, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" writes: > >> ... The test is also updated to explicitly check >> the commit messages rather than relying on grep to ensure they do not >> contain any stay commit headers. > > "stay" -> "stray" presumably. I see this is in next already, thanks for fixing that and the other typo pointed out by Marc >> To check the commit message the function test_commit_message() is moved >> from t3437-rebase-fixup-options.sh to test-lib.sh. As the function is >> now publicly available it is updated to provide better error detection >> and avoid overwriting the commonly used files "actual" and "expect". >> Support for reading the expected commit message from stdin is also >> added. > > It may make it cleaner to do the refactoring as a separate > preparatory patch, but the end-result is not so large from > the diffstat below, so it probably is OK. I was in two minds about splitting out the refactoring, but in the end I decided that as I was adding the ability to read the expected message from stdin for this commit it was easier include it all here rather than splitting it out. > I am not sure if deviating from expect vs actual is such a good > idea. It is not like use of two temporary files are transparent to > the caller of the new test helper---indeed, expect and actual are > likely to be used by the caller in tests that comes before or after > the ones that use test_commit_message, and by using a pair of files > that are different, the caller will now see two extra untracked > files left in the working tree. That's true, looking at test-lib-functions.sh it seems I unwittingly followed the example of test_cmp_config() which uses {actual,expect}.config when it calls test_cmp(). Anyway as this is in next I assume you're happy enough with the implementation as it stands. Best Wishes Phillip > The only case such a renaming could help callers is when they do > > cat >expect <<\-EOF && > here to prepare some outcome > EOF > git do-something-to-make-commit && > test_commit_message HEAD <<\-EOF && > here is what we expect to see in HEAD > EOF > git some-other-thing >actual && > test_cmp expect actual > > as use of files other than expect/actual in test_commit_message will > avoid stomping on the "expect" file that was already prepared. > > I suspect that it would be rare, and something we can fix when need > arises by allowing test_commit_message to accept an option to use > non-standard filenames for its temporaries. The current callers, > both the existing ones in t3437 and the new ones added by this > patch, would not benefit. The only externally visible side effect > is that the existing ones will have two extra untracked files in > their working tree. > >> Signed-off-by: Phillip Wood >> --- >> rebase --skip: fix commit message clean up when skipping squash >> >> This patch is based on maint. >> ... >> sequencer.c | 26 +++++++++++---- >> t/t3418-rebase-continue.sh | 58 +++++++++++++++++++++++---------- >> t/t3437-rebase-fixup-options.sh | 15 --------- >> t/test-lib-functions.sh | 33 +++++++++++++++++++ >> 4 files changed, 93 insertions(+), 39 deletions(-) > > OK. > >> diff --git a/sequencer.c b/sequencer.c >> index bceb6abcb6c..af271ab6fbd 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -5038,19 +5038,31 @@ static int commit_staged_changes(struct repository *r, >> * We need to update the squash message to skip >> * the latest commit message. >> */ >> + int res = 0; >> struct commit *commit; >> + const char *msg; >> const char *path = rebase_path_squash_msg(); >> const char *encoding = get_commit_output_encoding(); >> >> - if (parse_head(r, &commit) || >> - !(p = repo_logmsg_reencode(r, commit, NULL, encoding)) || >> - write_message(p, strlen(p), path, 0)) { >> - repo_unuse_commit_buffer(r, commit, p); >> - return error(_("could not write file: " >> + if (parse_head(r, &commit)) >> + return error(_("could not parse HEAD")); >> + >> + p = repo_logmsg_reencode(r, commit, NULL, encoding); >> + if (!p) { >> + res = error(_("could not parse commit %s"), >> + oid_to_hex(&commit->object.oid)); >> + goto unuse_commit_buffer; >> + } >> + find_commit_subject(p, &msg); >> + if (write_message(msg, strlen(msg), path, 0)) { >> + res = error(_("could not write file: " >> "'%s'"), path); >> + goto unuse_commit_buffer; >> } >> - repo_unuse_commit_buffer(r, >> - commit, p); >> + unuse_commit_buffer: >> + repo_unuse_commit_buffer(r, commit, p); >> + if (res) >> + return res; >> } >> } > > Just as described in the proposed log message. Looking good. > > Will queue. Thanks.