On 07/11/17 04:52, Junio C Hamano wrote:
> Phillip Wood <phillip.w...@talktalk.net> writes:
>
>> 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.
>
>> @@ -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.
That was more or less the conclusion I came to as well, though if the
user specifies a merge strategy other than recursive, then we were
relying on the "-s" passed to 'git commit' to add the sign-off. Now we
add the sign-off to the message ourselves in that case.
> 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...?
The interactive version of rebase does not support '--signoff' so this
is moot at the moment. I think that for conflicts with pick/edit/reword
then the sign-off is added to MERGE_MSG and that file is then picked up
by 'git commit'. For squash/fixup then the sign-off should have already
been added to the commit whose message is used for the first message in
SQUASH_MSG, but that will not be at the end of the message where we
expect Signed-off-by: to be. I'd need to check properly but I suspect we
also end up with a Signed-off-by: added at the end of SQUASH_MSG as well.
>
> 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.
Thanks for looking at these patches and your comments on them, I'll get
on with making the changes you and Johannes have suggested and wait to
hear from you about this one.
Best Wishes
Phillip
>
>> @@ -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) {
