threads / patch / 46305

patcharea: git-merge: add --signoff flag to git-merge

Subject: [PATCH] area: git-merge: add --signoff flag to git-merge

## tl;dr

15 messages between Jul 3, 2017 and Jul 26, 2017. Diffs are folded; open one to read it.

replies: 14people: 4as markdown or json

Łukasz Gryglicki· Jul 3, 2017, 09:57 UTC · lore

Added --signoff flag to `git-merge` command, added test coverage, updated documentation.

Signed-off-by: lukaszgryglicki <lukaszgryglicki@o2.pl>
---
 Documentation/git-merge.txt  |  8 ++++++
 builtin/merge.c              |  4 +++
 t/t9904-git-merge-signoff.sh | 60 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+)
 create mode 100755 t/t9904-git-merge-signoff.sh
Show changes to 3 files +72 −1

Documentation/git-merge.txt, builtin/merge.c, t/t9904-git-merge-signoff.sh

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 04fdd8cf086db..6b308ab6d0b52 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -64,6 +64,14 @@ OPTIONS
 -------
 include::merge-options.txt[]
 
+--signoff::
+	Add Signed-off-by line by the committer at the end of the commit
+	log message.  The meaning of a signoff depends on the project,
+	but it typically certifies that committer has
+	the rights to submit this work under the same license and
+	agrees to a Developer Certificate of Origin
+	(see http://developercertificate.org/ for more information).
+
 -S[<keyid>]::
 --gpg-sign[=<keyid>]::
 	GPG-sign the resulting merge commit. The `keyid` argument is
diff --git a/builtin/merge.c b/builtin/merge.c
index 900bafdb45d0b..cb09f4138136b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -70,6 +70,7 @@ static int continue_current_merge;
 static int allow_unrelated_histories;
 static int show_progress = -1;
 static int default_to_upstream = 1;
+static int signoff;
 static const char *sign_commit;
 
 static struct strategy all_strategy[] = {
@@ -233,6 +234,7 @@ static struct option builtin_merge_options[] = {
 	{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"),
 	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
+	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
 	OPT_END()
 };
 
@@ -775,6 +777,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	strbuf_stripspace(&msg, 0 < option_edit);
 	if (!msg.len)
 		abort_commit(remoteheads, _("Empty commit message."));
+	if (signoff)
+		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
 	strbuf_release(&merge_msg);
 	strbuf_addbuf(&merge_msg, &msg);
 	strbuf_release(&msg);
diff --git a/t/t9904-git-merge-signoff.sh b/t/t9904-git-merge-signoff.sh
new file mode 100755
index 0000000000000..eed15b9a85371
--- /dev/null
+++ b/t/t9904-git-merge-signoff.sh
@@ -0,0 +1,60 @@
+#!/bin/sh
+
+test_description='git merge --signoff
+
+This test runs git merge --signoff and make sure that it works.
+'
+
+. ./test-lib.sh
+
+# A simple files to commit
+cat >file1 <<EOF
+1
+EOF
+
+cat >file2 <<EOF
+2
+EOF
+
+cat >file3 <<EOF
+3
+EOF
+
+# Expected commit message after merge --signoff
+cat >expected-signed <<EOF
+Merge branch 'master' into other-branch
+
+Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/")
+EOF
+
+# Expected commit message after merge without --signoff (or with --no-signoff)
+cat >expected-unsigned <<EOF
+Merge branch 'master' into other-branch
+EOF
+
+
+# We configure an alias to do the merge --signoff so that
+# on the next subtest we can show that --no-signoff overrides the alias
+test_expect_success 'merge --signoff adds a sign-off line' '
+	git commit --allow-empty -m "Initial empty commit" &&
+  git checkout -b other-branch &&
+	git add file1 && git commit -m other-branch &&
+  git checkout master &&
+	git add file2 && git commit -m master-branch &&
+  git checkout other-branch &&
+  git config alias.msob "merge --signoff --no-edit" &&
+	git msob master &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
+	test_cmp expected-signed actual
+'
+
+test_expect_success 'master --no-signoff does not add a sign-off line' '
+	git checkout master &&
+  git add file3 && git commit -m master-branch-2 &&
+  git checkout other-branch &&
+	git msob --no-signoff master &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
+	test_cmp expected-unsigned actual
+'
+
+test_done

--
https://github.com/git/git/pull/381
Junio C Hamano· Jul 3, 2017, 17:57 UTC · re: Łukasz Gryglicki · lore

Re: [PATCH] area: git-merge: add --signoff flag to git-merge

Łukasz Gryglicki <lukaszgryglicki@o2.pl> writes:
> Subject: Re: [PATCH] area: git-merge: add --signoff flag to git-merge
s/area: //; 
> Added --signoff flag to `git-merge` command, added test coverage,
> updated documentation.

That can be seen from the title and the patch text. While it is not wrong to list what you did, that shouldn't be the only thing the log records. Explain the problem the patch attempts to fix, why it is a problem, and then give orders to the code to "be like so".

	[PATCH] git-merge: honor --signoff flag
	The "Signed-off-by:" line is a social convention to certify
	that you are legally allowed to do so when you are giving
	changes to or recording changes for the project.  
	The "git commit" command has a handy option "--signoff" to
	add it under your name, because committing is the primary
	way for you to record your changes (which may later be sent
	to the project in a patch form).
	On the other hand, the "git merge" command does not.  [You
	make an argument to justify why merge commits also should
	have signoffs here].
	Teach "git merge" to honor "--signoff" just like "git commit"
	does.

or something like that. It is a norm for a new feature to have documentation and test, so it is of lessor importance to say "Add tests and document the new feature" (on the other hand, those who do not test and document need to justify their omission).

Alternatively, if the problem is so obvious that it does not need to be explained, the solution often does not need more explanation than the patch title.

I this case, I think the "problem" is not that obvious; the need for signing off a merge commit deserves explanation in the log message.

> Signed-off-by: lukaszgryglicki <lukaszgryglicki@o2.pl>
> ---

"Signed-off-by: Łukasz Gryglicki <lukaszgryglicki@o2.pl>", like you wrote on your "From:" e-mail header.

Show 22 quoted lines
>  Documentation/git-merge.txt  |  8 ++++++
>  builtin/merge.c              |  4 +++
>  t/t9904-git-merge-signoff.sh | 60 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 72 insertions(+)
>  create mode 100755 t/t9904-git-merge-signoff.sh
>
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index 04fdd8cf086db..6b308ab6d0b52 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -64,6 +64,14 @@ OPTIONS
>  -------
>  include::merge-options.txt[]
>  
> +--signoff::
> +	Add Signed-off-by line by the committer at the end of the commit
> +	log message.  The meaning of a signoff depends on the project,
> +	but it typically certifies that committer has
> +	the rights to submit this work under the same license and
> +	agrees to a Developer Certificate of Origin
> +	(see http://developercertificate.org/ for more information).
> +
Good description.
Show 26 quoted lines
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 900bafdb45d0b..cb09f4138136b 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -70,6 +70,7 @@ static int continue_current_merge;
>  static int allow_unrelated_histories;
>  static int show_progress = -1;
>  static int default_to_upstream = 1;
> +static int signoff;
>  static const char *sign_commit;
>  
>  static struct strategy all_strategy[] = {
> @@ -233,6 +234,7 @@ static struct option builtin_merge_options[] = {
>  	{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"),
>  	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
>  	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
> +	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
>  	OPT_END()
>  };
>  
> @@ -775,6 +777,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)
>  	strbuf_stripspace(&msg, 0 < option_edit);
>  	if (!msg.len)
>  		abort_commit(remoteheads, _("Empty commit message."));
> +	if (signoff)
> +		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
I think this is a wrong place to do this, because 
  (1) it is too late to allow "prepare-commit-msg" to futz with it.
  (2) it is too late to allow the end user to further edit it.

A better place probably is before merge_editor_comment is added to the msg strbuf in the same function, but I didn't think it through.

Show 5 quoted lines
> diff --git a/t/t9904-git-merge-signoff.sh b/t/t9904-git-merge-signoff.sh
> new file mode 100755
> index 0000000000000..eed15b9a85371
> --- /dev/null
> +++ b/t/t9904-git-merge-signoff.sh

Do we need a new script? I think t7608 is about the messages in the merge commit.

Show 26 quoted lines
> +# A simple files to commit
> +cat >file1 <<EOF
> +1
> +EOF
> +
> +cat >file2 <<EOF
> +2
> +EOF
> +
> +cat >file3 <<EOF
> +3
> +EOF
>
> +
> +# Expected commit message after merge --signoff
> +cat >expected-signed <<EOF
> +Merge branch 'master' into other-branch
> +
> +Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/")
> +EOF
> +
> +# Expected commit message after merge without --signoff (or with --no-signoff)
> +cat >expected-unsigned <<EOF
> +Merge branch 'master' into other-branch
> +EOF
> +

All of the above should be done inside the first set-up test so that they can be protected from errors.

> +
> +# We configure an alias to do the merge --signoff so that
> +# on the next subtest we can show that --no-signoff overrides the alias

Do we even need to risk these tests broken by an unrelated breakage to the alias mechanism? Wouldn't testing

	git merge --signoff --no-signoff ...

directly sufficient? The alias test may be a good thing to have _in addition to_ such a basic test, though.

Show 8 quoted lines
> +test_expect_success 'merge --signoff adds a sign-off line' '
> +	git commit --allow-empty -m "Initial empty commit" &&
> +  git checkout -b other-branch &&
> +	git add file1 && git commit -m other-branch &&
> +  git checkout master &&
> +	git add file2 && git commit -m master-branch &&
> +  git checkout other-branch &&
> +  git config alias.msob "merge --signoff --no-edit" &&
Strange indentation.
> +	git msob master &&
> +	git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
Style: no space between redirection and target.
Show 13 quoted lines
> +	test_cmp expected-signed actual
> +'
> +
> +test_expect_success 'master --no-signoff does not add a sign-off line' '
> +	git checkout master &&
> +  git add file3 && git commit -m master-branch-2 &&
> +  git checkout other-branch &&
> +	git msob --no-signoff master &&
> +	git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
> +	test_cmp expected-unsigned actual
> +'
> +
> +test_done

Other than the above issues, looks like a very well done patch for somebody who is posting a patch for the first time here. Welcome to Git development community.

Łukasz Gryglicki· Jul 4, 2017, 06:20 UTC · re: Łukasz Gryglicki · lore

[PATCH v2] add --signoff flag to `git-merge` command.

Some projects require every commit to be signed off. Our workflow is to create feature branches and require every commit to be signed off. When feature is finally approved we need to merge it into master. Merge itself is usually trivial and is done by `git merge origin/master`. Unfortunatelly this command have no --signoff flag, so we need to either add signoff line manually or use `git commit --amend -s` after the merge. First solution is not ideal because not all developers are familiar with exact sign-off syntax. The second solution works, but is obviously tedious. This patch adds --signoff support to git-merge command. It works just like --signoff in `git-commit` command. More details here: https://public-inbox.org/git/CAHv71zK5SqbwrBFX=a8-DY9H3KT4FEyMgv__p2gZzNr0WUAPUw@mail.gmail.com/T/#u

Signed-off-by: lukaszgryglicki <lukaszgryglicki@o2.pl>
---
 Documentation/git-merge.txt  |  8 +++++
 builtin/merge.c              |  4 +++
 t/t9904-git-merge-signoff.sh | 75 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+)
 create mode 100755 t/t9904-git-merge-signoff.sh
Show changes to 3 files +87 −1

Documentation/git-merge.txt, builtin/merge.c, t/t9904-git-merge-signoff.sh

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 04fdd8cf086db..6b308ab6d0b52 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -64,6 +64,14 @@ OPTIONS
 -------
 include::merge-options.txt[]
 
+--signoff::
+	Add Signed-off-by line by the committer at the end of the commit
+	log message.  The meaning of a signoff depends on the project,
+	but it typically certifies that committer has
+	the rights to submit this work under the same license and
+	agrees to a Developer Certificate of Origin
+	(see http://developercertificate.org/ for more information).
+
 -S[<keyid>]::
 --gpg-sign[=<keyid>]::
 	GPG-sign the resulting merge commit. The `keyid` argument is
diff --git a/builtin/merge.c b/builtin/merge.c
index 900bafdb45d0b..78c36e9bf353b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -70,6 +70,7 @@ static int continue_current_merge;
 static int allow_unrelated_histories;
 static int show_progress = -1;
 static int default_to_upstream = 1;
+static int signoff;
 static const char *sign_commit;
 
 static struct strategy all_strategy[] = {
@@ -233,6 +234,7 @@ static struct option builtin_merge_options[] = {
 	{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"),
 	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
+	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
 	OPT_END()
 };
 
@@ -763,6 +765,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	strbuf_addch(&msg, '\n');
 	if (0 < option_edit)
 		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
+	if (signoff)
+		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
 	write_file_buf(git_path_merge_msg(), msg.buf, msg.len);
 	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
 			    git_path_merge_msg(), "merge", NULL))
diff --git a/t/t9904-git-merge-signoff.sh b/t/t9904-git-merge-signoff.sh
new file mode 100755
index 0000000000000..f542f136f5dda
--- /dev/null
+++ b/t/t9904-git-merge-signoff.sh
@@ -0,0 +1,75 @@
+#!/bin/sh
+
+test_description='git merge --signoff
+
+This test runs git merge --signoff and makes sure that it works.
+'
+
+. ./test-lib.sh
+
+# Setup test files
+test_setup() {
+	# A simples files to commit
+	echo "1" >file1
+	echo "2" >file2
+	echo "3" >file3
+	echo "4" >file4
+
+	# Expected commit message after merge --signoff
+	cat >expected-signed <<EOF
+Merge branch 'master' into other-branch
+
+Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/")
+EOF
+
+	# Expected commit message after merge without --signoff (or with --no-signoff)
+	cat >expected-unsigned <<EOF
+Merge branch 'master' into other-branch
+EOF
+
+	# Initial commit and feature branch to merge master into it.
+	git commit --allow-empty -m "Initial empty commit"
+	git checkout -b other-branch
+	git add file1
+	git commit -m other-branch
+}
+
+# Setup repository, files & feature branch
+test_expect_success 'setup' '
+	test_setup
+'
+
+# Test with --signoff flag
+test_expect_success 'git merge --signoff adds a sign-off line' '
+	git checkout master &&
+	git add file2 &&
+	git commit -m master-branch &&
+	git checkout other-branch &&
+	git merge master --signoff --no-edit &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+	test_cmp expected-signed actual
+'
+
+# Test without --signoff flag
+test_expect_success 'git merge does not add a sign-off line' '
+	git checkout master &&
+	git add file3 &&
+	git commit -m master-branch-2 &&
+	git checkout other-branch &&
+	git merge master --no-edit &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+	test_cmp expected-unsigned actual
+'
+
+# Test for --no-signoff flag
+test_expect_success 'git merge --no-signoff flag cancels --signoff flag' '
+	git checkout master &&
+	git add file4 &&
+	git commit -m master-branch-3 &&
+	git checkout other-branch &&
+	git merge master --no-edit --signoff --no-signoff &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+	test_cmp expected-unsigned actual
+'
+
+test_done

--
https://github.com/git/git/pull/382
Ævar Arnfjörð Bjarmason· Jul 4, 2017, 08:33 UTC · re: Łukasz Gryglicki · lore

Re: [PATCH v2] add --signoff flag to `git-merge` command.

On Tue, Jul 04 2017, Łukasz Gryglicki jotted:
> add --signoff flag to `git-merge` command.
We'd usually say this as:
merge: add a --signoff flag
Or something like that.
Show 11 quoted lines
> Some projects require every commit to be signed off.
> Our workflow is to create feature branches and require every commit to
> be signed off. When feature is finally approved we need to merge it into
> master. Merge itself is usually trivial and is done by
> `git merge origin/master`. Unfortunatelly this command have no --signoff
> flag, so we need to either add signoff line manually or use
> `git commit --amend -s` after the merge. First solution is not ideal
> because not all developers are familiar with exact sign-off syntax.
> The second solution works, but is obviously tedious.
> This patch adds --signoff support to git-merge command. It works just
> like --signoff in `git-commit` command.

It would be nice to split this into a at least a couple of paragraphs, and more closely follow the format suggested by Documentation/SubmittingPatches.

> More details here:
> https://public-inbox.org/git/CAHv71zK5SqbwrBFX=a8-DY9H3KT4FEyMgv__p2gZzNr0WUAPUw@mail.gmail.com/T/#u

These more details include my outstanding question in 87fueferd4.fsf@gmail.com which hasn't been answered yet.

Show 62 quoted lines
> Signed-off-by: lukaszgryglicki <lukaszgryglicki@o2.pl>
> ---
>  Documentation/git-merge.txt  |  8 +++++
>  builtin/merge.c              |  4 +++
>  t/t9904-git-merge-signoff.sh | 75 ++++++++++++++++++++++++++++++++++++++++++++
>
>  3 files changed, 87 insertions(+)
>  create mode 100755 t/t9904-git-merge-signoff.sh
>
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index 04fdd8cf086db..6b308ab6d0b52 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -64,6 +64,14 @@ OPTIONS
>  -------
>  include::merge-options.txt[]
>
> +--signoff::
> +	Add Signed-off-by line by the committer at the end of the commit
> +	log message.  The meaning of a signoff depends on the project,
> +	but it typically certifies that committer has
> +	the rights to submit this work under the same license and
> +	agrees to a Developer Certificate of Origin
> +	(see http://developercertificate.org/ for more information).
> +
>  -S[<keyid>]::
>  --gpg-sign[=<keyid>]::
>  	GPG-sign the resulting merge commit. The `keyid` argument is
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 900bafdb45d0b..78c36e9bf353b 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -70,6 +70,7 @@ static int continue_current_merge;
>  static int allow_unrelated_histories;
>  static int show_progress = -1;
>  static int default_to_upstream = 1;
> +static int signoff;
>  static const char *sign_commit;
>
>  static struct strategy all_strategy[] = {
> @@ -233,6 +234,7 @@ static struct option builtin_merge_options[] = {
>  	{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"),
>  	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
>  	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
> +	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
>  	OPT_END()
>  };
>
> @@ -763,6 +765,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)
>  	strbuf_addch(&msg, '\n');
>  	if (0 < option_edit)
>  		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
> +	if (signoff)
> +		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
>  	write_file_buf(git_path_merge_msg(), msg.buf, msg.len);
>  	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
>  			    git_path_merge_msg(), "merge", NULL))
> diff --git a/t/t9904-git-merge-signoff.sh b/t/t9904-git-merge-signoff.sh
> new file mode 100755
> index 0000000000000..f542f136f5dda
> --- /dev/null
> +++ b/t/t9904-git-merge-signoff.sh

The convention for adding new tests is not to add a new one after whatever name sorts the highest, see "Naming Tests" in t/README.

I.e. this should be somewhere in t[6-7]* with the other merge tests.
Show 35 quoted lines
> @@ -0,0 +1,75 @@
> +#!/bin/sh
> +
> +test_description='git merge --signoff
> +
> +This test runs git merge --signoff and makes sure that it works.
> +'
> +
> +. ./test-lib.sh
> +
> +# Setup test files
> +test_setup() {
> +	# A simples files to commit
> +	echo "1" >file1
> +	echo "2" >file2
> +	echo "3" >file3
> +	echo "4" >file4
> +
> +	# Expected commit message after merge --signoff
> +	cat >expected-signed <<EOF
> +Merge branch 'master' into other-branch
> +
> +Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/")
> +EOF
> +
> +	# Expected commit message after merge without --signoff (or with --no-signoff)
> +	cat >expected-unsigned <<EOF
> +Merge branch 'master' into other-branch
> +EOF
> +
> +	# Initial commit and feature branch to merge master into it.
> +	git commit --allow-empty -m "Initial empty commit"
> +	git checkout -b other-branch
> +	git add file1
> +	git commit -m other-branch

This setup function doesn't && its commands together, so a fail in any one of these won't be detecten. Any reason you don't just add this to the test_expect_success below, where we would detect this?

Also most of your commit/add etc. could probably be replaced with the test_commit helper, see t/README.

Junio C Hamano· Jul 4, 2017, 17:42 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v2] add --signoff flag to `git-merge` command.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 9 quoted lines
> On Tue, Jul 04 2017, Łukasz Gryglicki jotted:
>
>> add --signoff flag to `git-merge` command.
>
> We'd usually say this as:
>
> merge: add a --signoff flag
>
> Or something like that.

I thought I gave a fairly complete example that can be imitated, but apparently it didn't go through X-<.

Show 15 quoted lines
>> Some projects require every commit to be signed off.
>> Our workflow is to create feature branches and require every commit to
>> be signed off. When feature is finally approved we need to merge it into
>> master. Merge itself is usually trivial and is done by
>> `git merge origin/master`. Unfortunatelly this command have no --signoff
>> flag, so we need to either add signoff line manually or use
>> `git commit --amend -s` after the merge. First solution is not ideal
>> because not all developers are familiar with exact sign-off syntax.
>> The second solution works, but is obviously tedious.
>> This patch adds --signoff support to git-merge command. It works just
>> like --signoff in `git-commit` command.
>
> It would be nice to split this into a at least a couple of paragraphs,
> and more closely follow the format suggested by
> Documentation/SubmittingPatches.
Good suggestion.
Show 5 quoted lines
>> More details here:
>> https://public-inbox.org/git/CAHv71zK5SqbwrBFX=a8-DY9H3KT4FEyMgv__p2gZzNr0WUAPUw@mail.gmail.com/T/#u
>
> These more details include my outstanding question in
> 87fueferd4.fsf@gmail.com which hasn't been answered yet.

I have an opinion on that topic, but I'd prefer to hear from others first before I speak out.

Show 8 quoted lines
>> diff --git a/t/t9904-git-merge-signoff.sh b/t/t9904-git-merge-signoff.sh
>> new file mode 100755
>> index 0000000000000..f542f136f5dda
>> --- /dev/null
>> +++ b/t/t9904-git-merge-signoff.sh
>
> The convention for adding new tests is not to add a new one after
> whatever name sorts the highest, see "Naming Tests" in t/README.
Correct.
> I.e. this should be somewhere in t[6-7]* with the other merge tests.

Yeah. While most of t[67]??? series are about the contents of the merge, i.e. resulting trees and what happens in the working tree, there are some tests about the merge messages in there. t7608 is exactly about how the command prepares the messages before giving them to human to edit, and I think "merge can be told to optionally add sign-off" fits there just fine. All existing tests there are only interested about the title, but that does not mean there must not be tests that care more than the title in the script.

Also, as you suggest, these will become a lot shorter when the standard test helper shell functions are used. I do not think we necessarily want a brand new test script to test only three or so combinations (i.e. last one wins when --option and --no-option is given, --option has an effect, --no-option does not have an effect).

Łukasz Gryglicki· Jul 4, 2017, 09:33 UTC · re: Łukasz Gryglicki · lore

[PATCH v3] merge: add a --signoff flag.

Some projects require every commit to be signed off. Our workflow is to create feature branches and require every commit to be signed off. When feature is finally approved we need to merge it into master. Merge itself is usually trivial and is done by `git merge origin/master`.

Unfortunatelly `merge` command have no --signoff flag, so we need to either add signoff line manually or use `git commit --amend -s` after the merge.

First solution is not ideal because not all developers are familiar with exact sign-off syntax. The second solution works, but is obviously tedious.

This patch adds --signoff support to `git-merge` command. It works just like --signoff in `git-commit` command.

More details can be found here: https://public-inbox.org/git/CAHv71zK5SqbwrBFX=a8-DY9H3KT4FEyMgv__p2gZzNr0WUAPUw@mail.gmail.com/T/#u

Signed-off-by: lukaszgryglicki <lukaszgryglicki@o2.pl>
---
 Documentation/git-merge.txt |  8 ++++++
 builtin/merge.c             |  4 +++
 t/t7614-merge-signoff.sh    | 69 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+)
 create mode 100755 t/t7614-merge-signoff.sh
Show changes to 3 files +81 −1

Documentation/git-merge.txt, builtin/merge.c, t/t7614-merge-signoff.sh

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 04fdd8cf086db..6b308ab6d0b52 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -64,6 +64,14 @@ OPTIONS
 -------
 include::merge-options.txt[]
 
+--signoff::
+	Add Signed-off-by line by the committer at the end of the commit
+	log message.  The meaning of a signoff depends on the project,
+	but it typically certifies that committer has
+	the rights to submit this work under the same license and
+	agrees to a Developer Certificate of Origin
+	(see http://developercertificate.org/ for more information).
+
 -S[<keyid>]::
 --gpg-sign[=<keyid>]::
 	GPG-sign the resulting merge commit. The `keyid` argument is
diff --git a/builtin/merge.c b/builtin/merge.c
index 900bafdb45d0b..78c36e9bf353b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -70,6 +70,7 @@ static int continue_current_merge;
 static int allow_unrelated_histories;
 static int show_progress = -1;
 static int default_to_upstream = 1;
+static int signoff;
 static const char *sign_commit;
 
 static struct strategy all_strategy[] = {
@@ -233,6 +234,7 @@ static struct option builtin_merge_options[] = {
 	{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"),
 	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
+	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
 	OPT_END()
 };
 
@@ -763,6 +765,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	strbuf_addch(&msg, '\n');
 	if (0 < option_edit)
 		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
+	if (signoff)
+		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
 	write_file_buf(git_path_merge_msg(), msg.buf, msg.len);
 	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
 			    git_path_merge_msg(), "merge", NULL))
diff --git a/t/t7614-merge-signoff.sh b/t/t7614-merge-signoff.sh
new file mode 100755
index 0000000000000..c1b8446f491dc
--- /dev/null
+++ b/t/t7614-merge-signoff.sh
@@ -0,0 +1,69 @@
+#!/bin/sh
+
+test_description='git merge --signoff
+
+This test runs git merge --signoff and makes sure that it works.
+'
+
+. ./test-lib.sh
+
+# Setup test files
+test_setup() {
+	# Expected commit message after merge --signoff
+	cat >expected-signed <<EOF &&
+Merge branch 'master' into other-branch
+
+Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/")
+EOF
+
+	# Expected commit message after merge without --signoff (or with --no-signoff)
+	cat >expected-unsigned <<EOF &&
+Merge branch 'master' into other-branch
+EOF
+
+	# Initial commit and feature branch to merge master into it.
+	git commit --allow-empty -m "Initial empty commit" &&
+	git checkout -b other-branch &&
+	test_commit other-branch file1 1
+}
+
+# Setup repository, files & feature branch
+# This step must be run if You want to test 2,3 or 4
+# Order of 2,3,4 is not important, but 1 must be run before
+# For example `-r 1,4` or `-r 1,4,2 -v` etc
+# But not `-r 2` or `-r 4,3,2,1`
+test_expect_success 'setup' '
+	test_setup
+'
+
+# Test with --signoff flag
+test_expect_success 'git merge --signoff adds a sign-off line' '
+	git checkout master &&
+	test_commit master-branch-2 file2 2 &&
+	git checkout other-branch &&
+	git merge master --signoff --no-edit &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+	test_cmp expected-signed actual
+'
+
+# Test without --signoff flag
+test_expect_success 'git merge does not add a sign-off line' '
+	git checkout master &&
+	test_commit master-branch-3 file3 3 &&
+	git checkout other-branch &&
+	git merge master --no-edit &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+	test_cmp expected-unsigned actual
+'
+
+# Test for --no-signoff flag
+test_expect_success 'git merge --no-signoff flag cancels --signoff flag' '
+	git checkout master &&
+	test_commit master-branch-4 file4 4 &&
+	git checkout other-branch &&
+	git merge master --no-edit --signoff --no-signoff &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+	test_cmp expected-unsigned actual
+'
+
+test_done

--
https://github.com/git/git/pull/383
lukaszgryglicki· Jul 24, 2017, 06:14 UTC · re: Łukasz Gryglicki · lore

Re: [PATCH v3] merge: add a --signoff flag.

Hi, what is the state of this patch? What else should I do to get it merged into git? Thanks.

Show 156 quoted lines
> On 4 Jul 2017, at 11:33, Łukasz Gryglicki <lukaszgryglicki@o2.pl> wrote:
> 
> Some projects require every commit to be signed off.
> Our workflow is to create feature branches and require every commit to
> be signed off. When feature is finally approved we need to merge it into
> master. Merge itself is usually trivial and is done by
> `git merge origin/master`.
> 
> Unfortunatelly `merge` command have no --signoff
> flag, so we need to either add signoff line manually or use
> `git commit --amend -s` after the merge.
> 
> First solution is not ideal because not all developers are familiar with
> exact sign-off syntax. The second solution works, but is obviously tedious.
> 
> This patch adds --signoff support to `git-merge` command.
> It works just like --signoff in `git-commit` command.
> 
> More details can be found here:
> https://public-inbox.org/git/CAHv71zK5SqbwrBFX=a8-DY9H3KT4FEyMgv__p2gZzNr0WUAPUw@mail.gmail.com/T/#u
> 
> Signed-off-by: lukaszgryglicki <lukaszgryglicki@o2.pl>
> ---
> Documentation/git-merge.txt |  8 ++++++
> builtin/merge.c             |  4 +++
> t/t7614-merge-signoff.sh    | 69 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 81 insertions(+)
> create mode 100755 t/t7614-merge-signoff.sh
> 
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index 04fdd8cf086db..6b308ab6d0b52 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -64,6 +64,14 @@ OPTIONS
> -------
> include::merge-options.txt[]
> 
> +--signoff::
> +	Add Signed-off-by line by the committer at the end of the commit
> +	log message.  The meaning of a signoff depends on the project,
> +	but it typically certifies that committer has
> +	the rights to submit this work under the same license and
> +	agrees to a Developer Certificate of Origin
> +	(see http://developercertificate.org/ for more information).
> +
> -S[<keyid>]::
> --gpg-sign[=<keyid>]::
> 	GPG-sign the resulting merge commit. The `keyid` argument is
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 900bafdb45d0b..78c36e9bf353b 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -70,6 +70,7 @@ static int continue_current_merge;
> static int allow_unrelated_histories;
> static int show_progress = -1;
> static int default_to_upstream = 1;
> +static int signoff;
> static const char *sign_commit;
> 
> static struct strategy all_strategy[] = {
> @@ -233,6 +234,7 @@ static struct option builtin_merge_options[] = {
> 	{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"),
> 	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
> 	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
> +	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
> 	OPT_END()
> };
> 
> @@ -763,6 +765,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)
> 	strbuf_addch(&msg, '\n');
> 	if (0 < option_edit)
> 		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
> +	if (signoff)
> +		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
> 	write_file_buf(git_path_merge_msg(), msg.buf, msg.len);
> 	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
> 			    git_path_merge_msg(), "merge", NULL))
> diff --git a/t/t7614-merge-signoff.sh b/t/t7614-merge-signoff.sh
> new file mode 100755
> index 0000000000000..c1b8446f491dc
> --- /dev/null
> +++ b/t/t7614-merge-signoff.sh
> @@ -0,0 +1,69 @@
> +#!/bin/sh
> +
> +test_description='git merge --signoff
> +
> +This test runs git merge --signoff and makes sure that it works.
> +'
> +
> +. ./test-lib.sh
> +
> +# Setup test files
> +test_setup() {
> +	# Expected commit message after merge --signoff
> +	cat >expected-signed <<EOF &&
> +Merge branch 'master' into other-branch
> +
> +Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/")
> +EOF
> +
> +	# Expected commit message after merge without --signoff (or with --no-signoff)
> +	cat >expected-unsigned <<EOF &&
> +Merge branch 'master' into other-branch
> +EOF
> +
> +	# Initial commit and feature branch to merge master into it.
> +	git commit --allow-empty -m "Initial empty commit" &&
> +	git checkout -b other-branch &&
> +	test_commit other-branch file1 1
> +}
> +
> +# Setup repository, files & feature branch
> +# This step must be run if You want to test 2,3 or 4
> +# Order of 2,3,4 is not important, but 1 must be run before
> +# For example `-r 1,4` or `-r 1,4,2 -v` etc
> +# But not `-r 2` or `-r 4,3,2,1`
> +test_expect_success 'setup' '
> +	test_setup
> +'
> +
> +# Test with --signoff flag
> +test_expect_success 'git merge --signoff adds a sign-off line' '
> +	git checkout master &&
> +	test_commit master-branch-2 file2 2 &&
> +	git checkout other-branch &&
> +	git merge master --signoff --no-edit &&
> +	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
> +	test_cmp expected-signed actual
> +'
> +
> +# Test without --signoff flag
> +test_expect_success 'git merge does not add a sign-off line' '
> +	git checkout master &&
> +	test_commit master-branch-3 file3 3 &&
> +	git checkout other-branch &&
> +	git merge master --no-edit &&
> +	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
> +	test_cmp expected-unsigned actual
> +'
> +
> +# Test for --no-signoff flag
> +test_expect_success 'git merge --no-signoff flag cancels --signoff flag' '
> +	git checkout master &&
> +	test_commit master-branch-4 file4 4 &&
> +	git checkout other-branch &&
> +	git merge master --no-edit --signoff --no-signoff &&
> +	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
> +	test_cmp expected-unsigned actual
> +'
> +
> +test_done
> 
> --
> https://github.com/git/git/pull/383
> 
Junio C Hamano· Jul 24, 2017, 07:02 UTC · re: lukaszgryglicki · lore

Re: [PATCH v3] merge: add a --signoff flag.

lukaszgryglicki <lukaszgryglicki@o2.pl> writes:
> Hi, what is the state of this patch?

I was expecting you to respond to Ævar's <87tw2sbnyl.fsf@gmail.com> to explain how your new version addresses his concerns, or him to respond to your new patch to say that it addresses his concerns adequately. I think neither has happened, so the topic is still in limbo, I guess...

Junio C Hamano· Jul 24, 2017, 20:42 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] merge: add a --signoff flag.

Junio C Hamano <gitster@pobox.com> writes:
Show 9 quoted lines
> lukaszgryglicki <lukaszgryglicki@o2.pl> writes:
>
>> Hi, what is the state of this patch?
>
> I was expecting you to respond to Ævar's <87tw2sbnyl.fsf@gmail.com>
> to explain how your new version addresses his concerns, or him to
> respond to your new patch to say that it addresses his concerns
> adequately.  I think neither has happened, so the topic is still in
> limbo, I guess...
Sorry, Ævar's message I meant was <87fueferd4.fsf@gmail.com>.
lukaszgryglicki· Jul 25, 2017, 05:00 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] merge: add a --signoff flag.

I can't reach Him, every time I send an email it is returned "no such address". Can You please ask Him to take a look?

Show 15 quoted lines
> On 24 Jul 2017, at 22:42, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> lukaszgryglicki <lukaszgryglicki@o2.pl> writes:
>> 
>>> Hi, what is the state of this patch?
>> 
>> I was expecting you to respond to Ævar's <87tw2sbnyl.fsf@gmail.com>
>> to explain how your new version addresses his concerns, or him to
>> respond to your new patch to say that it addresses his concerns
>> adequately.  I think neither has happened, so the topic is still in
>> limbo, I guess...
> 
> Sorry, Ævar's message I meant was <87fueferd4.fsf@gmail.com>.
Stefan Beller· Jul 25, 2017, 05:10 UTC · re: lukaszgryglicki · lore

Re: [PATCH v3] merge: add a --signoff flag.

On Mon, Jul 24, 2017 at 10:00 PM, lukaszgryglicki <lukaszgryglicki@o2.pl> wrote:
> I can't reach Him, every time I send an email it is returned "no such address".
> Can You please ask Him to take a look?

Junio referred to https://public-inbox.org/git/87fueferd4.fsf@gmail.com/ His actual email address is in the cc list.

Show 16 quoted lines
>> On 24 Jul 2017, at 22:42, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> lukaszgryglicki <lukaszgryglicki@o2.pl> writes:
>>>
>>>> Hi, what is the state of this patch?
>>>
>>> I was expecting you to respond to Ævar's <87tw2sbnyl.fsf@gmail.com>
>>> to explain how your new version addresses his concerns, or him to
>>> respond to your new patch to say that it addresses his concerns
>>> adequately.  I think neither has happened, so the topic is still in
>>> limbo, I guess...
>>
>> Sorry, Ævar's message I meant was <87fueferd4.fsf@gmail.com>.
>
lukaszgryglicki· Jul 25, 2017, 11:28 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] merge: add a --signoff flag.

Hi, can You take a look at my newest patch version (v3)?
Show 15 quoted lines
> On 24 Jul 2017, at 22:42, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> lukaszgryglicki <lukaszgryglicki@o2.pl> writes:
>> 
>>> Hi, what is the state of this patch?
>> 
>> I was expecting you to respond to Ævar's <87tw2sbnyl.fsf@gmail.com>
>> to explain how your new version addresses his concerns, or him to
>> respond to your new patch to say that it addresses his concerns
>> adequately.  I think neither has happened, so the topic is still in
>> limbo, I guess...
> 
> Sorry, Ævar's message I meant was <87fueferd4.fsf@gmail.com>.
Junio C Hamano· Jul 25, 2017, 18:41 UTC · re: Łukasz Gryglicki · lore

Re: [PATCH v3] merge: add a --signoff flag.

Łukasz Gryglicki <lukaszgryglicki@o2.pl> writes:
Show 9 quoted lines
> Some projects require every commit to be signed off.
> Our workflow is to create feature branches and require every commit to
> be signed off. When feature is finally approved we need to merge it into
> master. Merge itself is usually trivial and is done by
> `git merge origin/master`.
>
> Unfortunatelly `merge` command have no --signoff
> flag, so we need to either add signoff line manually or use
> `git commit --amend -s` after the merge.

Who are "we" in the above? Certainly not the Git development project who stands behind the log message. I also find the first paragraph overly verbose.

Perhaps something like this instead?
    Some projects require every commit, even merges, to be signed
    off [*1*].  Because "git merge" does not have a "--signoff"
    option like "git commit" does, the user needs to add one
    manually when the command presents an editor to describe the
    merge, or later use "git commit --amend --signoff".
    Help developers of these projects by teaching "--signoff" option
    to "git merge".
    *1* https://public-inbox.org/git/CAHv71zK5SqbwrBFX=a8-DY9H3KT4FEyMgv__p2gZzNr0WUAPUw@mail.gmail.com/T/#u
    Requested-by: Dan Kohn <dan@linuxfoundation.org>
    Signed-off-by: Łukasz Gryglicki <lukaszgryglicki@o2.pl>

Notice that I updated your s-o-b line in the above illustration because we prefer to see the same name as patch author (which is usually taken from your e-mail From: header) there.

Show 7 quoted lines
> +--signoff::
> +	Add Signed-off-by line by the committer at the end of the commit
> +	log message.  The meaning of a signoff depends on the project,
> +	but it typically certifies that committer has
> +	the rights to submit this work under the same license and
> +	agrees to a Developer Certificate of Origin
> +	(see http://developercertificate.org/ for more information).

This is taken verbatim from Documentation/git-commit.txt and "this work" in that context is entirely sensible, but it is not quite clear what it means in the context of "git merge".

Show 29 quoted lines
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 900bafdb45d0b..78c36e9bf353b 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -70,6 +70,7 @@ static int continue_current_merge;
>  static int allow_unrelated_histories;
>  static int show_progress = -1;
>  static int default_to_upstream = 1;
> +static int signoff;
>  static const char *sign_commit;
>  
>  static struct strategy all_strategy[] = {
> @@ -233,6 +234,7 @@ static struct option builtin_merge_options[] = {
>  	{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"),
>  	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
>  	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
> +	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
>  	OPT_END()
>  };
>  
> @@ -763,6 +765,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)
>  	strbuf_addch(&msg, '\n');
>  	if (0 < option_edit)
>  		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
> +	if (signoff)
> +		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
>  	write_file_buf(git_path_merge_msg(), msg.buf, msg.len);
>  	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
>  			    git_path_merge_msg(), "merge", NULL))

The invocation of the editor comes after this post-context, and the new code seems to sit at the right place. Good.

Show 17 quoted lines
> diff --git a/t/t7614-merge-signoff.sh b/t/t7614-merge-signoff.sh
> new file mode 100755
> index 0000000000000..c1b8446f491dc
> --- /dev/null
> +++ b/t/t7614-merge-signoff.sh
> @@ -0,0 +1,69 @@
> +#!/bin/sh
> +
> +test_description='git merge --signoff
> +
> +This test runs git merge --signoff and makes sure that it works.
> +'
> +
> +. ./test-lib.sh
> +
> +# Setup test files
> +test_setup() {
Style: "test_setup () {" but see below.
Show 6 quoted lines
> +	# Expected commit message after merge --signoff
> +	cat >expected-signed <<EOF &&
> +Merge branch 'master' into other-branch
> +
> +Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/")
> +EOF
Indenting the here-text with "<<-EOF" makes it easier to read, e.g.
	cat >expected-signed <<-EOF &&
	Merge branch 'master' into other-branch
	Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/")
	EOF
Likewise for the other one.
> +...
> +}

I do not see much point in making this a shell function. I'd just do all of the above in the first "test_expect_success 'setup'" thing, if I were doing this patch.

Show 6 quoted lines
> +
> +# Setup repository, files & feature branch
> +# This step must be run if You want to test 2,3 or 4
> +# Order of 2,3,4 is not important, but 1 must be run before
> +# For example `-r 1,4` or `-r 1,4,2 -v` etc
> +# But not `-r 2` or `-r 4,3,2,1`

That is pretty much the standard practice to require 'setup' to always run; no need to waste 5 lines to document it.

Show 5 quoted lines
> +test_expect_success 'setup' '
> +	test_setup
> +'
> +
> +# Test with --signoff flag
That can already be seen on the test title below.  Remove it?
Show 7 quoted lines
> +test_expect_success 'git merge --signoff adds a sign-off line' '
> +...
> +test_expect_success 'git merge does not add a sign-off line' '
> +...
> +test_expect_success 'git merge --no-signoff flag cancels --signoff flag' '
> +...
> +'

They all look sensible thing to check. We would also need to make sure that the end user sees the S-o-b: prepopulated when the editor is spawned, I would think, perhaps like (completely untested)

	write_script save-editor <<-\EOF &&
	cp "$1" "$1".saved
	EOF
	GIT_EDITOR=./save-editor git merge --signoff --no-signoff ... &&
 	... check the contents of the MERGE_MSG.saved file to 
	... ensure string Signed-off-by: appears (or does not
	... appear) here, e.g.
	test_i18ngrep ! "^Signed-off-by: " .git/MERGE_MSG.saved
Thanks.
Łukasz Gryglicki· Jul 26, 2017, 07:39 UTC · re: Łukasz Gryglicki · lore

[PATCH v4] merge: add a --signoff flag.

Some projects require every commit, even merges, to be signed off [*1*]. Because "git merge" does not have a "--signoff" option like "git commit" does, the user needs to add one manually when the command presents an editor to describe the merge, or later use "git commit --amend --signoff".

Help developers of these projects by teaching "--signoff" option to "git merge".

*1* https://public-inbox.org/git/CAHv71zK5SqbwrBFX=a8-DY9H3KT4FEyMgv__p2gZzNr0WUAPUw@mail.gmail.com/T/#u

Requested-by: Dan Kohn <dan@linuxfoundation.org>
Signed-off-by: Łukasz Gryglicki <lukaszgryglicki@o2.pl>
---
 Documentation/git-merge.txt |  8 +++++
 builtin/merge.c             |  4 +++
 t/t7614-merge-signoff.sh    | 88 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 100 insertions(+)
 create mode 100755 t/t7614-merge-signoff.sh
Show changes to 3 files +100 −1

Documentation/git-merge.txt, builtin/merge.c, t/t7614-merge-signoff.sh

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 04fdd8cf086db..630cb4b7f90d7 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -64,6 +64,14 @@ OPTIONS
 -------
 include::merge-options.txt[]
 
+--signoff::
+	Add Signed-off-by line by the committer at the end of the commit
+	log message.  The meaning of a signoff depends on the project,
+	but it typically certifies that committer has
+	the rights to merge work under the same license and
+	agrees to a Developer Certificate of Origin
+	(see http://developercertificate.org/ for more information).
+
 -S[<keyid>]::
 --gpg-sign[=<keyid>]::
 	GPG-sign the resulting merge commit. The `keyid` argument is
diff --git a/builtin/merge.c b/builtin/merge.c
index 900bafdb45d0b..78c36e9bf353b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -70,6 +70,7 @@ static int continue_current_merge;
 static int allow_unrelated_histories;
 static int show_progress = -1;
 static int default_to_upstream = 1;
+static int signoff;
 static const char *sign_commit;
 
 static struct strategy all_strategy[] = {
@@ -233,6 +234,7 @@ static struct option builtin_merge_options[] = {
 	{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"),
 	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
+	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
 	OPT_END()
 };
 
@@ -763,6 +765,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	strbuf_addch(&msg, '\n');
 	if (0 < option_edit)
 		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
+	if (signoff)
+		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
 	write_file_buf(git_path_merge_msg(), msg.buf, msg.len);
 	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
 			    git_path_merge_msg(), "merge", NULL))
diff --git a/t/t7614-merge-signoff.sh b/t/t7614-merge-signoff.sh
new file mode 100755
index 0000000000000..823d840d423df
--- /dev/null
+++ b/t/t7614-merge-signoff.sh
@@ -0,0 +1,88 @@
+#!/bin/sh
+
+test_description='git merge --signoff
+
+This test runs git merge --signoff and makes sure that it works.
+'
+
+. ./test-lib.sh
+
+# Setup test files
+test_setup () {
+  # Expected commit message after merge --signoff
+  printf "Merge branch 'master' into other-branch\n\n" >expected-signed &&
+  printf "Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/")\n" >>expected-signed &&
+
+  # Expected commit message after merge without --signoff (or with --no-signoff)
+  echo "Merge branch 'master' into other-branch" >expected-unsigned &&
+
+  # Initial commit and feature branch to merge master into it.
+  git commit --allow-empty -m "Initial empty commit" &&
+  git checkout -b other-branch &&
+  test_commit other-branch file1 1
+}
+
+# Create fake editor that just copies file
+create_fake_editor () {
+  echo 'cp "$1" "$1.saved"' | write_script save-editor
+}
+
+test_expect_success 'setup' '
+  test_setup && create_fake_editor
+'
+
+test_expect_success 'git merge --signoff adds a sign-off line' '
+  git checkout master &&
+  test_commit master-branch-2 file2 2 &&
+  git checkout other-branch &&
+  git merge master --signoff --no-edit &&
+  git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+  test_cmp expected-signed actual
+'
+
+test_expect_success 'git merge does not add a sign-off line' '
+  git checkout master &&
+  test_commit master-branch-3 file3 3 &&
+  git checkout other-branch &&
+  git merge master --no-edit &&
+  git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+  test_cmp expected-unsigned actual
+'
+
+test_expect_success 'git merge --no-signoff flag cancels --signoff flag' '
+  git checkout master &&
+  test_commit master-branch-4 file4 4 &&
+  git checkout other-branch &&
+  git merge master --no-edit --signoff --no-signoff &&
+  git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+  test_cmp expected-unsigned actual
+'
+
+test_expect_success 'git merge --signoff adds S-o-b line in commit message editor' '
+  git checkout master &&
+  test_commit master-branch-5 file5 5 &&
+  git checkout other-branch &&
+  GIT_EDITOR=./save-editor git merge master -m "My Message" --edit --signoff &&
+  test_i18ngrep "^My Message" .git/MERGE_MSG.saved &&
+  test_i18ngrep "^Signed-off-by: " .git/MERGE_MSG.saved
+'
+
+test_expect_success 'git merge --no-signoff does not add S-o-b line in commit message editor' '
+  git checkout master &&
+  test_commit master-branch-6 file6 6 &&
+  git checkout other-branch &&
+  GIT_EDITOR=./save-editor git merge master -m "My Message" --edit --no-signoff &&
+  test_i18ngrep "^My Message" .git/MERGE_MSG.saved &&
+  test_i18ngrep ! "^Signed-off-by: " .git/MERGE_MSG.saved
+'
+
+test_expect_success 'git merge --no-signoff cancels --signoff flag in commit message editor' '
+  git checkout master &&
+  test_commit master-branch-7 file7 7 &&
+  git checkout other-branch &&
+  GIT_EDITOR=./save-editor git merge master -m "My Message" --edit --signoff --no-signoff &&
+  test_i18ngrep "^My Message" .git/MERGE_MSG.saved &&
+  test_i18ngrep ! "^Signed-off-by: " .git/MERGE_MSG.saved
+'
+
+test_done

--
https://github.com/git/git/pull/390
Junio C Hamano· Jul 26, 2017, 07:34 UTC · lore

Re: [PATCH v3] merge: add a --signoff flag.

lukaszgryglicki <lukaszgryglicki@o2.pl> writes:
Show 14 quoted lines
>>> +--signoff::
>>> +	Add Signed-off-by line by the committer at the end of the commit
>>> +	log message.  The meaning of a signoff depends on the project,
>>> +	but it typically certifies that committer has
>>> +	the rights to submit this work under the same license and
>>> +	agrees to a Developer Certificate of Origin
>>> +	(see http://developercertificate.org/ for more information).
>> 
>> This is taken verbatim from Documentation/git-commit.txt and "this
>> work" in that context is entirely sensible, but it is not quite
>> clear what it means in the context of "git merge”.
>
> Changed slightly, but I’m not a native English speaker, and
> don’t really see what is wrong there.
Oh, the language is not the issue.  

I was trying to remind you that a "merge" may be an automated and mechanical one, in which there is no original work that requires signing it off in the first place, which was the point Ævar raised when Dan's original post came up.

← back to recent threads