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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

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

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

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

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