From: Junio C Hamano <gitster@pobox.com>
To: "Łukasz Gryglicki" <lukaszgryglicki@o2.pl>
Cc: git@vger.kernel.org, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH v3] merge: add a --signoff flag.
Date: Tue, 25 Jul 2017 11:41:33 -0700 [thread overview]
Message-ID: <xmqqeft4bbqa.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <0102015d0cf235f7-9be8e1fc-a926-4e6f-8180-c131da1c4161-000000@eu-west-1.amazonses.com> ("Łukasz Gryglicki"'s message of "Tue, 4 Jul 2017 09:33:06 +0000")
Łukasz Gryglicki <lukaszgryglicki@o2.pl> 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 <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.
> +--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".
> 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.
> 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.
next prev parent reply other threads:[~2017-07-25 18:41 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-03 9:57 [PATCH] area: git-merge: add --signoff flag to git-merge Łukasz Gryglicki
2017-07-03 17:57 ` Junio C Hamano
2017-07-04 6:20 ` [PATCH v2] add --signoff flag to `git-merge` command Łukasz Gryglicki
2017-07-04 8:33 ` Ævar Arnfjörð Bjarmason
2017-07-04 17:42 ` Junio C Hamano
2017-07-04 9:33 ` [PATCH v3] merge: add a --signoff flag Łukasz Gryglicki
2017-07-24 6:14 ` lukaszgryglicki
2017-07-24 7:02 ` Junio C Hamano
2017-07-24 20:42 ` Junio C Hamano
2017-07-25 5:00 ` lukaszgryglicki
2017-07-25 5:10 ` Stefan Beller
2017-07-25 11:28 ` lukaszgryglicki
2017-07-25 18:41 ` Junio C Hamano [this message]
[not found] ` <7A9ED766-4A25-4F34-8E02-3DFCE1D63ADF@o2.pl>
2017-07-26 7:34 ` Junio C Hamano
2017-07-26 7:39 ` [PATCH v4] " Łukasz Gryglicki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqeft4bbqa.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=lukaszgryglicki@o2.pl \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).