On 08/10/2019 11:11, Phillip Wood wrote:
> Hi Toon & Zeger-Jan
>
> On 08/10/2019 08:49, Toon Claes wrote:
>> Add support to provide the Co-author when committing. For each
>> co-author provided with --coauthor=<coauthor>, a line is added at the
>> bottom of the commit message, like this:
>>
>> Co-authored-by: <coauthor>
>>
>> It's a common practice use when pairing up with other people and both
>> authors want to in the commit message.
>
> Thanks for the patch, it's looking good. I can see this being useful to
> some people - I like the way the patch itself is co-authored.
> [...]
>> @@ -803,6 +805,10 @@ static int prepare_to_commit(const char
>> *index_file, const char *prefix,
>> if (clean_message_contents)
>> strbuf_stripspace(&sb, 0);
>>
>> + for (i = 0; i < coauthors.nr; i++) {
>> + append_coauthor(&sb, coauthors.items[i].string);
>
> If you look at the existing code that's calling append_signoff() it does
> append_signoff(&sb, ignore_non_trailer(sb.buf, sb.len), 0)
> The purpose of ignore_non_trailer is to ignore comment and conflicts
> messages at the end of the commit message (there's more detail in a
> comment above it's definition in builtin/commit.c). I think we need to
> pass this offset when adding co-authors as well.
>
> One question - what is the desired de-duplication behavior of
> Co-authored-by:? What happens if there is already a matching
> Co-authored-by: footer? (It is also possible for the trailers code to
> only ignore an existing footer if it's the last one.) What happens if
> the same Co-authored-by: is duplicated on the command line? It would be
> nice to have this defined and tests to check it's enforced.
I should give a bit more detail here. git-interpret-trailers gives more
control over handling of duplicates that is configurable via 'git
config' than 'commit --signoff' does. The reason for this is that
'commit --signoff' predates the interpret-trailers stuff. As we're
adding a new footer command we should decide if we want it to act like
--signoff or give the user the ability to configure the de-duplication
behavior by using the interpret-trailers code path instead. (I think
format-patch --signoff respects the interpret-trailers config variables
but 'am --signoff' and 'cherry-pick --signoff' do not.
>
> Another useful addition would be to check that the footer value looks
> sane but that could come later
Looking at the way commit handles --author (grep for force_author in
builtin/commit.c) it should be simple to add these checks - just call
split_ident() and check it returns zero. --author also checks if the
string contains '<' and if it doesn't it uses the string given on the
command line to lookup a matching author in the commit log - that could
be a nice feature to use here too (see the code that calls
find_author_by_nickname()).
Best Wishes
Phillip
- I don't think we do that for any other
> footers at the moment (though I haven't checked to see if that's really
> true)
>
>> + }
>> +
>> if (signoff)
>> append_signoff(&sb, ignore_non_trailer(sb.buf, sb.len), 0);
>>
>> @@ -1504,6 +1510,7 @@ int cmd_commit(int argc, const char **argv,
>> const char *prefix)
>> OPT_STRING(0, "squash", &squash_message, N_("commit"),
>> N_("use autosquash formatted message to squash specified commit")),
>> OPT_BOOL(0, "reset-author", &renew_authorship, N_("the
>> commit is authored by me now (used with -C/-c/--amend)")),
>> OPT_BOOL('s', "signoff", &signoff, N_("add Signed-off-by:")),
>> + OPT_STRING_LIST(0, "coauthor", &coauthors, N_("co-author"),
>> N_("add Co-authored-by:")),
>> OPT_FILENAME('t', "template", &template_file, N_("use
>> specified template file")),
>> OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
>> OPT_CLEANUP(&cleanup_arg),
>> diff --git a/sequencer.c b/sequencer.c
>> index d648aaf416..8958a22470 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -36,6 +36,7 @@
>> #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>>
>> static const char sign_off_header[] = "Signed-off-by: ";
>> +static const char coauthor_header[] = "Co-authored-by: ";
>> static const char cherry_picked_prefix[] = "(cherry picked from
>> commit ";
>>
>> GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG")
>> @@ -4385,15 +4386,9 @@ int sequencer_pick_revisions(struct repository *r,
>> return res;
>> }
>>
>> -void append_signoff(struct strbuf *msgbuf, size_t ignore_footer,
>> unsigned flag)
>> +static void append_footer(struct strbuf *msgbuf, struct strbuf* sob,
>> size_t ignore_footer, size_t no_dup_sob)
>> {
>> - unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;
>> - struct strbuf sob = STRBUF_INIT;
>> - int has_footer;
>> -
>> - strbuf_addstr(&sob, sign_off_header);
>> - strbuf_addstr(&sob, fmt_name(WANT_COMMITTER_IDENT));
>> - strbuf_addch(&sob, '\n');
>> + size_t has_footer;
>>
>> if (!ignore_footer)
>> strbuf_complete_line(msgbuf);
>> @@ -4402,11 +4397,11 @@ void append_signoff(struct strbuf *msgbuf,
>> size_t ignore_footer, unsigned flag)
>> * If the whole message buffer is equal to the sob, pretend that we
>> * found a conforming footer with a matching sob
>> */
>> - if (msgbuf->len - ignore_footer == sob.len &&
>> - !strncmp(msgbuf->buf, sob.buf, sob.len))
>> + if (msgbuf->len - ignore_footer == sob->len &&
>> + !strncmp(msgbuf->buf, sob->buf, sob->len))
>> has_footer = 3;
>> else
>> - has_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);
>> + has_footer = has_conforming_footer(msgbuf, sob, ignore_footer);
>>
>> if (!has_footer) {
>> const char *append_newlines = NULL;
>> @@ -4440,7 +4435,32 @@ void append_signoff(struct strbuf *msgbuf,
>> size_t ignore_footer, unsigned flag)
>>
>> if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
>> strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
>> - sob.buf, sob.len);
>> + sob->buf, sob->len);
>> +}
>> +
>> +void append_signoff(struct strbuf *msgbuf, size_t ignore_footer,
>> unsigned flag)
>> +{
>> + unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;
>> + struct strbuf sob = STRBUF_INIT;
>> +
>> + strbuf_addstr(&sob, sign_off_header);
>> + strbuf_addstr(&sob, fmt_name(WANT_COMMITTER_IDENT));
>> + strbuf_addch(&sob, '\n');
>> +
>> + append_footer(msgbuf, &sob, ignore_footer, no_dup_sob);
>> +
>> + strbuf_release(&sob);
>> +}
>> +
>> +void append_coauthor(struct strbuf *msgbuf, const char *coauthor)
>> +{
>> + struct strbuf sob = STRBUF_INIT;
>> +
>> + strbuf_addstr(&sob, coauthor_header);
>> + strbuf_addstr(&sob, coauthor);
>> + strbuf_addch(&sob, '\n');
>> +
>> + append_footer(msgbuf, &sob, 0, 1);
>
> As we have a constant for APPEND_SIGNOFF_DEDUP can we use it here please
> rather than '1' which does not covey the same meaning to the author.
> Also as I said above I think you want to pass in a real value for
> ignore_footer not zero
>
>>
>> strbuf_release(&sob);
>> }
>> diff --git a/sequencer.h b/sequencer.h
>> index 574260f621..e36489fce7 100644
>> --- a/sequencer.h
>> +++ b/sequencer.h
>> @@ -170,6 +170,8 @@ int todo_list_rearrange_squash(struct todo_list
>> *todo_list);
>> */
>> void append_signoff(struct strbuf *msgbuf, size_t ignore_footer,
>> unsigned flag);
>>
>> +void append_coauthor(struct strbuf *msgbuf, const char* co_author);
>> +
>> void append_conflicts_hint(struct index_state *istate,
>> struct strbuf *msgbuf, enum commit_msg_cleanup_mode
>> cleanup_mode);
>> enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
>> diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
>> index 14c92e4c25..5ed6735cf4 100755
>> --- a/t/t7502-commit-porcelain.sh
>> +++ b/t/t7502-commit-porcelain.sh
>> @@ -138,6 +138,17 @@ test_expect_success 'partial removal' '
>>
>> '
>>
>> +test_expect_success 'co-author' '
>> +
>> + >coauthor &&
>> + git add coauthor &&
>> + git commit -m "thank you" --co-author="Author
>> <author@xxxxxxxxxxx>" &&
>> + git cat-file commit HEAD >commit.msg &&
>> + sed -ne "s/Co-authored-by: //p" commit.msg >actual &&
>> + echo "Author <author@xxxxxxxxxxx>" >expected &&
>> + test_cmp expected actual
>> +'
>
> This is fine as far as it goes but it would be nice to test the
> de-duplication behavior once that's defined
>
> Best Wishes
>
> Phillip
>
>> test_expect_success 'sign off' '
>>
>> >positive &&
>> --
>> 2.22.0.rc3
>>
