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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.

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

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

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