{"thread":{"id":"39325","subject":"[PATCH 3/4] Documentation/config.txt: clarify that pull.ff overrides merge.ff","startedAt":"2015-05-13T09:52:16Z","lastAt":"2015-05-18T20:58:16Z","messageCount":17,"participants":["Paul Tan","Johannes Schindelin","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"261167","messageId":"1431510740-9710-1-git-send-email-pyokagan@gmail.com","threadId":"39325","inReplyTo":null,"subject":"[PATCH 0/4] make pull.ff=true override merge.ff","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-13T09:52:16Z","receivedAt":"2015-05-13T09:52:16Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since b814da8 (pull: add pull.ff configuration, 2014-01-15), running\ngit-pull with the configuration pull.ff=false or pull.ff=only is\nequivalent to passing --no-ff and --ff-only to git-merge. However, if\npull.ff=true, no switch is passed to git-merge. This leads to the\nconfusing behavior where pull.ff=false or pull.ff=only is able to\noverride merge.ff, while pull.ff=true is unable to.\n\nThis patch series adds a failing test to demonstrates the above, and fixes it.\n\nThe documentation is also tweaked to clarify that pull.ff is meant to override\nmerge.ff.\n\nThe last patch makes pull.ff consistent with merge.ff by supporting the config\naliases of true and false (on, off, 0, 1).\n\nPaul Tan (4):\n t7601: test for pull.ff=true overrides merge.ff=false\n pull: make pull.ff=true override merge.ff\n Documentation/config.txt: clarify that pull.ff overrides merge.ff\n pull: parse pull.ff as a bool or string\n\n Documentation/config.txt | 2 +-\n git-pull.sh | 5 ++++-\n t/t7601-merge-pull-config.sh | 8 ++++++++\n 3 files changed, 13 insertions(+), 2 deletions(-)\n\n-- \n2.1.4\n"},{"id":"261168","messageId":"1431510740-9710-2-git-send-email-pyokagan@gmail.com","threadId":"39325","inReplyTo":"1431510740-9710-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH 1/4] t7601: test for pull.ff=true overrides merge.ff=false","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-13T09:52:17Z","receivedAt":"2015-05-13T09:52:17Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since b814da8 (pull: add pull.ff configuration, 2014-01-15), running\ngit-pull with the configuration pull.ff=false or pull.ff=only is\nequivalent to passing --no-ff and --ff-only to git-merge. However, if\npull.ff=true, no switch is passed to git-merge. This leads to the\nconfusing behavior where pull.ff=false or pull.ff=only is able to\noverride merge.ff, while pull.ff=true is unable to.\n\nAdd a failing test that demonstrates this case.\n\nSigned-off-by: Paul Tan \n---\n\n t/t7601-merge-pull-config.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\nindex f768c90..cef94e6 100755\n--- a/t/t7601-merge-pull-config.sh\n+++ b/t/t7601-merge-pull-config.sh\n@@ -45,6 +45,14 @@ test_expect_success 'fast-forward pull succeeds with \"true\" in pull.ff' '\n \ttest \"$(git rev-parse HEAD)\" = \"$(git rev-parse c1)\"\n '\n \n+test_expect_failure 'pull.ff=true overrides merge.ff=false' '\n+\tgit reset --hard c0 &&\n+\ttest_config merge.ff false &&\n+\ttest_config pull.ff true &&\n+\tgit pull . c1 &&\n+\tverbose test \"$(git rev-parse HEAD)\" = \"$(git rev-parse c1)\"\n+'\n+\n test_expect_success 'fast-forward pull creates merge with \"false\" in pull.ff' '\n \tgit reset --hard c0 &&\n \ttest_config pull.ff false &&\n-- \n2.1.4\n"},{"id":"261169","messageId":"1431510740-9710-3-git-send-email-pyokagan@gmail.com","threadId":"39325","inReplyTo":"1431510740-9710-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH 2/4] pull: make pull.ff=true override merge.ff","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-13T09:52:18Z","receivedAt":"2015-05-13T09:52:18Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"While since b814da8 (pull: add pull.ff configuration, 2014-01-15)\ngit-pull would set the --no-ff or --ff-only switch if pull.ff was false\nand only respectively, it did not set the --ff switch if pull.ff was\ntrue. This led to the inconsistent behavior that pull.ff=false and\npull.ff=only would override merge.ff, but pull.ff=true would not.\n\nFix this by adding the --ff switch if pull.ff=true.\n---\n git-pull.sh | 3 +++\n t/t7601-merge-pull-config.sh | 2 +-\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/git-pull.sh b/git-pull.sh\nindex 9ed01fd..e51dd37 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -56,6 +56,9 @@ fi\n # Setup default fast-forward options via `pull.ff`\n pull_ff=$(git config pull.ff)\n case \"$pull_ff\" in\n+true)\n+\tno_ff=--ff\n+\t;;\n false)\n \tno_ff=--no-ff\n \t;;\ndiff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\nindex cef94e6..d16ef8b 100755\n--- a/t/t7601-merge-pull-config.sh\n+++ b/t/t7601-merge-pull-config.sh\n@@ -45,7 +45,7 @@ test_expect_success 'fast-forward pull succeeds with \"true\" in pull.ff' '\n \ttest \"$(git rev-parse HEAD)\" = \"$(git rev-parse c1)\"\n '\n \n-test_expect_failure 'pull.ff=true overrides merge.ff=false' '\n+test_expect_success 'pull.ff=true overrides merge.ff=false' '\n \tgit reset --hard c0 &&\n \ttest_config merge.ff false &&\n \ttest_config pull.ff true &&\n-- \n2.1.4\n"},{"id":"261165","messageId":"1431510740-9710-4-git-send-email-pyokagan@gmail.com","threadId":"39325","inReplyTo":"1431510740-9710-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH 3/4] Documentation/config.txt: clarify that pull.ff overrides merge.ff","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-13T09:52:19Z","receivedAt":"2015-05-13T09:52:19Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since the pull.ff setting was implemented in b814da8 (pull: add pull.ff\nconfiguration, 2014-01-15) pull.ff has always overridden merge.ff,\nalthough not mentioned in the documentation.\n\nAs such, clarify that this is so.\n\nSigned-off-by: Paul Tan \n---\n Documentation/config.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 948b8b0..ec0f9ef 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2042,7 +2042,7 @@ pull.ff::\n \ta case (equivalent to giving the `--no-ff` option from the command\n \tline). When set to `only`, only such fast-forward merges are\n \tallowed (equivalent to giving the `--ff-only` option from the\n-\tcommand line).\n+\tcommand line). This setting overrides `merge.ff` when pulling.\n \n pull.rebase::\n \tWhen true, rebase branches on top of the fetched branch, instead\n-- \n2.1.4\n"},{"id":"261166","messageId":"1431510740-9710-5-git-send-email-pyokagan@gmail.com","threadId":"39325","inReplyTo":"1431510740-9710-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH 4/4] pull: parse pull.ff as a bool or string","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-13T09:52:20Z","receivedAt":"2015-05-13T09:52:20Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since b814da8 (pull: add pull.ff configuration, 2014-01-15) git-pull\nsupported setting --(no-)ff via the pull.ff configuration value.\nHowever, as it only matches the string values of \"true\" and \"false\", it\ndoes not support other boolean aliases such as \"on\", \"off\", \"1\", \"0\".\nThis is inconsistent with the merge.ff setting, which supports these\naliases.\n\nFix this by using the bool_or_string_config function to retrieve the\nvalue of pull.ff.\n\nSigned-off-by: Paul Tan \n---\n git-pull.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-pull.sh b/git-pull.sh\nindex e51dd37..09bc678 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -54,7 +54,7 @@ then\n fi\n \n # Setup default fast-forward options via `pull.ff`\n-pull_ff=$(git config pull.ff)\n+pull_ff=$(bool_or_string_config pull.ff)\n case \"$pull_ff\" in\n true)\n \tno_ff=--ff\n-- \n2.1.4\n"},{"id":"261225","messageId":"f80d59c142b64c56fe7159452d4d10e3@www.dscho.org","threadId":"39325","inReplyTo":"1431510740-9710-1-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH 0/4] make pull.ff=true override merge.ff","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-05-14T12:59:35Z","receivedAt":"2015-05-14T12:59:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Paul,\n\nOn 2015-05-13 11:52, Paul Tan wrote:\n> Since b814da8 (pull: add pull.ff configuration, 2014-01-15), running\n> git-pull with the configuration pull.ff=false or pull.ff=only is\n> equivalent to passing --no-ff and --ff-only to git-merge. However, if\n> pull.ff=true, no switch is passed to git-merge. This leads to the\n> confusing behavior where pull.ff=false or pull.ff=only is able to\n> override merge.ff, while pull.ff=true is unable to.\n> \n> This patch series adds a failing test to demonstrates the above, and fixes it.\n> \n> The documentation is also tweaked to clarify that pull.ff is meant to override\n> merge.ff.\n> \n> The last patch makes pull.ff consistent with merge.ff by supporting the config\n> aliases of true and false (on, off, 0, 1).\n\nLooks very good!\n\nReviewed-by: Johannes Schindelin \n\nCiao,\nDscho\n"},{"id":"261233","messageId":"2a5e49ff1cc3a0275e09328a3ad07866@www.dscho.org","threadId":"39325","inReplyTo":"1431510740-9710-2-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH 1/4] t7601: test for pull.ff=true overrides merge.ff=false","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-05-14T13:06:18Z","receivedAt":"2015-05-14T13:06:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn 2015-05-13 11:52, Paul Tan wrote:\n> diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\n> index f768c90..cef94e6 100755\n> --- a/t/t7601-merge-pull-config.sh\n> +++ b/t/t7601-merge-pull-config.sh\n> @@ -45,6 +45,14 @@ test_expect_success 'fast-forward pull succeeds\n> with \"true\" in pull.ff' '\n> \ttest \"$(git rev-parse HEAD)\" = \"$(git rev-parse c1)\"\n> '\n> \n> +test_expect_failure 'pull.ff=true overrides merge.ff=false' '\n> +\tgit reset --hard c0 &&\n> +\ttest_config merge.ff false &&\n> +\ttest_config pull.ff true &&\n> +\tgit pull . c1 &&\n> +\tverbose test \"$(git rev-parse HEAD)\" = \"$(git rev-parse c1)\"\n\nGiven that Junio objected to this \"verbose test\", maybe you want to remove the \"verbose\"? Or introduce a `test_assert_equal` of the form\n\n```sh\ntest_assert_equal () {\n test \"a$1\" = \"a$2\" || {\n echo \"$1 != $2\" >&2\n false\n }\n}\n```\n\nHmm. Now that I think about it, `test_eq` is probably a better name, still.\n\nFor the sake of having better reporting, say, in Continuous Integration (where re-running tests via `sh -x t????-*.sh -i -v` -- as Junio suggested -- is not an option), I agree that it would be good to report the non-matching strings.\n\nCiao,\nDscho\n"},{"id":"261237","messageId":"xmqqy4kr85wg.fsf@gitster.dls.corp.google.com","threadId":"39325","inReplyTo":"2a5e49ff1cc3a0275e09328a3ad07866@www.dscho.org","subject":"Re: [PATCH 1/4] t7601: test for pull.ff=true overrides merge.ff=false","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-14T16:45:19Z","receivedAt":"2015-05-14T16:45:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> +\tverbose test \"$(git rev-parse HEAD)\" = \"$(git rev-parse c1)\"\n> \n> Given that Junio objected to this \"verbose test\", maybe you want to remove the \"verbose\"? Or introduce a `test_assert_equal` of the form\n>\n> ```sh\n> test_assert_equal () {\n> test \"a$1\" = \"a$2\" || {\n> echo \"$1 != $2\" >&2\n> false\n> }\n> }\n> ```\n>\n> Hmm. Now that I think about it, `test_eq` is probably a better name, still.\n\nI do not think using \"two strings\" check for that one is very\nuseful. You cannot tell where two 40-HEX came from.\n\nAnd that is why I suggested \"-i -v -x\".\n\nBy the way, I cannot think of a reason why your CI cannot always run\nthe tests with GIT_TEST_OPTS=\"-v -x\" exported.\n\n> For the sake of having better reporting, say, in Continuous Integration (where re-running tests via `sh -x t????-*.sh -i -v` -- as Junio suggested -- is not an option), I agree that it would be good to report the non-matching strings.\n>\n> Ciao,\n> Dscho\n"},{"id":"261238","messageId":"81926313e0a131cd7224f0f6f8984dd2@www.dscho.org","threadId":"39325","inReplyTo":"xmqqy4kr85wg.fsf@gitster.dls.corp.google.com","subject":"sh -x -i -v with continuous integration, was Re: [PATCH 1/4] t7601: test for pull.ff=true overrides merge.ff=false","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-05-14T16:53:19Z","receivedAt":"2015-05-14T16:53:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn 2015-05-14 18:45, Junio C Hamano wrote:\n\n> By the way, I cannot think of a reason why your CI cannot always run\n> the tests with GIT_TEST_OPTS=\"-v -x\" exported.\n\nAsketh and ye shall be given: without running the tests in parallel, our Jenkins would take *even longer* than the three hours per test suite run (which is really painful, still, by the way). And as you know, running the tests with \"-v -x\" is awfully useless if you run the test suite in parallel.\n\nCome to think of it, the proposed \"verbose test\" will not help this use case one bit, neither would test_eq.\n\nCiao,\nDscho\n"},{"id":"261240","messageId":"xmqqh9rf84we.fsf@gitster.dls.corp.google.com","threadId":"39325","inReplyTo":"81926313e0a131cd7224f0f6f8984dd2@www.dscho.org","subject":"Re: sh -x -i -v with continuous integration, was Re: [PATCH 1/4] t7601: test for pull.ff=true overrides merge.ff=false","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-14T17:06:57Z","receivedAt":"2015-05-14T17:06:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Asketh and ye shall be given: without running the tests in parallel,\n> our Jenkins would take *even longer* than the three hours per test\n> suite run (which is really painful, still, by the way). And as you\n> know, running the tests with \"-v -x\" is awfully useless if you run the\n> test suite in parallel.\n>\n> Come to think of it, the proposed \"verbose test\" will not help this\n> use case one bit, neither would test_eq.\n\nYeah, I'd agree. So let's forget about adding a way to make the\ntest output more verbose for CI's sake.\n\nAnd I would say that \"verbose test\" is not a useful way to make the\ntest output more verbose for Human's sake; GIT_TEST_OPTS=\"-v -x -i\"\non the other hand is.\n\nI am very tempted to suggest doing this ;-)\n\n t/test-lib-functions.sh | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 0698ce7..c6c67e8 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -638,8 +638,14 @@ test_cmp_bin() {\n }\n \n # Call any command \"$@\" but be more verbose about its\n-# failure. This is handy for commands like \"test\" which do\n-# not output anything when they fail.\n+# failure. This may seem handy for commands like \"test\" which do\n+# not output anything when they fail, but in practice not\n+# very useful for things like 'test \"$actual\" = \"$expect\"',\n+# as this only shows the actual values (i.e. after $actual and\n+# $expect are turned into the values in these variables).\n+#\n+# In short, do not add use of this; this is kept only to\n+# avoid having to remove its use (for now).\n verbose () {\n \t\"$@\" && return 0\n \techo >&2 \"command failed: $(git rev-parse --sq-quote \"$@\")\"\n"},{"id":"261389","messageId":"CACRoPnTQCKJbuFcoTBAvJS8X+b7Yv35MKgo+HhAhfOvuyMWJ-w@mail.gmail.com","threadId":"39325","inReplyTo":"81926313e0a131cd7224f0f6f8984dd2@www.dscho.org","subject":"Re: sh -x -i -v with continuous integration, was Re: [PATCH 1/4] t7601: test for pull.ff=true overrides merge.ff=false","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-16T12:33:28Z","receivedAt":"2015-05-16T12:33:28Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi Johannes,\n\nOn Fri, May 15, 2015 at 12:53 AM, Johannes Schindelin\n wrote:\n> Asketh and ye shall be given: without running the tests in parallel, our Jenkins would take *even longer* than the three hours per test suite run (which is really painful, still, by the way). And as you know, running the tests with \"-v -x\" is awfully useless if you run the test suite in parallel.\n\nWould the --tee option work for this case?\n\n --tee::\n In addition to printing the test output to the terminal,\n write it to files named 't/test-results/$TEST_NAME.out'.\n As the names depend on the tests' file names, it is safe to\n run the tests with this option in parallel.\n\nThanks,\nPaul\n"},{"id":"261391","messageId":"20150516152858.GA19269@peff.net","threadId":"39325","inReplyTo":"xmqqh9rf84we.fsf@gitster.dls.corp.google.com","subject":"Re: sh -x -i -v with continuous integration, was Re: [PATCH 1/4] t7601: test for pull.ff=true overrides merge.ff=false","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-16T15:28:58Z","receivedAt":"2015-05-16T15:28:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 14, 2015 at 10:06:57AM -0700, Junio C Hamano wrote:\n\n> And I would say that \"verbose test\" is not a useful way to make the\n> test output more verbose for Human's sake; GIT_TEST_OPTS=\"-v -x -i\"\n> on the other hand is.\n> \n> I am very tempted to suggest doing this ;-)\n> \n> t/test-lib-functions.sh | 10 ++++++++--\n> 1 file changed, 8 insertions(+), 2 deletions(-)\n> \n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 0698ce7..c6c67e8 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -638,8 +638,14 @@ test_cmp_bin() {\n> }\n> \n> # Call any command \"$@\" but be more verbose about its\n> -# failure. This is handy for commands like \"test\" which do\n> -# not output anything when they fail.\n> +# failure. This may seem handy for commands like \"test\" which do\n> +# not output anything when they fail, but in practice not\n> +# very useful for things like 'test \"$actual\" = \"$expect\"',\n> +# as this only shows the actual values (i.e. after $actual and\n> +# $expect are turned into the values in these variables).\n> +#\n> +# In short, do not add use of this; this is kept only to\n> +# avoid having to remove its use (for now).\n\nSorry, but I completely disagree here (no surprise, as I was the one who\nadded \"verbose\").\n\nIt's true that it does not tell you the original commands that were\nhanded to the shell. But neither does the output of \"test_cmp\". However,\ncombined with the output of the test code snippet that we already\nprovide, it is often clear.\n\nE.g., let's \"break\" a verbose test from the test suite and break it:\n\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 1b2e981..f3206a3 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -113,7 +113,7 @@ test_expect_success 'diff-filter=M' '\n \n \tactual=$(git log --pretty=\"format:%s\" --diff-filter=M HEAD) &&\n \texpect=$(echo second) &&\n-\tverbose test \"$actual\" = \"$expect\"\n+\tverbose test \"$actual\" = \"foo-$expect\"\n \n '\n \n\nRight now the output is:\n\n expecting success: \n \n actual=$(git log --pretty=\"format:%s\" --diff-filter=M HEAD) &&\n expect=$(echo second) &&\n verbose test \"$actual\" = \"foo-$expect\"\n \n \n command failed: 'test' 'second' '=' 'foo-second'\n not ok 10 - diff-filter=M\n\nwhich IMHO is pretty helpful. If we take away the \"verbose\", we get:\n\n expecting success: \n \n actual=$(git log --pretty=\"format:%s\" --diff-filter=M HEAD) &&\n expect=$(echo second) &&\n test \"$actual\" = \"foo-$expect\"\n \n \n not ok 10 - diff-filter=M\n\nwhich leaves you with head-scratching (did log exit non-zero, or did the\ncomparison fail?). If we add in \"-x\", we get:\n\n expecting success: \n\n actual=$(git log --pretty=\"format:%s\" --diff-filter=M HEAD) &&\n expect=$(echo second) &&\n test \"$actual\" = \"foo-$expect\"\n \n \n + git log --pretty=format:%s --diff-filter=M HEAD\n + actual=second\n + echo second\n + expect=second\n + test second = foo-second\n error: last command exited with $?=1\n not ok 10 - diff-filter=M\n\nThat essentially gives us the same data, but with a lot of other cruft.\nIn this case, I don't think it's too bad (the test isn't that long), but\nsome of the \"-x\" cases can get pretty verbose. I guess things are a bit\nbetter since I added the \"last command exited...\" in red, but I still\nthink the \"verbose\" version is the most straightforward.\n\nDo we object to having to sprinkle the \"verbose\" throughout the test\nscripts? IMHO it is not any worse than test_cmp. But if that is a\nproblem, would it be too magical to do something like:\n\n test () {\n\tverbose command test \"$@\"\n }\n\n(but only inside the test snippets)?\n\n-Peff\n"},{"id":"261394","messageId":"xmqq1tigfij8.fsf@gitster.dls.corp.google.com","threadId":"39325","inReplyTo":"20150516152858.GA19269@peff.net","subject":"Re: sh -x -i -v with continuous integration, was Re: [PATCH 1/4] t7601: test for pull.ff=true overrides merge.ff=false","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-16T19:07:23Z","receivedAt":"2015-05-16T19:07:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Do we object to having to sprinkle the \"verbose\" throughout the test\n> scripts?\n\nYes. \n\nAn unconstrained \"verbose\" that applies to anything would make\npeople less careful to come up with more useful abstractions,\ne.g. test_line_count, which I view as a bigger problem.\n\n\"verbose test\", regardless of how it is spelled, either as two words\n\"verbose test\", a \"verbose_test\" helper, or a helper function that\noverrides \"test\", share the same issue, as their unconstrained-ness\ncomes from the fact that test is too broad a command. I'd rather\nwant to see us move more in the direction of encouraging things like\ntest_line_count, which makes it easier to write new tests with less\nmistakes (e.g. by hiding the BSD wc pitfall \"$(wc -l Jeff King writes:\n> \n> > Do we object to having to sprinkle the \"verbose\" throughout the test\n> > scripts?\n> \n> Yes. \n> \n> An unconstrained \"verbose\" that applies to anything would make\n> people less careful to come up with more useful abstractions,\n> e.g. test_line_count, which I view as a bigger problem.\n\nI don't think that is true at all. They serve two different purposes.\n\nWe need \"test_line_count\" because of portability considerations. But we\ndo not need \"test_eq\" for that; the only reason to add it is for\ndebuggable output. We _could_ add a bunch of little helpers to cover\neach situation, but empirically we have not[1], because it's annoying to\ndo so. Instead we just write a bare \"test\". I had hoped that \"verbose\"\nwould make it sufficiently easy to improve upon the status quo that\npeople would actually use it[2].\n\nSo I don't really buy the argument that \"verbose\" makes things worse. I\nthink I _do_ buy the argument that it is not worth the extra effort\ngiven that I implemented \"-x\" around the same time. I think the\n\"verbose\" output is a little nicer, as I said earlier, but having the\ntest-writer do literally zero work may be worth it.\n\n-Peff\n\n[1] The exception, AFAIK, is test_path_is_*.\n\n[2] For the most part, actually, I think the status quo is using\n test_cmp, and most of the uses of \"test\" are old. Using test_cmp is\n more verbose, but it does have the advantage of checking the exit\n code of the sub-programs (and also its output is much saner for\n multi-line comparisons, but that isn't relevant to Paul's patches).\n\n I'd be in favor of helpers that made it shorter to use test_cmp.\n E.g.:\n\n # \"actual\" contains \"foo\\n\"; we save one line of \"echo\"\n git foo >actual &&\n test_cmp_str foo actual\n\n # same, but take multiline input on stdin. Saves one line of \"cat\"\n git foo >actual &&\n test_cmp_str - actual <<-\\EOF\n foo\n EOF\n\n # similar, but it runs the command for us and compares its\n # stdout, saving another line. Note that we take stdout as-is,\n # so you get no opportunity to post-process it (but you\n # can still do it the \"long\" way as above if you need to).\n test_cmp_cmd foo git foo\n\n # same, with stdin; obviously this doesn't work if you need to\n # send something over stdin to the command, but again, you can\n # fallback to the old way.\n test_cmp_cmd - git foo <<-\\EOF\n foo\n EOF\n\n These also go in the direction of \"verbose\", by providing generic\n solutions rather than specific ones. So by the argument you made\n above, this is a bad thing.\n"},{"id":"261509","messageId":"xmqq8uclbouc.fsf@gitster.dls.corp.google.com","threadId":"39325","inReplyTo":"20150518184528.GA11463@peff.net","subject":"Re: sh -x -i -v with continuous integration, was Re: [PATCH 1/4] t7601: test for pull.ff=true overrides merge.ff=false","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-18T20:41:31Z","receivedAt":"2015-05-18T20:41:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> [2] For the most part, actually, I think the status quo is using\n> test_cmp, and most of the uses of \"test\" are old. Using test_cmp is\n> more verbose, but it does have the advantage of checking the exit\n> code of the sub-programs (and also its output is much saner for\n> multi-line comparisons, but that isn't relevant to Paul's patches).\n>\n> I'd be in favor of helpers that made it shorter to use test_cmp.\n> E.g.:\n>\n> # \"actual\" contains \"foo\\n\"; we save one line of \"echo\"\n> git foo >actual &&\n> test_cmp_str foo actual\n>\n> # same, but take multiline input on stdin. Saves one line of \"cat\"\n> git foo >actual &&\n> test_cmp_str - actual <<-\\EOF\n> foo\n> EOF\n>\n> # similar, but it runs the command for us and compares its\n> # stdout, saving another line. Note that we take stdout as-is,\n> # so you get no opportunity to post-process it (but you\n> # can still do it the \"long\" way as above if you need to).\n> test_cmp_cmd foo git foo\n>\n> # same, with stdin; obviously this doesn't work if you need to\n> # send something over stdin to the command, but again, you can\n> # fallback to the old way.\n> test_cmp_cmd - git foo <<-\\EOF\n> foo\n> EOF\n>\n> These also go in the direction of \"verbose\", by providing generic\n> solutions rather than specific ones. So by the argument you made\n> above, this is a bad thing.\n\nI do not agree with that. Perhaps \"unconstained\" was an unfit\nphrase.\n\n\"test_cmp_cmd expected-string git foo\" may be \"test $a = $b\" inside,\nbut its debug error message could be made to say \"output from 'git\nfoo' is different from 'expected-string'\", which is a very good\nthing. Contrast that with:\n\n\tverbose test \"$(git foo)\" = \"expected-string\"\n\nwhich would not give you that information (and you'd need to do the\n\"-x\" thing to find out where the string that is different from the\nexpected string came from).\n\nUltimately that is because 'test' that an individual test script\nwrites in it does not constrain what is on each side of comparison\n(for that matter, it does not even constrain that what is tested is\ncomparison between two things), which makes it very hard to give\nuseful diagnoses. 'test' itself operates at too low a level, and\nwrapping it with 'verbose' does not recover the information already\nlost when invoking 'test'.\n\nSo, I suspect we want more or less the same thing.\n\nWe agree that we need shorter, easier to use, and less error prone\nidioms, like test_cmp (comparing two files and show the differences\nas diagnosis), test_cmp_rev (comparing two extended SHA-1\nexpressions as object names), and test_line_count (checking the\nnumber of lines in a file). I just do not think \"verbose\" that can\napply to any command will help us move in that direction very much,\nand I find that \"verbose\" applied to something overly versatile like\n\"test\" is a prime example that it does not help us much.\n"},{"id":"261510","messageId":"20150518205008.GB30217@peff.net","threadId":"39325","inReplyTo":"xmqq8uclbouc.fsf@gitster.dls.corp.google.com","subject":"Re: sh -x -i -v with continuous integration, was Re: [PATCH 1/4] t7601: test for pull.ff=true overrides merge.ff=false","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-18T20:50:08Z","receivedAt":"2015-05-18T20:50:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 18, 2015 at 01:41:31PM -0700, Junio C Hamano wrote:\n\n> So, I suspect we want more or less the same thing.\n> \n> We agree that we need shorter, easier to use, and less error prone\n> idioms, like test_cmp (comparing two files and show the differences\n> as diagnosis), test_cmp_rev (comparing two extended SHA-1\n> expressions as object names), and test_line_count (checking the\n> number of lines in a file). I just do not think \"verbose\" that can\n> apply to any command will help us move in that direction very much,\n> and I find that \"verbose\" applied to something overly versatile like\n> \"test\" is a prime example that it does not help us much.\n\nI'd agree with that if you replace \"need\" with \"it would be nice, but it\nclearly isn't high enough priority for people to actually write the\nhelpers, so...\". :)\n\nI dunno. Maybe there are only a few helpers needed, and we can just add\nthem and start using them. But if we start having to grow a lot of them,\nit can be annoying.\n\nWhat do you want to do with the \"verbose\" calls I have been sprinkling\nthrough the test suite (and the function itself)? Leave them or remove\nthem? A grep for \"verbose \" (with the trailing space) shows some\nexamples.\n\n-Peff\n"},{"id":"261512","messageId":"xmqqzj51a9hz.fsf@gitster.dls.corp.google.com","threadId":"39325","inReplyTo":"20150518205008.GB30217@peff.net","subject":"Re: sh -x -i -v with continuous integration, was Re: [PATCH 1/4] t7601: test for pull.ff=true overrides merge.ff=false","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-18T20:58:16Z","receivedAt":"2015-05-18T20:58:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> What do you want to do with the \"verbose\" calls I have been sprinkling\n> through the test suite (and the function itself)? Leave them or remove\n> them? A grep for \"verbose \" (with the trailing space) shows some\n> examples.\n\nThat was one of the things I wanted to do next, as I suspected that\nthe small number of existing ones might be mostly \"obviously good\"\nones (I never said any 'verbose test' is automatically bad; it is\njust 'verbose test \"$a\" = \"$b\"' that hides what $a and $b were are\nas useful as without 'verbose'), but I didn't offhand think of\nexamples of obviously good ones.\n"}]}