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

[Junio C Hamano]

Łukasz Gryglicki <lukaszgrygli...@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 <lukaszgrygli...@o2.pl>
> ---

"Signed-off-by: Łukasz Gryglicki <lukaszgrygli...@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", _commit, N_("key-id"),
> N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
>   OPT_BOOL(0, "overwrite-ignore", _ignore, N_("update ignored 
> files (default)")),
> + OPT_BOOL(0, "signoff", , N_("add Signed-off-by:")),
>   OPT_END()
>  };
>  
> @@ -775,6 +777,8 @@ static void prepare_to_commit(struct commit_list 
> *remoteheads)
>   strbuf_stripspace(, 0 < option_edit);
>   if (!msg.len)
>   abort_commit(remoteheads, _("Empty commit message."));
> + if (signoff)
> + append_signoff(, 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 0..eed15b9a85371
> --- /dev/null
> +++ b/t/t9904-git-merge-signoff.sh

Do we need a new script?I thin

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

[Łukasz Gryglicki]

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

Signed-off-by: lukaszgryglicki 
---
 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).
+
 -S[]::
 --gpg-sign[=]::
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", _commit, N_("key-id"),
  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
OPT_BOOL(0, "overwrite-ignore", _ignore, N_("update ignored 
files (default)")),
+   OPT_BOOL(0, "signoff", , N_("add Signed-off-by:")),
OPT_END()
 };
 
@@ -775,6 +777,8 @@ static void prepare_to_commit(struct commit_list 
*remoteheads)
strbuf_stripspace(, 0 < option_edit);
if (!msg.len)
abort_commit(remoteheads, _("Empty commit message."));
+   if (signoff)
+   append_signoff(, ignore_non_trailer(msg.buf, msg.len), 0);
strbuf_release(_msg);
strbuf_addbuf(_msg, );
strbuf_release();
diff --git a/t/t9904-git-merge-signoff.sh b/t/t9904-git-merge-signoff.sh
new file mode 100755
index 0..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
+
+# Expected commit message after merge without --signoff (or with --no-signoff)
+cat >expected-unsigned < 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