e379fdf3 (merge: refuse to create too cool a merge by default, 2016-03-18) gave a reason for *not* passing options from pull to merge: ...because such a "two project merge" would be done after fetching the other project into some location in the working tree of an existing project and making sure how well they fit together... That's certainly an acceptable workflow, but I'd also like to support merge options in pull for folks who want to optimistically pull and then sort out "how well they fit together" after pull exits (possibly with a merge failure). And in cases where an optimistic pull is likely to succeed, suggesting it is easier to explain to Git newbies than a FETCH_HEAD merge or remote-addition/merge/remote-removal. 09c2cb87 (pull: pass --allow-unrelated-histories to "git merge", 2016-03-18) added a pull-to-merge pass for a different option but didn't motivate its change, only citing the reason from e379fdf3 for not adding the pull-to-merge pass for that option. I'm personally in favor of pull-to-merge passing for any unambiguous options, but if the decision for pull-to-merge passes depends on the specific option, then --allow-unrelated-histories is probably the weakest candidate because unrelated-history merged are more likely to have "fit together" issues than the other merge-only options. The test_has_trailer helper gives folks a convenient way check these sorts of things. I haven't gone through and updated existing trailer checks (e.g. the ones in t7614-merge-signoff.sh) to avoid the "only to correct the inconconsistency" issue discussed in SubmittingPatches. Other test may gradually migrate to the new helper if they find it useful. The helper may be useful enough to eventually become a plumbing command (a read version of interpret-trailers with an API similar to 'git config ...'?), but I'm not going that far in this commit ;). The order of options in merge-options.txt isn't clear to me, but I've put --signoff between --log and --stat as somewhat alphabetized and having an "add to the commit message" function like --log. Helped-by: Junio C Hamano <gitster@xxxxxxxxx> Signed-off-by: W. Trevor King <wking@xxxxxxxxxx> --- Changes since v1 [1]: * Dropped "Following" paragraph. Junio took issue with the phrasing [2], and the implementation in v2 has diverged sufficiently from 09c2cb87 and 14d01b4f that I don't think citing them as implementation references is useful anymore. * Lead the commit message with reworked motivation paragraphs, since Junio read the v1 motivation paragraph as off-topic [2]. * Add a test_has_trailer helper, which I'd floated in [3] after Junio's get_signoff suggestion in [2]. * Drop subshells in favor of '-C <directory>' in the tests, as suggested in [2]. * Add tests for the bare pull and lonely --no-signoff cases, as suggested in [2]. With these additions in place, I've dropped v1's "The tests aren't as extensive..." paragraph from the commit message. * Use OPT_PASSTHRU in pull.c. I'm not sure why --allow-unrelated-histories didn't go this route, but there are lots of other pull-to-merge options using OPT_PASSTHRU, so using it for --signoff isn't breaking consistency. Not changed since v1: * The merge-options.txt order paragraph. Junio had suggested it be moved after the break [2], but I think having some commit-message discussion of merge-options.txt ordering is useful, even though I don't have strong opinions on what the ordering should be [3]. This patch (and v1) are based on v2.15.0-rc1, although I expect they'll apply cleanly to anything in that vicinity. Cheers, Trevor [1]: https://public-inbox.org/git/18953f46ffb5e3dbc4da8fbda7fe3ab4298d7cbd.1507752482.git.wking@xxxxxxxxxx/ [2]: https://public-inbox.org/git/xmqqefq92mgw.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxx/ [3]: https://public-inbox.org/git/20171012053002.GZ11004@xxxxxxxxxxxxxxxxxxx/ Documentation/git-merge.txt | 8 -------- Documentation/merge-options.txt | 10 ++++++++++ builtin/pull.c | 6 ++++++ t/t5521-pull-options.sh | 40 ++++++++++++++++++++++++++++++++++++++ t/test-lib-functions.sh | 43 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 99 insertions(+), 8 deletions(-) diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt index 4df6431c34..0ada8c856b 100644 --- a/Documentation/git-merge.txt +++ b/Documentation/git-merge.txt @@ -64,14 +64,6 @@ 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/Documentation/merge-options.txt b/Documentation/merge-options.txt index 4e32304301..f394622d65 100644 --- a/Documentation/merge-options.txt +++ b/Documentation/merge-options.txt @@ -51,6 +51,16 @@ set to `no` at the beginning of them. With --no-log do not list one-line descriptions from the actual commits being merged. +--signoff:: +--no-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). ++ +With --no-signoff do not add a Signed-off-by line. --stat:: -n:: diff --git a/builtin/pull.c b/builtin/pull.c index 6f772e8a22..0413c78a3a 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -86,6 +86,7 @@ static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT; static enum rebase_type opt_rebase = -1; static char *opt_diffstat; static char *opt_log; +static char *opt_signoff; static char *opt_squash; static char *opt_commit; static char *opt_edit; @@ -142,6 +143,9 @@ static struct option pull_options[] = { OPT_PASSTHRU(0, "log", &opt_log, N_("n"), N_("add (at most <n>) entries from shortlog to merge commit message"), PARSE_OPT_OPTARG), + OPT_PASSTHRU(0, "signoff", &opt_signoff, NULL, + N_("add Signed-off-by:"), + PARSE_OPT_OPTARG), OPT_PASSTHRU(0, "squash", &opt_squash, NULL, N_("create a single commit instead of doing a merge"), PARSE_OPT_NOARG), @@ -594,6 +598,8 @@ static int run_merge(void) argv_array_push(&args, opt_diffstat); if (opt_log) argv_array_push(&args, opt_log); + if (opt_signoff) + argv_array_push(&args, opt_signoff); if (opt_squash) argv_array_push(&args, opt_squash); if (opt_commit) diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh index ded8f98dbe..82680a30f8 100755 --- a/t/t5521-pull-options.sh +++ b/t/t5521-pull-options.sh @@ -165,4 +165,44 @@ test_expect_success 'git pull --allow-unrelated-histories' ' ) ' +test_expect_success 'git pull does not add a sign-off line' ' + test_when_finished "rm -fr src dst" && + git init src && + test_commit -C src one && + git clone src dst && + test_commit -C src two && + git -C dst pull --no-ff && + ! test_has_trailer -C dst HEAD Signed-off-by +' + +test_expect_success 'git pull --no-signoff does not add sign-off line' ' + test_when_finished "rm -fr src dst" && + git init src && + test_commit -C src one && + git clone src dst && + test_commit -C src two && + git -C dst pull --no-signoff --no-ff && + ! test_has_trailer -C dst HEAD Signed-off-by +' + +test_expect_success 'git pull --signoff add a sign-off line' ' + test_when_finished "rm -fr src dst" && + git init src && + test_commit -C src one && + git clone src dst && + test_commit -C src two && + git -C dst pull --signoff --no-ff && + test_has_trailer -C dst HEAD Signed-off-by "$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" +' + +test_expect_success 'git pull --no-signoff flag cancels --signoff flag' ' + test_when_finished "rm -fr src dst actual" && + git init src && + test_commit -C src one && + git clone src dst && + test_commit -C src two && + git -C dst pull --signoff --no-signoff --no-ff && + ! test_has_trailer -C dst HEAD Signed-off-by +' + test_done diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 1701fe2a06..08409b1c25 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -726,6 +726,49 @@ test_must_be_empty () { fi } +# Call test_has_trailer with the arguments: +# [-C <directory>] <object> <token> [<value>] +# where <object> is an object name as described in gitrevisions(7), +# <token> is a trailer token (e.g. 'Signed-off-by'), and +# <value> is an optional value (e.g. 'A U Thor <author@xxxxxxxxxxx>'). +# test_has_trailer returns success if the specified trailer is found +# in the object content. If <value> is unset, any value will match. +# +# Both <token> and <value> are basic regular expressions. +# +# If the first argument is "-C", the second argument is used as a path for +# the git invocations. +test_has_trailer () { + INDIR= + case "$1" in + -C) + INDIR="$2" + shift 2 || error "<directory> not specified" + ;; + esac + INDIR="${INDIR:+${INDIR}/}" + OBJECT="$1" + shift || error "<object> not specified" + TOKEN="$1" + shift || error "<token> not specified" + SEP=':' # FIXME: read from trailer.separators? + CONTENT="$(git ${INDIR:+ -C "${INDIR}"} cat-file -p "${OBJECT}")" || error "object ${OBJECT} not found${INDIR:+ in ${INDIR}}" + PATTERN="^${TOKEN}${SEP}" + if test 0 -lt "$#" + then + VALUE="$1" + PATTERN="${PATTERN} *${VALUE}$" + fi + if (echo "${CONTENT}" | grep -q "${PATTERN}") + then + printf "%s found in:\n%s\n" "${PATTERN}" "${CONTENT}" + return 0 + else + printf "%s not found in:\n%s\n" "${PATTERN}" "${CONTENT}" + return 1 + fi +} + # Tests that its two parameters refer to the same revision test_cmp_rev () { git rev-parse --verify "$1" >expect.rev && -- 2.13.6
