Re: [PATCH v3] merge: add a --signoff flag.
From: Junio C Hamano <hidden>
Date: 2017-07-25 18:41:48
Łukasz Gryglicki [off-list ref] writes:
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 [off-list ref]
Signed-off-by: Łukasz Gryglicki [off-list ref]
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.
+--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".
quoted hunk
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.
quoted hunk
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.
+ # 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.
+ +# 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.
+test_expect_success 'setup' ' + test_setup +' + +# Test with --signoff flag
That can already be seen on the test title below. Remove it?
+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.