Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
> From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
>
> Add an strbuf to "struct replay_ctx" to hold the current commit
> message. This does not change the behavior but it will allow us to fix a
> bug with "git rebase --signoff" in the next commit. A future patch
> series will use the changes here to avoid writing the commit message to
> disc unless there are conflicts or the commit is being reworded.
Is the machinery stopping due to conflicts the only reason why we
may want to write the message out to the filesystem? I am wondering
if there are hooks that kick in before each commit gets picked, and
observe what the message being prepared to be used for the commit
is.
By the way, while I personally am fond of the spelling "disc",
nobody in the recent commit uses it (I saw another reference or two
in earlier steps of this series). Perhaps "disc" -> "file", because
many filesystems are no longer on a spinning medium these days?
> The changes in do_pick_commit() are a mechanical replacement of "msgbuf"
> with "ctx->message". In do_merge() the code to write commit message to
> disc is factored out of the conditional now that both branches store the
> message in the same buffer.
Nice.
> Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
> ---
> sequencer.c | 96 ++++++++++++++++++++++++++++-------------------------
> 1 file changed, 50 insertions(+), 46 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index edc49c94cce..31e4d3fdec0 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -211,6 +211,11 @@ static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, "rebase-merge/keep_redu
> * A 'struct replay_ctx' represents the private state of the sequencer.
> */
> struct replay_ctx {
> + /*
> + * The commit message that will be used except at the end of a
> + * chain of fixup and squash commands.
> + */
> + struct strbuf message;
> /*
> * The list of completed fixup and squash commands in the
> * current chain.
> @@ -226,13 +231,18 @@ struct replay_ctx {
> * current chain.
> */
> int current_fixup_count;
> + /*
> + * Whether message contains a commit message.
> + */
> + unsigned have_message :1;
> };
OK. Having this member means we specifically allow message.len==0
to be a valid commit log message.
If it weren't for alignment and padding concern, it would have been
much nicer to have this member next to the .message member, and then
we do not even need such comment, as long as the name of the member
is clear enough (say, .message_valid). Alas, we do want to have the
larger stuff near the front of the struct, and this member has to
sit near the end, so we need a comment to tell readers the linkage
between the two. I still do not see a need for a 3-line comment for
this member, though ;-)
> + ctx->have_message = 1;
> + if (write_message(ctx->message.buf, ctx->message.len,
> + git_path_merge_msg(r), 0)) {
> + ret = error_errno(_("could not write '%s'"),
> + git_path_merge_msg(r));
> + goto leave_merge;
> + }
As we are writing the in-core message to disc, we know they are in
sync, in other words, if we wanted to "read" the message, we know we
do not have to go back to disc and instead use the in-core version.
So, when do we *not* have .have_message member true? Before we
found out the original message by reading the commit? In this step,
the .have_message member is only set and not acted upon, so it is a
bit hard to see how it is meant to be used before moving to the next
step. Let's see how it goes.
Thanks.
