{"thread":{"id":"61333","subject":"[PATCH 1/5] sequencer: always free \"struct replay_opts\"","startedAt":"2024-04-18T13:14:44Z","lastAt":"2024-04-18T21:16:49Z","messageCount":10,"participants":["Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"493132","messageId":"9d96300b1c8ce3ab88affa571b2e29cd5b52160d.1713445918.git.phillip.wood@dunelm.org.uk","threadId":"61333","inReplyTo":"cover.1713445918.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 1/5] sequencer: always free \"struct replay_opts\"","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-18T13:14:05Z","receivedAt":"2024-04-18T13:14:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nsequencer_post_commit_cleanup() initializes an instance of \"struct\nreplay_opts\" but does not call replay_opts_release(). Currently this\ndoes not leak memory because the code paths called don't allocate any of\nthe struct members. That will change in the next commit so add call to\nreplay_opts_release() to prevent a memory leak in \"git commit\" that\nbreaks all of the leak free tests.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f49a871ac06..e4146b4cdfa 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2823,12 +2823,14 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)\n \t\t\tNULL, REF_NO_DEREF);\n \n \tif (!need_cleanup)\n-\t\treturn;\n+\t\tgoto out;\n \n \tif (!have_finished_the_last_pick())\n-\t\treturn;\n+\t\tgoto out;\n \n \tsequencer_remove_state(&opts);\n+out:\n+\treplay_opts_release(&opts);\n }\n \n static void todo_list_write_total_nr(struct todo_list *todo_list)\n-- \n2.44.0.661.ge68cfcc6c2f\n\n"},{"id":"493133","messageId":"cover.1713445918.git.phillip.wood@dunelm.org.uk","threadId":"61333","inReplyTo":null,"subject":"[PATCH 0/5] rebase -m: fix --signoff with conflicts","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-18T13:14:04Z","receivedAt":"2024-04-18T13:14:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nWhen rebasing with \"--signoff\" the commit created by \"rebase --continue\"\nafter resolving conflicts or editing a commit fails to add the\n\"Signed-off-by:\" trailer. This happens because the message from the\noriginal commit is reused instead of the one that would have been used\nif the sequencer had not stopped for the user interaction. This series\nfixes that by introducing an strbuf to hold the message which we then\nwrite to rebase_path_message() when stopping for user interaction.\n\nThe patches are structured as follows:\n\nPatches 1–3 add private \"struct replay_ctx\" to \"struct replay_opts\"\nand move the private members from the latter into the former. These\nchanges are largely mechanical.\n\nPatch 4 adds strbuf to \"struct replay_ctx\" to hold the commit\nmessage. This change is also largely mechanical.\n\nPatch 5 fixes the bug by using the changes in patch 4 to write the\ncorrect message to disc when stopping for user interaction.\n\nThis series is based on a merge of 'maint' and 'pw/t3428-cleanup'. In\nprinciple it would be passible to avoid the refactoring in patches 1–3\nand add a new member to \"struct replay_opts\" instead. However the\nchanges in those patches are largely mechanical so should be low-risk\nand they pave the way for more improvements and bug fixes in the\nfuture.\n\n\nbase-commit: 7bd541c104a2e383dd250f5f8e514785c4cabbc0\nPublished-As: https://github.com/phillipwood/git/releases/tag/pw%2Frebase-fix-signoff%2Fv1\nView-Changes-At: https://github.com/phillipwood/git/compare/7bd541c10...4c8f88437\nFetch-It-Via: git fetch https://github.com/phillipwood/git pw/rebase-fix-signoff/v1\n\nPhillip Wood (5):\n  sequencer: always free \"struct replay_opts\"\n  sequencer: start removing private fields from public API\n  sequencer: move current fixups to private context\n  sequencer: store commit message in private context\n  rebase -m: fix --signoff with conflicts\n\n sequencer.c               | 250 ++++++++++++++++++++++++--------------\n sequencer.h               |  11 +-\n t/t3428-rebase-signoff.sh |  96 ++++++++++++---\n t/t3434-rebase-i18n.sh    |   2 +-\n 4 files changed, 243 insertions(+), 116 deletions(-)\n\n\nbase-commit: 7bd541c104a2e383dd250f5f8e514785c4cabbc0\n-- \n2.44.0.661.ge68cfcc6c2f\n\n"},{"id":"493134","messageId":"2beedb4547013629a507d646d582ca6b3f420c2c.1713445918.git.phillip.wood@dunelm.org.uk","threadId":"61333","inReplyTo":"cover.1713445918.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 2/5] sequencer: start removing private fields from public API","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-18T13:14:06Z","receivedAt":"2024-04-18T13:14:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\n\"struct replay_opts\" has a number of fields that are for internal\nuse. While they are marked as private having them in a public struct is\na distraction for callers and means that every time the internal details\nare changed we have to recompile all the files that include sequencer.h\neven though the public API is unchanged. This commit starts the process\nof removing the private fields by adding an opaque pointer to a \"struct\nreplay_ctx\" to \"struct replay_opts\" and moving the \"reflog_message\"\nmember to the new private struct.\n\nThe sequencer currently updates the state files on disc each time it\nprocesses a command in the todo list. This is an artifact of the\nscripted implementation and makes the code hard to reason about as it is\nnot possible to get a complete view of the state in memory. In the\nfuture we will add new members to \"struct replay_ctx\" to remedy this and\navoid writing state to disc unless the sequencer stops for user\ninteraction.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c | 36 ++++++++++++++++++++++++++++++------\n sequencer.h |  6 +++++-\n 2 files changed, 35 insertions(+), 7 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex e4146b4cdfa..6ddf6a8aa1e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -207,6 +207,24 @@ static GIT_PATH_FUNC(rebase_path_no_reschedule_failed_exec, \"rebase-merge/no-res\n static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, \"rebase-merge/drop_redundant_commits\")\n static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, \"rebase-merge/keep_redundant_commits\")\n \n+/*\n+ * A 'struct replay_ctx' represents the private state of the sequencer.\n+ */\n+struct replay_ctx {\n+\t/*\n+\t * Stores the reflog message that will be used when creating a\n+\t * commit. Points to a static buffer and should not be free()'d.\n+\t */\n+\tconst char *reflog_message;\n+};\n+\n+struct replay_ctx* replay_ctx_new(void)\n+{\n+\tstruct replay_ctx *ctx = xcalloc(1, sizeof(*ctx));\n+\n+\treturn ctx;\n+}\n+\n /**\n  * A 'struct update_refs_record' represents a value in the update-refs\n  * list. We use a string_list to map refs to these (before, after) pairs.\n@@ -377,6 +395,7 @@ void replay_opts_release(struct replay_opts *opts)\n \tif (opts->revs)\n \t\trelease_revisions(opts->revs);\n \tfree(opts->revs);\n+\tfree(opts->ctx);\n }\n \n int sequencer_remove_state(struct replay_opts *opts)\n@@ -1054,6 +1073,7 @@ static int run_git_commit(const char *defmsg,\n \t\t\t  struct replay_opts *opts,\n \t\t\t  unsigned int flags)\n {\n+\tstruct replay_ctx *ctx = opts->ctx;\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \n \tif ((flags & CLEANUP_MSG) && (flags & VERBATIM_MSG))\n@@ -1071,7 +1091,7 @@ static int run_git_commit(const char *defmsg,\n \t\t\t     gpg_opt, gpg_opt);\n \t}\n \n-\tstrvec_pushf(&cmd.env, GIT_REFLOG_ACTION \"=%s\", opts->reflog_message);\n+\tstrvec_pushf(&cmd.env, GIT_REFLOG_ACTION \"=%s\", ctx->reflog_message);\n \n \tif (opts->committer_date_is_author_date)\n \t\tstrvec_pushf(&cmd.env, \"GIT_COMMITTER_DATE=%s\",\n@@ -1457,6 +1477,7 @@ static int try_to_commit(struct repository *r,\n \t\t\t struct replay_opts *opts, unsigned int flags,\n \t\t\t struct object_id *oid)\n {\n+\tstruct replay_ctx *ctx = opts->ctx;\n \tstruct object_id tree;\n \tstruct commit *current_head = NULL;\n \tstruct commit_list *parents = NULL;\n@@ -1618,7 +1639,7 @@ static int try_to_commit(struct repository *r,\n \t\tgoto out;\n \t}\n \n-\tif (update_head_with_reflog(current_head, oid, opts->reflog_message,\n+\tif (update_head_with_reflog(current_head, oid, ctx->reflog_message,\n \t\t\t\t    msg, &err)) {\n \t\tres = error(\"%s\", err.buf);\n \t\tgoto out;\n@@ -4725,11 +4746,12 @@ static int pick_one_commit(struct repository *r,\n \t\t\t   struct replay_opts *opts,\n \t\t\t   int *check_todo, int* reschedule)\n {\n+\tstruct replay_ctx *ctx = opts->ctx;\n \tint res;\n \tstruct todo_item *item = todo_list->items + todo_list->current;\n \tconst char *arg = todo_item_get_arg(todo_list, item);\n \tif (is_rebase_i(opts))\n-\t\topts->reflog_message = reflog_message(\n+\t\tctx->reflog_message = reflog_message(\n \t\t\topts, command_to_string(item->command), NULL);\n \n \tres = do_pick_commit(r, item, opts, is_final_fixup(todo_list),\n@@ -4786,9 +4808,10 @@ static int pick_commits(struct repository *r,\n \t\t\tstruct todo_list *todo_list,\n \t\t\tstruct replay_opts *opts)\n {\n+\tstruct replay_ctx *ctx = opts->ctx;\n \tint res = 0, reschedule = 0;\n \n-\topts->reflog_message = sequencer_reflog_action(opts);\n+\tctx->reflog_message = sequencer_reflog_action(opts);\n \tif (opts->allow_ff)\n \t\tassert(!(opts->signoff || opts->no_commit ||\n \t\t\t opts->record_origin || should_edit(opts) ||\n@@ -5205,6 +5228,7 @@ static int commit_staged_changes(struct repository *r,\n \n int sequencer_continue(struct repository *r, struct replay_opts *opts)\n {\n+\tstruct replay_ctx *ctx = opts->ctx;\n \tstruct todo_list todo_list = TODO_LIST_INIT;\n \tint res;\n \n@@ -5224,7 +5248,7 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n \t\t\tunlink(rebase_path_dropped());\n \t\t}\n \n-\t\topts->reflog_message = reflog_message(opts, \"continue\", NULL);\n+\t\tctx->reflog_message = reflog_message(opts, \"continue\", NULL);\n \t\tif (commit_staged_changes(r, opts, &todo_list)) {\n \t\t\tres = -1;\n \t\t\tgoto release_todo_list;\n@@ -5276,7 +5300,7 @@ static int single_pick(struct repository *r,\n \t\t\tTODO_PICK : TODO_REVERT;\n \titem.commit = cmit;\n \n-\topts->reflog_message = sequencer_reflog_action(opts);\n+\topts->ctx->reflog_message = sequencer_reflog_action(opts);\n \treturn do_pick_commit(r, &item, opts, 0, &check_todo);\n }\n \ndiff --git a/sequencer.h b/sequencer.h\nindex dcef7bb99c0..069f06400d2 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -29,6 +29,9 @@ enum commit_msg_cleanup_mode {\n \tCOMMIT_MSG_CLEANUP_ALL\n };\n \n+struct replay_ctx;\n+struct replay_ctx* replay_ctx_new(void);\n+\n struct replay_opts {\n \tenum replay_action action;\n \n@@ -78,13 +81,14 @@ struct replay_opts {\n \tstruct rev_info *revs;\n \n \t/* Private use */\n-\tconst char *reflog_message;\n+\tstruct replay_ctx *ctx;\n };\n #define REPLAY_OPTS_INIT {\t\t\t\\\n \t.edit = -1,\t\t\t\t\\\n \t.action = -1,\t\t\t\t\\\n \t.current_fixups = STRBUF_INIT,\t\t\\\n \t.xopts = STRVEC_INIT,\t\t\t\\\n+\t.ctx = replay_ctx_new(),\t\t\\\n }\n \n /*\n-- \n2.44.0.661.ge68cfcc6c2f\n\n"},{"id":"493135","messageId":"2e4a2168733cb226a6fd660cb0d0bd700a1e7d86.1713445918.git.phillip.wood@dunelm.org.uk","threadId":"61333","inReplyTo":"cover.1713445918.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 3/5] sequencer: move current fixups to private context","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-18T13:14:07Z","receivedAt":"2024-04-18T13:14:46Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nThe list of current fixups is an implementation detail of the sequencer\nand so it should not be stored in the public options struct.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c | 89 ++++++++++++++++++++++++++++++++++-------------------\n sequencer.h |  5 ---\n 2 files changed, 57 insertions(+), 37 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 6ddf6a8aa1e..edc49c94cce 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -211,17 +211,29 @@ static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, \"rebase-merge/keep_redu\n  * A 'struct replay_ctx' represents the private state of the sequencer.\n  */\n struct replay_ctx {\n+\t/*\n+\t * The list of completed fixup and squash commands in the\n+\t * current chain.\n+\t */\n+\tstruct strbuf current_fixups;\n \t/*\n \t * Stores the reflog message that will be used when creating a\n \t * commit. Points to a static buffer and should not be free()'d.\n \t */\n \tconst char *reflog_message;\n+\t/*\n+\t * The number of completed fixup and squash commands in the\n+\t * current chain.\n+\t */\n+\tint current_fixup_count;\n };\n \n struct replay_ctx* replay_ctx_new(void)\n {\n \tstruct replay_ctx *ctx = xcalloc(1, sizeof(*ctx));\n \n+\tstrbuf_init(&ctx->current_fixups, 0);\n+\n \treturn ctx;\n }\n \n@@ -384,17 +396,24 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)\n \treturn buf.buf;\n }\n \n+static void replay_ctx_release(struct replay_ctx *ctx)\n+{\n+\tstrbuf_release(&ctx->current_fixups);\n+}\n+\n void replay_opts_release(struct replay_opts *opts)\n {\n+\tstruct replay_ctx *ctx = opts->ctx;\n+\n \tfree(opts->gpg_sign);\n \tfree(opts->reflog_action);\n \tfree(opts->default_strategy);\n \tfree(opts->strategy);\n \tstrvec_clear (&opts->xopts);\n-\tstrbuf_release(&opts->current_fixups);\n \tif (opts->revs)\n \t\trelease_revisions(opts->revs);\n \tfree(opts->revs);\n+\treplay_ctx_release(ctx);\n \tfree(opts->ctx);\n }\n \n@@ -1876,10 +1895,10 @@ static void add_commented_lines(struct strbuf *buf, const void *str, size_t len)\n }\n \n /* Does the current fixup chain contain a squash command? */\n-static int seen_squash(struct replay_opts *opts)\n+static int seen_squash(struct replay_ctx *ctx)\n {\n-\treturn starts_with(opts->current_fixups.buf, \"squash\") ||\n-\t\tstrstr(opts->current_fixups.buf, \"\\nsquash\");\n+\treturn starts_with(ctx->current_fixups.buf, \"squash\") ||\n+\t\tstrstr(ctx->current_fixups.buf, \"\\nsquash\");\n }\n \n static void update_comment_bufs(struct strbuf *buf1, struct strbuf *buf2, int n)\n@@ -1955,6 +1974,7 @@ static int append_squash_message(struct strbuf *buf, const char *body,\n \t\t\t enum todo_command command, struct replay_opts *opts,\n \t\t\t unsigned flag)\n {\n+\tstruct replay_ctx *ctx = opts->ctx;\n \tconst char *fixup_msg;\n \tsize_t commented_len = 0, fixup_off;\n \t/*\n@@ -1963,21 +1983,21 @@ static int append_squash_message(struct strbuf *buf, const char *body,\n \t * squashing commit messages.\n \t */\n \tif (starts_with(body, \"amend!\") ||\n-\t    ((command == TODO_SQUASH || seen_squash(opts)) &&\n+\t    ((command == TODO_SQUASH || seen_squash(ctx)) &&\n \t     (starts_with(body, \"squash!\") || starts_with(body, \"fixup!\"))))\n \t\tcommented_len = commit_subject_length(body);\n \n \tstrbuf_addf(buf, \"\\n%c \", comment_line_char);\n \tstrbuf_addf(buf, _(nth_commit_msg_fmt),\n-\t\t    ++opts->current_fixup_count + 1);\n+\t\t    ++ctx->current_fixup_count + 1);\n \tstrbuf_addstr(buf, \"\\n\\n\");\n \tstrbuf_add_commented_lines(buf, body, commented_len, comment_line_char);\n \t/* buf->buf may be reallocated so store an offset into the buffer */\n \tfixup_off = buf->len;\n \tstrbuf_addstr(buf, body + commented_len);\n \n \t/* fixup -C after squash behaves like squash */\n-\tif (is_fixup_flag(command, flag) && !seen_squash(opts)) {\n+\tif (is_fixup_flag(command, flag) && !seen_squash(ctx)) {\n \t\t/*\n \t\t * We're replacing the commit message so we need to\n \t\t * append the Signed-off-by: trailer if the user\n@@ -2011,12 +2031,13 @@ static int update_squash_messages(struct repository *r,\n \t\t\t\t  struct replay_opts *opts,\n \t\t\t\t  unsigned flag)\n {\n+\tstruct replay_ctx *ctx = opts->ctx;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint res = 0;\n \tconst char *message, *body;\n \tconst char *encoding = get_commit_output_encoding();\n \n-\tif (opts->current_fixup_count > 0) {\n+\tif (ctx->current_fixup_count > 0) {\n \t\tstruct strbuf header = STRBUF_INIT;\n \t\tchar *eol;\n \n@@ -2029,10 +2050,10 @@ static int update_squash_messages(struct repository *r,\n \n \t\tstrbuf_addf(&header, \"%c \", comment_line_char);\n \t\tstrbuf_addf(&header, _(combined_commit_msg_fmt),\n-\t\t\t    opts->current_fixup_count + 2);\n+\t\t\t    ctx->current_fixup_count + 2);\n \t\tstrbuf_splice(&buf, 0, eol - buf.buf, header.buf, header.len);\n \t\tstrbuf_release(&header);\n-\t\tif (is_fixup_flag(command, flag) && !seen_squash(opts))\n+\t\tif (is_fixup_flag(command, flag) && !seen_squash(ctx))\n \t\t\tupdate_squash_message_for_fixup(&buf);\n \t} else {\n \t\tstruct object_id head;\n@@ -2079,7 +2100,7 @@ static int update_squash_messages(struct repository *r,\n \t} else if (command == TODO_FIXUP) {\n \t\tstrbuf_addf(&buf, \"\\n%c \", comment_line_char);\n \t\tstrbuf_addf(&buf, _(skip_nth_commit_msg_fmt),\n-\t\t\t    ++opts->current_fixup_count + 1);\n+\t\t\t    ++ctx->current_fixup_count + 1);\n \t\tstrbuf_addstr(&buf, \"\\n\\n\");\n \t\tstrbuf_add_commented_lines(&buf, body, strlen(body),\n \t\t\t\t\t   comment_line_char);\n@@ -2093,12 +2114,12 @@ static int update_squash_messages(struct repository *r,\n \tstrbuf_release(&buf);\n \n \tif (!res) {\n-\t\tstrbuf_addf(&opts->current_fixups, \"%s%s %s\",\n-\t\t\t    opts->current_fixups.len ? \"\\n\" : \"\",\n+\t\tstrbuf_addf(&ctx->current_fixups, \"%s%s %s\",\n+\t\t\t    ctx->current_fixups.len ? \"\\n\" : \"\",\n \t\t\t    command_to_string(command),\n \t\t\t    oid_to_hex(&commit->object.oid));\n-\t\tres = write_message(opts->current_fixups.buf,\n-\t\t\t\t    opts->current_fixups.len,\n+\t\tres = write_message(ctx->current_fixups.buf,\n+\t\t\t\t    ctx->current_fixups.len,\n \t\t\t\t    rebase_path_current_fixups(), 0);\n \t}\n \n@@ -2176,6 +2197,7 @@ static int do_pick_commit(struct repository *r,\n \t\t\t  struct replay_opts *opts,\n \t\t\t  int final_fixup, int *check_todo)\n {\n+\tstruct replay_ctx *ctx = opts->ctx;\n \tunsigned int flags = should_edit(opts) ? EDIT_MSG : 0;\n \tconst char *msg_file = should_edit(opts) ? NULL : git_path_merge_msg(r);\n \tstruct object_id head;\n@@ -2456,8 +2478,8 @@ static int do_pick_commit(struct repository *r,\n \t\tunlink(rebase_path_fixup_msg());\n \t\tunlink(rebase_path_squash_msg());\n \t\tunlink(rebase_path_current_fixups());\n-\t\tstrbuf_reset(&opts->current_fixups);\n-\t\topts->current_fixup_count = 0;\n+\t\tstrbuf_reset(&ctx->current_fixups);\n+\t\tctx->current_fixup_count = 0;\n \t}\n \n leave:\n@@ -3019,6 +3041,8 @@ static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)\n \n static int read_populate_opts(struct replay_opts *opts)\n {\n+\tstruct replay_ctx *ctx = opts->ctx;\n+\n \tif (is_rebase_i(opts)) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tint ret = 0;\n@@ -3078,13 +3102,13 @@ static int read_populate_opts(struct replay_opts *opts)\n \t\tread_strategy_opts(opts, &buf);\n \t\tstrbuf_reset(&buf);\n \n-\t\tif (read_oneliner(&opts->current_fixups,\n+\t\tif (read_oneliner(&ctx->current_fixups,\n \t\t\t\t  rebase_path_current_fixups(),\n \t\t\t\t  READ_ONELINER_SKIP_IF_EMPTY)) {\n-\t\t\tconst char *p = opts->current_fixups.buf;\n-\t\t\topts->current_fixup_count = 1;\n+\t\t\tconst char *p = ctx->current_fixups.buf;\n+\t\t\tctx->current_fixup_count = 1;\n \t\t\twhile ((p = strchr(p, '\\n'))) {\n-\t\t\t\topts->current_fixup_count++;\n+\t\t\t\tctx->current_fixup_count++;\n \t\t\t\tp++;\n \t\t\t}\n \t\t}\n@@ -5066,6 +5090,7 @@ static int commit_staged_changes(struct repository *r,\n \t\t\t\t struct replay_opts *opts,\n \t\t\t\t struct todo_list *todo_list)\n {\n+\tstruct replay_ctx *ctx = opts->ctx;\n \tunsigned int flags = ALLOW_EMPTY | EDIT_MSG;\n \tunsigned int final_fixup = 0, is_clean;\n \n@@ -5102,7 +5127,7 @@ static int commit_staged_changes(struct repository *r,\n \t\t * the commit message and if there was a squash, let the user\n \t\t * edit it.\n \t\t */\n-\t\tif (!is_clean || !opts->current_fixup_count)\n+\t\tif (!is_clean || !ctx->current_fixup_count)\n \t\t\t; /* this is not the final fixup */\n \t\telse if (!oideq(&head, &to_amend) ||\n \t\t\t !file_exists(rebase_path_stopped_sha())) {\n@@ -5111,20 +5136,20 @@ static int commit_staged_changes(struct repository *r,\n \t\t\t\tunlink(rebase_path_fixup_msg());\n \t\t\t\tunlink(rebase_path_squash_msg());\n \t\t\t\tunlink(rebase_path_current_fixups());\n-\t\t\t\tstrbuf_reset(&opts->current_fixups);\n-\t\t\t\topts->current_fixup_count = 0;\n+\t\t\t\tstrbuf_reset(&ctx->current_fixups);\n+\t\t\t\tctx->current_fixup_count = 0;\n \t\t\t}\n \t\t} else {\n \t\t\t/* we are in a fixup/squash chain */\n-\t\t\tconst char *p = opts->current_fixups.buf;\n-\t\t\tint len = opts->current_fixups.len;\n+\t\t\tconst char *p = ctx->current_fixups.buf;\n+\t\t\tint len = ctx->current_fixups.len;\n \n-\t\t\topts->current_fixup_count--;\n+\t\t\tctx->current_fixup_count--;\n \t\t\tif (!len)\n \t\t\t\tBUG(\"Incorrect current_fixups:\\n%s\", p);\n \t\t\twhile (len && p[len - 1] != '\\n')\n \t\t\t\tlen--;\n-\t\t\tstrbuf_setlen(&opts->current_fixups, len);\n+\t\t\tstrbuf_setlen(&ctx->current_fixups, len);\n \t\t\tif (write_message(p, len, rebase_path_current_fixups(),\n \t\t\t\t\t  0) < 0)\n \t\t\t\treturn error(_(\"could not write file: '%s'\"),\n@@ -5141,7 +5166,7 @@ static int commit_staged_changes(struct repository *r,\n \t\t\t * actually need to re-commit with a cleaned up commit\n \t\t\t * message.\n \t\t\t */\n-\t\t\tif (opts->current_fixup_count > 0 &&\n+\t\t\tif (ctx->current_fixup_count > 0 &&\n \t\t\t    !is_fixup(peek_command(todo_list, 0))) {\n \t\t\t\tfinal_fixup = 1;\n \t\t\t\t/*\n@@ -5214,14 +5239,14 @@ static int commit_staged_changes(struct repository *r,\n \t\tunlink(rebase_path_fixup_msg());\n \t\tunlink(rebase_path_squash_msg());\n \t}\n-\tif (opts->current_fixup_count > 0) {\n+\tif (ctx->current_fixup_count > 0) {\n \t\t/*\n \t\t * Whether final fixup or not, we just cleaned up the commit\n \t\t * message...\n \t\t */\n \t\tunlink(rebase_path_current_fixups());\n-\t\tstrbuf_reset(&opts->current_fixups);\n-\t\topts->current_fixup_count = 0;\n+\t\tstrbuf_reset(&ctx->current_fixups);\n+\t\tctx->current_fixup_count = 0;\n \t}\n \treturn 0;\n }\ndiff --git a/sequencer.h b/sequencer.h\nindex 069f06400d2..c15b9a64020 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -69,10 +69,6 @@ struct replay_opts {\n \t/* Reflog */\n \tchar *reflog_action;\n \n-\t/* Used by fixup/squash */\n-\tstruct strbuf current_fixups;\n-\tint current_fixup_count;\n-\n \t/* placeholder commit for -i --root */\n \tstruct object_id squash_onto;\n \tint have_squash_onto;\n@@ -86,7 +82,6 @@ struct replay_opts {\n #define REPLAY_OPTS_INIT {\t\t\t\\\n \t.edit = -1,\t\t\t\t\\\n \t.action = -1,\t\t\t\t\\\n-\t.current_fixups = STRBUF_INIT,\t\t\\\n \t.xopts = STRVEC_INIT,\t\t\t\\\n \t.ctx = replay_ctx_new(),\t\t\\\n }\n-- \n2.44.0.661.ge68cfcc6c2f\n\n"},{"id":"493136","messageId":"fcef79ede656ba8e6dcd153e115095c123e0f05d.1713445918.git.phillip.wood@dunelm.org.uk","threadId":"61333","inReplyTo":"cover.1713445918.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 4/5] sequencer: store commit message in private context","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-18T13:14:08Z","receivedAt":"2024-04-18T13:14:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nAdd an strbuf to \"struct replay_ctx\" to hold the current commit\nmessage. This does not change the behavior but it will allow us to fix a\nbug with \"git rebase --signoff\" in the next commit. A future patch\nseries will use the changes here to avoid writing the commit message to\ndisc unless there are conflicts or the commit is being reworded.\n\nThe changes in do_pick_commit() are a mechanical replacement of \"msgbuf\"\nwith \"ctx->message\". In do_merge() the code to write commit message to\ndisc is factored out of the conditional now that both branches store the\nmessage in the same buffer.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c | 96 ++++++++++++++++++++++++++++-------------------------\n 1 file changed, 50 insertions(+), 46 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex edc49c94cce..31e4d3fdec0 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -211,6 +211,11 @@ static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, \"rebase-merge/keep_redu\n  * A 'struct replay_ctx' represents the private state of the sequencer.\n  */\n struct replay_ctx {\n+\t/*\n+\t * The commit message that will be used except at the end of a\n+\t * chain of fixup and squash commands.\n+\t */\n+\tstruct strbuf message;\n \t/*\n \t * The list of completed fixup and squash commands in the\n \t * current chain.\n@@ -226,13 +231,18 @@ struct replay_ctx {\n \t * current chain.\n \t */\n \tint current_fixup_count;\n+\t/*\n+\t * Whether message contains a commit message.\n+\t */\n+\tunsigned have_message :1;\n };\n \n struct replay_ctx* replay_ctx_new(void)\n {\n \tstruct replay_ctx *ctx = xcalloc(1, sizeof(*ctx));\n \n \tstrbuf_init(&ctx->current_fixups, 0);\n+\tstrbuf_init(&ctx->message, 0);\n \n \treturn ctx;\n }\n@@ -399,6 +409,7 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)\n static void replay_ctx_release(struct replay_ctx *ctx)\n {\n \tstrbuf_release(&ctx->current_fixups);\n+\tstrbuf_release(&ctx->message);\n }\n \n void replay_opts_release(struct replay_opts *opts)\n@@ -2205,7 +2216,6 @@ static int do_pick_commit(struct repository *r,\n \tconst char *base_label, *next_label;\n \tchar *author = NULL;\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL };\n-\tstruct strbuf msgbuf = STRBUF_INIT;\n \tint res, unborn = 0, reword = 0, allow, drop_commit;\n \tenum todo_command command = item->command;\n \tstruct commit *commit = item->commit;\n@@ -2304,7 +2314,7 @@ static int do_pick_commit(struct repository *r,\n \t\tnext = parent;\n \t\tnext_label = msg.parent_label;\n \t\tif (opts->commit_use_reference) {\n-\t\t\tstrbuf_addstr(&msgbuf,\n+\t\t\tstrbuf_addstr(&ctx->message,\n \t\t\t\t\"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\");\n \t\t} else if (skip_prefix(msg.subject, \"Revert \\\"\", &orig_subject) &&\n \t\t\t   /*\n@@ -2313,21 +2323,21 @@ static int do_pick_commit(struct repository *r,\n \t\t\t    * thus requiring excessive complexity to deal with.\n \t\t\t    */\n \t\t\t   !starts_with(orig_subject, \"Revert \\\"\")) {\n-\t\t\tstrbuf_addstr(&msgbuf, \"Reapply \\\"\");\n-\t\t\tstrbuf_addstr(&msgbuf, orig_subject);\n+\t\t\tstrbuf_addstr(&ctx->message, \"Reapply \\\"\");\n+\t\t\tstrbuf_addstr(&ctx->message, orig_subject);\n \t\t} else {\n-\t\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n-\t\t\tstrbuf_addstr(&msgbuf, msg.subject);\n-\t\t\tstrbuf_addstr(&msgbuf, \"\\\"\");\n+\t\t\tstrbuf_addstr(&ctx->message, \"Revert \\\"\");\n+\t\t\tstrbuf_addstr(&ctx->message, msg.subject);\n+\t\t\tstrbuf_addstr(&ctx->message, \"\\\"\");\n \t\t}\n-\t\tstrbuf_addstr(&msgbuf, \"\\n\\nThis reverts commit \");\n-\t\trefer_to_commit(opts, &msgbuf, commit);\n+\t\tstrbuf_addstr(&ctx->message, \"\\n\\nThis reverts commit \");\n+\t\trefer_to_commit(opts, &ctx->message, commit);\n \n \t\tif (commit->parents && commit->parents->next) {\n-\t\t\tstrbuf_addstr(&msgbuf, \", reversing\\nchanges made to \");\n-\t\t\trefer_to_commit(opts, &msgbuf, parent);\n+\t\t\tstrbuf_addstr(&ctx->message, \", reversing\\nchanges made to \");\n+\t\t\trefer_to_commit(opts, &ctx->message, parent);\n \t\t}\n-\t\tstrbuf_addstr(&msgbuf, \".\\n\");\n+\t\tstrbuf_addstr(&ctx->message, \".\\n\");\n \t} else {\n \t\tconst char *p;\n \n@@ -2336,21 +2346,22 @@ static int do_pick_commit(struct repository *r,\n \t\tnext = commit;\n \t\tnext_label = msg.label;\n \n-\t\t/* Append the commit log message to msgbuf. */\n+\t\t/* Append the commit log message to ctx->message. */\n \t\tif (find_commit_subject(msg.message, &p))\n-\t\t\tstrbuf_addstr(&msgbuf, p);\n+\t\t\tstrbuf_addstr(&ctx->message, p);\n \n \t\tif (opts->record_origin) {\n-\t\t\tstrbuf_complete_line(&msgbuf);\n-\t\t\tif (!has_conforming_footer(&msgbuf, NULL, 0))\n-\t\t\t\tstrbuf_addch(&msgbuf, '\\n');\n-\t\t\tstrbuf_addstr(&msgbuf, cherry_picked_prefix);\n-\t\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n-\t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n+\t\t\tstrbuf_complete_line(&ctx->message);\n+\t\t\tif (!has_conforming_footer(&ctx->message, NULL, 0))\n+\t\t\t\tstrbuf_addch(&ctx->message, '\\n');\n+\t\t\tstrbuf_addstr(&ctx->message, cherry_picked_prefix);\n+\t\t\tstrbuf_addstr(&ctx->message, oid_to_hex(&commit->object.oid));\n+\t\t\tstrbuf_addstr(&ctx->message, \")\\n\");\n \t\t}\n \t\tif (!is_fixup(command))\n \t\t\tauthor = get_author(msg.message);\n \t}\n+\tctx->have_message = 1;\n \n \tif (command == TODO_REWORD)\n \t\treword = 1;\n@@ -2381,7 +2392,7 @@ static int do_pick_commit(struct repository *r,\n \t}\n \n \tif (opts->signoff && !is_fixup(command))\n-\t\tappend_signoff(&msgbuf, 0, 0);\n+\t\tappend_signoff(&ctx->message, 0, 0);\n \n \tif (is_rebase_i(opts) && write_author_script(msg.message) < 0)\n \t\tres = -1;\n@@ -2390,17 +2401,17 @@ static int do_pick_commit(struct repository *r,\n \t\t !strcmp(opts->strategy, \"ort\") ||\n \t\t command == TODO_REVERT) {\n \t\tres = do_recursive_merge(r, base, next, base_label, next_label,\n-\t\t\t\t\t &head, &msgbuf, opts);\n+\t\t\t\t\t &head, &ctx->message, opts);\n \t\tif (res < 0)\n \t\t\tgoto leave;\n \n-\t\tres |= write_message(msgbuf.buf, msgbuf.len,\n+\t\tres |= write_message(ctx->message.buf, ctx->message.len,\n \t\t\t\t     git_path_merge_msg(r), 0);\n \t} else {\n \t\tstruct commit_list *common = NULL;\n \t\tstruct commit_list *remotes = NULL;\n \n-\t\tres = write_message(msgbuf.buf, msgbuf.len,\n+\t\tres = write_message(ctx->message.buf, ctx->message.len,\n \t\t\t\t    git_path_merge_msg(r), 0);\n \n \t\tcommit_list_insert(base, &common);\n@@ -2485,7 +2496,6 @@ static int do_pick_commit(struct repository *r,\n leave:\n \tfree_message(commit, &msg);\n \tfree(author);\n-\tstrbuf_release(&msgbuf);\n \tupdate_abort_safety_file();\n \n \treturn res;\n@@ -3952,6 +3962,7 @@ static int do_merge(struct repository *r,\n \t\t    const char *arg, int arg_len,\n \t\t    int flags, int *check_todo, struct replay_opts *opts)\n {\n+\tstruct replay_ctx *ctx = opts->ctx;\n \tint run_commit_flags = 0;\n \tstruct strbuf ref_name = STRBUF_INIT;\n \tstruct commit *head_commit, *merge_commit, *i;\n@@ -4080,40 +4091,31 @@ static int do_merge(struct repository *r,\n \t\twrite_author_script(message);\n \t\tfind_commit_subject(message, &body);\n \t\tlen = strlen(body);\n-\t\tret = write_message(body, len, git_path_merge_msg(r), 0);\n+\t\tstrbuf_add(&ctx->message, body, len);\n \t\trepo_unuse_commit_buffer(r, commit, message);\n-\t\tif (ret) {\n-\t\t\terror_errno(_(\"could not write '%s'\"),\n-\t\t\t\t    git_path_merge_msg(r));\n-\t\t\tgoto leave_merge;\n-\t\t}\n \t} else {\n \t\tstruct strbuf buf = STRBUF_INIT;\n-\t\tint len;\n \n \t\tstrbuf_addf(&buf, \"author %s\", git_author_info(0));\n \t\twrite_author_script(buf.buf);\n-\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_release(&buf);\n \n \t\tif (oneline_offset < arg_len) {\n-\t\t\tp = arg + oneline_offset;\n-\t\t\tlen = arg_len - oneline_offset;\n+\t\t\tstrbuf_add(&ctx->message, arg + oneline_offset,\n+\t\t\t\t   arg_len - oneline_offset);\n \t\t} else {\n-\t\t\tstrbuf_addf(&buf, \"Merge %s '%.*s'\",\n+\t\t\tstrbuf_addf(&ctx->message, \"Merge %s '%.*s'\",\n \t\t\t\t    to_merge->next ? \"branches\" : \"branch\",\n \t\t\t\t    merge_arg_len, arg);\n-\t\t\tp = buf.buf;\n-\t\t\tlen = buf.len;\n-\t\t}\n-\n-\t\tret = write_message(p, len, git_path_merge_msg(r), 0);\n-\t\tstrbuf_release(&buf);\n-\t\tif (ret) {\n-\t\t\terror_errno(_(\"could not write '%s'\"),\n-\t\t\t\t    git_path_merge_msg(r));\n-\t\t\tgoto leave_merge;\n \t\t}\n \t}\n+\tctx->have_message = 1;\n+\tif (write_message(ctx->message.buf, ctx->message.len,\n+\t\t\t  git_path_merge_msg(r), 0)) {\n+\t\t    ret = error_errno(_(\"could not write '%s'\"),\n+\t\t\t\t      git_path_merge_msg(r));\n+\t\t    goto leave_merge;\n+\t}\n \n \tif (strategy || to_merge->next) {\n \t\t/* Octopus merge */\n@@ -4885,6 +4887,8 @@ static int pick_commits(struct repository *r,\n \t\t\t\treturn stopped_at_head(r);\n \t\t\t}\n \t\t}\n+\t\tstrbuf_reset(&ctx->message);\n+\t\tctx->have_message = 0;\n \t\tif (item->command <= TODO_SQUASH) {\n \t\t\tres = pick_one_commit(r, todo_list, opts, &check_todo,\n \t\t\t\t\t      &reschedule);\n-- \n2.44.0.661.ge68cfcc6c2f\n\n"},{"id":"493137","messageId":"4c8f8843780f3ac23262f1e45a5000d183adca6b.1713445918.git.phillip.wood@dunelm.org.uk","threadId":"61333","inReplyTo":"cover.1713445918.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 5/5] rebase -m: fix --signoff with conflicts","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-18T13:14:09Z","receivedAt":"2024-04-18T13:14:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nWhen rebasing with \"--signoff\" the commit created by \"rebase --continue\"\nafter resolving conflicts or editing a commit fails to add the\n\"Signed-off-by:\" trailer. This happens because the message from the\noriginal commit is reused instead of the one that would have been used\nif the sequencer had not stopped for the user interaction. The correct\nmessage is stored in ctx->message and so with a couple of exceptions\nthis is written to rebase_path_message() when stopping for user\ninteraction instead. The exceptions are (i) \"fixup\" and \"squash\"\ncommands where the file is written by error_failed_squash() and (ii)\n\"edit\" commands that are fast-forwarded where the original message is\nstill reused. The latter is safe because \"--signoff\" will never\nfast-forward.\n\nNote this introduces a change in behavior as the message file now\ncontains conflict comments. This is safe because commit_staged_changes()\npasses an explicit cleanup flag when not editing the message and when\nthe message is being edited it will be cleaned up automatically. This\nmeans user now sees the same message comments in editor with \"rebase\n--continue\" as they would if they ran \"git commit\" themselves before\ncontinuing the rebase. It also matches the behavior of \"git\ncherry-pick\", \"git merge\" etc. which all list the files with merge\nconflicts.\n\nThe tests are extended to check that all commits made after continuing a\nrebase have a \"Signed-off-by:\" trailer. Sadly there are a couple of\nleaks in apply.c which I've not been able to track down that mean this\ntest file is no-longer leak free when testing \"git rebase --apply\n--signoff\" with conflicts.\n\nReported-by: David Bimmler <david.bimmler@isovalent.com>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c               | 23 +++++++---\n t/t3428-rebase-signoff.sh | 96 ++++++++++++++++++++++++++++++++-------\n t/t3434-rebase-i18n.sh    |  2 +-\n 3 files changed, 97 insertions(+), 24 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 31e4d3fdec0..e29a01944be 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3636,13 +3636,24 @@ static int error_with_patch(struct repository *r,\n \t\t\t    struct replay_opts *opts,\n \t\t\t    int exit_code, int to_amend)\n {\n-\tif (commit) {\n-\t\tif (make_patch(r, commit, opts))\n+\tstruct replay_ctx *ctx = opts->ctx;\n+\n+\t/*\n+\t * Write the commit message to be used by \"git rebase\n+\t * --continue\". If a \"fixup\" or \"squash\" command has conflicts\n+\t * then we will have already written rebase_path_message() in\n+\t * error_failed_squash(). If an \"edit\" command was\n+\t * fast-forwarded then we don't have a message in ctx->message\n+\t * and rely on make_patch() to write rebase_path_message()\n+\t * instead.\n+\t */\n+\tif (ctx->have_message && !file_exists(rebase_path_message()) &&\n+\t    write_message(ctx->message.buf, ctx->message.len,\n+\t\t\t  rebase_path_message(), 0))\n+\t\treturn error(_(\"could not write commit message file\"));\n+\n+\tif (commit && make_patch(r, commit, opts))\n \t\t\treturn -1;\n-\t} else if (copy_file(rebase_path_message(),\n-\t\t\t     git_path_merge_msg(r), 0666))\n-\t\treturn error(_(\"unable to copy '%s' to '%s'\"),\n-\t\t\t     git_path_merge_msg(r), rebase_path_message());\n \n \tif (to_amend) {\n \t\tif (intend_to_amend())\ndiff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh\nindex 1bebd1ce74a..6f57aed9fac 100755\n--- a/t/t3428-rebase-signoff.sh\n+++ b/t/t3428-rebase-signoff.sh\n@@ -5,12 +5,17 @@ test_description='git rebase --signoff\n This test runs git rebase --signoff and make sure that it works.\n '\n \n-TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-rebase.sh\n \n test_expect_success 'setup' '\n \tgit commit --allow-empty -m \"Initial empty commit\" &&\n \ttest_commit first file a &&\n+\ttest_commit second file &&\n+\tgit checkout -b conflict-branch first &&\n+\ttest_commit file-2 file-2 &&\n+\ttest_commit conflict file &&\n+\ttest_commit third file &&\n \n \tident=\"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\" &&\n \n@@ -28,6 +33,22 @@ test_expect_success 'setup' '\n \tSigned-off-by: $ident\n \tEOF\n \n+\t# Expected commit message after conflict resolution for rebase --signoff\n+\tcat >expected-signed-conflict <<-EOF &&\n+\tthird\n+\n+\tSigned-off-by: $ident\n+\n+\tconflict\n+\n+\tSigned-off-by: $ident\n+\n+\tfile-2\n+\n+\tSigned-off-by: $ident\n+\n+\tEOF\n+\n \t# Expected commit message after rebase without --signoff (or with --no-signoff)\n \tcat >expected-unsigned <<-EOF &&\n \tfirst\n@@ -39,8 +60,12 @@ test_expect_success 'setup' '\n # We configure an alias to do the rebase --signoff so that\n # on the next subtest we can show that --no-signoff overrides the alias\n test_expect_success 'rebase --apply --signoff adds a sign-off line' '\n-\tgit rbs --apply HEAD^ &&\n-\ttest_commit_message HEAD expected-signed\n+\ttest_must_fail git rbs --apply second third &&\n+\tgit checkout --theirs file &&\n+\tgit add file &&\n+\tgit rebase --continue &&\n+\tgit log --format=%B -n3 >actual &&\n+\ttest_cmp expected-signed-conflict actual\n '\n \n test_expect_success 'rebase --no-signoff does not add a sign-off line' '\n@@ -51,28 +76,65 @@ test_expect_success 'rebase --no-signoff does not add a sign-off line' '\n \n test_expect_success 'rebase --exec --signoff adds a sign-off line' '\n \ttest_when_finished \"rm exec\" &&\n-\tgit commit --amend -m \"first\" &&\n-\tgit rebase --exec \"touch exec\" --signoff HEAD^ &&\n+\tgit rebase --exec \"touch exec\" --signoff first^ first &&\n \ttest_path_is_file exec &&\n \ttest_commit_message HEAD expected-signed\n '\n \n test_expect_success 'rebase --root --signoff adds a sign-off line' '\n-\tgit commit --amend -m \"first\" &&\n+\tgit checkout first &&\n \tgit rebase --root --keep-empty --signoff &&\n \ttest_commit_message HEAD^ expected-initial-signed &&\n \ttest_commit_message HEAD expected-signed\n '\n \n-test_expect_success 'rebase -i --signoff fails' '\n-\tgit commit --amend -m \"first\" &&\n-\tgit rebase -i --signoff HEAD^ &&\n-\ttest_commit_message HEAD expected-signed\n-'\n-\n-test_expect_success 'rebase -m --signoff fails' '\n-\tgit commit --amend -m \"first\" &&\n-\tgit rebase -m --signoff HEAD^ &&\n-\ttest_commit_message HEAD expected-signed\n-'\n+test_expect_success 'rebase -m --signoff adds a sign-off line' '\n+\ttest_must_fail git rebase -m --signoff second third &&\n+\tgit checkout --theirs file &&\n+\tgit add file &&\n+\tGIT_EDITOR=\"sed -n /Conflicts:/,/^\\\\\\$/p >actual\" \\\n+\t\tgit rebase --continue &&\n+\tcat >expect <<-\\EOF &&\n+\t# Conflicts:\n+\t#\tfile\n+\n+\tEOF\n+\ttest_cmp expect actual &&\n+\tgit log --format=%B -n3 >actual &&\n+\ttest_cmp expected-signed-conflict actual\n+'\n+\n+test_expect_success 'rebase -i --signoff adds a sign-off line when editing commit' '\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_LINES=\"edit 1 edit 3 edit 2\" \\\n+\t\t\tgit rebase -i --signoff first third\n+\t) &&\n+\techo a >a &&\n+\tgit add a &&\n+\ttest_must_fail git rebase --continue &&\n+\tgit checkout --ours file &&\n+\techo b >a &&\n+\tgit add a file &&\n+\tgit rebase --continue &&\n+\techo c >a &&\n+\tgit add a &&\n+\tgit log --format=%B -n3 >actual &&\n+\tcat >expect <<-EOF &&\n+\tconflict\n+\n+\tSigned-off-by: $ident\n+\n+\tthird\n+\n+\tSigned-off-by: $ident\n+\n+\tfile-2\n+\n+\tSigned-off-by: $ident\n+\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t3434-rebase-i18n.sh b/t/t3434-rebase-i18n.sh\nindex e6fef696bbc..a4e482d2cd0 100755\n--- a/t/t3434-rebase-i18n.sh\n+++ b/t/t3434-rebase-i18n.sh\n@@ -71,7 +71,7 @@ test_rebase_continue_update_encode () {\n \t\tgit config i18n.commitencoding $new &&\n \t\ttest_must_fail git rebase -m main &&\n \t\ttest -f .git/rebase-merge/message &&\n-\t\tgit stripspace <.git/rebase-merge/message >two.t &&\n+\t\tgit stripspace -s <.git/rebase-merge/message >two.t &&\n \t\tgit add two.t &&\n \t\tgit rebase --continue &&\n \t\tcompare_msg $msgfile $old $new &&\n-- \n2.44.0.661.ge68cfcc6c2f\n\n"},{"id":"493156","messageId":"xmqqa5lqs8r7.fsf@gitster.g","threadId":"61333","inReplyTo":"2beedb4547013629a507d646d582ca6b3f420c2c.1713445918.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH 2/5] sequencer: start removing private fields from public API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-18T20:42:20Z","receivedAt":"2024-04-18T20:42:23Z","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> +/*\n> + * A 'struct replay_ctx' represents the private state of the sequencer.\n> + */\n> +struct replay_ctx {\n> +\t/*\n> +\t * Stores the reflog message that will be used when creating a\n> +\t * commit. Points to a static buffer and should not be free()'d.\n> +\t */\n> +\tconst char *reflog_message;\n> +};\n> +\n> +struct replay_ctx* replay_ctx_new(void)\n\nStyle.\n\nAlso if you intend to make the structure opaque and private,\nshouldn't you make this static to make it truly internal?  The\ninitializer of the containing replay_opts structure would be the\nonly thing that needs to know how to instantiate it, no?  Or does a\nreplay_ctx has reason to exist outside the context of an instance of\nreplay_opts?\n\ni.e.\n\n\tstatic struct replay_ctx *replay_ctx_new(void)\n\nThe remainder seem mechanical so I'll skip them.\n"},{"id":"493157","messageId":"xmqq34ris8hq.fsf@gitster.g","threadId":"61333","inReplyTo":"2e4a2168733cb226a6fd660cb0d0bd700a1e7d86.1713445918.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH 3/5] sequencer: move current fixups to private context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-18T20:48:01Z","receivedAt":"2024-04-18T20:48:05Z","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> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> The list of current fixups is an implementation detail of the sequencer\n> and so it should not be stored in the public options struct.\n\nIt feels curious how 2/5 and 3/5 treat \"fixups\" and \"reflog\"\ndifferently.\n\nI would have expected them to be either in a single patch, or if we\nreally wanted to split them for readability, do a preliminary change\nin 2/5 that argues that \"fixups\" does not really belong to _opts but\nis a private state just like \"reflog\" and moves it from \"/* Used by\nfixup/squash */\" section to the \"/* Private use */\" section at the\nend, and then introduce the _ctx in 3/5 to move these three\ntogether.\n\nThe end result would be the same, but I would have found the flow of\nthought more straight-forward if presented that way.\n"},{"id":"493158","messageId":"xmqqcyqmqt6t.fsf@gitster.g","threadId":"61333","inReplyTo":"fcef79ede656ba8e6dcd153e115095c123e0f05d.1713445918.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH 4/5] sequencer: store commit message in private context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-18T21:03:54Z","receivedAt":"2024-04-18T21:03:57Z","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> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> Add an strbuf to \"struct replay_ctx\" to hold the current commit\n> message. This does not change the behavior but it will allow us to fix a\n> bug with \"git rebase --signoff\" in the next commit. A future patch\n> series will use the changes here to avoid writing the commit message to\n> disc unless there are conflicts or the commit is being reworded.\n\nIs the machinery stopping due to conflicts the only reason why we\nmay want to write the message out to the filesystem?  I am wondering\nif there are hooks that kick in before each commit gets picked, and\nobserve what the message being prepared to be used for the commit\nis.\n\nBy the way, while I personally am fond of the spelling \"disc\",\nnobody in the recent commit uses it (I saw another reference or two\nin earlier steps of this series).  Perhaps \"disc\" -> \"file\", because\nmany filesystems are no longer on a spinning medium these days?\n\n> The changes in do_pick_commit() are a mechanical replacement of \"msgbuf\"\n> with \"ctx->message\". In do_merge() the code to write commit message to\n> disc is factored out of the conditional now that both branches store the\n> message in the same buffer.\n\nNice.\n\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  sequencer.c | 96 ++++++++++++++++++++++++++++-------------------------\n>  1 file changed, 50 insertions(+), 46 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index edc49c94cce..31e4d3fdec0 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -211,6 +211,11 @@ static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, \"rebase-merge/keep_redu\n>   * A 'struct replay_ctx' represents the private state of the sequencer.\n>   */\n>  struct replay_ctx {\n> +\t/*\n> +\t * The commit message that will be used except at the end of a\n> +\t * chain of fixup and squash commands.\n> +\t */\n> +\tstruct strbuf message;\n>  \t/*\n>  \t * The list of completed fixup and squash commands in the\n>  \t * current chain.\n> @@ -226,13 +231,18 @@ struct replay_ctx {\n>  \t * current chain.\n>  \t */\n>  \tint current_fixup_count;\n> +\t/*\n> +\t * Whether message contains a commit message.\n> +\t */\n> +\tunsigned have_message :1;\n>  };\n\nOK.  Having this member means we specifically allow message.len==0\nto be a valid commit log message.\n\nIf it weren't for alignment and padding concern, it would have been\nmuch nicer to have this member next to the .message member, and then\nwe do not even need such comment, as long as the name of the member\nis clear enough (say, .message_valid).  Alas, we do want to have the\nlarger stuff near the front of the struct, and this member has to\nsit near the end, so we need a comment to tell readers the linkage\nbetween the two.  I still do not see a need for a 3-line comment for\nthis member, though ;-)\n\n> +\tctx->have_message = 1;\n> +\tif (write_message(ctx->message.buf, ctx->message.len,\n> +\t\t\t  git_path_merge_msg(r), 0)) {\n> +\t\t    ret = error_errno(_(\"could not write '%s'\"),\n> +\t\t\t\t      git_path_merge_msg(r));\n> +\t\t    goto leave_merge;\n> +\t}\n\nAs we are writing the in-core message to disc, we know they are in\nsync, in other words, if we wanted to \"read\" the message, we know we\ndo not have to go back to disc and instead use the in-core version.\n\nSo, when do we *not* have .have_message member true?  Before we\nfound out the original message by reading the commit?  In this step,\nthe .have_message member is only set and not acted upon, so it is a\nbit hard to see how it is meant to be used before moving to the next\nstep.  Let's see how it goes.\n\nThanks.\n"},{"id":"493159","messageId":"xmqq7cguqsln.fsf@gitster.g","threadId":"61333","inReplyTo":"4c8f8843780f3ac23262f1e45a5000d183adca6b.1713445918.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH 5/5] rebase -m: fix --signoff with conflicts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-18T21:16:36Z","receivedAt":"2024-04-18T21:16:49Z","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> +\tstruct replay_ctx *ctx = opts->ctx;\n> +\n> +\t/*\n> +\t * Write the commit message to be used by \"git rebase\n> +\t * --continue\". If a \"fixup\" or \"squash\" command has conflicts\n> +\t * then we will have already written rebase_path_message() in\n> +\t * error_failed_squash(). If an \"edit\" command was\n> +\t * fast-forwarded then we don't have a message in ctx->message\n> +\t * and rely on make_patch() to write rebase_path_message()\n> +\t * instead.\n> +\t */\n> +\tif (ctx->have_message && !file_exists(rebase_path_message()) &&\n> +\t    write_message(ctx->message.buf, ctx->message.len,\n> +\t\t\t  rebase_path_message(), 0))\n> +\t\treturn error(_(\"could not write commit message file\"));\n\nMakes the readers wonder if there are cases where we have written to\ndisc, but .have_message is true and the on-disc contents and in-core\ncontents are different.  If a codepath that writes to the file had\nthe message in-core, and it needs to tweak the message (e.g., add a\nsign-off trailer) before writing it out to the file but forgot to do\nso, then later we have tweaked the message in-core, such a bug would\nresult in the necessary \"tweak\" we have done not appear on disc.\nOptionally keeping an in-core copy of what we have on disc does not\ndirectly reduces the possibility of introducing such a bug, and I am\nwondering if we can do anything clever about it.\n\nThanks.\n"}]}