{"thread":{"id":"41862","subject":"[PATCH 0/5] modify tests for --[no-]autostash option","startedAt":"2016-03-29T13:29:55Z","lastAt":"2016-04-04T18:25:32Z","messageCount":22,"participants":["Mehul Jain","Eric Sunshine","Junio C Hamano","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"282057","messageId":"1459258200-32444-1-git-send-email-mehul.jain2029@gmail.com","threadId":"41862","inReplyTo":null,"subject":"[PATCH 0/5] modify tests for --[no-]autostash option","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-29T13:29:55Z","receivedAt":"2016-03-29T13:29:55Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"The following patch series is applicable on mj/pull-rebase-autostash.\n\nThis series contain changes suggested by Eric and Matthieu on following\nseries\n\n http://thread.gmane.org/gmane.comp.version-control.git/289434\n\nChanges made:\n * [Patch 4/5] reduces the code needed to test possible\n combinations of --autostash and rebase.autostash by introducing\n two functions.\n\n Also introduce a loop to tackle the repetitive code used to\n test the usage of --[no-]autostash without --rebase.\n\n * [Patch 5/5] introduces two new tests to check the cases when\n \"git pull --[no-]autostash\" is called with pull.rebase=true.\n\n\nMehul Jain (5):\n t/t5520: change rebase.autoStash to rebase.autostash\n t/t5520: explicitly unset rebase.autostash\n t/t5520: use test_i18ngrep instead of test_cmp\n t/t5520: modify tests to reduce common code\n t/t5520: test --[no-]autostash with pull.rebase=true\n\n t/t5520-pull.sh | 120 ++++++++++++++++++++++++++++----------------------------\n 1 file changed, 61 insertions(+), 59 deletions(-)\n\n-- \n2.7.1.340.g69eb491.dirty\n"},{"id":"282058","messageId":"1459258200-32444-2-git-send-email-mehul.jain2029@gmail.com","threadId":"41862","inReplyTo":"1459258200-32444-1-git-send-email-mehul.jain2029@gmail.com","subject":"[PATCH 1/5] t/t5520: change rebase.autoStash to rebase.autostash","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-29T13:29:56Z","receivedAt":"2016-03-29T13:29:56Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"Signed-off-by: Mehul Jain \n---\n t/t5520-pull.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex 745e59e..5be39df 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -267,7 +267,7 @@ test_expect_success 'pull --rebase --autostash & rebase.autostash=true' '\n \ttest \"$(cat file)\" = \"modified again\"\n '\n \n-test_expect_success 'pull --rebase --autostash & rebase.autoStash=false' '\n+test_expect_success 'pull --rebase --autostash & rebase.autostash=false' '\n \ttest_config rebase.autostash false &&\n \tgit reset --hard before-rebase &&\n \techo dirty >new_file &&\n@@ -278,7 +278,7 @@ test_expect_success 'pull --rebase --autostash & rebase.autoStash=false' '\n \ttest \"$(cat file)\" = \"modified again\"\n '\n \n-test_expect_success 'pull --rebase: --autostash & rebase.autoStash unset' '\n+test_expect_success 'pull --rebase: --autostash & rebase.autostash unset' '\n \tgit reset --hard before-rebase &&\n \techo dirty >new_file &&\n \tgit add new_file &&\n-- \n2.7.1.340.g69eb491.dirty\n"},{"id":"282059","messageId":"1459258200-32444-3-git-send-email-mehul.jain2029@gmail.com","threadId":"41862","inReplyTo":"1459258200-32444-1-git-send-email-mehul.jain2029@gmail.com","subject":"[PATCH 2/5] t/t5520: explicitly unset rebase.autostash","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-29T13:29:57Z","receivedAt":"2016-03-29T13:29:57Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"Tests title suggest that tests are done with rebase.autostash unset,\nbut doesn not take any action to make sure that it is indeed unset.\n\nMake sure that rebase.autostash is unset by explicitly setting it.\n\nSigned-off-by: Mehul Jain \n---\n t/t5520-pull.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex 5be39df..9ee2218 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -279,6 +279,7 @@ test_expect_success 'pull --rebase --autostash & rebase.autostash=false' '\n '\n \n test_expect_success 'pull --rebase: --autostash & rebase.autostash unset' '\n+\ttest_unconfig rebase.autostash &&\n \tgit reset --hard before-rebase &&\n \techo dirty >new_file &&\n \tgit add new_file &&\n@@ -307,6 +308,7 @@ test_expect_success 'pull --rebase --no-autostash & rebase.autostash=false' '\n '\n \n test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '\n+\ttest_unconfig rebase.autostash &&\n \tgit reset --hard before-rebase &&\n \techo dirty >new_file &&\n \tgit add new_file &&\n-- \n2.7.1.340.g69eb491.dirty\n"},{"id":"282061","messageId":"1459258200-32444-4-git-send-email-mehul.jain2029@gmail.com","threadId":"41862","inReplyTo":"1459258200-32444-1-git-send-email-mehul.jain2029@gmail.com","subject":"[PATCH 3/5] t/t5520: use test_i18ngrep instead of test_cmp","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-29T13:29:58Z","receivedAt":"2016-03-29T13:29:58Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"test_cmp is used for error checking when test_i18ngrep could be used.\n\nUse test_i18ngrep to check for the valid error.\n\nSigned-off-by: Mehul Jain \n---\n t/t5520-pull.sh | 10 ++++------\n 1 file changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex 9ee2218..d03cb84 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -317,15 +317,13 @@ test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '\n '\n \n test_expect_success 'pull --autostash (without --rebase) should error out' '\n-\ttest_must_fail git pull --autostash . copy 2>actual &&\n-\techo \"fatal: --[no-]autostash option is only valid with --rebase.\" >expect &&\n-\ttest_i18ncmp actual expect\n+\ttest_must_fail git pull --autostash . copy 2>err &&\n+\ttest_i18ngrep \"only valid with --rebase\" err\n '\n \n test_expect_success 'pull --no-autostash (without --rebase) should error out' '\n-\ttest_must_fail git pull --no-autostash . copy 2>actual &&\n-\techo \"fatal: --[no-]autostash option is only valid with --rebase.\" >expect &&\n-\ttest_i18ncmp actual expect\n+\ttest_must_fail git pull --no-autostash . copy 2>err &&\n+\ttest_i18ngrep \"only valid with --rebase\" err\n '\n \n test_expect_success 'pull.rebase' '\n-- \n2.7.1.340.g69eb491.dirty\n"},{"id":"282062","messageId":"1459258200-32444-5-git-send-email-mehul.jain2029@gmail.com","threadId":"41862","inReplyTo":"1459258200-32444-1-git-send-email-mehul.jain2029@gmail.com","subject":"[PATCH 4/5] t/t5520: modify tests to reduce common code","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-29T13:29:59Z","receivedAt":"2016-03-29T13:29:59Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"There exist three groups of tests which have repetitive lines of code.\n\nIntroduce two functions test_rebase_autostash() and\ntest_rebase_no_autostash() to reduce the number of lines. Also introduce\nloops to futher reduce the current implementation.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Mehul Jain \n---\n t/t5520-pull.sh | 100 +++++++++++++++++++++++---------------------------------\n 1 file changed, 41 insertions(+), 59 deletions(-)\n\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex d03cb84..2611170 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -9,6 +9,24 @@ modify () {\n \tmv \"$2.x\" \"$2\"\n }\n \n+test_rebase_autostash () {\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\tgit pull --rebase --autostash . copy &&\n+\ttest_cmp_rev HEAD^ copy &&\n+\ttest \"$(cat new_file)\" = dirty &&\n+\ttest \"$(cat file)\" = \"modified again\"\n+}\n+\n+test_rebase_no_autostash () {\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\ttest_must_fail git pull --rebase --no-autostash . copy 2>err &&\n+\ttest_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n+}\n+\n test_expect_success setup '\n \techo file >file &&\n \tgit add file &&\n@@ -256,75 +274,39 @@ test_expect_success 'pull --rebase succeeds with dirty working directory and reb\n \ttest \"$(cat file)\" = \"modified again\"\n '\n \n-test_expect_success 'pull --rebase --autostash & rebase.autostash=true' '\n-\ttest_config rebase.autostash true &&\n-\tgit reset --hard before-rebase &&\n-\techo dirty >new_file &&\n-\tgit add new_file &&\n-\tgit pull --rebase --autostash . copy &&\n-\ttest_cmp_rev HEAD^ copy &&\n-\ttest \"$(cat new_file)\" = dirty &&\n-\ttest \"$(cat file)\" = \"modified again\"\n-'\n-\n-test_expect_success 'pull --rebase --autostash & rebase.autostash=false' '\n-\ttest_config rebase.autostash false &&\n-\tgit reset --hard before-rebase &&\n-\techo dirty >new_file &&\n-\tgit add new_file &&\n-\tgit pull --rebase --autostash . copy &&\n-\ttest_cmp_rev HEAD^ copy &&\n-\ttest \"$(cat new_file)\" = dirty &&\n-\ttest \"$(cat file)\" = \"modified again\"\n-'\n+for i in true false\n+\tdo\n+\t\ttest_expect_success \"pull --rebase --autostash & rebase.autostash=$i\" '\n+\t\t\ttest_config rebase.autostash $i &&\n+\t\t\ttest_rebase_autostash\n+\t\t'\n+\tdone\n \n test_expect_success 'pull --rebase: --autostash & rebase.autostash unset' '\n \ttest_unconfig rebase.autostash &&\n-\tgit reset --hard before-rebase &&\n-\techo dirty >new_file &&\n-\tgit add new_file &&\n-\tgit pull --rebase --autostash . copy &&\n-\ttest_cmp_rev HEAD^ copy &&\n-\ttest \"$(cat new_file)\" = dirty &&\n-\ttest \"$(cat file)\" = \"modified again\"\n+\ttest_rebase_autostash\n '\n \n-test_expect_success 'pull --rebase --no-autostash & rebase.autostash=true' '\n-\ttest_config rebase.autostash true &&\n-\tgit reset --hard before-rebase &&\n-\techo dirty >new_file &&\n-\tgit add new_file &&\n-\ttest_must_fail git pull --rebase --no-autostash . copy 2>err &&\n-\ttest_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n-'\n-\n-test_expect_success 'pull --rebase --no-autostash & rebase.autostash=false' '\n-\ttest_config rebase.autostash false &&\n-\tgit reset --hard before-rebase &&\n-\techo dirty >new_file &&\n-\tgit add new_file &&\n-\ttest_must_fail git pull --rebase --no-autostash . copy 2>err &&\n-\ttest_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n-'\n+for i in true false\n+\tdo\n+\t\ttest_expect_success \"pull --rebase --no-autostash & rebase.autostash=$i\" '\n+\t\t\ttest_config rebase.autostash $i &&\n+\t\t\ttest_rebase_no_autostash\n+\t\t'\n+\tdone\n \n test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '\n \ttest_unconfig rebase.autostash &&\n-\tgit reset --hard before-rebase &&\n-\techo dirty >new_file &&\n-\tgit add new_file &&\n-\ttest_must_fail git pull --rebase --no-autostash . copy 2>err &&\n-\ttest_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n+\ttest_rebase_no_autostash\n '\n \n-test_expect_success 'pull --autostash (without --rebase) should error out' '\n-\ttest_must_fail git pull --autostash . copy 2>err &&\n-\ttest_i18ngrep \"only valid with --rebase\" err\n-'\n-\n-test_expect_success 'pull --no-autostash (without --rebase) should error out' '\n-\ttest_must_fail git pull --no-autostash . copy 2>err &&\n-\ttest_i18ngrep \"only valid with --rebase\" err\n-'\n+for i in --autostash --no-autostash\n+\tdo\n+\t\ttest_expect_success \"pull $i (without --rebase) is illegal\" '\n+\t\t\ttest_must_fail git pull $i . copy 2>actual &&\n+\t\t\ttest_i18ngrep \"only valid with --rebase\" actual\n+\t\t'\n+\tdone\n \n test_expect_success 'pull.rebase' '\n \tgit reset --hard before-rebase &&\n-- \n2.7.1.340.g69eb491.dirty\n"},{"id":"282063","messageId":"1459258200-32444-6-git-send-email-mehul.jain2029@gmail.com","threadId":"41862","inReplyTo":"1459258200-32444-1-git-send-email-mehul.jain2029@gmail.com","subject":"[PATCH 5/5] t/t5520: test --[no-]autostash with pull.rebase=true","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-29T13:30:00Z","receivedAt":"2016-03-29T13:30:00Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"\"--[no-]autostash\" option for git-pull is only valid in rebase mode.\nThat is, either --rebase is used or pull.rebase=true. Existing tests\nalready check the cases when --rebase is used but fails to check for\npull.rebase=true case.\n\nAdd two new tests to check that --[no-]autostash option works with\npull.rebase=true.\n\nSigned-off-by: Mehul Jain \n---\n t/t5520-pull.sh | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex 2611170..4da9e52 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -316,6 +316,26 @@ test_expect_success 'pull.rebase' '\n \ttest new = \"$(git show HEAD:file2)\"\n '\n \n+test_expect_success 'pull --autostash & pull.rebase=true' '\n+\ttest_config pull.rebase true &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\tgit pull --autostash . copy &&\n+\ttest_cmp_rev HEAD^ copy &&\n+\ttest \"$(cat new_file)\" = dirty &&\n+\ttest \"$(cat file)\" = \"modified again\"\n+'\n+\n+test_expect_success 'pull --no-autostash & pull.rebase=true' '\n+\ttest_config pull.rebase true &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\ttest_must_fail git pull --no-autostash . copy 2>err &&\n+\ttest_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n+'\n+\n test_expect_success 'branch.to-rebase.rebase' '\n \tgit reset --hard before-rebase &&\n \ttest_config branch.to-rebase.rebase true &&\n-- \n2.7.1.340.g69eb491.dirty\n"},{"id":"282103","messageId":"CAPig+cRv98OSAt1RVUV9CuqQwJ75U+FF8+t7wxQ-ih=V5yi+jw@mail.gmail.com","threadId":"41862","inReplyTo":"1459258200-32444-2-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH 1/5] t/t5520: change rebase.autoStash to rebase.autostash","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-29T20:06:09Z","receivedAt":"2016-03-29T20:06:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 29, 2016 at 9:29 AM, Mehul Jain wrote:\n> t/t5520: change rebase.autoStash to rebase.autostash\n\nThis subject is written at too low a level, talking about details of\nthe patch rather than giving a high-level overview. A further\nshortcoming is that there's no explanation of *why* this change is\ndesirable. Here's an attempt which addresses both problems.\n\n t5520: use consistent capitalization in test titles\n\n(Note that I dropped the leading \"t/\" since it's implied.)\n\nThe patch itself is fine.\n\n> Signed-off-by: Mehul Jain \n> ---\n> t/t5520-pull.sh | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\n> index 745e59e..5be39df 100755\n> --- a/t/t5520-pull.sh\n> +++ b/t/t5520-pull.sh\n> @@ -267,7 +267,7 @@ test_expect_success 'pull --rebase --autostash & rebase.autostash=true' '\n> test \"$(cat file)\" = \"modified again\"\n> '\n>\n> -test_expect_success 'pull --rebase --autostash & rebase.autoStash=false' '\n> +test_expect_success 'pull --rebase --autostash & rebase.autostash=false' '\n> test_config rebase.autostash false &&\n> git reset --hard before-rebase &&\n> echo dirty >new_file &&\n> @@ -278,7 +278,7 @@ test_expect_success 'pull --rebase --autostash & rebase.autoStash=false' '\n> test \"$(cat file)\" = \"modified again\"\n> '\n>\n> -test_expect_success 'pull --rebase: --autostash & rebase.autoStash unset' '\n> +test_expect_success 'pull --rebase: --autostash & rebase.autostash unset' '\n> git reset --hard before-rebase &&\n> echo dirty >new_file &&\n> git add new_file &&\n> --\n> 2.7.1.340.g69eb491.dirty\n"},{"id":"282109","messageId":"xmqqshz9vy8e.fsf@gitster.mtv.corp.google.com","threadId":"41862","inReplyTo":"1459258200-32444-5-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH 4/5] t/t5520: modify tests to reduce common code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-29T20:13:37Z","receivedAt":"2016-03-29T20:13:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mehul Jain writes:\n\n> There exist three groups of tests which have repetitive lines of code.\n>\n> Introduce two functions test_rebase_autostash() and\n> test_rebase_no_autostash() to reduce the number of lines. Also introduce\n> loops to futher reduce the current implementation.\n\nSound like sensible idea.\n\n> +for i in true false\n> +\tdo\n> +\t\ttest_expect_success \"pull --rebase --autostash & rebase.autostash=$i\" '\n> +\t\t\ttest_config rebase.autostash $i &&\n> +\t\t\ttest_rebase_autostash\n> +\t\t'\n> +\tdone\n\nThe lines between do..done is over-indented (will locally fix--no\nneed to resend).\n"},{"id":"282108","messageId":"CAPig+cROGO0kSgTL7OpLGYN+cA7RKWHz0ES=h+FNDREcp65GJA@mail.gmail.com","threadId":"41862","inReplyTo":"1459258200-32444-3-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH 2/5] t/t5520: explicitly unset rebase.autostash","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-29T20:16:31Z","receivedAt":"2016-03-29T20:16:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 29, 2016 at 9:29 AM, Mehul Jain wrote:\n> t/t5520: explicitly unset rebase.autostash\n\nAs with patch 1/5, this subject is written at too low a level, talking\nabout details of the patch rather than giving a high-level overview.\nWhat the patch is really doing is ensuring consistent conditions\nwithin the test even if some future change pollutes the global\nconfiguration. Maybe:\n\n t5520: ensure consistent test conditions\n\nor:\n\n t5520: make test expectations explicit\n\nor something.\n\n> Tests title suggest that tests are done with rebase.autostash unset,\n> but doesn not take any action to make sure that it is indeed unset.\n\nThis is just paraphrasing my earlier review comment[1], however,\n\"suggest\" is a weak argument for why this change is desirable. State\ninstead that this change ensures a consistent condition for tests in\nwhich rebase.autostash should not be set and protects against some\nfuture change polluting the global configuration.\n\n> Make sure that rebase.autostash is unset by explicitly setting it.\n\nThe patch itself looks ok.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/289860\n\n> Signed-off-by: Mehul Jain \n> ---\n> t/t5520-pull.sh | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\n> index 5be39df..9ee2218 100755\n> --- a/t/t5520-pull.sh\n> +++ b/t/t5520-pull.sh\n> @@ -279,6 +279,7 @@ test_expect_success 'pull --rebase --autostash & rebase.autostash=false' '\n> '\n>\n> test_expect_success 'pull --rebase: --autostash & rebase.autostash unset' '\n> + test_unconfig rebase.autostash &&\n> git reset --hard before-rebase &&\n> echo dirty >new_file &&\n> git add new_file &&\n> @@ -307,6 +308,7 @@ test_expect_success 'pull --rebase --no-autostash & rebase.autostash=false' '\n> '\n>\n> test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '\n> + test_unconfig rebase.autostash &&\n> git reset --hard before-rebase &&\n> echo dirty >new_file &&\n> git add new_file &&\n> --\n> 2.7.1.340.g69eb491.dirty\n"},{"id":"282112","messageId":"CAPig+cSLD2hKpckKU_tn=AhK8ZDgW13D2YMAf2p9Q-CpwOAM4g@mail.gmail.com","threadId":"41862","inReplyTo":"1459258200-32444-4-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH 3/5] t/t5520: use test_i18ngrep instead of test_cmp","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-29T20:27:39Z","receivedAt":"2016-03-29T20:27:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 29, 2016 at 9:29 AM, Mehul Jain wrote:\n> t/t5520: use test_i18ngrep instead of test_cmp\n\nAs mentioned for earlier patches, this is too low-level, whereas it\nshould be giving a high-level overview.\n\n> test_cmp is used for error checking when test_i18ngrep could be used.\n>\n> Use test_i18ngrep to check for the valid error.\n\n\"could be used\" is not sufficient justification to explain why this\nchange is desirable. See [1] for a good explanation of why this change\nshould be made.\n\nThe patch itself looks fine.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/289077\n\n> Signed-off-by: Mehul Jain \n> ---\n> t/t5520-pull.sh | 10 ++++------\n> 1 file changed, 4 insertions(+), 6 deletions(-)\n>\n> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\n> index 9ee2218..d03cb84 100755\n> --- a/t/t5520-pull.sh\n> +++ b/t/t5520-pull.sh\n> @@ -317,15 +317,13 @@ test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '\n> '\n>\n> test_expect_success 'pull --autostash (without --rebase) should error out' '\n> - test_must_fail git pull --autostash . copy 2>actual &&\n> - echo \"fatal: --[no-]autostash option is only valid with --rebase.\" >expect &&\n> - test_i18ncmp actual expect\n> + test_must_fail git pull --autostash . copy 2>err &&\n> + test_i18ngrep \"only valid with --rebase\" err\n> '\n>\n> test_expect_success 'pull --no-autostash (without --rebase) should error out' '\n> - test_must_fail git pull --no-autostash . copy 2>actual &&\n> - echo \"fatal: --[no-]autostash option is only valid with --rebase.\" >expect &&\n> - test_i18ncmp actual expect\n> + test_must_fail git pull --no-autostash . copy 2>err &&\n> + test_i18ngrep \"only valid with --rebase\" err\n> '\n>\n> test_expect_success 'pull.rebase' '\n> --\n> 2.7.1.340.g69eb491.dirty\n"},{"id":"282118","messageId":"CAPig+cQ3gaAdKU0M3v4q5AzvQSTciwHYv7fAAdCGTKYoOkJTow@mail.gmail.com","threadId":"41862","inReplyTo":"1459258200-32444-5-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH 4/5] t/t5520: modify tests to reduce common code","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-29T21:01:34Z","receivedAt":"2016-03-29T21:01:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 29, 2016 at 9:29 AM, Mehul Jain wrote:\n> t/t5520: modify tests to reduce common code\n\nAs this is indeed a patch, \"modify\" is implied. Perhaps:\n\n t5520: factor out common code\n\n> There exist three groups of tests which have repetitive lines of code.\n>\n> Introduce two functions test_rebase_autostash() and\n> test_rebase_no_autostash() to reduce the number of lines. Also introduce\n> loops to futher reduce the current implementation.\n\nThis patch is doing so much that it's difficult to review for\ncorrectness. Taking [1] into consideration, better would be to split\nit into at least three patches:\n\n1. Factor out code into test_rebase_autostash() and modify the four\ntests to call it.\n\n2. Factor out code into test_rebase_autostash_fail() and modify the\nthree tests to call it.\n\n3. Fold the two \"pull $i (without --rebase) is illegal\" tests into a for-loop.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/289434/focus=289860\n\n> Helped-by: Eric Sunshine \n> Signed-off-by: Mehul Jain \n> ---\n> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\n> @@ -9,6 +9,24 @@ modify () {\n> +test_rebase_no_autostash () {\n> + git reset --hard before-rebase &&\n> + echo dirty >new_file &&\n> + git add new_file &&\n> + test_must_fail git pull --rebase --no-autostash . copy 2>err &&\n> + test_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n\nIn the spirit of patch 3/5 and [1], you could grep for a substring\nrather than the full message, but that's a minor point, not worth a\nre-roll.\n\n test_i18ngrep \"uncommitted changes\" err\n\n> +}\n> @@ -256,75 +274,39 @@ test_expect_success 'pull --rebase succeeds with dirty working directory and reb\n> +for i in true false\n> + do\n> + test_expect_success \"pull --rebase --autostash & rebase.autostash=$i\" '\n> + test_config rebase.autostash $i &&\n> + test_rebase_autostash\n> + '\n> + done\n\nI don't care too strongly, but I'm not convinced that this for-loop is\nbuying you much for these two cases since each test already has been\nreduced to two simple lines, and the added abstraction of the for-loop\nincreases cognitive load a bit.\n\n> +for i in --autostash --no-autostash\n> + do\n> + test_expect_success \"pull $i (without --rebase) is illegal\" '\n> + test_must_fail git pull $i . copy 2>actual &&\n> + test_i18ngrep \"only valid with --rebase\" actual\n> + '\n> + done\n\nYou might then ask why I suggested[1] the for-loop in this case but\nnot for the true/false case. Even though these are also two-line\ntests, they are not quite as simple as two lines down to which the\ntrue/false tests devolve. Anyhow, this alone is not worth a re-roll.\n"},{"id":"282119","messageId":"CAPig+cQ93+dCqJMRcQYSRHLDuYtwkeK_aSrfv2=2=g7ZhO85TQ@mail.gmail.com","threadId":"41862","inReplyTo":"1459258200-32444-6-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH 5/5] t/t5520: test --[no-]autostash with pull.rebase=true","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-29T21:16:06Z","receivedAt":"2016-03-29T21:16:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 29, 2016 at 9:30 AM, Mehul Jain wrote:\n> \"--[no-]autostash\" option for git-pull is only valid in rebase mode.\n> That is, either --rebase is used or pull.rebase=true. Existing tests\n> already check the cases when --rebase is used but fails to check for\n> pull.rebase=true case.\n>\n> Add two new tests to check that --[no-]autostash option works with\n> pull.rebase=true.\n>\n> Signed-off-by: Mehul Jain \n> ---\n> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\n> @@ -316,6 +316,26 @@ test_expect_success 'pull.rebase' '\n> +test_expect_success 'pull --autostash & pull.rebase=true' '\n> + test_config pull.rebase true &&\n> + git reset --hard before-rebase &&\n> + echo dirty >new_file &&\n> + git add new_file &&\n> + git pull --autostash . copy &&\n> + test_cmp_rev HEAD^ copy &&\n> + test \"$(cat new_file)\" = dirty &&\n> + test \"$(cat file)\" = \"modified again\"\n> +'\n\nWith the exception of the missing --rebase argument, this is exactly\nthe same code as in test_rebase_autostash(), right? Rather than\nrepeating this code yet again, it might be nice to augment that\nfunction to accept a (possibly) optional argument controlling whether\n--rebase is used.\n\n> +\n> +test_expect_success 'pull --no-autostash & pull.rebase=true' '\n> + test_config pull.rebase true &&\n> + git reset --hard before-rebase &&\n> + echo dirty >new_file &&\n> + git add new_file &&\n> + test_must_fail git pull --no-autostash . copy 2>err &&\n> + test_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n> +'\n\nDitto with regard to test_rebase_no_autostash() (or\ntest_rebase_autostash_fail() as I suggested in my patch 4/5 review).\n"},{"id":"282259","messageId":"CA+DCAeQPr2vxvm6MKiOLpDtmpC2d=RcvYhuFeimSn+xX2TAvtQ@mail.gmail.com","threadId":"41862","inReplyTo":"CAPig+cQ93+dCqJMRcQYSRHLDuYtwkeK_aSrfv2=2=g7ZhO85TQ@mail.gmail.com","subject":"Re: [PATCH 5/5] t/t5520: test --[no-]autostash with pull.rebase=true","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-30T19:00:14Z","receivedAt":"2016-03-30T19:00:14Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"Hi Eric,\n\nThanks for the reviews on this series.\n\nOn Wed, Mar 30, 2016 at 2:46 AM, Eric Sunshine wrote:\n> With the exception of the missing --rebase argument, this is exactly\n> the same code as in test_rebase_autostash(), right? Rather than\n> repeating this code yet again, it might be nice to augment that\n> function to accept a (possibly) optional argument controlling whether\n> --rebase is used.\n\nThanks for the idea. I have come up with something like this:\n\n * Introduce two function test_pull() and test_pull_fail() in\nthe place of\n test_rebase_autostash() and test_rebase_no_autostash.()\n\n Using these functions we can easily re-write all the 6 tests which\n deals with combination of autostash and rebase.autostash. Plus\n these functions helped in writing two new tests which deals with\n combination of pull.rebase and autostash. Thus reducing the code\n base to simpler and fewer lines of code. Also I could re-write one\n of the old test to reduce the repetition with them.\n\nHere are the functions and there implementations:\n\n---\n\ntest_pull () {\n git reset --hard before-rebase &&\n echo dirty >new_file &&\n git add new_file &&\n git pull $@ . copy &&\n test_cmp_rev HEAD^ copy &&\n test \"$(cat new_file)\" = dirty &&\n test \"$(cat file)\" = \"modified again\"\n}\n\ntest_pull_fail () {\n git reset --hard before-rebase &&\n echo dirty >new_file &&\n git add new_file &&\n test_must_fail git pull $@ . copy 2>err &&\n test_i18ngrep \"uncommitted changes.\" err\n}\n\ntest_expect_success 'pull --rebase succeeds with dirty working\ndirectory and rebase.autostash set' '\n test_config rebase.autostash true &&\n test_pull --rebase\n'\n\ntest_expect_success \"pull --rebase --autostash & rebase.autostash=true\" '\n test_config rebase.autostash true &&\n test_pull --rebase --autostash\n'\n\ntest_expect_success \"pull --rebase --autostash & rebase.autostash=false\" '\n test_config rebase.autostash false &&\n test_pull --rebase --autostash\n'\n\ntest_expect_success 'pull --rebase: --autostash & rebase.autostash unset' '\n test_unconfig rebase.autostash &&\n test_pull --rebase --autostash\n'\n\ntest_expect_success \"pull --rebase --no-autostash & rebase.autostash=true\" '\n test_config rebase.autostash true &&\n test_pull_fail --rebase --no-autostash\n'\n\ntest_expect_success \"pull --rebase --no-autostash & rebase.autostash=false\" '\n test_config rebase.autostash false &&\n test_pull_fail --rebase --no-autostash\n'\n\ntest_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '\n test_unconfig rebase.autostash &&\n test_pull_fail --rebase --no-autostash\n'\n\ntest_expect_success 'pull --autostash & pull.rebase=true' '\n test_config pull.rebase true &&\n test_pull --autostash\n'\n\ntest_expect_success 'pull --no-autostash & pull.rebase=true' '\n test_config pull.rebase true &&\n test_pull_fail --no-autostash\n'\n---\n\nI'm sorry if this is bit difficult to digest without diff output. I\njust wanted to\nknow if the above mention functions looks suitable to you.\n\nAlso I've read your comments on other patches of this series, I will make\nchanges accordingly ones above mention functions, tests looks fit for a\nre-roll.\n\nThanks,\nMehul\n"},{"id":"282267","messageId":"CAPig+cQyHu1J=FYOtgsmi3ghuN7YyjNgAz-VgO06isfrS+kUSg@mail.gmail.com","threadId":"41862","inReplyTo":"CA+DCAeQPr2vxvm6MKiOLpDtmpC2d=RcvYhuFeimSn+xX2TAvtQ@mail.gmail.com","subject":"Re: [PATCH 5/5] t/t5520: test --[no-]autostash with pull.rebase=true","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-30T20:31:07Z","receivedAt":"2016-03-30T20:31:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 30, 2016 at 3:00 PM, Mehul Jain wrote:\n> On Wed, Mar 30, 2016 at 2:46 AM, Eric Sunshine wrote:\n>> With the exception of the missing --rebase argument, this is exactly\n>> the same code as in test_rebase_autostash(), right? Rather than\n>> repeating this code yet again, it might be nice to augment that\n>> function to accept a (possibly) optional argument controlling whether\n>> --rebase is used.\n>\n> Thanks for the idea. I have come up with something like this:\n>\n> test_pull () {\n> git reset --hard before-rebase &&\n> echo dirty >new_file &&\n> git add new_file &&\n> git pull $@ . copy &&\n> test_cmp_rev HEAD^ copy &&\n> test \"$(cat new_file)\" = dirty &&\n> test \"$(cat file)\" = \"modified again\"\n> }\n>\n> test_pull_fail () {\n> git reset --hard before-rebase &&\n> echo dirty >new_file &&\n> git add new_file &&\n> test_must_fail git pull $@ . copy 2>err &&\n> test_i18ngrep \"uncommitted changes.\" err\n> }\n\nConsidering that these are specifically testing behavior related to\nautostashing, it might make sense to have \"autostash\" in the function\nnames, but that's a very minor point.\n\n> test_expect_success 'pull --rebase succeeds with dirty working\n> directory and rebase.autostash set' '\n> test_config rebase.autostash true &&\n> test_pull --rebase\n> '\n> [...]\n> test_expect_success 'pull --no-autostash & pull.rebase=true' '\n> test_config pull.rebase true &&\n> test_pull_fail --no-autostash\n> '\n>\n> I'm sorry if this is bit difficult to digest without diff output. I\n> just wanted to\n> know if the above mention functions looks suitable to you.\n\nThis is exactly what I had in mind for simplifying the tests, and it's\nperfectly easy to read in this form (a diff would be worse for this\nillustration).\n\nOne other possibility would be to make this all table-driven by\ncollecting all of the above state information into a table and then\nfeeding that into a function (either as its argument list or via\nstdin). For instance:\n\n test_autostash <<\\-EOF\n ok,--rebase,rebase.autostash=true\n ok,--rebase --autostash,rebase.autostash=true\n ok,--rebase --autostash,rebase.autostash=false\n ok,--rebase --autostash,rebase.autostash=\n err,--rebase --no-autostash,rebase.autostash=true\n err,--rebase --no-autostash,rebase.autostash=false\n err,--rebase --no-autostash,rebase.autostash=\n ok,--autostash,pull.rebase=true\n err,--no-autostash,pull.rebase=true\n EOF\n\nThe function would loop over the input, split each line apart by\nsetting IFS=, and then run the test based upon the state information.\n\"ok\" means autostash is expected to succeed, and err means it is\nexpected to fail. The function would want to specially recognize the\n\"foo.bar=\" in the last argument in order to invoke test_unconfig()\nrather than test_config().\n\nHowever, this may be a case of diminishing returns. The tests as you\nillustrated them are sufficiently simple and easy to grok that the\ntable-driven approach may not add much value (aside from making it\neasier to see at a glance if any cases were omitted).\n"},{"id":"282439","messageId":"CA+DCAeT1DQvHnRpJeApcm2vO6KhXaMaRXZg9HCUmiiBv=hfxzw@mail.gmail.com","threadId":"41862","inReplyTo":"CAPig+cQyHu1J=FYOtgsmi3ghuN7YyjNgAz-VgO06isfrS+kUSg@mail.gmail.com","subject":"Re: [PATCH 5/5] t/t5520: test --[no-]autostash with pull.rebase=true","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-04-01T10:27:27Z","receivedAt":"2016-04-01T10:27:27Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"Hi Eric,\n\nOn Thu, Mar 31, 2016 at 2:01 AM, Eric Sunshine wrote:\n> One other possibility would be to make this all table-driven by\n> collecting all of the above state information into a table and then\n> feeding that into a function (either as its argument list or via\n> stdin). For instance:\n>\n> test_autostash <<\\-EOF\n> ok,--rebase,rebase.autostash=true\n> ok,--rebase --autostash,rebase.autostash=true\n> ok,--rebase --autostash,rebase.autostash=false\n> ok,--rebase --autostash,rebase.autostash=\n> err,--rebase --no-autostash,rebase.autostash=true\n> err,--rebase --no-autostash,rebase.autostash=false\n> err,--rebase --no-autostash,rebase.autostash=\n> ok,--autostash,pull.rebase=true\n> err,--no-autostash,pull.rebase=true\n> EOF\n>\n> The function would loop over the input, split each line apart by\n> setting IFS=, and then run the test based upon the state information.\n> \"ok\" means autostash is expected to succeed, and err means it is\n> expected to fail. The function would want to specially recognize the\n> \"foo.bar=\" in the last argument in order to invoke test_unconfig()\n> rather than test_config().\n\nI tried out this method also. Below is the script that I wrote for this:\n\n---\n\ntest_autostash () {\n OLDIFS=$IFS\n IFS=', ='\n while read -r expect cmd config_variable value\n do\n test_expect_success \"$cmd, $config_variable=$value\" '\n if [ \"$value\" = \"\" ]; then\n test_unconfig $config_variable\n else\n test_config $config_variable $value\n fi &&\n\n git reset --hard before-rebase &&\n echo dirty >new_file &&\n git add new_file &&\n\n if [ $expect = \"ok\" ]; then\n git pull '$cmd' . copy &&\n echo test_cmp_rev HEAD^ copy &&\n test \"$(cat new_file)\" = dirty &&\n test \"$(cat file)\" = \"modified again\"\n else\n test_must_fail git pull '$cmd' . copy 2>err &&\n test_i18ngrep \"uncommitted changes.\" err\n fi\n '\n done\n IFS=$OLDIFS\n}\n\n\ntest_autostash <<-\\EOF\n ok,--rebase,rebase.autostash=true\n ok,--rebase --autostash,rebase.autostash=true\n ok,--rebase --autostash,rebase.autostash=false\n ok,--rebase --autostash,rebase.autostash=\n err,--rebase --no-autostash,rebase.autostash=true\n err,--rebase --no-autostash,rebase.autostash=false\n err,--rebase --no-autostash,rebase.autostash=\n ok,--autostash,pull.rebase=true\n err,--no-autostash,pull.rebase=true\n EOF\n\n\n---\n\nThings worked out perfectly.\n\nUnfortunately there was a strange behaviour that I noticed\nand frankly I don't understand why it happened.\n\nIn test_autostash() there's a line\n\n echo test_cmp_rev HEAD^ copy &&\n\nOriginally it should have been\n\n test_cmp_rev HEAD^ copy &&\n\nbut this raise following error while testing\n\n ./t5520-pull.sh: 684: eval: diff -u: not found\n\nI'm not able to understand why putting an \"echo\" before\ntest_cmp didn't raise the above error. This looks quite\nstrange. Any thoughts?\n\nThough the above code works perfectly and can be used in\nplace of previous tests. Only problem remains is tests titles.\nCurrently with this script, test titles will be:\n\nok 21 - --rebase, rebase.autostash=true\nok 22 - --rebase --autostash, rebase.autostash=true\nok 23 - --rebase --autostash, rebase.autostash=false\nok 24 - --rebase --autostash, rebase.autostash=\nok 25 - --rebase --no-autostash, rebase.autostash=true\nok 26 - --rebase --no-autostash, rebase.autostash=false\nok 27 - --rebase --no-autostash, rebase.autostash=\nok 28 - --autostash, pull.rebase=true\nok 29 - --no-autostash, pull.rebase=true\n\nAny thoughts/suggestions on them?\n\nThanks,\nMehul\n"},{"id":"282571","messageId":"CAPig+cSR9Um5FUWzkzHGAM5RanaKssAysA5hGOP4+E5oA0Y5oA@mail.gmail.com","threadId":"41862","inReplyTo":"CA+DCAeT1DQvHnRpJeApcm2vO6KhXaMaRXZg9HCUmiiBv=hfxzw@mail.gmail.com","subject":"Re: [PATCH 5/5] t/t5520: test --[no-]autostash with pull.rebase=true","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-03T19:28:04Z","receivedAt":"2016-04-03T19:28:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Apr 1, 2016 at 6:27 AM, Mehul Jain wrote:\n> I tried out this method also. Below is the script that I wrote for this:\n>\n> test_autostash () {\n> OLDIFS=$IFS\n> IFS=', ='\n> while read -r expect cmd config_variable value\n> do\n> test_expect_success \"$cmd, $config_variable=$value\" '\n> if [ \"$value\" = \"\" ]; then\n> test_unconfig $config_variable\n> else\n> test_config $config_variable $value\n> fi &&\n>\n> git reset --hard before-rebase &&\n> echo dirty >new_file &&\n> git add new_file &&\n>\n> if [ $expect = \"ok\" ]; then\n> git pull '$cmd' . copy &&\n> echo test_cmp_rev HEAD^ copy &&\n> test \"$(cat new_file)\" = dirty &&\n> test \"$(cat file)\" = \"modified again\"\n> else\n> test_must_fail git pull '$cmd' . copy 2>err &&\n> test_i18ngrep \"uncommitted changes.\" err\n> fi\n> '\n> done\n> IFS=$OLDIFS\n> }\n>\n> test_autostash <<-\\EOF\n> ok,--rebase,rebase.autostash=true\n> [...]\n> err,--no-autostash,pull.rebase=true\n> EOF\n>\n> Things worked out perfectly.\n>\n> Unfortunately there was a strange behaviour that I noticed\n> and frankly I don't understand why it happened.\n>\n> In test_autostash() there's a line\n>\n> echo test_cmp_rev HEAD^ copy &&\n>\n> Originally it should have been\n>\n> test_cmp_rev HEAD^ copy &&\n>\n> but this raise following error while testing\n>\n> ./t5520-pull.sh: 684: eval: diff -u: not found\n\nThis is caused by the custom IFS=',\\t=' which is still in effect when\ntest_cmp_rev() is invoked. You need to restore IFS within the loop\nitself.\n\n> I'm not able to understand why putting an \"echo\" before\n> test_cmp didn't raise the above error. This looks quite\n> strange. Any thoughts?\n\nWith 'echo', test_cmp_rev() is never even invoked (the command is just\nprinted by 'echo'), and 'echo' succeeds (returns 0), so the test\nsucceeds, but isn't actually doing an revision verification.\n\nThe test also behaves incorrectly on these lines:\n\n git pull '$cmd' . copy &&\n\nand:\n\n test_must_fail git pull '$cmd' . copy 2>err &&\n\nThose single quotes around $cmd are within the second argument to\ntest_expect_success(), which itself is a single quoted string. So,\n'$cmd' is actually ending and re-starting the \"outer\" quoted string.\nThis isn't a problem when $cmd has a single token (such as\n\"--rebase\"), but it causes test_expect_success() to complain about\nincorrect number of arguments when $cmd is composed of multiple tokens\n(such as \"--rebase --autostash\").\n\nMoreover, $cmd shouldn't be quoted at all. When $cmd is \"--rebase\n--autostash\", you want git-pull to see the --rebase and --autostash as\nseparate arguments, but the quoting causes them to be treated as a\nsingle argument, which git-pull doesn't recognize. So, dropping the\nquotes around $cmd is the correct thing to do.\n\n> Though the above code works perfectly and can be used in\n> place of previous tests. Only problem remains is tests titles.\n> Currently with this script, test titles will be:\n>\n> ok 21 - --rebase, rebase.autostash=true\n> ok 22 - --rebase --autostash, rebase.autostash=true\n> ok 23 - --rebase --autostash, rebase.autostash=false\n> ok 24 - --rebase --autostash, rebase.autostash=\n> ok 25 - --rebase --no-autostash, rebase.autostash=true\n> ok 26 - --rebase --no-autostash, rebase.autostash=false\n> ok 27 - --rebase --no-autostash, rebase.autostash=\n> ok 28 - --autostash, pull.rebase=true\n> ok 29 - --no-autostash, pull.rebase=true\n>\n> Any thoughts/suggestions on them?\n\nI don't see a particular problem with the titles. You could prefix\nthem with \"pull \" if that's what you mean.\n"},{"id":"282619","messageId":"CA+DCAeRqY7-qZt-upa5=nY8OkUL4Q76ogk5nrF_WAaiFiWOy1A@mail.gmail.com","threadId":"41862","inReplyTo":"CAPig+cSR9Um5FUWzkzHGAM5RanaKssAysA5hGOP4+E5oA0Y5oA@mail.gmail.com","subject":"Re: [PATCH 5/5] t/t5520: test --[no-]autostash with pull.rebase=true","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-04-04T16:42:59Z","receivedAt":"2016-04-04T16:42:59Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"On Mon, Apr 4, 2016 at 12:58 AM, Eric Sunshine wrote:\n> On Fri, Apr 1, 2016 at 6:27 AM, Mehul Jain wrote:\n>> In test_autostash() there's a line\n>>\n>> echo test_cmp_rev HEAD^ copy &&\n>>\n>> Originally it should have been\n>>\n>> test_cmp_rev HEAD^ copy &&\n>>\n>> but this raise following error while testing\n>>\n>> ./t5520-pull.sh: 684: eval: diff -u: not found\n>\n> This is caused by the custom IFS=',\\t=' which is still in effect when\n> test_cmp_rev() is invoked. You need to restore IFS within the loop\n> itself.\n\nThanks for pointing it out. I made a mistake by not considering\nthe consequences of setting IFS=',\\t='. I tried it out again and\nthis time all tests passed perfectly.\n\nI should been more careful in the first place while playing\nwith IFS, but instead of that, I kept on thinking that there is some\nother problem with the script which lead to me making foolish\nchanges in the script like putting an echo before \"test_cmp_rev ...\".\n\nIt was nice of you to take out some time and point it out :)\n\nAlso now that I have sent v2[1] of this series, which goes\nin different direction as far as implementation of these tests\nare concerned. I think the script now is useless (but I\nlearned a bit about shell while writing it).\n\nThanks,\nMehul\n"},{"id":"282621","messageId":"vpq4mbhmi3g.fsf@anie.imag.fr","threadId":"41862","inReplyTo":"CA+DCAeRqY7-qZt-upa5=nY8OkUL4Q76ogk5nrF_WAaiFiWOy1A@mail.gmail.com","subject":"Re: [PATCH 5/5] t/t5520: test --[no-]autostash with pull.rebase=true","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-04-04T16:52:51Z","receivedAt":"2016-04-04T16:52:51Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Mehul Jain writes:\n\n> On Mon, Apr 4, 2016 at 12:58 AM, Eric Sunshine wrote:\n>> On Fri, Apr 1, 2016 at 6:27 AM, Mehul Jain wrote:\n>>> In test_autostash() there's a line\n>>>\n>>> echo test_cmp_rev HEAD^ copy &&\n>>>\n>>> Originally it should have been\n>>>\n>>> test_cmp_rev HEAD^ copy &&\n>>>\n>>> but this raise following error while testing\n>>>\n>>> ./t5520-pull.sh: 684: eval: diff -u: not found\n>>\n>> This is caused by the custom IFS=',\\t=' which is still in effect when\n>> test_cmp_rev() is invoked. You need to restore IFS within the loop\n>> itself.\n>\n> Thanks for pointing it out. I made a mistake by not considering\n> the consequences of setting IFS=',\\t='. I tried it out again and\n> this time all tests passed perfectly.\n\nI think it would be much simpler to drop the loop, and write instead\nsomething like (untested):\n\ntest_autostash () {\n\texpect=\"$1\"\n cmd=\"$2\"\n config_variable=\"$3\"\n value=\"$4\"\n test_expect_success \"$cmd, $config_variable=$value\" '\n if [ \"$value\" = \"\" ]; then\n test_unconfig $config_variable\n else\n test_config $config_variable $value\n fi &&\n\n git reset --hard before-rebase &&\n echo dirty >new_file &&\n git add new_file &&\n\n if [ $expect = \"ok\" ]; then\n git pull '$cmd' . copy &&\n test_cmp_rev HEAD^ copy &&\n test \"$(cat new_file)\" = dirty &&\n test \"$(cat file)\" = \"modified again\"\n else\n test_must_fail git pull '$cmd' . copy 2>err &&\n test_i18ngrep \"uncommitted changes.\" err\n fi\n '\n}\n\ntest_autostash ok --rebase rebase.autostash=true\ntest_autostash ok '--rebase --autostash' rebase.autostash=true\ntest_autostash ok '--rebase --autostash' rebase.autostash=false\ntest_autostash ok '--rebase --autostash' rebase.autostash=\ntest_autostash err '--rebase --no-autostash' rebase.autostash=true\ntest_autostash err '--rebase --no-autostash' rebase.autostash=false\ntest_autostash err '--rebase --no-autostash' rebase.autostash=\ntest_autostash ok --autostash pull.rebase=true\ntest_autostash err --no-autostash pull.rebase=true\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"282630","messageId":"CA+DCAeTm7wjgdjLwR__pcyev-EsqecdAT8xdGEFfuekg4ToKSA@mail.gmail.com","threadId":"41862","inReplyTo":"vpq4mbhmi3g.fsf@anie.imag.fr","subject":"Re: [PATCH 5/5] t/t5520: test --[no-]autostash with pull.rebase=true","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-04-04T17:36:29Z","receivedAt":"2016-04-04T17:36:29Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"On Mon, Apr 4, 2016 at 10:22 PM, Matthieu Moy\n wrote:\n> I think it would be much simpler to drop the loop, and write instead\n> something like (untested):\n\nI tested it (with few minor changes), and worked fine.\n\ntest_autostash () {\n OLDIFS=$IFS\n IFS='='\n set -- $*\n IFS=$OLDIFS\n expect=$1\n cmd=$2\n config_variable=$3\n value=$4\n test_expect_success \"$cmd, $config_variable=$value\" '\n if [ \"$value\" = \"\" ]; then\n test_unconfig $config_variable\n else\n test_config $config_variable $value\n fi &&\n\n git reset --hard before-rebase &&\n echo dirty >new_file &&\n git add new_file &&\n\n if [ $expect = \"ok\" ]; then\n git pull $cmd . copy &&\n test_cmp_rev HEAD^ copy &&\n test \"$(cat new_file)\" = dirty &&\n test \"$(cat file)\" = \"modified again\"\n else\n test_must_fail git pull $cmd . copy 2>err &&\n test_i18ngrep \"uncommitted changes.\" err\n fi\n '\n}\n\ntest_autostash ok '--rebase' rebase.autostash=true\ntest_autostash ok '--rebase --autostash' rebase.autostash=true\ntest_autostash ok '--rebase --autostash' rebase.autostash=false\ntest_autostash ok '--rebase --autostash' rebase.autostash=\ntest_autostash err '--rebase --no-autostash' rebase.autostash=true\ntest_autostash err '--rebase --no-autostash' rebase.autostash=false\ntest_autostash err '--rebase --no-autostash' rebase.autostash=\ntest_autostash ok '--autostash' pull.rebase=true\ntest_autostash err '--no-autostash' pull.rebase=true\n\nPerhaps this looks better than the one with the loop. Even better than\nthe implementation in v2[1].\n\nI think it would be wise to go with the above script for v3 (as I will\nbe doing a re-roll of the series[1]).\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/290596\n\nThanks,\nMehul\n"},{"id":"282632","messageId":"CAPig+cTSHQcMh=gTLgE3kCgLqBr55ar9wn3gwXLbvRiOyqch1A@mail.gmail.com","threadId":"41862","inReplyTo":"CA+DCAeTm7wjgdjLwR__pcyev-EsqecdAT8xdGEFfuekg4ToKSA@mail.gmail.com","subject":"Re: [PATCH 5/5] t/t5520: test --[no-]autostash with pull.rebase=true","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-04T17:48:10Z","receivedAt":"2016-04-04T17:48:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 4, 2016 at 1:36 PM, Mehul Jain wrote:\n> On Mon, Apr 4, 2016 at 10:22 PM, Matthieu Moy\n> wrote:\n>> I think it would be much simpler to drop the loop, and write instead\n>> something like (untested):\n>\n> I tested it (with few minor changes), and worked fine.\n>\n> test_autostash () {\n> OLDIFS=$IFS\n> IFS='='\n> set -- $*\n> IFS=$OLDIFS\n> expect=$1\n> cmd=$2\n> config_variable=$3\n> value=$4\n> test_expect_success \"$cmd, $config_variable=$value\" '\n> if [ \"$value\" = \"\" ]; then\n> test_unconfig $config_variable\n> else\n> test_config $config_variable $value\n> fi &&\n>\n> git reset --hard before-rebase &&\n> echo dirty >new_file &&\n> git add new_file &&\n>\n> if [ $expect = \"ok\" ]; then\n> git pull $cmd . copy &&\n> test_cmp_rev HEAD^ copy &&\n> test \"$(cat new_file)\" = dirty &&\n> test \"$(cat file)\" = \"modified again\"\n> else\n> test_must_fail git pull $cmd . copy 2>err &&\n> test_i18ngrep \"uncommitted changes.\" err\n> fi\n> '\n> }\n>\n> test_autostash ok '--rebase' rebase.autostash=true\n> test_autostash ok '--rebase --autostash' rebase.autostash=true\n> test_autostash ok '--rebase --autostash' rebase.autostash=false\n> test_autostash ok '--rebase --autostash' rebase.autostash=\n> test_autostash err '--rebase --no-autostash' rebase.autostash=true\n> test_autostash err '--rebase --no-autostash' rebase.autostash=false\n> test_autostash err '--rebase --no-autostash' rebase.autostash=\n> test_autostash ok '--autostash' pull.rebase=true\n> test_autostash err '--no-autostash' pull.rebase=true\n>\n> Perhaps this looks better than the one with the loop. Even better than\n> the implementation in v2[1].\n>\n> I think it would be wise to go with the above script for v3 (as I will\n> be doing a re-roll of the series[1]).\n\nThis new function is sufficiently complex that it increases cognitive\nload enough for me to question if it is really a win for such a small\nnumber of tests. The individual tests, as implemented in the current\nround, are quite easy to understand, and don't place any significant\ncognitive burden on the reader.\n\nAlthough I'm the one who brought up the idea of \"automating\" these\ntests, I'm not convinced that it's an improvement in this case, but I\ndon't feel so strongly that I'd forbid it. So, choose the approach\nwhich seems best to you while weighing comprehension load for people\nnew to these tests, as well as maintainability costs.\n"},{"id":"282637","messageId":"vpqy48ti6ad.fsf@anie.imag.fr","threadId":"41862","inReplyTo":"CA+DCAeTm7wjgdjLwR__pcyev-EsqecdAT8xdGEFfuekg4ToKSA@mail.gmail.com","subject":"Re: [PATCH 5/5] t/t5520: test --[no-]autostash with pull.rebase=true","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-04-04T18:21:30Z","receivedAt":"2016-04-04T18:21:30Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Mehul Jain writes:\n\n> On Mon, Apr 4, 2016 at 10:22 PM, Matthieu Moy\n> wrote:\n>> I think it would be much simpler to drop the loop, and write instead\n>> something like (untested):\n>\n> I tested it (with few minor changes), and worked fine.\n>\n> test_autostash () {\n> OLDIFS=$IFS\n> IFS='='\n> set -- $*\n> IFS=$OLDIFS\n\nThis $IFS dance is not needed. If you need to split variable and value,\nthen just pass two arguments on the caller side.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"282639","messageId":"vpqmvp9i63n.fsf@anie.imag.fr","threadId":"41862","inReplyTo":"CAPig+cTSHQcMh=gTLgE3kCgLqBr55ar9wn3gwXLbvRiOyqch1A@mail.gmail.com","subject":"Re: [PATCH 5/5] t/t5520: test --[no-]autostash with pull.rebase=true","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-04-04T18:25:32Z","receivedAt":"2016-04-04T18:25:32Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Eric Sunshine writes:\n\n> Although I'm the one who brought up the idea of \"automating\" these\n> tests, I'm not convinced that it's an improvement in this case, but I\n> don't feel so strongly that I'd forbid it.\n\nAnother option is to define helper functions to shorten the \"manual\"\ntests, e.g. define:\n\nsetup_rebase_test () {\n\tgit reset --hard before-rebase &&\n\techo dirty >new_file &&\n\tgit add new_file\n}\n\nrebase_test_ok () {\n git pull $1 . copy &&\n test_cmp_rev HEAD^ copy &&\n test \"$(cat new_file)\" = dirty &&\n test \"$(cat file)\" = \"modified again\"\n}\n\nrebase_test_err () {\n test_must_fail git pull $1 . copy 2>err &&\n test_i18ngrep \"uncommitted changes.\" err\n}\n\nI'm also OK with keeping the \"manual\" tests.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"}]}