{"thread":{"id":"46953","subject":"[PATCH] pull: pass --signoff/--no-signoff to \"git merge\"","startedAt":"2017-10-11T20:19:09Z","lastAt":"2017-10-13T01:48:12Z","messageCount":11,"participants":["W. Trevor King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"330207","messageId":"18953f46ffb5e3dbc4da8fbda7fe3ab4298d7cbd.1507752482.git.wking@tremily.us","threadId":"46953","inReplyTo":null,"subject":"[PATCH] pull: pass --signoff/--no-signoff to \"git merge\"","fromName":"W. Trevor King","fromEmail":"wking@tremily.us","sentAt":"2017-10-11T20:10:47Z","receivedAt":"2017-10-11T20:19:09Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"Following 09c2cb87 (pull: pass --allow-unrelated-histories to \"git\nmerge\", 2016-03-18) with the tests also drawing on 14d01b4f (merge:\nadd a --signoff flag, 2017-07-04).\n\nThe order of options in merge-options.txt isn't clear to me, but I've\nput --signoff between --log and --stat as somewhat alphabetized and\nhaving an \"add to the commit message\" function like --log.\n\nThe tests aren't as extensive as t7614-merge-signoff.sh, but they\nexercises both the --signoff and --no-signoff options.  There may be a\nmore efficient way to set them up (like t7614-merge-signoff.sh's\ntest_setup), but with all the pull options packed into a single test\nscript it seemed easiest to just copy/paste the duplicate setup code.\n\n09c2cb87 didn't motivate the addition of --allow-unrelated-histories\nto pull; only citing the reason from e379fdf3 (merge: refuse to create\ntoo cool a merge by default, 2016-03-18) gave for *not* including it.\nI like having both exposed in pull because while the fetch-and-merge\napproach might be a more popular way to judge \"how well they fit\ntogether\", you can also do that after an optimistic pull.  And in\ncases where an optimistic pull is likely to succeed, suggesting it is\neasier to explain to Git newbies than a FETCH_HEAD merge.\n\nSigned-off-by: W. Trevor King <wking@tremily.us>\n---\n Documentation/git-merge.txt     |  8 --------\n Documentation/merge-options.txt | 10 ++++++++++\n builtin/pull.c                  |  8 ++++++++\n t/t5521-pull-options.sh         | 43 +++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 61 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\nindex 4df6431c34..0ada8c856b 100644\n--- a/Documentation/git-merge.txt\n+++ b/Documentation/git-merge.txt\n@@ -64,14 +64,6 @@ OPTIONS\n -------\n include::merge-options.txt[]\n \n---signoff::\n-\tAdd Signed-off-by line by the committer at the end of the commit\n-\tlog message.  The meaning of a signoff depends on the project,\n-\tbut it typically certifies that committer has\n-\tthe rights to submit this work under the same license and\n-\tagrees to a Developer Certificate of Origin\n-\t(see http://developercertificate.org/ for more information).\n-\n -S[<keyid>]::\n --gpg-sign[=<keyid>]::\n \tGPG-sign the resulting merge commit. The `keyid` argument is\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex 4e32304301..f394622d65 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -51,6 +51,16 @@ set to `no` at the beginning of them.\n With --no-log do not list one-line descriptions from the\n actual commits being merged.\n \n+--signoff::\n+--no-signoff::\n+\tAdd Signed-off-by line by the committer at the end of the commit\n+\tlog message.  The meaning of a signoff depends on the project,\n+\tbut it typically certifies that committer has\n+\tthe rights to submit this work under the same license and\n+\tagrees to a Developer Certificate of Origin\n+\t(see http://developercertificate.org/ for more information).\n++\n+With --no-signoff do not add a Signed-off-by line.\n \n --stat::\n -n::\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 6f772e8a22..4469342f51 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -97,6 +97,7 @@ static struct argv_array opt_strategies = ARGV_ARRAY_INIT;\n static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;\n static char *opt_gpg_sign;\n static int opt_allow_unrelated_histories;\n+static int opt_signoff;\n \n /* Options passed to git-fetch */\n static char *opt_all;\n@@ -175,6 +176,9 @@ static struct option pull_options[] = {\n \tOPT_SET_INT(0, \"allow-unrelated-histories\",\n \t\t    &opt_allow_unrelated_histories,\n \t\t    N_(\"allow merging unrelated histories\"), 1),\n+\tOPT_BOOL(0, \"signoff\",\n+\t\t    &opt_signoff,\n+\t\t    N_(\"add Signed-off-by:\")),\n \n \t/* Options passed to git-fetch */\n \tOPT_GROUP(N_(\"Options related to fetching\")),\n@@ -610,6 +614,10 @@ static int run_merge(void)\n \t\targv_array_push(&args, opt_gpg_sign);\n \tif (opt_allow_unrelated_histories > 0)\n \t\targv_array_push(&args, \"--allow-unrelated-histories\");\n+\tif (opt_signoff > 0)\n+\t\targv_array_push(&args, \"--signoff\");\n+\telse\n+\t\targv_array_push(&args, \"--no-signoff\");\n \n \targv_array_push(&args, \"FETCH_HEAD\");\n \tret = run_command_v_opt(args.argv, RUN_GIT_CMD);\ndiff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\nindex ded8f98dbe..d95789ab8c 100755\n--- a/t/t5521-pull-options.sh\n+++ b/t/t5521-pull-options.sh\n@@ -165,4 +165,47 @@ test_expect_success 'git pull --allow-unrelated-histories' '\n \t)\n '\n \n+test_expect_success 'git pull --signoff add a sign-off line' '\n+\ttest_when_finished \"rm -fr src dst actual expected\" &&\n+\tcat >expected <<-EOF &&\n+\t\tSigned-off-by: $(git var GIT_COMMITTER_IDENT | sed -e \"s/>.*/>/\")\n+\tEOF\n+\tgit init src &&\n+\t(\n+\t\tcd src &&\n+\t\ttest_commit one\n+\t) &&\n+\tgit clone src dst &&\n+\t(\n+\t\tcd src &&\n+\t\ttest_commit two\n+\t) &&\n+\t(\n+\t\tcd dst &&\n+\t\tgit pull --signoff --no-ff &&\n+\t\tgit cat-file commit HEAD | tail -n1 >../actual\n+\t) &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'git pull --no-signoff flag cancels --signoff flag' '\n+\ttest_when_finished \"rm -fr src dst actual\" &&\n+\tgit init src &&\n+\t(\n+\t\tcd src &&\n+\t\ttest_commit one\n+\t) &&\n+\tgit clone src dst &&\n+\t(\n+\t\tcd src &&\n+\t\ttest_commit two\n+\t) &&\n+\t(\n+\t\tcd dst &&\n+\t\tgit pull --signoff --no-signoff --no-ff &&\n+\t\tgit cat-file commit HEAD | sed -n /Signed-off-by/p >../actual\n+\t) &&\n+\ttest_must_be_empty actual\n+'\n+\n test_done\n-- \n2.13.6\n\n"},{"id":"330221","messageId":"xmqqefq92mgw.fsf@gitster.mtv.corp.google.com","threadId":"46953","inReplyTo":"18953f46ffb5e3dbc4da8fbda7fe3ab4298d7cbd.1507752482.git.wking@tremily.us","subject":"Re: [PATCH] pull: pass --signoff/--no-signoff to \"git merge\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-12T01:17:51Z","receivedAt":"2017-10-12T01:17:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"W. Trevor King\" <wking@tremily.us> writes:\n\n> Following 09c2cb87 (pull: pass --allow-unrelated-histories to \"git\n> merge\", 2016-03-18) with the tests also drawing on 14d01b4f (merge:\n> add a --signoff flag, 2017-07-04).\n\nI cannot find a verb in the above.\n\n> The order of options in merge-options.txt isn't clear to me, but I've\n> put --signoff between --log and --stat as somewhat alphabetized and\n> having an \"add to the commit message\" function like --log.\n>\n> The tests aren't as extensive as t7614-merge-signoff.sh, but they\n> exercises both the --signoff and --no-signoff options.  There may be a\n> more efficient way to set them up (like t7614-merge-signoff.sh's\n> test_setup), but with all the pull options packed into a single test\n> script it seemed easiest to just copy/paste the duplicate setup code.\n\nThe above two paragraphs read more like \"requesting help for hints\nto improve this patch\" than commit log message.  Perhaps move them\nbelow the three-dash line and instead describe what you actually did\nhere (if they were worth explaining, that is)?\n\n> 09c2cb87 didn't motivate the addition of --allow-unrelated-histories\n> to pull; only citing the reason from e379fdf3 (merge: refuse to create\n> too cool a merge by default, 2016-03-18) gave for *not* including it.\n> I like having both exposed in pull because while the fetch-and-merge\n> approach might be a more popular way to judge \"how well they fit\n> together\", you can also do that after an optimistic pull.  And in\n> cases where an optimistic pull is likely to succeed, suggesting it is\n> easier to explain to Git newbies than a FETCH_HEAD merge.\n\nI find this paragraph totally unrelated to what the patch does.\nSave it for the patch you add to pass --allow-unrelated-histories\ngiven to pull down to underlying merge, perhaps?\n\n>\n> Signed-off-by: W. Trevor King <wking@tremily.us>\n> ---\n>  Documentation/git-merge.txt     |  8 --------\n>  Documentation/merge-options.txt | 10 ++++++++++\n>  builtin/pull.c                  |  8 ++++++++\n>  t/t5521-pull-options.sh         | 43 +++++++++++++++++++++++++++++++++++++++++\n>  4 files changed, 61 insertions(+), 8 deletions(-)\n> ...\n> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\n> index ded8f98dbe..d95789ab8c 100755\n> --- a/t/t5521-pull-options.sh\n> +++ b/t/t5521-pull-options.sh\n> @@ -165,4 +165,47 @@ test_expect_success 'git pull --allow-unrelated-histories' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'git pull --signoff add a sign-off line' '\n> +\ttest_when_finished \"rm -fr src dst actual expected\" &&\n> +\tcat >expected <<-EOF &&\n> +\t\tSigned-off-by: $(git var GIT_COMMITTER_IDENT | sed -e \"s/>.*/>/\")\n> +\tEOF\n\n\techo \"Signed-off-by: $GIT_COMMITER_NAME <$GIT_COMMITTER_EMAIL>\" >expect\n\nor\n\n\tgit var GIT_COMMITTER_IDENT |\n\tsed -e 's/^\\([^>]*>\\).*/Signed-off-by: \\1/' >expect\n\n> +\tgit init src &&\n> +\t(\n> +\t\tcd src &&\n> +\t\ttest_commit one\n> +\t) &&\n\nI suspect somebody will suggest \"test_commit -C\" ;-)\n\n> +\tgit clone src dst &&\n> +\t(\n> +\t\tcd src &&\n> +\t\ttest_commit two\n> +\t) &&\n> +\t(\n> +\t\tcd dst &&\n> +\t\tgit pull --signoff --no-ff &&\n> +\t\tgit cat-file commit HEAD | tail -n1 >../actual\n\nI think it makes it more robust to replace \"tail\" with \"collect all\nthe signed-off-by lines\" like the other test (below) does.  Perhaps\nhave a helper function and use it in both?\n\n\tget_signoff () {\n\t\tgit cat-file commit \"$1\" | sed -n -e '/^Signed-off-by: /p'\n\t}\n\nSome may say \"cat-file can fail, and having it on the LHS of a pipe\nhides its failure\", advocating for something like:\n\n\tget_signoff () {\n\t\tgit cat-file commit \"$1\" >sign-off-temp &&\n\t\tsed -n -e '/^Signed-off-by: /p' sign-off-temp\n\t}\n\n> +\t) &&\n> +\ttest_cmp expected actual\n> +'\n\n> +test_expect_success 'git pull --no-signoff flag cancels --signoff flag' '\n> +\ttest_when_finished \"rm -fr src dst actual\" &&\n> +\tgit init src &&\n> +\t(\n> +\t\tcd src &&\n> +\t\ttest_commit one\n> +\t) &&\n> +\tgit clone src dst &&\n> +\t(\n> +\t\tcd src &&\n> +\t\ttest_commit two\n> +\t) &&\n> +\t(\n> +\t\tcd dst &&\n> +\t\tgit pull --signoff --no-signoff --no-ff &&\n> +\t\tgit cat-file commit HEAD | sed -n /Signed-off-by/p >../actual\n> +\t) &&\n> +\ttest_must_be_empty actual\n> +'\n> +\n>  test_done\n\nI think \"--signoff\" and \"--signoff --no-signoff\" are reasonable\nminimum things to test.  Two more cases, i.e. running it without\neither and with \"--no-signoff\" alone, to ensure that the sign-off\nmechanism does not kick in would make it even better.\n\nThanks.\n\n"},{"id":"330241","messageId":"20171012053002.GZ11004@valgrind.tremily.us","threadId":"46953","inReplyTo":"xmqqefq92mgw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pull: pass --signoff/--no-signoff to \"git merge\"","fromName":"W. Trevor King","fromEmail":"wking@tremily.us","sentAt":"2017-10-12T05:30:02Z","receivedAt":"2017-10-12T05:31:41Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"On Thu, Oct 12, 2017 at 10:17:51AM +0900, Junio C Hamano wrote:\n> \"W. Trevor King\" <wking@tremily.us> writes:\n> \n> > Following 09c2cb87 (pull: pass --allow-unrelated-histories to \"git\n> > merge\", 2016-03-18) with the tests also drawing on 14d01b4f (merge:\n> > add a --signoff flag, 2017-07-04).\n> \n> I cannot find a verb in the above.\n\nI'd meant it as either a continuation of the subject line, or with an\nimplicit leading “I did this…” :p.  I can reword if you like, maybe\njust “Following” → “Follow”?  Something more drastic?\n\n> > The order of options in merge-options.txt isn't clear to me, but\n> > I've put --signoff between --log and --stat as somewhat\n> > alphabetized and having an \"add to the commit message\" function\n> > like --log.\n> >\n> > The tests aren't as extensive as t7614-merge-signoff.sh, but they\n> > exercises both the --signoff and --no-signoff options.  There may\n> > be a more efficient way to set them up (like\n> > t7614-merge-signoff.sh's test_setup), but with all the pull\n> > options packed into a single test script it seemed easiest to just\n> > copy/paste the duplicate setup code.\n>\n> The above two paragraphs read more like \"requesting help for hints\n> to improve this patch\" than commit log message.  Perhaps move them\n> below the three-dash line and instead describe what you actually did\n> here (if they were worth explaining, that is)?\n\nI think something about merge-options.txt ordering should end up in\nthe history of that content.  Reading through:\n\n  $ git log Documentation/merge-options.txt\n\nonly turned up 690b2975 (Documentation/merge-options.txt: group \"ff\"\nrelated options together, 2012-02-22) discussing option order (it\nsuggested grouping similar options together, although --ff and\n--ff-only would also be close alphabetically).\n\nI agree that the first paragraph you quote above doesn't have me\ncoming down firmly in favor of a particular ordering strategy, but I\nthink having something like it in the Git history will help whoever\nends up giving merge-options.txt a well-defined strategy by showing I\ndidn't have any strong opinions to account for ;).  Silence can mean\n“doesn't have a strong opinion”, but sometimes it means “feels the\nchoice is so obvious that it doesn't need explicit motivation”.\n\nI'm fine moving the second paragraph you quote below the fold in a v2,\nalthough you're calling for more tests below, and it won't apply\nanymore once I've added those :).\n\n> > 09c2cb87 didn't motivate the addition of --allow-unrelated-histories\n> > to pull; only citing the reason from e379fdf3 (merge: refuse to create\n> > too cool a merge by default, 2016-03-18) gave for *not* including it.\n> > I like having both exposed in pull because while the fetch-and-merge\n> > approach might be a more popular way to judge \"how well they fit\n> > together\", you can also do that after an optimistic pull.  And in\n> > cases where an optimistic pull is likely to succeed, suggesting it is\n> > easier to explain to Git newbies than a FETCH_HEAD merge.\n> \n> I find this paragraph totally unrelated to what the patch does.\n> Save it for the patch you add to pass --allow-unrelated-histories\n> given to pull down to underlying merge, perhaps?\n\n09c2cb87 is your commit in master (v2.9.0-rc0~88^2) that is doing just\nthat.  I haven't gone through the list history to figure out why it\nended up getting landed with its current commit message; “Prepare a\npatch to make it a reality, just in case it is needed” sounds more\nlike it was “here's the code in case folks want it, I'll reroll the\nmotivation if they do”.  This paragraph was aiming to motivate both\nthe --signoff pass-through I'm adding here and (retroactively) the\n--allow-unrelated-histories pass-through you added there.  I'll add\nmore context in v2 to try to make that more clear.\n\n> > +\tcat >expected <<-EOF &&\n> > +\t\tSigned-off-by: $(git var GIT_COMMITTER_IDENT | sed -e \"s/>.*/>/\")\n> > +\tEOF\n> \n> \techo \"Signed-off-by: $GIT_COMMITER_NAME <$GIT_COMMITTER_EMAIL>\" >expect\n\nMuch nicer, thanks.  I'll add a patch to v2 to make the same change to\nt7614.\n\n> > +\tgit init src &&\n> > +\t(\n> > +\t\tcd src &&\n> > +\t\ttest_commit one\n> > +\t) &&\n> \n> I suspect somebody will suggest \"test_commit -C\" ;-)\n\nSounds good.  I'll add a patch to v2 to make the same change to the\nexisting t5521 --allow-unrelated-histories test.\n\n> > +\tgit clone src dst &&\n> > +\t(\n> > +\t\tcd src &&\n> > +\t\ttest_commit two\n> > +\t) &&\n> > +\t(\n> > +\t\tcd dst &&\n> > +\t\tgit pull --signoff --no-ff &&\n> > +\t\tgit cat-file commit HEAD | tail -n1 >../actual\n> \n> I think it makes it more robust to replace \"tail\" with \"collect all\n> the signed-off-by lines\" like the other test (below) does.  Perhaps\n> have a helper function and use it in both?\n> \n> \tget_signoff () {\n> \t\tgit cat-file commit \"$1\" | sed -n -e '/^Signed-off-by: /p'\n> \t}\n> \n> Some may say \"cat-file can fail, and having it on the LHS of a pipe\n> hides its failure\", advocating for something like:\n> \n> \tget_signoff () {\n> \t\tgit cat-file commit \"$1\" >sign-off-temp &&\n> \t\tsed -n -e '/^Signed-off-by: /p' sign-off-temp\n> \t}\n\nThere are several existing consumers using grep and sed for this:\n\n  wking@ullr ~/src/git/git $ git grep Signed-off-by v2.15.0-rc1 -- 't/*.sh' | grep 'grep\\|sed'\n  …\n  v2.15.0-rc1:t/t3501-revert-cherry-pick.sh:      git cat-file commit HEAD | grep ^Signed-off-by: >signoff &&\n  v2.15.0-rc1:t/t3507-cherry-pick-conflict.sh:    test_i18ngrep -e \"Signed-off-by\" .git/MERGE_MSG\n  v2.15.0-rc1:t/t3507-cherry-pick-conflict.sh:    test $(git show -s |grep -c \"Signed-off-by\") = 1\n  v2.15.0-rc1:t/t3507-cherry-pick-conflict.sh:    grep -e \"^# Conflicts:\" -e '^Signed-off-by' <.git/COMMIT_EDITMSG >actual &&\n  v2.15.0-rc1:t/t3507-cherry-pick-conflict.sh:    grep -e \"^Conflicts:\" -e '^Signed-off-by' <.git/COMMIT_EDITMSG >actual &&\n  v2.15.0-rc1:t/t3510-cherry-pick-sequence.sh:    ! grep \"Signed-off-by:\" initial_msg &&\n  v2.15.0-rc1:t/t3510-cherry-pick-sequence.sh:    grep \"Signed-off-by:\" unrelatedpick_msg &&\n  v2.15.0-rc1:t/t3510-cherry-pick-sequence.sh:    ! grep \"Signed-off-by:\" picked_msg &&\n  v2.15.0-rc1:t/t3510-cherry-pick-sequence.sh:    grep \"Signed-off-by:\" anotherpick_msg\n  v2.15.0-rc1:t/t3510-cherry-pick-sequence.sh:    ! grep Signed-off-by: msg\n  v2.15.0-rc1:t/t3510-cherry-pick-sequence.sh:    ! grep Signed-off-by: msg\n  v2.15.0-rc1:t/t4014-format-patch.sh:    grep \"^Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\" out\n  v2.15.0-rc1:t/t4014-format-patch.sh:    ! sed \"/^Signed-off-by: /q\" out | grep \"test message\" &&\n  v2.15.0-rc1:t/t4014-format-patch.sh:    sed \"1,/^Signed-off-by: /d\" out | grep \"test message\" &&\n  v2.15.0-rc1:t/t4153-am-resume-override-opts.sh: git cat-file commit HEAD^ | grep \"Signed-off-by:\" >actual &&\n  v2.15.0-rc1:t/t4153-am-resume-override-opts.sh: test $(git cat-file commit HEAD | grep -c \"Signed-off-by:\") -eq 0\n  v2.15.0-rc1:t/t7501-commit.sh:          sed -e \"s/>.*/>/\" -e \"s/^/Signed-off-by: /\"\n  v2.15.0-rc1:t/t7501-commit.sh:          sed -e \"s/>.*/>/\" -e \"s/^/Signed-off-by: /\"\n  v2.15.0-rc1:t/t7501-commit.sh:          sed -e \"s/>.*/>/\" -e \"s/^/Signed-off-by: /\"\n  v2.15.0-rc1:t/t7501-commit.sh:          sed -e \"s/>.*/>/\" -e \"s/^/Signed-off-by: /\"\n  v2.15.0-rc1:t/t7502-commit.sh:  actual=$(git cat-file commit HEAD | sed -ne \"s/Signed-off-by: //p\") &&\n  v2.15.0-rc1:t/t7614-merge-signoff.sh:Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e \"s/>.*/>/\")\n\nPerhaps we want something like:\n\n  test_has_trailer $OBJECT $TOKEN $VALUE\n\nand:\n\n  test_has_no_trailer $OBJECT $TOKEN\n\nin test-lib-functions.sh that we can use in all of these cases?\n\n> I think \"--signoff\" and \"--signoff --no-signoff\" are reasonable\n> minimum things to test.  Two more cases, i.e. running it without\n> either and with \"--no-signoff\" alone, to ensure that the sign-off\n> mechanism does not kick in would make it even better.\n\nSounds good, I'll add those in v2.\n\nCheers,\nTrevor\n\n-- \nThis email may be signed or encrypted with GnuPG (http://www.gnupg.org).\nFor more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy\n"},{"id":"330243","messageId":"xmqq60bk2a7t.fsf@gitster.mtv.corp.google.com","threadId":"46953","inReplyTo":"20171012053002.GZ11004@valgrind.tremily.us","subject":"Re: [PATCH] pull: pass --signoff/--no-signoff to \"git merge\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-12T05:42:30Z","receivedAt":"2017-10-12T05:42:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"W. Trevor King\" <wking@tremily.us> writes:\n\n> On Thu, Oct 12, 2017 at 10:17:51AM +0900, Junio C Hamano wrote:\n>> \"W. Trevor King\" <wking@tremily.us> writes:\n>> \n>> > Following 09c2cb87 (pull: pass --allow-unrelated-histories to \"git\n>> > merge\", 2016-03-18) with the tests also drawing on 14d01b4f (merge:\n>> > add a --signoff flag, 2017-07-04).\n>> \n>> I cannot find a verb in the above.\n>\n> I'd meant it as either a continuation of the subject line, or with an\n\nNever do that.  The title should be able to stand on its own, and\nmust not be an early part of incomplete sentence.\n\n> Much nicer, thanks.  I'll add a patch to v2 to make the same change to\n> t7614.\n> ...\n> Sounds good.  I'll add a patch to v2 to make the same change to the\n> existing t5521 --allow-unrelated-histories test.\n\nPlease don't, unless you are actively working on the features that\nthey test.  We do not have infinite amount of bandwidth to deal with\nchanges for the sake of perceived consistency and no other real\ngain.\n\n\n"},{"id":"330245","messageId":"20171012062351.GB11004@valgrind.tremily.us","threadId":"46953","inReplyTo":"xmqq60bk2a7t.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pull: pass --signoff/--no-signoff to \"git merge\"","fromName":"W. Trevor King","fromEmail":"wking@tremily.us","sentAt":"2017-10-12T06:23:51Z","receivedAt":"2017-10-12T06:23:30Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"On Thu, Oct 12, 2017 at 02:42:30PM +0900, Junio C Hamano wrote:\n> \"W. Trevor King\" <wking@tremily.us> writes:\n> > On Thu, Oct 12, 2017 at 10:17:51AM +0900, Junio C Hamano wrote:\n> >> \"W. Trevor King\" <wking@tremily.us> writes:\n> >> \n> >> > Following 09c2cb87 (pull: pass --allow-unrelated-histories to \"git\n> >> > merge\", 2016-03-18) with the tests also drawing on 14d01b4f (merge:\n> >> > add a --signoff flag, 2017-07-04).\n> >> \n> >> I cannot find a verb in the above.\n> >\n> > I'd meant it as either a continuation of the subject line, or with an\n> \n> Never do that.  The title should be able to stand on its own, and\n> must not be an early part of incomplete sentence.\n\n“Following” to an imperative “Follow” it is then, unless you want a\nmore drastic rewording.\n\n> > Sounds good.  I'll add a patch to v2 to make the same change to\n> > the existing t5521 --allow-unrelated-histories test.\n> \n> Please don't, unless you are actively working on the features that\n> they test.  We do not have infinite amount of bandwidth to deal with\n> changes for the sake of perceived consistency and no other real\n> gain.\n\nBy extention, I'm guessing that means that while the:\n\n  test_has_trailer $OBJECT $TOKEN $VALUE\n\nand:\n\n  test_has_no_trailer $OBJECT $TOKEN\n\ntest-lib-functions.sh helpers I floated may be acceptable (or not, no\nneed to commit before you've seen a patch), you don't want me updating\nexisting tests to use them.  I'll just use them in my new tests, and\nfolks can gradually transition existing tests to them as they touch\nthose tests (if they remember the helpers exist ;).\n\nCheers,\nTrevor\n\n-- \nThis email may be signed or encrypted with GnuPG (http://www.gnupg.org).\nFor more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy\n"},{"id":"330250","messageId":"51d67d6d707182d4973d9961ab29358f26c4988a.1507796638.git.wking@tremily.us","threadId":"46953","inReplyTo":"18953f46ffb5e3dbc4da8fbda7fe3ab4298d7cbd.1507752482.git.wking@tremily.us","subject":"[PATCH v2] pull: pass --signoff/--no-signoff to \"git merge\"","fromName":"W. Trevor King","fromEmail":"wking@tremily.us","sentAt":"2017-10-12T08:46:39Z","receivedAt":"2017-10-12T08:46:48Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"e379fdf3 (merge: refuse to create too cool a merge by default,\n2016-03-18) gave a reason for *not* passing options from pull to\nmerge:\n\n  ...because such a \"two project merge\" would be done after fetching\n  the other project into some location in the working tree of an\n  existing project and making sure how well they fit together...\n\nThat's certainly an acceptable workflow, but I'd also like to support\nmerge options in pull for folks who want to optimistically pull and\nthen sort out \"how well they fit together\" after pull exits (possibly\nwith a merge failure).  And in cases where an optimistic pull is\nlikely to succeed, suggesting it is easier to explain to Git newbies\nthan a FETCH_HEAD merge or remote-addition/merge/remote-removal.\n\n09c2cb87 (pull: pass --allow-unrelated-histories to \"git merge\",\n2016-03-18) added a pull-to-merge pass for a different option but\ndidn't motivate its change, only citing the reason from e379fdf3 for\nnot adding the pull-to-merge pass for that option.  I'm personally in\nfavor of pull-to-merge passing for any unambiguous options, but if the\ndecision for pull-to-merge passes depends on the specific option, then\n--allow-unrelated-histories is probably the weakest candidate because\nunrelated-history merged are more likely to have \"fit together\" issues\nthan the other merge-only options.\n\nThe test_has_trailer helper gives folks a convenient way check these\nsorts of things.  I haven't gone through and updated existing trailer\nchecks (e.g. the ones in t7614-merge-signoff.sh) to avoid the \"only to\ncorrect the inconconsistency\" issue discussed in SubmittingPatches.\nOther test may gradually migrate to the new helper if they find it\nuseful.  The helper may be useful enough to eventually become a\nplumbing command (a read version of interpret-trailers with an API\nsimilar to 'git config ...'?), but I'm not going that far in this\ncommit ;).\n\nThe order of options in merge-options.txt isn't clear to me, but I've\nput --signoff between --log and --stat as somewhat alphabetized and\nhaving an \"add to the commit message\" function like --log.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: W. Trevor King <wking@tremily.us>\n---\nChanges since v1 [1]:\n\n* Dropped \"Following\" paragraph.  Junio took issue with the phrasing\n  [2], and the implementation in v2 has diverged sufficiently from\n  09c2cb87 and 14d01b4f that I don't think citing them as\n  implementation references is useful anymore.\n\n* Lead the commit message with reworked motivation paragraphs, since\n  Junio read the v1 motivation paragraph as off-topic [2].\n\n* Add a test_has_trailer helper, which I'd floated in [3] after\n  Junio's get_signoff suggestion in [2].\n\n* Drop subshells in favor of '-C <directory>' in the tests, as\n  suggested in [2].\n\n* Add tests for the bare pull and lonely --no-signoff cases, as\n  suggested in [2].  With these additions in place, I've dropped v1's\n  \"The tests aren't as extensive...\" paragraph from the commit\n  message.\n\n* Use OPT_PASSTHRU in pull.c.  I'm not sure why\n  --allow-unrelated-histories didn't go this route, but there are lots\n  of other pull-to-merge options using OPT_PASSTHRU, so using it for\n  --signoff isn't breaking consistency.\n\nNot changed since v1:\n\n* The merge-options.txt order paragraph.  Junio had suggested it be\n  moved after the break [2], but I think having some commit-message\n  discussion of merge-options.txt ordering is useful, even though I\n  don't have strong opinions on what the ordering should be [3].\n\nThis patch (and v1) are based on v2.15.0-rc1, although I expect\nthey'll apply cleanly to anything in that vicinity.\n\nCheers,\nTrevor\n\n[1]: https://public-inbox.org/git/18953f46ffb5e3dbc4da8fbda7fe3ab4298d7cbd.1507752482.git.wking@tremily.us/\n[2]: https://public-inbox.org/git/xmqqefq92mgw.fsf@gitster.mtv.corp.google.com/\n[3]: https://public-inbox.org/git/20171012053002.GZ11004@valgrind.tremily.us/\n\n Documentation/git-merge.txt     |  8 --------\n Documentation/merge-options.txt | 10 ++++++++++\n builtin/pull.c                  |  6 ++++++\n t/t5521-pull-options.sh         | 40 ++++++++++++++++++++++++++++++++++++++\n t/test-lib-functions.sh         | 43 +++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 99 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\nindex 4df6431c34..0ada8c856b 100644\n--- a/Documentation/git-merge.txt\n+++ b/Documentation/git-merge.txt\n@@ -64,14 +64,6 @@ OPTIONS\n -------\n include::merge-options.txt[]\n \n---signoff::\n-\tAdd Signed-off-by line by the committer at the end of the commit\n-\tlog message.  The meaning of a signoff depends on the project,\n-\tbut it typically certifies that committer has\n-\tthe rights to submit this work under the same license and\n-\tagrees to a Developer Certificate of Origin\n-\t(see http://developercertificate.org/ for more information).\n-\n -S[<keyid>]::\n --gpg-sign[=<keyid>]::\n \tGPG-sign the resulting merge commit. The `keyid` argument is\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex 4e32304301..f394622d65 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -51,6 +51,16 @@ set to `no` at the beginning of them.\n With --no-log do not list one-line descriptions from the\n actual commits being merged.\n \n+--signoff::\n+--no-signoff::\n+\tAdd Signed-off-by line by the committer at the end of the commit\n+\tlog message.  The meaning of a signoff depends on the project,\n+\tbut it typically certifies that committer has\n+\tthe rights to submit this work under the same license and\n+\tagrees to a Developer Certificate of Origin\n+\t(see http://developercertificate.org/ for more information).\n++\n+With --no-signoff do not add a Signed-off-by line.\n \n --stat::\n -n::\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 6f772e8a22..0413c78a3a 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -86,6 +86,7 @@ static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n static enum rebase_type opt_rebase = -1;\n static char *opt_diffstat;\n static char *opt_log;\n+static char *opt_signoff;\n static char *opt_squash;\n static char *opt_commit;\n static char *opt_edit;\n@@ -142,6 +143,9 @@ static struct option pull_options[] = {\n \tOPT_PASSTHRU(0, \"log\", &opt_log, N_(\"n\"),\n \t\tN_(\"add (at most <n>) entries from shortlog to merge commit message\"),\n \t\tPARSE_OPT_OPTARG),\n+\tOPT_PASSTHRU(0, \"signoff\", &opt_signoff, NULL,\n+\t\tN_(\"add Signed-off-by:\"),\n+\t\tPARSE_OPT_OPTARG),\n \tOPT_PASSTHRU(0, \"squash\", &opt_squash, NULL,\n \t\tN_(\"create a single commit instead of doing a merge\"),\n \t\tPARSE_OPT_NOARG),\n@@ -594,6 +598,8 @@ static int run_merge(void)\n \t\targv_array_push(&args, opt_diffstat);\n \tif (opt_log)\n \t\targv_array_push(&args, opt_log);\n+\tif (opt_signoff)\n+\t\targv_array_push(&args, opt_signoff);\n \tif (opt_squash)\n \t\targv_array_push(&args, opt_squash);\n \tif (opt_commit)\ndiff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\nindex ded8f98dbe..82680a30f8 100755\n--- a/t/t5521-pull-options.sh\n+++ b/t/t5521-pull-options.sh\n@@ -165,4 +165,44 @@ test_expect_success 'git pull --allow-unrelated-histories' '\n \t)\n '\n \n+test_expect_success 'git pull does not add a sign-off line' '\n+\ttest_when_finished \"rm -fr src dst\" &&\n+\tgit init src &&\n+\ttest_commit -C src one &&\n+\tgit clone src dst &&\n+\ttest_commit -C src two &&\n+\tgit -C dst pull --no-ff &&\n+\t! test_has_trailer -C dst HEAD Signed-off-by\n+'\n+\n+test_expect_success 'git pull --no-signoff does not add sign-off line' '\n+\ttest_when_finished \"rm -fr src dst\" &&\n+\tgit init src &&\n+\ttest_commit -C src one &&\n+\tgit clone src dst &&\n+\ttest_commit -C src two &&\n+\tgit -C dst pull --no-signoff --no-ff &&\n+\t! test_has_trailer -C dst HEAD Signed-off-by\n+'\n+\n+test_expect_success 'git pull --signoff add a sign-off line' '\n+\ttest_when_finished \"rm -fr src dst\" &&\n+\tgit init src &&\n+\ttest_commit -C src one &&\n+\tgit clone src dst &&\n+\ttest_commit -C src two &&\n+\tgit -C dst pull --signoff --no-ff &&\n+\ttest_has_trailer -C dst HEAD Signed-off-by \"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n+'\n+\n+test_expect_success 'git pull --no-signoff flag cancels --signoff flag' '\n+\ttest_when_finished \"rm -fr src dst actual\" &&\n+\tgit init src &&\n+\ttest_commit -C src one &&\n+\tgit clone src dst &&\n+\ttest_commit -C src two &&\n+\tgit -C dst pull --signoff --no-signoff --no-ff &&\n+\t! test_has_trailer -C dst HEAD Signed-off-by\n+'\n+\n test_done\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 1701fe2a06..08409b1c25 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -726,6 +726,49 @@ test_must_be_empty () {\n \tfi\n }\n \n+# Call test_has_trailer with the arguments:\n+# [-C <directory>] <object> <token> [<value>]\n+# where <object> is an object name as described in gitrevisions(7),\n+# <token> is a trailer token (e.g. 'Signed-off-by'), and\n+# <value> is an optional value (e.g. 'A U Thor <author@example.com>').\n+# test_has_trailer returns success if the specified trailer is found\n+# in the object content.  If <value> is unset, any value will match.\n+#\n+# Both <token> and <value> are basic regular expressions.\n+#\n+# If the first argument is \"-C\", the second argument is used as a path for\n+# the git invocations.\n+test_has_trailer () {\n+\tINDIR=\n+\tcase \"$1\" in\n+\t-C)\n+\t\tINDIR=\"$2\"\n+\t\tshift 2 || error \"<directory> not specified\"\n+\t\t;;\n+\tesac\n+\tINDIR=\"${INDIR:+${INDIR}/}\"\n+\tOBJECT=\"$1\"\n+\tshift || error \"<object> not specified\"\n+\tTOKEN=\"$1\"\n+\tshift || error \"<token> not specified\"\n+\tSEP=':'  # FIXME: read from trailer.separators?\n+\tCONTENT=\"$(git ${INDIR:+ -C \"${INDIR}\"} cat-file -p \"${OBJECT}\")\" || error \"object ${OBJECT} not found${INDIR:+ in ${INDIR}}\"\n+\tPATTERN=\"^${TOKEN}${SEP}\"\n+\tif test 0 -lt \"$#\"\n+\tthen\n+\t\tVALUE=\"$1\"\n+\t\tPATTERN=\"${PATTERN} *${VALUE}$\"\n+\tfi\n+\tif (echo \"${CONTENT}\" | grep -q \"${PATTERN}\")\n+\tthen\n+\t\tprintf \"%s found in:\\n%s\\n\" \"${PATTERN}\" \"${CONTENT}\"\n+\t\treturn 0\n+\telse\n+\t\tprintf \"%s not found in:\\n%s\\n\" \"${PATTERN}\" \"${CONTENT}\"\n+\t\treturn 1\n+\tfi\n+}\n+\n # Tests that its two parameters refer to the same revision\n test_cmp_rev () {\n \tgit rev-parse --verify \"$1\" >expect.rev &&\n-- \n2.13.6\n\n"},{"id":"330255","messageId":"20171012091822.GA27403@valgrind.us","threadId":"46953","inReplyTo":"51d67d6d707182d4973d9961ab29358f26c4988a.1507796638.git.wking@tremily.us","subject":"Re: [PATCH v2] pull: pass --signoff/--no-signoff to \"git merge\"","fromName":"W. Trevor King","fromEmail":"wking@tremily.us","sentAt":"2017-10-12T09:18:22Z","receivedAt":"2017-10-12T09:19:59Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"On Thu, Oct 12, 2017 at 01:46:39AM -0700, W. Trevor King wrote:\n> The order of options in merge-options.txt isn't clear to me, but\n> I've put --signoff between --log and --stat as somewhat alphabetized\n> and having an \"add to the commit message\" function like --log.\n\nThe order of options in merge-options.txt was intended to be by\n\"alphabetical groups\", at least back in 7c85d274\n(Documentation/merge-options.txt: order options in alphabetical\ngroups, 2009-10-22).  I'm not quite clear on what that means.  After\n7c85d274 landed there were already long-option irregularities:\n\n  $ git grep -h ^-- 7c85d27 -- Documentation/merge-options.txt\n  --commit::\n  --no-commit::\n  --ff::\n  --no-ff::\n  --log::\n  --no-log::\n  --stat::\n  --no-stat::\n  --squash::\n  --no-squash::\n  --strategy=<strategy>::\n  --summary::\n  --no-summary::\n  --quiet::\n  --verbose::\n\nIf the order was purely alphabetical, --stat/--no-stat should have\nafter --squash/--no-squash, and --quiet should have been much earlier.\nAnd putting --signoff after --log is still alphabetical in v2.15.0-rc1\n(ignoring a few outliers).  So I don't think it's a reason to change\nwhere I'd put the option, but in v3 of this patch I'll update the\ncommit message to cite 7c85d274 when motivating the location.\n\nCheers,\nTrevor\n\n-- \nThis email may be signed or encrypted with GnuPG (http://www.gnupg.org).\nFor more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy\n"},{"id":"330259","messageId":"xmqqk200znel.fsf@gitster.mtv.corp.google.com","threadId":"46953","inReplyTo":"51d67d6d707182d4973d9961ab29358f26c4988a.1507796638.git.wking@tremily.us","subject":"Re: [PATCH v2] pull: pass --signoff/--no-signoff to \"git merge\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-12T10:11:14Z","receivedAt":"2017-10-12T10:11:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"W. Trevor King\" <wking@tremily.us> writes:\n\n> e379fdf3 (merge: refuse to create too cool a merge by default,\n> 2016-03-18) gave a reason for *not* passing options from pull to\n> merge:\n>\n>   ...because such a \"two project merge\" would be done after fetching\n>   the other project into some location in the working tree of an\n>   existing project and making sure how well they fit together...\n\nRead the above again and notice the phrase \"two project merge\".  The\nreasoning applies only to the --allow-unrelated-histories option.\nIt gave a reason for not passing *THAT* option and nothing else, and\ndoes not mean to say anything about passing or not passing any other\noptions.  \n\nThat is why I said the reference to that commit was irrelevant in\nthe context of this patch.\n\nIf you find somebody saying \"we should not pass --signoff from pull\nto merge\" when we taught \"--signoff\" to \"merge\", then that may be\nworth referring to, as this commit _will_ be changing that earlier\ndecision.  I however do not think that is a case.  Just saying\n\"merge can take --signoff, but without pull passing --signoff down,\nit is inconvenient, so let's pass it through\" is sufficient to\njustify this change.\n\n> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\n> index ded8f98dbe..82680a30f8 100755\n> --- a/t/t5521-pull-options.sh\n> +++ b/t/t5521-pull-options.sh\n> @@ -165,4 +165,44 @@ test_expect_success 'git pull --allow-unrelated-histories' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'git pull does not add a sign-off line' '\n> +\ttest_when_finished \"rm -fr src dst\" &&\n> +\tgit init src &&\n> +\ttest_commit -C src one &&\n> +\tgit clone src dst &&\n> +\ttest_commit -C src two &&\n> +\tgit -C dst pull --no-ff &&\n> +\t! test_has_trailer -C dst HEAD Signed-off-by\n> +'\n> +\n> +test_expect_success 'git pull --no-signoff does not add sign-off line' '\n> +\ttest_when_finished \"rm -fr src dst\" &&\n> +\tgit init src &&\n> +\ttest_commit -C src one &&\n> +\tgit clone src dst &&\n> +\ttest_commit -C src two &&\n> +\tgit -C dst pull --no-signoff --no-ff &&\n> +\t! test_has_trailer -C dst HEAD Signed-off-by\n> +'\n> +\n> +test_expect_success 'git pull --signoff add a sign-off line' '\n> +\ttest_when_finished \"rm -fr src dst\" &&\n> +\tgit init src &&\n> +\ttest_commit -C src one &&\n> +\tgit clone src dst &&\n> +\ttest_commit -C src two &&\n> +\tgit -C dst pull --signoff --no-ff &&\n> +\ttest_has_trailer -C dst HEAD Signed-off-by \"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n> +'\n> +\n> +test_expect_success 'git pull --no-signoff flag cancels --signoff flag' '\n> +\ttest_when_finished \"rm -fr src dst actual\" &&\n> +\tgit init src &&\n> +\ttest_commit -C src one &&\n> +\tgit clone src dst &&\n> +\ttest_commit -C src two &&\n> +\tgit -C dst pull --signoff --no-signoff --no-ff &&\n> +\t! test_has_trailer -C dst HEAD Signed-off-by\n> +'\n> +\n>  test_done\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 1701fe2a06..08409b1c25 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -726,6 +726,49 @@ test_must_be_empty () {\n>  \tfi\n>  }\n>  \n> +# Call test_has_trailer with the arguments:\n> +# [-C <directory>] <object> <token> [<value>]\n> +# where <object> is an object name as described in gitrevisions(7),\n> +# <token> is a trailer token (e.g. 'Signed-off-by'), and\n> +# <value> is an optional value (e.g. 'A U Thor <author@example.com>').\n> +# test_has_trailer returns success if the specified trailer is found\n> +# in the object content.  If <value> is unset, any value will match.\n> +#\n> +# Both <token> and <value> are basic regular expressions.\n> +#\n> +# If the first argument is \"-C\", the second argument is used as a path for\n> +# the git invocations.\n> +test_has_trailer () {\n> +\tINDIR=\n> +\tcase \"$1\" in\n> +\t-C)\n> +\t\tINDIR=\"$2\"\n> +\t\tshift 2 || error \"<directory> not specified\"\n> +\t\t;;\n> +\tesac\n> +\tINDIR=\"${INDIR:+${INDIR}/}\"\n> +\tOBJECT=\"$1\"\n> +\tshift || error \"<object> not specified\"\n> +\tTOKEN=\"$1\"\n> +\tshift || error \"<token> not specified\"\n> +\tSEP=':'  # FIXME: read from trailer.separators?\n> +\tCONTENT=\"$(git ${INDIR:+ -C \"${INDIR}\"} cat-file -p \"${OBJECT}\")\" || error \"object ${OBJECT} not found${INDIR:+ in ${INDIR}}\"\n> +\tPATTERN=\"^${TOKEN}${SEP}\"\n> +\tif test 0 -lt \"$#\"\n> +\tthen\n> +\t\tVALUE=\"$1\"\n> +\t\tPATTERN=\"${PATTERN} *${VALUE}$\"\n> +\tfi\n> +\tif (echo \"${CONTENT}\" | grep -q \"${PATTERN}\")\n> +\tthen\n> +\t\tprintf \"%s found in:\\n%s\\n\" \"${PATTERN}\" \"${CONTENT}\"\n> +\t\treturn 0\n> +\telse\n> +\t\tprintf \"%s not found in:\\n%s\\n\" \"${PATTERN}\" \"${CONTENT}\"\n> +\t\treturn 1\n> +\tfi\n> +}\n\nThe reason why I suggested a simple \"sed -n -e ...p\" you used in\nyour original was because it could be used to extract not just one\nSigned-off-by: lines to store in >actual, to be compared with an\nexpect that has multiple S-o-b lines and the output is in correct\norder, etc.  An elaborate filter that can onlyl give \"found/not\nfound\" boolean looks a bit over-engineered for no real gain.\n\n>  # Tests that its two parameters refer to the same revision\n>  test_cmp_rev () {\n>  \tgit rev-parse --verify \"$1\" >expect.rev &&\n"},{"id":"330266","messageId":"xmqq7ew0zkqv.fsf@gitster.mtv.corp.google.com","threadId":"46953","inReplyTo":"xmqqefq92mgw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pull: pass --signoff/--no-signoff to \"git merge\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-12T11:08:40Z","receivedAt":"2017-10-12T11:08:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> \tget_signoff () {\n> \t\tgit cat-file commit \"$1\" | sed -n -e '/^Signed-off-by: /p'\n> \t}\n>\n> Some may say \"cat-file can fail, and having it on the LHS of a pipe\n> hides its failure\", advocating for something like:\n>\n> \tget_signoff () {\n> \t\tgit cat-file commit \"$1\" >sign-off-temp &&\n> \t\tsed -n -e '/^Signed-off-by: /p' sign-off-temp\n> \t}\n\nActually we should use git itself for things like this, e.g.\n\n\tgit -C dst show -s --pretty='format:%(trailers)' HEAD >actual &&\n\ttest_cmp expect actual\n\n\n\n"},{"id":"330296","messageId":"129274f0cc768b7a309f41315580fe1013636516.1507832722.git.wking@tremily.us","threadId":"46953","inReplyTo":"51d67d6d707182d4973d9961ab29358f26c4988a.1507796638.git.wking@tremily.us","subject":"[PATCH v3] pull: pass --signoff/--no-signoff to \"git merge\"","fromName":"W. Trevor King","fromEmail":"wking@tremily.us","sentAt":"2017-10-12T18:35:42Z","receivedAt":"2017-10-12T18:35:51Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"merge can take --signoff, but without pull passing --signoff down, it\nis inconvenient, so let's pass it through.\n\nThe order of options in merge-options.txt is mostly alphabetical by\nlong option since 7c85d274 (Documentation/merge-options.txt: order\noptions in alphabetical groups, 2009-10-22).  The long-option bit\ndidn't make it into the commit message, but it's under the fold in\n[1].  I've put --signoff between --log and --stat to preserve the\nalphabetical order.\n\n[1]: https://public-inbox.org/git/87iqe7zspn.fsf@jondo.cante.net/\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: W. Trevor King <wking@tremily.us>\n---\nChanges since v2 [1]:\n\n* Replace the two motivational paragraphs with Junio's suggested\n  sentence [2].\n* Drop test_hash_trailer in favor of --pretty='format:%(trailers)'\n  [3].  It turns out that the builtin tooling I was looking for while\n  working on test_hash_trailer already exists :).\n\nThis patch (like v1 and v2) is based on v2.15.0-rc1, although I expect\nit will apply cleanly to anything in that vicinity.\n\nCheers,\nTrevor\n\n[1]: https://public-inbox.org/git/51d67d6d707182d4973d9961ab29358f26c4988a.1507796638.git.wking@tremily.us/\n[2]: https://public-inbox.org/git/xmqqk200znel.fsf@gitster.mtv.corp.google.com/\n[3]: https://public-inbox.org/git/xmqq7ew0zkqv.fsf@gitster.mtv.corp.google.com/\n\n Documentation/git-merge.txt     |  8 --------\n Documentation/merge-options.txt | 10 +++++++++\n builtin/pull.c                  |  6 ++++++\n t/t5521-pull-options.sh         | 45 +++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 61 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\nindex 4df6431c34..0ada8c856b 100644\n--- a/Documentation/git-merge.txt\n+++ b/Documentation/git-merge.txt\n@@ -64,14 +64,6 @@ OPTIONS\n -------\n include::merge-options.txt[]\n \n---signoff::\n-\tAdd Signed-off-by line by the committer at the end of the commit\n-\tlog message.  The meaning of a signoff depends on the project,\n-\tbut it typically certifies that committer has\n-\tthe rights to submit this work under the same license and\n-\tagrees to a Developer Certificate of Origin\n-\t(see http://developercertificate.org/ for more information).\n-\n -S[<keyid>]::\n --gpg-sign[=<keyid>]::\n \tGPG-sign the resulting merge commit. The `keyid` argument is\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex 4e32304301..f394622d65 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -51,6 +51,16 @@ set to `no` at the beginning of them.\n With --no-log do not list one-line descriptions from the\n actual commits being merged.\n \n+--signoff::\n+--no-signoff::\n+\tAdd Signed-off-by line by the committer at the end of the commit\n+\tlog message.  The meaning of a signoff depends on the project,\n+\tbut it typically certifies that committer has\n+\tthe rights to submit this work under the same license and\n+\tagrees to a Developer Certificate of Origin\n+\t(see http://developercertificate.org/ for more information).\n++\n+With --no-signoff do not add a Signed-off-by line.\n \n --stat::\n -n::\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 6f772e8a22..0413c78a3a 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -86,6 +86,7 @@ static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n static enum rebase_type opt_rebase = -1;\n static char *opt_diffstat;\n static char *opt_log;\n+static char *opt_signoff;\n static char *opt_squash;\n static char *opt_commit;\n static char *opt_edit;\n@@ -142,6 +143,9 @@ static struct option pull_options[] = {\n \tOPT_PASSTHRU(0, \"log\", &opt_log, N_(\"n\"),\n \t\tN_(\"add (at most <n>) entries from shortlog to merge commit message\"),\n \t\tPARSE_OPT_OPTARG),\n+\tOPT_PASSTHRU(0, \"signoff\", &opt_signoff, NULL,\n+\t\tN_(\"add Signed-off-by:\"),\n+\t\tPARSE_OPT_OPTARG),\n \tOPT_PASSTHRU(0, \"squash\", &opt_squash, NULL,\n \t\tN_(\"create a single commit instead of doing a merge\"),\n \t\tPARSE_OPT_NOARG),\n@@ -594,6 +598,8 @@ static int run_merge(void)\n \t\targv_array_push(&args, opt_diffstat);\n \tif (opt_log)\n \t\targv_array_push(&args, opt_log);\n+\tif (opt_signoff)\n+\t\targv_array_push(&args, opt_signoff);\n \tif (opt_squash)\n \t\targv_array_push(&args, opt_squash);\n \tif (opt_commit)\ndiff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\nindex ded8f98dbe..c19d8dbc9d 100755\n--- a/t/t5521-pull-options.sh\n+++ b/t/t5521-pull-options.sh\n@@ -165,4 +165,49 @@ test_expect_success 'git pull --allow-unrelated-histories' '\n \t)\n '\n \n+test_expect_success 'git pull does not add a sign-off line' '\n+\ttest_when_finished \"rm -fr src dst actual\" &&\n+\tgit init src &&\n+\ttest_commit -C src one &&\n+\tgit clone src dst &&\n+\ttest_commit -C src two &&\n+\tgit -C dst pull --no-ff &&\n+\tgit -C dst show -s --pretty=\"format:%(trailers)\" HEAD >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success 'git pull --no-signoff does not add sign-off line' '\n+\ttest_when_finished \"rm -fr src dst actual\" &&\n+\tgit init src &&\n+\ttest_commit -C src one &&\n+\tgit clone src dst &&\n+\ttest_commit -C src two &&\n+\tgit -C dst pull --no-signoff --no-ff &&\n+\tgit -C dst show -s --pretty=\"format:%(trailers)\" HEAD >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success 'git pull --signoff add a sign-off line' '\n+\ttest_when_finished \"rm -fr src dst expected actual\" &&\n+\techo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\" >expected &&\n+\tgit init src &&\n+\ttest_commit -C src one &&\n+\tgit clone src dst &&\n+\ttest_commit -C src two &&\n+\tgit -C dst pull --signoff --no-ff &&\n+\tgit -C dst show -s --pretty=\"format:%(trailers)\" HEAD >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'git pull --no-signoff flag cancels --signoff flag' '\n+\ttest_when_finished \"rm -fr src dst actual\" &&\n+\tgit init src &&\n+\ttest_commit -C src one &&\n+\tgit clone src dst &&\n+\ttest_commit -C src two &&\n+\tgit -C dst pull --signoff --no-signoff --no-ff &&\n+\tgit -C dst show -s --pretty=\"format:%(trailers)\" HEAD >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_done\n-- \n2.13.6\n\n"},{"id":"330330","messageId":"xmqqd15rx1gq.fsf@gitster.mtv.corp.google.com","threadId":"46953","inReplyTo":"129274f0cc768b7a309f41315580fe1013636516.1507832722.git.wking@tremily.us","subject":"Re: [PATCH v3] pull: pass --signoff/--no-signoff to \"git merge\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-13T01:48:05Z","receivedAt":"2017-10-13T01:48:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"W. Trevor King\" <wking@tremily.us> writes:\n\n> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\n> index 4df6431c34..0ada8c856b 100644\n> --- a/Documentation/git-merge.txt\n> +++ b/Documentation/git-merge.txt\n> @@ -64,14 +64,6 @@ OPTIONS\n>  -------\n>  include::merge-options.txt[]\n>  \n> ---signoff::\n> -\tAdd Signed-off-by line by the committer at the end of the commit\n> -\tlog message.  The meaning of a signoff depends on the project,\n> -\tbut it typically certifies that committer has\n> -\tthe rights to submit this work under the same license and\n> -\tagrees to a Developer Certificate of Origin\n> -\t(see http://developercertificate.org/ for more information).\n> -\n>  -S[<keyid>]::\n>  --gpg-sign[=<keyid>]::\n>  \tGPG-sign the resulting merge commit. The `keyid` argument is\n> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\n> index 4e32304301..f394622d65 100644\n> --- a/Documentation/merge-options.txt\n> +++ b/Documentation/merge-options.txt\n> @@ -51,6 +51,16 @@ set to `no` at the beginning of them.\n>  With --no-log do not list one-line descriptions from the\n>  actual commits being merged.\n>  \n> +--signoff::\n> +--no-signoff::\n> +\tAdd Signed-off-by line by the committer at the end of the commit\n> +\tlog message.  The meaning of a signoff depends on the project,\n> +\tbut it typically certifies that committer has\n> +\tthe rights to submit this work under the same license and\n> +\tagrees to a Developer Certificate of Origin\n> +\t(see http://developercertificate.org/ for more information).\n> ++\n> +With --no-signoff do not add a Signed-off-by line.\n\nMakes sense.  Thanks, will queue.\n"}]}