Re: [PATCH v2] pull: pass --signoff/--no-signoff to "git merge"
From: Junio C Hamano <hidden>
Date: 2017-10-12 10:11:22
"W. Trevor King" [off-list ref] writes:
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...
Read the above again and notice the phrase "two project merge". The reasoning applies only to the --allow-unrelated-histories option. It gave a reason for not passing *THAT* option and nothing else, and does not mean to say anything about passing or not passing any other options. That is why I said the reference to that commit was irrelevant in the context of this patch. If you find somebody saying "we should not pass --signoff from pull to merge" when we taught "--signoff" to "merge", then that may be worth referring to, as this commit _will_ be changing that earlier decision. I however do not think that is a case. Just saying "merge can take --signoff, but without pull passing --signoff down, it is inconvenient, so let's pass it through" is sufficient to justify this change.
quoted hunk
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_donediff --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@example.com>'). +# 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 +}
The reason why I suggested a simple "sed -n -e ...p" you used in your original was because it could be used to extract not just one Signed-off-by: lines to store in >actual, to be compared with an expect that has multiple S-o-b lines and the output is in correct order, etc. An elaborate filter that can onlyl give "found/not found" boolean looks a bit over-engineered for no real gain.
# Tests that its two parameters refer to the same revision
test_cmp_rev () {
git rev-parse --verify "$1" >expect.rev &&