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.


Co-authored-by: Zeger-Jan van de Weg <g...@zjvandeweg.nl>
Signed-off-by: Toon Claes <t...@iotcl.com>
---
  Documentation/git-commit.txt |  5 ++++
  builtin/commit.c             |  7 ++++++
  sequencer.c                  | 44 ++++++++++++++++++++++++++----------
  sequencer.h                  |  2 ++
  t/t7502-commit-porcelain.sh  | 11 +++++++++
  5 files changed, 57 insertions(+), 12 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index afa7b75a23..c059944e38 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -140,6 +140,11 @@ OPTIONS
        commit by that author (i.e. rev-list --all -i --author=<author>);
        the commit author is then copied from the first such commit found.

+--coauthor=<coauthor>::
+        Add a Co-authored-by line with the specified author. Specify the
+       author using the standard `Co Artur <co-ar...@example.com>`
+       format.
+
  --date=<date>::
        Override the author date used in the commit.

diff --git a/builtin/commit.c b/builtin/commit.c
index ae7aaf6dc6..feb423ed6f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -110,6 +110,7 @@ static int config_commit_verbose = -1; /* unspecified */
  static int no_post_rewrite, allow_empty_message;
  static char *untracked_files_arg, *force_date, *ignore_submodule_arg, 
*ignored_arg;
  static char *sign_commit;
+static struct string_list coauthors = STRING_LIST_INIT_NODUP;

  /*
   * The default commit message cleanup mode will remove the lines
@@ -672,6 +673,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
        int clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);
        int old_display_comment_prefix;
        int merge_contains_scissors = 0;
+       int i;

        /* This checks and barfs if author is badly specified */
        determine_author_info(author_ident);
@@ -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.

Another useful addition would be to check that the footer value looks sane but that could come later - 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 <aut...@example.com>" &&
+       git cat-file commit HEAD >commit.msg &&
+       sed -ne "s/Co-authored-by: //p" commit.msg >actual &&
+       echo "Author <aut...@example.com>" >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

Reply via email to