Re: [PATCH 1/1] commit: add support to provide --coauthor

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
>>




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux