Re: [PATCH v1 6/8] sequencer: simplify adding Signed-off-by: trailer
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 7, 2017, 04:52 UTC
- Message-ID
- <xmqq375qg071.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20171106112709.2121-7-phillip.wood@talktalk.net>
Phillip Wood <phillip.wood@talktalk.net> writes:
Show 14 quoted lines
> diff --git a/sequencer.c b/sequencer.c > index ae24405c23d021ed7916e5e2d9df6de27f867a2e..3e4c3bbb265db58df22cfcb5a321fb74d822327e 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -477,9 +477,6 @@ static int do_recursive_merge(struct commit *base, struct commit *next, > _(action_name(opts))); > rollback_lock_file(&index_lock); > > - if (opts->signoff) > - append_signoff(msgbuf, 0, 0); > - > if (!clean) > append_conflicts_hint(msgbuf); >
This function is called from only one place, do_pick_commit(), and then the message returned from here in msgbuf is written to merge_msg(), even when the merge conflicted.
And when the merge conflicts, sequencer would stop and gives the control back to you---the MERGE_MSG file would have had the sign-off when you conclude the conflict resolution.
With the new code, we instead add the sign-off before calling the function to compensate for the above change, so MERGE_MSG file would have the sign-off as before, when the sequencer stops.
Show 9 quoted lines
> @@ -657,8 +654,6 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts, > argv_array_push(&cmd.args, "--amend"); > if (opts->gpg_sign) > argv_array_pushf(&cmd.args, "-S%s", opts->gpg_sign); > - if (opts->signoff) > - argv_array_push(&cmd.args, "-s"); > if (defmsg) > argv_array_pushl(&cmd.args, "-F", defmsg, NULL); > if ((flags & CLEANUP_MSG))
This has two callers.
The caller in do_pick_commit() is a bit curious; as we saw already, the message file should already have the sign-off and then we used to give another "-s" here. Were we depending on "-s" to become no-op when the last sign-off is by the same person, I wonder? In any case, the removal of "-s" from here won't hurt that caller.
The other caller is commit_staged_changes() which is called when doing "rebase -i continue". I am not quite sure where the contents stored in the file rebase_path_message() comes from. The function error_failed_squash() moves SQUASH_MSG to it and then makes a copy of it to MERGE_MSG, but that should only be relevant for squashed commit and no other cases, so...?
I need to block a bit more time to read the relevant code to comment on this step, especially on this removal.
Thanks for working on this, anyway.
Show 10 quoted lines
> @@ -1347,6 +1342,9 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
> }
> }
>
> + if (opts->signoff)
> + append_signoff(&msgbuf, 0, 0);
> +
> if (is_rebase_i(opts) && write_author_script(msg.message) < 0)
> res = -1;
> else if (!opts->strategy || !strcmp(opts->strategy, "recursive") || command == TODO_REVERT) {