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

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

[Toon Claes]

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.

Co-authored-by: Zeger-Jan van de Weg <git@zjvandeweg.nl>
Signed-off-by: Toon Claes <toon@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-artur@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 (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);

 	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@example.com>" &&
+	git cat-file commit HEAD >commit.msg &&
+	sed -ne "s/Co-authored-by: //p" commit.msg >actual &&
+	echo "Author <author@example.com>" >expected &&
+	test_cmp expected actual
+'
+
 test_expect_success 'sign off' '

 	>positive &&
--
2.22.0.rc3

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

[Denton Liu]

Welcome to the Git community, Toon!

Wow, I never realised that people actually read my braindump of issues
on GGG. Thanks for taking this on.

First some housekeeping, when formatting your patches, it's a good idea
to use `git format-patch --thread` so that your patches are grouped
together by thread.

On Tue, Oct 08, 2019 at 09:49:35AM +0200, 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

I know you're taking the name from my GGG issue but perhaps --co-author
would look better? 

> 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.
> 
> Co-authored-by: Zeger-Jan van de Weg <git@zjvandeweg.nl>

Nice ;)

> Signed-off-by: Toon Claes <toon@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

This line is indented with spaces but it should be changed to a single
tab.

> +	author using the standard `Co Artur <co-artur@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 (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)

The * for pointers should go with the name, not the type. Also, `sob` is
a misleading name since it means "Signed-off-by". In this case, we're
using it as a general footer (since it can include Co-author lines too)
so perhaps rename this to `footer`? 

Finally, as a nit, can we mark sob as const?

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

Why was this changed into a size_t? has_conforming_footer() below
returns int so we should leave it as is.

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

Since you're touching this area, could you please a prepatory patch
before this one that changes the function signature:

	- static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob, size_t ignore_footer)                                                          
	+ static int has_conforming_footer(struct strbuf *sb, const struct strbuf *footer, size_t ignore_footer)                                                          

That way, you'll be able to mark `sob` const in append_footer() and
also, `sob` can be renamed to `footer` which should be less misleading.

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

Same, this `sob` should definitely be written as `footer` or maybe
`coauthor`.
> +
> +	strbuf_addstr(&sob, coauthor_header);
> +	strbuf_addstr(&sob, coauthor);
> +	strbuf_addch(&sob, '\n');
> +
> +	append_footer(msgbuf, &sob, 0, 1);
> 
>  	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);

Asterisk should also be attached to the name.

> +
>  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@example.com>" &&
> +	git cat-file commit HEAD >commit.msg &&
> +	sed -ne "s/Co-authored-by: //p" commit.msg >actual &&
> +	echo "Author <author@example.com>" >expected &&
> +	test_cmp expected actual
> +'
> +

This test looks good to me.

Thanks again for taking this on,

Denton

>  test_expect_success 'sign off' '
> 
>  	>positive &&
> --
> 2.22.0.rc3

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

[Phillip Wood]

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 <git@zjvandeweg.nl>
> Signed-off-by: Toon Claes <toon@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-artur@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 <author@example.com>" &&
> +	git cat-file commit HEAD >commit.msg &&
> +	sed -ne "s/Co-authored-by: //p" commit.msg >actual &&
> +	echo "Author <author@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
> 

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

[Phillip Wood]

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@example.com>" &&
>> +    git cat-file commit HEAD >commit.msg &&
>> +    sed -ne "s/Co-authored-by: //p" commit.msg >actual &&
>> +    echo "Author <author@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
>>

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

[SZEDER Gábor]

On Tue, Oct 08, 2019 at 09:49:35AM +0200, 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.

I wonder how we are supposed to use this trailer in the Git project,
in particular in combination with Signed-off-by.  Should all
(co)authors sign off as well?  Or will Co-authored-by imply
Signed-off-by?

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

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Tue, Oct 08, 2019 at 09:49:35AM +0200, 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.
>
> I wonder how we are supposed to use this trailer in the Git project,
> in particular in combination with Signed-off-by.  Should all
> (co)authors sign off as well?  Or will Co-authored-by imply
> Signed-off-by?

I think we have been happy with (1) a comment at the end of the log
message that says X worked together with Y and Z to produce this
patch, and (2) the trailer block that has S-o-b: from X, Y and Z,
without any need for Co-authored-by: trailer so far, and I do not
see any reason to change it in this project.

If other projects wants to use such a footer, that's their choice,
but I am fairly negative to the idea to open the gate to unbounded
number of new options for new trailer lines.  We do not even have
such options like --acked=<acker>, --reported=<reporter>, for the
trailers that are actively used already (and to make sure nobody
misunderstands, I do not think it is a good idea to add such
individual options).

Thanks.

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1541 bytes --]

On 2019-10-09 at 02:19:47, Junio C Hamano wrote:
> I think we have been happy with (1) a comment at the end of the log
> message that says X worked together with Y and Z to produce this
> patch, and (2) the trailer block that has S-o-b: from X, Y and Z,
> without any need for Co-authored-by: trailer so far, and I do not
> see any reason to change it in this project.
> 
> If other projects wants to use such a footer, that's their choice,
> but I am fairly negative to the idea to open the gate to unbounded
> number of new options for new trailer lines.  We do not even have
> such options like --acked=<acker>, --reported=<reporter>, for the
> trailers that are actively used already (and to make sure nobody
> misunderstands, I do not think it is a good idea to add such
> individual options).

If we don't want to add this option explicitly, which I understand, it
may be useful to add an option to add arbitrary commit trailers to a
commit message when creating it.

Git uses a patch-based workflow, where adding trailers is relatively
easy, but for folks using pull request-based workflows without signoffs,
using Co-authored-by can be a great way to note when folks have worked
together on a patch.  An option to git commit such as --trailer would
allow folks to add whatever trailers they wish, including this one,
without us needing to bless particular options.

I, for one, would be excited to see a series which did this.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

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

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> ...  An option to git commit such as --trailer would
> allow folks to add whatever trailers they wish, including this one,
> without us needing to bless particular options.

Yes, that was what I was hoping to become the "core" of the idea,
with possibly syntax sugar to make it easier to use.

Thanks for filling in what I left unsaid ;-)

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

[Jeff King]

On Wed, Oct 09, 2019 at 11:19:47AM +0900, Junio C Hamano wrote:

> > I wonder how we are supposed to use this trailer in the Git project,
> > in particular in combination with Signed-off-by.  Should all
> > (co)authors sign off as well?  Or will Co-authored-by imply
> > Signed-off-by?
> 
> I think we have been happy with (1) a comment at the end of the log
> message that says X worked together with Y and Z to produce this
> patch, and (2) the trailer block that has S-o-b: from X, Y and Z,
> without any need for Co-authored-by: trailer so far, and I do not
> see any reason to change it in this project.

One advantage to making a machine-readable version is that tools on the
reading side can then count contributions, etc. For instance:

  https://github.com/git/git/commit/69f272b922df153c86db520bf9b6018a9808c2a6

shows all of the co-author avatars, and you can click through to their
pages.

> If other projects wants to use such a footer, that's their choice,
> but I am fairly negative to the idea to open the gate to unbounded
> number of new options for new trailer lines.  We do not even have
> such options like --acked=<acker>, --reported=<reporter>, for the
> trailers that are actively used already (and to make sure nobody
> misunderstands, I do not think it is a good idea to add such
> individual options).

Yeah, I'd agree that we should start first with a generic trailer line.
There might be some advantage to building trailer-specific intelligence
on top of that (for instance, it would be nice for coauthor trailers to
expand names the way --author does). But that can come after, and might
not even be in the form of specific command-line options. E.g., if the
coauthor trailer could be marked in config as "this is an ident", then
we we would know to expand it. And the same could apply to acked,
reported, etc.

-Peff

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

[Toon Claes]

[-- Attachment #1: Type: text/plain, Size: 2402 bytes --]

Jeff King <peff@peff.net> writes: 
 
> Yeah, I'd agree that we should start first with a generic 
> trailer line.

IIUC you are suggesting something like this?

  git commit --trailer="Co-authored-by: <coauthor>"

I really want to consider this, but I do not understand how that 
improves the user experience compared to adding that trailer 
manually when typing the commit message in your $EDITOR?

> There might be some advantage to building trailer-specific 
> intelligence on top of that (for instance, it would be nice for 
> coauthor trailers to expand names the way --author does). But 
> that can come after, and might not even be in the form of 
> specific command-line options. E.g., if the coauthor trailer 
> could be marked in config as "this is an ident", then we we 
> would know to expand it. And the same could apply to acked, 
> reported, etc.

Wouldn't making it a generic --trailer option make this more 
complex? I can image users might even want to use the --trailer 
argument to indicate which issue the commit closes:

  git commit --trailer="Closes: $BUGNUMBER"

So, how can we make the config understand it has to expand 
Co-authored-by and not Closes?

Maybe, because I was looking at 
https://git.wiki.kernel.org/index.php/CommitMessageConventions#Trailers,
it will probably be safe to expand a name when a '-by' suffix is 
used.

With this pattern in place there is a slight improvement compared 
to typing the trailer in your $EDITOR, because the user can pass 
--trailer="Anything-by: name" and the trailer is expanded to 
`Anything-by: name <name@example.com>".

But I'd like to note another thing, and let me circle back to 
SZEDER Gábor's reply:

> I wonder how we are supposed to use this trailer in the Git 
> project, in particular in combination with Signed-off-by. 
> Should all (co)authors sign off as well?  Or will Co-authored-by 
> imply Signed-off-by?

For this purpose I think it's useful git understands what 
"Co-authored-by" means, so when you run: 

  git commit -s --coauthor=<coauthor>

The following trailer will be added:

  Co-authored-by: <coauthor>
  Signed-off-by: <author>
  Signed-off-by: <coauthor>

So I'm still pro of adding a --co-author option, but I do 
understand the concerns to avoid adding an option for all the 
possible trailers found in the link above.

-- 
Toon

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Johannes Schindelin]

Hi,

On Wed, 9 Oct 2019, Jeff King wrote:

> On Wed, Oct 09, 2019 at 11:19:47AM +0900, Junio C Hamano wrote:
>
> > > I wonder how we are supposed to use this trailer in the Git project,
> > > in particular in combination with Signed-off-by.  Should all
> > > (co)authors sign off as well?  Or will Co-authored-by imply
> > > Signed-off-by?
> >
> > I think we have been happy with (1) a comment at the end of the log
> > message that says X worked together with Y and Z to produce this
> > patch, and (2) the trailer block that has S-o-b: from X, Y and Z,
> > without any need for Co-authored-by: trailer so far, and I do not
> > see any reason to change it in this project.
>
> One advantage to making a machine-readable version is that tools on the
> reading side can then count contributions, etc. For instance:
>
>   https://github.com/git/git/commit/69f272b922df153c86db520bf9b6018a9808c2a6
>
> shows all of the co-author avatars, and you can click through to their
> pages.

FWIW I really like this. It bugged me ever since that GitMerge talk
(https://www.youtube.com/watch?v=usQgAy8YDVA) that we did not have any
standardized way to document co-authored commits.

> > If other projects wants to use such a footer, that's their choice,
> > but I am fairly negative to the idea to open the gate to unbounded
> > number of new options for new trailer lines.  We do not even have
> > such options like --acked=<acker>, --reported=<reporter>, for the
> > trailers that are actively used already (and to make sure nobody
> > misunderstands, I do not think it is a good idea to add such
> > individual options).
>
> Yeah, I'd agree that we should start first with a generic trailer line.
> There might be some advantage to building trailer-specific intelligence
> on top of that (for instance, it would be nice for coauthor trailers to
> expand names the way --author does). But that can come after, and might
> not even be in the form of specific command-line options. E.g., if the
> coauthor trailer could be marked in config as "this is an ident", then
> we we would know to expand it. And the same could apply to acked,
> reported, etc.

Yep, and we have to start somewhere. I think this patch is a good start.

FWIW I would not even mind introducing the synonym `--co-author` for
`--coauthor`. But that's just a very minor suggestion.

Ciao,
Dscho

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

[Jeff King]

On Thu, Oct 10, 2019 at 10:49:23AM +0200, Toon Claes wrote:

> > Yeah, I'd agree that we should start first with a generic trailer line.
> 
> IIUC you are suggesting something like this?
> 
>  git commit --trailer="Co-authored-by: <coauthor>"
> 
> I really want to consider this, but I do not understand how that improves
> the user experience compared to adding that trailer manually when typing the
> commit message in your $EDITOR?

I agree that it's a lot worse to type than "--coauthor". And I don't
really have a problem with us ending up with "--coauthor". My reasoning
in starting with a generic form was mostly:

  - by having _any_ way to do this on the command-line, it makes it
    possible to use in aliases, etc.

  - having a generic form, even if we later add syntactic sugar on
    top, lets people easily experiment with their own trailers

> > There might be some advantage to building trailer-specific intelligence
> > on top of that (for instance, it would be nice for coauthor trailers to
> > expand names the way --author does). But that can come after, and might
> > not even be in the form of specific command-line options. E.g., if the
> > coauthor trailer could be marked in config as "this is an ident", then
> > we we would know to expand it. And the same could apply to acked,
> > reported, etc.
> 
> Wouldn't making it a generic --trailer option make this more complex? I can
> image users might even want to use the --trailer argument to indicate which
> issue the commit closes:
> 
>  git commit --trailer="Closes: $BUGNUMBER"
> 
> So, how can we make the config understand it has to expand Co-authored-by
> and not Closes?

We already have config blocks for specific trailers to describe how they
should be parsed or added. I was thinking that you'd set an option like
"trailer.co-authored-by.ident" to "true". And possibly that could be
used in other places, too (e.g., interpret-trailers code could make sure
it's syntactically valid, but I didn't really think through the
implications there).

And of course we could bake in the defaults for particular trailers if
we wanted to (I think we already do for trailer.signoff.*).

> > I wonder how we are supposed to use this trailer in the Git project, in
> > particular in combination with Signed-off-by. Should all (co)authors
> > sign off as well?  Or will Co-authored-by imply Signed-off-by?
> 
> For this purpose I think it's useful git understands what "Co-authored-by"
> means, so when you run:
> 
>  git commit -s --coauthor=<coauthor>
> 
> The following trailer will be added:
> 
>  Co-authored-by: <coauthor>
>  Signed-off-by: <author>
>  Signed-off-by: <coauthor>
> 
> So I'm still pro of adding a --co-author option, but I do understand the
> concerns to avoid adding an option for all the possible trailers found in
> the link above.

Yes, I agree that ordering and de-duplication rules are useful, too.
Some of that can be expressed already in trailer.* config, but I don't
know if it would be capable enough to do everything you want (though
again, it would be really nice to _make_ it capable enough so that other
types besides co-authored-by can make use of them).

I don't have a hard belief that we have to do it that way (generic
before specific), and I can believe that when you get down to the
details that it might be hard to express some of this stuff in config
rather than C code. But I think we should at least take a look at
whether it's possible, because the benefits of having a generic solution
are nice.

-Peff

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

[Denton Liu]

On Thu, Oct 10, 2019 at 01:49:03PM +0200, Johannes Schindelin wrote:
> Hi,
> 
> On Wed, 9 Oct 2019, Jeff King wrote:
> 
> > On Wed, Oct 09, 2019 at 11:19:47AM +0900, Junio C Hamano wrote:
> >
> > > > I wonder how we are supposed to use this trailer in the Git project,
> > > > in particular in combination with Signed-off-by.  Should all
> > > > (co)authors sign off as well?  Or will Co-authored-by imply
> > > > Signed-off-by?
> > >
> > > I think we have been happy with (1) a comment at the end of the log
> > > message that says X worked together with Y and Z to produce this
> > > patch, and (2) the trailer block that has S-o-b: from X, Y and Z,
> > > without any need for Co-authored-by: trailer so far, and I do not
> > > see any reason to change it in this project.
> >
> > One advantage to making a machine-readable version is that tools on the
> > reading side can then count contributions, etc. For instance:
> >
> >   https://github.com/git/git/commit/69f272b922df153c86db520bf9b6018a9808c2a6
> >
> > shows all of the co-author avatars, and you can click through to their
> > pages.

Yep, this is the reason why I raised the suggestion[1] in the
first place. Since special support for this trailer is implemented in
GitHub (and GitLab too, as I recently learned), I think this could be
considered a defacto standard for co-authored commits.

> 
> FWIW I really like this. It bugged me ever since that GitMerge talk
> (https://www.youtube.com/watch?v=usQgAy8YDVA) that we did not have any
> standardized way to document co-authored commits.

Yep, and this isn't the first time this has been brought up. I remember
stumbling on this thread[2] a while back about someone asking for
co-author functionality by default so it would be nice to have a
standard way of doing it.

Thanks,

Denton

[1]: https://github.com/gitgitgadget/git/issues/343
[2]: https://public-inbox.org/git/CAOvwQ4i_HL7XGnxZrVu3oSnsbnTyxbg8Vh6vzi4c1isSrrexYQ@mail.gmail.com/

> 
> > > If other projects wants to use such a footer, that's their choice,
> > > but I am fairly negative to the idea to open the gate to unbounded
> > > number of new options for new trailer lines.  We do not even have
> > > such options like --acked=<acker>, --reported=<reporter>, for the
> > > trailers that are actively used already (and to make sure nobody
> > > misunderstands, I do not think it is a good idea to add such
> > > individual options).
> >
> > Yeah, I'd agree that we should start first with a generic trailer line.
> > There might be some advantage to building trailer-specific intelligence
> > on top of that (for instance, it would be nice for coauthor trailers to
> > expand names the way --author does). But that can come after, and might
> > not even be in the form of specific command-line options. E.g., if the
> > coauthor trailer could be marked in config as "this is an ident", then
> > we we would know to expand it. And the same could apply to acked,
> > reported, etc.
> 
> Yep, and we have to start somewhere. I think this patch is a good start.
> 
> FWIW I would not even mind introducing the synonym `--co-author` for
> `--coauthor`. But that's just a very minor suggestion.
> 
> Ciao,
> Dscho

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 964 bytes --]

On 2019-10-10 at 08:49:23, Toon Claes wrote:
> Jeff King <peff@peff.net> writes:
> 
> > Yeah, I'd agree that we should start first with a generic trailer line.
> 
> IIUC you are suggesting something like this?
> 
>  git commit --trailer="Co-authored-by: <coauthor>"
> 
> I really want to consider this, but I do not understand how that improves
> the user experience compared to adding that trailer manually when typing the
> commit message in your $EDITOR?

The --trailer option to git interpret-trailers knows how to interpret
configuration options and expand them.  For example, you could
abbreviate "Co-authored-by" as "cab", and if you used that alias, you
could write "git commit --trailer='cab=peff'" and then have your command
look up "peff" and find the proper identification either from your repo
or from your Git hosting solution of choice (or wherever).
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yes, I agree that ordering and de-duplication rules are useful, too.
> Some of that can be expressed already in trailer.* config, but I don't
> know if it would be capable enough to do everything you want (though
> again, it would be really nice to _make_ it capable enough so that other
> types besides co-authored-by can make use of them).

Yup, absolutely.

As a mature system, unlike the early days, randomly adding a
"desired" feature without first considering if a generalized form
can be added is simply irresponsible to our users.  Response by
Brian on the side thread also indicates that we have already spent
enough effort to the generalized trailer support and building on
top, instead of randomly adding an ad-hoc "feature", may be the
right and feasible approach.