On 03/08/2023 21:20, Junio C Hamano wrote:
"Phillip Wood via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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 <phillip.wood@xxxxxxxxxxxxx>
---
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.
