{"thread":{"id":"39430","subject":"[PATCH 1/3] t4150-am: refactor and clean common setup","startedAt":"2015-05-26T21:32:53Z","lastAt":"2015-05-29T11:50:43Z","messageCount":11,"participants":["Remi Lespinet","Paul Tan","Junio C Hamano","Eric Sunshine","Matthieu Moy","Remi LESPINET"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"262162","messageId":"1432675975-11020-1-git-send-email-remi.lespinet@ensimag.grenoble-inp.fr","threadId":"39430","inReplyTo":null,"subject":"[PATCH 1/3] t4150-am: refactor and clean common setup","fromName":"Remi Lespinet","fromEmail":"remi.lespinet@ensimag.grenoble-inp.fr","sentAt":"2015-05-26T21:32:53Z","receivedAt":"2015-05-26T21:32:53Z","isPatch":true,"sender":{"key":"remi.lespinet@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/11941160?v=4"},"body":"Add new functions to keep the setup cleaner:\n - setup_temporary_branch: creates a new branch, check it out\n   and automatically delete it after the test is over\n - setup_fixed_branch: creates a fixed branch, which can be re-used\n   in later tests\n\nSigned-off-by: Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr>\n---\n t/t4150-am.sh | 138 ++++++++++++++++++++--------------------------------------\n 1 file changed, 47 insertions(+), 91 deletions(-)\n\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex 306e6f3..8370951 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -4,6 +4,20 @@ test_description='git am running'\n \n . ./test-lib.sh\n \n+setup_temporary_branch () {\n+\ttmp_name=${2-\"temporary\"}\n+\tgit reset --hard &&\n+\trm -fr .git/rebase-apply &&\n+\ttest_when_finished \"git checkout $1 && git branch -D $tmp_name\" &&\n+\tgit checkout -b \"$tmp_name\" \"$1\"\n+}\n+\n+setup_fixed_branch () {\n+\tgit reset --hard &&\n+\trm -fr .git/rebase-apply &&\n+\tgit checkout -b \"$1\" \"$2\"\n+}\n+\n test_expect_success 'setup: messages' '\n \tcat >msg <<-\\EOF &&\n \tsecond\n@@ -143,9 +157,7 @@ test_expect_success setup '\n '\n \n test_expect_success 'am applies patch correctly' '\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout first &&\n+\tsetup_temporary_branch first &&\n \ttest_tick &&\n \tgit am <patch1 &&\n \ttest_path_is_missing .git/rebase-apply &&\n@@ -155,9 +167,7 @@ test_expect_success 'am applies patch correctly' '\n '\n \n test_expect_success 'am applies patch e-mail not in a mbox' '\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout first &&\n+\tsetup_temporary_branch first &&\n \tgit am patch1.eml &&\n \ttest_path_is_missing .git/rebase-apply &&\n \tgit diff --exit-code second &&\n@@ -166,9 +176,7 @@ test_expect_success 'am applies patch e-mail not in a mbox' '\n '\n \n test_expect_success 'am applies patch e-mail not in a mbox with CRLF' '\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout first &&\n+\tsetup_temporary_branch first &&\n \tgit am patch1-crlf.eml &&\n \ttest_path_is_missing .git/rebase-apply &&\n \tgit diff --exit-code second &&\n@@ -177,9 +185,7 @@ test_expect_success 'am applies patch e-mail not in a mbox with CRLF' '\n '\n \n test_expect_success 'am applies patch e-mail with preceding whitespace' '\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout first &&\n+\tsetup_temporary_branch first &&\n \tgit am patch1-ws.eml &&\n \ttest_path_is_missing .git/rebase-apply &&\n \tgit diff --exit-code second &&\n@@ -203,9 +209,7 @@ compare () {\n \n test_expect_success 'am changes committer and keeps author' '\n \ttest_tick &&\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout first &&\n+\tsetup_temporary_branch first &&\n \tgit am patch2 &&\n \ttest_path_is_missing .git/rebase-apply &&\n \ttest \"$(git rev-parse master^^)\" = \"$(git rev-parse HEAD^^)\" &&\n@@ -218,9 +222,7 @@ test_expect_success 'am changes committer and keeps author' '\n '\n \n test_expect_success 'am --signoff adds Signed-off-by: line' '\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout -b master2 first &&\n+\tsetup_fixed_branch master2 first &&\n \tgit am --signoff <patch2 &&\n \tprintf \"%s\\n\" \"$signoff\" >expected &&\n \techo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\" >>expected &&\n@@ -255,9 +257,7 @@ test_expect_success 'am without --keep removes Re: and [PATCH] stuff' '\n '\n \n test_expect_success 'am --keep really keeps the subject' '\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout HEAD^ &&\n+\tsetup_temporary_branch master2^ &&\n \tgit am --keep patch4 &&\n \ttest_path_is_missing .git/rebase-apply &&\n \tgit cat-file commit HEAD >actual &&\n@@ -265,9 +265,7 @@ test_expect_success 'am --keep really keeps the subject' '\n '\n \n test_expect_success 'am --keep-non-patch really keeps the non-patch part' '\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout HEAD^ &&\n+\tsetup_temporary_branch master2^ &&\n \tgit am --keep-non-patch patch4 &&\n \ttest_path_is_missing .git/rebase-apply &&\n \tgit cat-file commit HEAD >actual &&\n@@ -275,9 +273,7 @@ test_expect_success 'am --keep-non-patch really keeps the non-patch part' '\n '\n \n test_expect_success 'am -3 falls back to 3-way merge' '\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout -b lorem2 master2 &&\n+\tsetup_fixed_branch lorem2 master2 &&\n \tsed -n -e \"3,\\$p\" msg >file &&\n \thead -n 9 msg >>file &&\n \tgit add file &&\n@@ -289,9 +285,7 @@ test_expect_success 'am -3 falls back to 3-way merge' '\n '\n \n test_expect_success 'am -3 -p0 can read --no-prefix patch' '\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout -b lorem3 master2 &&\n+\tsetup_temporary_branch lorem2 &&\n \tsed -n -e \"3,\\$p\" msg >file &&\n \thead -n 9 msg >>file &&\n \tgit add file &&\n@@ -303,10 +297,8 @@ test_expect_success 'am -3 -p0 can read --no-prefix patch' '\n '\n \n test_expect_success 'am can rename a file' '\n+\tsetup_temporary_branch lorem &&\n \tgrep \"^rename from\" rename.patch &&\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout lorem^0 &&\n \tgit am rename.patch &&\n \ttest_path_is_missing .git/rebase-apply &&\n \tgit update-index --refresh &&\n@@ -314,10 +306,8 @@ test_expect_success 'am can rename a file' '\n '\n \n test_expect_success 'am -3 can rename a file' '\n+\tsetup_temporary_branch lorem &&\n \tgrep \"^rename from\" rename.patch &&\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout lorem^0 &&\n \tgit am -3 rename.patch &&\n \ttest_path_is_missing .git/rebase-apply &&\n \tgit update-index --refresh &&\n@@ -325,10 +315,8 @@ test_expect_success 'am -3 can rename a file' '\n '\n \n test_expect_success 'am -3 can rename a file after falling back to 3-way merge' '\n+\tsetup_temporary_branch lorem &&\n \tgrep \"^rename from\" rename-add.patch &&\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout lorem^0 &&\n \tgit am -3 rename-add.patch &&\n \ttest_path_is_missing .git/rebase-apply &&\n \tgit update-index --refresh &&\n@@ -336,9 +324,7 @@ test_expect_success 'am -3 can rename a file after falling back to 3-way merge'\n '\n \n test_expect_success 'am -3 -q is quiet' '\n-\trm -fr .git/rebase-apply &&\n-\tgit checkout -f lorem2 &&\n-\tgit reset master2 --hard &&\n+\tsetup_temporary_branch lorem2 &&\n \tsed -n -e \"3,\\$p\" msg >file &&\n \thead -n 9 msg >>file &&\n \tgit add file &&\n@@ -349,11 +335,9 @@ test_expect_success 'am -3 -q is quiet' '\n '\n \n test_expect_success 'am pauses on conflict' '\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout lorem2^^ &&\n+\tsetup_temporary_branch lorem2^^ &&\n \ttest_must_fail git am lorem-move.patch &&\n-\ttest -d .git/rebase-apply\n+\ttest_path_is_dir .git/rebase-apply\n '\n \n test_expect_success 'am --skip works' '\n@@ -371,12 +355,10 @@ test_expect_success 'am --abort removes a stray directory' '\n '\n \n test_expect_success 'am --resolved works' '\n+\tsetup_temporary_branch lorem2^^ &&\n \techo goodbye >expected &&\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout lorem2^^ &&\n \ttest_must_fail git am lorem-move.patch &&\n-\ttest -d .git/rebase-apply &&\n+\ttest_path_is_dir .git/rebase-apply &&\n \techo resolved >>file &&\n \tgit add file &&\n \tgit am --resolved &&\n@@ -385,25 +367,21 @@ test_expect_success 'am --resolved works' '\n '\n \n test_expect_success 'am takes patches from a Pine mailbox' '\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout first &&\n+\tsetup_temporary_branch first &&\n \tcat pine patch1 | git am &&\n \ttest_path_is_missing .git/rebase-apply &&\n \tgit diff --exit-code master^..HEAD\n '\n \n test_expect_success 'am fails on mail without patch' '\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n+\tsetup_temporary_branch first &&\n \ttest_must_fail git am <failmail &&\n \tgit am --abort &&\n \ttest_path_is_missing .git/rebase-apply\n '\n \n test_expect_success 'am fails on empty patch' '\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n+\tsetup_temporary_branch first &&\n \techo \"---\" >>failmail &&\n \ttest_must_fail git am <failmail &&\n \tgit am --skip &&\n@@ -411,10 +389,8 @@ test_expect_success 'am fails on empty patch' '\n '\n \n test_expect_success 'am works from stdin in subdirectory' '\n+\tsetup_temporary_branch first &&\n \trm -fr subdir &&\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout first &&\n \t(\n \t\tmkdir -p subdir &&\n \t\tcd subdir &&\n@@ -424,10 +400,8 @@ test_expect_success 'am works from stdin in subdirectory' '\n '\n \n test_expect_success 'am works from file (relative path given) in subdirectory' '\n+\tsetup_temporary_branch first &&\n \trm -fr subdir &&\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout first &&\n \t(\n \t\tmkdir -p subdir &&\n \t\tcd subdir &&\n@@ -437,10 +411,8 @@ test_expect_success 'am works from file (relative path given) in subdirectory' '\n '\n \n test_expect_success 'am works from file (absolute path given) in subdirectory' '\n+\tsetup_temporary_branch first &&\n \trm -fr subdir &&\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout first &&\n \tP=$(pwd) &&\n \t(\n \t\tmkdir -p subdir &&\n@@ -451,9 +423,7 @@ test_expect_success 'am works from file (absolute path given) in subdirectory' '\n '\n \n test_expect_success 'am --committer-date-is-author-date' '\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout first &&\n+\tsetup_temporary_branch first &&\n \ttest_tick &&\n \tgit am --committer-date-is-author-date patch1 &&\n \tgit cat-file commit HEAD | sed -e \"/^\\$/q\" >head1 &&\n@@ -463,9 +433,7 @@ test_expect_success 'am --committer-date-is-author-date' '\n '\n \n test_expect_success 'am without --committer-date-is-author-date' '\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout first &&\n+\tsetup_temporary_branch first &&\n \ttest_tick &&\n \tgit am patch1 &&\n \tgit cat-file commit HEAD | sed -e \"/^\\$/q\" >head1 &&\n@@ -479,9 +447,7 @@ test_expect_success 'am without --committer-date-is-author-date' '\n # by test_tick that uses -0700 timezone; if this feature does not\n # work, we will see that instead of +0000.\n test_expect_success 'am --ignore-date' '\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout first &&\n+\tsetup_temporary_branch first &&\n \ttest_tick &&\n \tgit am --ignore-date patch1 &&\n \tgit cat-file commit HEAD | sed -e \"/^\\$/q\" >head1 &&\n@@ -490,9 +456,8 @@ test_expect_success 'am --ignore-date' '\n '\n \n test_expect_success 'am into an unborn branch' '\n+\tsetup_temporary_branch first &&\n \tgit rev-parse first^{tree} >expected &&\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n \trm -fr subdir &&\n \tmkdir subdir &&\n \tgit format-patch --numbered-files -o subdir -1 first &&\n@@ -509,9 +474,7 @@ test_expect_success 'am into an unborn branch' '\n '\n \n test_expect_success 'am newline in subject' '\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout first &&\n+\tsetup_temporary_branch first &&\n \ttest_tick &&\n \tsed -e \"s/second/second \\\\\\n foo/\" patch1 >patchnl &&\n \tgit am <patchnl >output.out 2>&1 &&\n@@ -519,17 +482,14 @@ test_expect_success 'am newline in subject' '\n '\n \n test_expect_success 'am -q is quiet' '\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout first &&\n+\tsetup_temporary_branch first &&\n \ttest_tick &&\n \tgit am -q <patch1 >output.out 2>&1 &&\n \t! test -s output.out\n '\n \n test_expect_success 'am empty-file does not infloop' '\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n+\tsetup_temporary_branch first &&\n \ttouch empty-file &&\n \ttest_tick &&\n \ttest_must_fail git am empty-file 2>actual &&\n@@ -538,9 +498,7 @@ test_expect_success 'am empty-file does not infloop' '\n '\n \n test_expect_success 'am --message-id really adds the message id' '\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout HEAD^ &&\n+\tsetup_temporary_branch first &&\n \tgit am --message-id patch1.eml &&\n \ttest_path_is_missing .git/rebase-apply &&\n \tgit cat-file commit HEAD | tail -n1 >actual &&\n@@ -549,9 +507,7 @@ test_expect_success 'am --message-id really adds the message id' '\n '\n \n test_expect_success 'am --message-id -s signs off after the message id' '\n-\trm -fr .git/rebase-apply &&\n-\tgit reset --hard &&\n-\tgit checkout HEAD^ &&\n+\tsetup_temporary_branch first &&\n \tgit am -s --message-id patch1.eml &&\n \ttest_path_is_missing .git/rebase-apply &&\n \tgit cat-file commit HEAD | tail -n2 | head -n1 >actual &&\n-- \n1.9.1\n"},{"id":"262164","messageId":"1432675975-11020-2-git-send-email-remi.lespinet@ensimag.grenoble-inp.fr","threadId":"39430","inReplyTo":"1432675975-11020-1-git-send-email-remi.lespinet@ensimag.grenoble-inp.fr","subject":"[PATCH 2/3] t4150-am: refactor am -3 tests","fromName":"Remi Lespinet","fromEmail":"remi.lespinet@ensimag.grenoble-inp.fr","sentAt":"2015-05-26T21:32:54Z","receivedAt":"2015-05-26T21:32:54Z","isPatch":true,"sender":{"key":"remi.lespinet@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/11941160?v=4"},"body":"Move the creation of the file, commit and branch used in git am -3 tests\nin a setup test, to avoid creating this setup several time.\n\nSigned-off-by: Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr>\n---\n t/t4150-am.sh | 18 ++++++------------\n 1 file changed, 6 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex 8370951..8f85098 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -272,13 +272,17 @@ test_expect_success 'am --keep-non-patch really keeps the non-patch part' '\n \tgrep \"^\\[foo\\] third\" actual\n '\n \n+test_expect_success 'setup: am -3' '\n+\tsetup_fixed_branch lorem2 master2 &&\n+\tsed -n -e \"3,\\$p\" msg >file &&\n+\thead -n 9 msg >>file &&\n+\tgit add file &&\n+\ttest_tick &&\n+\tgit commit -m \"copied stuff\"\n+'\n+\n test_expect_success 'am -3 falls back to 3-way merge' '\n+\tsetup_temporary_branch lorem2 &&\n-\tsetup_fixed_branch lorem2 master2 &&\n-\tsed -n -e \"3,\\$p\" msg >file &&\n-\thead -n 9 msg >>file &&\n-\tgit add file &&\n-\ttest_tick &&\n-\tgit commit -m \"copied stuff\" &&\n \tgit am -3 lorem-move.patch &&\n \ttest_path_is_missing .git/rebase-apply &&\n \tgit diff --exit-code lorem\n@@ -286,11 +290,6 @@ test_expect_success 'am -3 falls back to 3-way merge' '\n \n test_expect_success 'am -3 -p0 can read --no-prefix patch' '\n \tsetup_temporary_branch lorem2 &&\n-\tsed -n -e \"3,\\$p\" msg >file &&\n-\thead -n 9 msg >>file &&\n-\tgit add file &&\n-\ttest_tick &&\n-\tgit commit -m \"copied stuff\" &&\n \tgit am -3 -p0 lorem-zero.patch &&\n \ttest_path_is_missing .git/rebase-apply &&\n \tgit diff --exit-code lorem\n@@ -325,11 +324,6 @@ test_expect_success 'am -3 can rename a file after falling back to 3-way merge'\n \n test_expect_success 'am -3 -q is quiet' '\n \tsetup_temporary_branch lorem2 &&\n-\tsed -n -e \"3,\\$p\" msg >file &&\n-\thead -n 9 msg >>file &&\n-\tgit add file &&\n-\ttest_tick &&\n-\tgit commit -m \"copied stuff\" &&\n \tgit am -3 -q lorem-move.patch >output.out 2>&1 &&\n \t! test -s output.out\n '\n-- \n1.9.1\n"},{"id":"262165","messageId":"1432675975-11020-3-git-send-email-remi.lespinet@ensimag.grenoble-inp.fr","threadId":"39430","inReplyTo":"1432675975-11020-1-git-send-email-remi.lespinet@ensimag.grenoble-inp.fr","subject":"[PATCH 3/3] git-am: add am.threeWay config variable","fromName":"Remi Lespinet","fromEmail":"remi.lespinet@ensimag.grenoble-inp.fr","sentAt":"2015-05-26T21:32:55Z","receivedAt":"2015-05-26T21:32:55Z","isPatch":true,"sender":{"key":"remi.lespinet@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/11941160?v=4"},"body":"Add the am.threeWay configuration variable to use the -3 or --3way\noption of git am by default. When am.threeway is set and not desired\nfor a specific git am command, the --no-3way option can be used to\noverride it.\n\nSigned-off-by: Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr>\n---\n Even if git am will be rewritten soon, the code that will have to be\n ported is not the most important part of the patch and the tests and\n documentation parts can be reused.\n\n Documentation/config.txt |  7 +++++++\n Documentation/git-am.txt |  6 ++++--\n git-am.sh                |  7 +++++++\n t/t4150-am.sh            | 15 +++++++++++++++\n 4 files changed, 33 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex d44bc85..8e42752 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -769,6 +769,13 @@ am.keepcr::\n \tby giving '--no-keep-cr' from the command line.\n \tSee linkgit:git-am[1], linkgit:git-mailsplit[1].\n \n+am.threeWay::\n+\tIf true, git-am will fall back on 3-way merge when the patch\n+\tcannot be applied cleanly, in the same way as the '-3' or\n+\t'--3-way' option. Can be overridden by giving '--no-3-way'\n+\tfrom the command line.\n+\tSee linkgit:git-am[1].\n+\n apply.ignoreWhitespace::\n \tWhen set to 'change', tells 'git apply' to ignore changes in\n \twhitespace, in the same way as the '--ignore-space-change'\ndiff --git a/Documentation/git-am.txt b/Documentation/git-am.txt\nindex 0d8ba48..3190c05 100644\n--- a/Documentation/git-am.txt\n+++ b/Documentation/git-am.txt\n@@ -89,11 +89,13 @@ default.   You can use `--no-utf8` to override this.\n \tlinkgit:git-mailinfo[1]).\n \n -3::\n---3way::\n+--[no-]3way::\n \tWhen the patch does not apply cleanly, fall back on\n \t3-way merge if the patch records the identity of blobs\n \tit is supposed to apply to and we have those blobs\n-\tavailable locally.\n+\tavailable locally.  `am.threeWay` configuration variable\n+\tcan be used to specify the default behaviour.  `--no-3way`\n+\tis useful to override `am.threeWay`.\n \n --ignore-space-change::\n --ignore-whitespace::\ndiff --git a/git-am.sh b/git-am.sh\nindex 761befb..781507c 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -389,6 +389,11 @@ then\n     keepcr=t\n fi\n \n+if test \"$(git config --bool --get am.threeWay)\" = true\n+then\n+    threeway=t\n+fi\n+\n while test $# != 0\n do\n \tcase \"$1\" in\n@@ -400,6 +405,8 @@ it will be removed. Please do not use it anymore.\"\n \t\t;;\n \t-3|--3way)\n \t\tthreeway=t ;;\n+\t--no-3way)\n+\t\tthreeway=f ;;\n \t-s|--signoff)\n \t\tsign=t ;;\n \t-u|--utf8)\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex 8f85098..e16ef0e 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -288,6 +288,21 @@ test_expect_success 'am -3 falls back to 3-way merge' '\n \tgit diff --exit-code lorem\n '\n \n+test_expect_success 'am with config am.threeWay falls back to 3-way merge' '\n+\tsetup_temporary_branch lorem2 &&\n+\ttest_config am.threeWay 1 &&\n+\tgit am lorem-move.patch &&\n+\ttest_path_is_missing .git/rebase-apply &&\n+\tgit diff --exit-code lorem\n+'\n+\n+test_expect_success 'am with config am.threeWay overridden by --no-3way' '\n+\tsetup_temporary_branch lorem2 &&\n+\ttest_config am.threeWay 1 &&\n+\ttest_must_fail git am --no-3way lorem-move.patch &&\n+\ttest_path_is_dir .git/rebase-apply\n+'\n+\n test_expect_success 'am -3 -p0 can read --no-prefix patch' '\n \tsetup_temporary_branch lorem2 &&\n \tgit am -3 -p0 lorem-zero.patch &&\n-- \n1.9.1\n"},{"id":"262328","messageId":"CACRoPnQEJHPfoz0LjSs2X1CrW-TdVGb54XSrg6VWXP2tdyu5Uw@mail.gmail.com","threadId":"39430","inReplyTo":"1432675975-11020-1-git-send-email-remi.lespinet@ensimag.grenoble-inp.fr","subject":"Re: [PATCH 1/3] t4150-am: refactor and clean common setup","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-28T13:10:11Z","receivedAt":"2015-05-28T13:10:11Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi,\n\nTake these comments/suggestions with a pinch of salt because I'm not\nthat experienced with the code base as well ;-).\n\nOn Wed, May 27, 2015 at 5:32 AM, Remi Lespinet\n<remi.lespinet@ensimag.grenoble-inp.fr> wrote:\n> Add new functions to keep the setup cleaner:\n>  - setup_temporary_branch: creates a new branch, check it out\n>    and automatically delete it after the test is over\n\nAgreed. This removes a lot of boilerplate from the tests. Another\npositive effect is that we can be sure that any commits made on that\nthrowaway branch will not affect the other parts of the test suite,\nwhich makes understanding and editing the test suite much easier.\n\n>  - setup_fixed_branch: creates a fixed branch, which can be re-used\n>    in later tests\n\nLooking at the patch, I see that setup_fixed_branch() is only used in\n2 places, so I don't think it is a common pattern that would require\nits own function. Also, see below.\n\n> Signed-off-by: Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr>\n> ---\n>  t/t4150-am.sh | 138 ++++++++++++++++++++--------------------------------------\n>  1 file changed, 47 insertions(+), 91 deletions(-)\n>\n> diff --git a/t/t4150-am.sh b/t/t4150-am.sh\n> index 306e6f3..8370951 100755\n> --- a/t/t4150-am.sh\n> +++ b/t/t4150-am.sh\n> @@ -4,6 +4,20 @@ test_description='git am running'\n>\n>  . ./test-lib.sh\n>\n> +setup_temporary_branch () {\n\nMaybe name this checkout_temp_branch() or something, since it more or\nless is just a wrapper around git-checkout.\n\n> +       tmp_name=${2-\"temporary\"}\n\nI don't think the quotes are required. Also, I don't feel good about\nswapping the order of the arguments to git-checkout. (or making $2 an\noptional argument). As the patch stands, if I see\n\nsetup_temporary_branch lorem\n\nI will think: so we are creating a temporary branch \"lorem\". But nope,\nwe are creating a temporary branch \"temporary\" that branches from\n\"lorem\". I don't think writing setup_temporary_branch \"temporary\"\n\"lorem\" explicitly is that bad.\n\nThis is just personal preference though.\n\n> +       git reset --hard &&\n> +       rm -fr .git/rebase-apply &&\n> +       test_when_finished \"git checkout $1 && git branch -D $tmp_name\" &&\n\nI think you should use \"git checkout -f $1\" as if the working tree is\ndirty the test will fail at cleanup with no error message at all,\nwhich is annoying for debugging. Furthermore, the test_when_finished\nshould be shifted below the git checkout -b below, as git branch -D\nwill fail if the branch does not exist.\n\n> +       git checkout -b \"$tmp_name\" \"$1\"\n\nIf you use git checkout -f here then there's no need for the git reset\n--hard above.\n\n> +}\n> +\n> +setup_fixed_branch () {\n> +       git reset --hard &&\n> +       rm -fr .git/rebase-apply &&\n> +       git checkout -b \"$1\" \"$2\"\n\nAgain, by using git checkout -f we can drop the git reset --hard. But\nthat reduces the function to 2 lines, and thus I don't think that this\nusage pattern needs its own function.\n\n> +}\n> +\n>  test_expect_success 'setup: messages' '\n>         cat >msg <<-\\EOF &&\n>         second\n> @@ -143,9 +157,7 @@ test_expect_success setup '\n>  '\n\nI haven't looked at the rest of the patch in detail because I'm not\nthat well-acquainted with t4150 yet, but it looks okay.\n\nThanks,\nPaul\n"},{"id":"262331","messageId":"CACRoPnSnwA5GcYZjHHTepz4WzXYuJ+9nJqGsL92r_vnsNrN_-w@mail.gmail.com","threadId":"39430","inReplyTo":"1432675975-11020-3-git-send-email-remi.lespinet@ensimag.grenoble-inp.fr","subject":"Re: [PATCH 3/3] git-am: add am.threeWay config variable","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-28T13:47:47Z","receivedAt":"2015-05-28T13:47:47Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi,\n\nOn Wed, May 27, 2015 at 5:32 AM, Remi Lespinet\n<remi.lespinet@ensimag.grenoble-inp.fr> wrote:\n> Add the am.threeWay configuration variable to use the -3 or --3way\n> option of git am by default. When am.threeway is set and not desired\n> for a specific git am command, the --no-3way option can be used to\n> override it.\n>\n> Signed-off-by: Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr>\n> ---\n>  Even if git am will be rewritten soon, the code that will have to be\n>  ported is not the most important part of the patch and the tests and\n>  documentation parts can be reused.\n\nYup, there's no problem on my side.\n\n>  Documentation/config.txt |  7 +++++++\n>  Documentation/git-am.txt |  6 ++++--\n>  git-am.sh                |  7 +++++++\n>  t/t4150-am.sh            | 15 +++++++++++++++\n>  4 files changed, 33 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index d44bc85..8e42752 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -769,6 +769,13 @@ am.keepcr::\n>         by giving '--no-keep-cr' from the command line.\n>         See linkgit:git-am[1], linkgit:git-mailsplit[1].\n>\n> +am.threeWay::\n> +       If true, git-am will fall back on 3-way merge when the patch\n> +       cannot be applied cleanly, in the same way as the '-3' or\n> +       '--3-way' option. Can be overridden by giving '--no-3-way'\n> +       from the command line.\n> +       See linkgit:git-am[1].\n> +\n\nMaybe we should start by mentioning the default behavior for git-am.\nSomething like,\n\nBy default, git-am will fail if the patch does not apply cleanly. When\nset to true, this setting tells git-am to fall back on 3-way merge if\nthe patch records the identity of blobs it is supposed to apply to and\nwe have those blobs available locally (equivalent to giving the --3way\noption from the command line).\n\n(I stole most of the text from the git-am and git-config documentation)\n\n>  apply.ignoreWhitespace::\n>         When set to 'change', tells 'git apply' to ignore changes in\n>         whitespace, in the same way as the '--ignore-space-change'\n> diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt\n> index 0d8ba48..3190c05 100644\n> --- a/Documentation/git-am.txt\n> +++ b/Documentation/git-am.txt\n> @@ -89,11 +89,13 @@ default.   You can use `--no-utf8` to override this.\n>         linkgit:git-mailinfo[1]).\n>\n>  -3::\n> ---3way::\n> +--[no-]3way::\n\nThere's no need to mention --no-3way, it's assumed that most of the\noptions have the equivalent --no-option because it is implemented\nautomatically by the option parser.\n\n>         When the patch does not apply cleanly, fall back on\n>         3-way merge if the patch records the identity of blobs\n>         it is supposed to apply to and we have those blobs\n> -       available locally.\n> +       available locally.  `am.threeWay` configuration variable\n> +       can be used to specify the default behaviour.  `--no-3way`\n> +       is useful to override `am.threeWay`.\n\nUsually configuration settings are mentioned in a separate section in\nthe documentation \"CONFIGURATION\" (or not mentioned at all). Also,\nthere's no need to mention that --no-3way can be used to mention the\nconfiguration, as its usual (and expected) that the configuration\nvalue sets the default behavior, and the command-line switch can\noverride i.\n\n>  --ignore-space-change::\n>  --ignore-whitespace::\n> diff --git a/git-am.sh b/git-am.sh\n> index 761befb..781507c 100755\n> --- a/git-am.sh\n> +++ b/git-am.sh\n> @@ -389,6 +389,11 @@ then\n>      keepcr=t\n>  fi\n>\n> +if test \"$(git config --bool --get am.threeWay)\" = true\n> +then\n> +    threeway=t\n> +fi\n> +\n>  while test $# != 0\n>  do\n>         case \"$1\" in\n> @@ -400,6 +405,8 @@ it will be removed. Please do not use it anymore.\"\n>                 ;;\n>         -3|--3way)\n>                 threeway=t ;;\n> +       --no-3way)\n> +               threeway=f ;;\n\nOkay.\n\nThis patch on its own, though, does not handle the case where:\n\n1. am.threeWay is set to \"true\"\n2. we passed \"--no-3way\" in the command-line\n3. git-am failed to apply a patch and quit\n4. We git-am --skip or git-am --continue. When continuing, git-am will\nnot remember the --no-3way (unless we provide it again on the command\nline)\n\nThey key is to tweak the following lines where the \"threeway\" setting is loaded:\n\n    if test \"$(cat \"$dotest/threeway\")\" = t\n    then\n        threeway=t\n    fi\n\nTo something like\n\n    if test \"$(cat \"$dotest/threeway\")\" = t\n    then\n        threeway=t\n    else\n        threeway=f\n    fi\n\n>         -s|--signoff)\n>                 sign=t ;;\n>         -u|--utf8)\n> diff --git a/t/t4150-am.sh b/t/t4150-am.sh\n> index 8f85098..e16ef0e 100755\n> --- a/t/t4150-am.sh\n> +++ b/t/t4150-am.sh\n> @@ -288,6 +288,21 @@ test_expect_success 'am -3 falls back to 3-way merge' '\n>         git diff --exit-code lorem\n>  '\n>\n> +test_expect_success 'am with config am.threeWay falls back to 3-way merge' '\n> +       setup_temporary_branch lorem2 &&\n> +       test_config am.threeWay 1 &&\n> +       git am lorem-move.patch &&\n> +       test_path_is_missing .git/rebase-apply &&\n> +       git diff --exit-code lorem\n> +'\n> +\n> +test_expect_success 'am with config am.threeWay overridden by --no-3way' '\n> +       setup_temporary_branch lorem2 &&\n> +       test_config am.threeWay 1 &&\n> +       test_must_fail git am --no-3way lorem-move.patch &&\n> +       test_path_is_dir .git/rebase-apply\n> +'\n> +\n>  test_expect_success 'am -3 -p0 can read --no-prefix patch' '\n>         setup_temporary_branch lorem2 &&\n>         git am -3 -p0 lorem-zero.patch &&\n\nThis looks good.\n\n> --\n> 1.9.1\n\nTo end off, some off-tangent issues that are not related to the patch\nseries in question, but since I'm looking at git-am.sh....\n\nI've noticed that in the block above that initializes all the variables,\n\n    sign= utf8=t keep= keepcr= skip= interactive= resolved= rebasing= abort=\n    messageid= resolvemsg= resume= scissors= no_inbody_headers=\n    git_apply_opt=\n    committer_date_is_author_date=\n    ignore_date=\n    allow_rerere_autoupdate=\n    gpg_sign_opt=\n\nthreeway is not initialized at all, and thus I think running\n\"threeway=t git am blah\" will affect the behavior of git-am.\n\nAlso, I noticed that we do not check for --no-interactive,\n--no-signoff, --no-keep, --no-whitespace, etc.\n\nThanks,\nPaul\n"},{"id":"262353","messageId":"xmqqy4k8h9f8.fsf@gitster.dls.corp.google.com","threadId":"39430","inReplyTo":"CACRoPnSnwA5GcYZjHHTepz4WzXYuJ+9nJqGsL92r_vnsNrN_-w@mail.gmail.com","subject":"Re: [PATCH 3/3] git-am: add am.threeWay config variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-28T17:57:31Z","receivedAt":"2015-05-28T17:57:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan <pyokagan@gmail.com> writes:\n\n>> diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt\n>> index 0d8ba48..3190c05 100644\n>> --- a/Documentation/git-am.txt\n>> +++ b/Documentation/git-am.txt\n>> @@ -89,11 +89,13 @@ default.   You can use `--no-utf8` to override this.\n>>         linkgit:git-mailinfo[1]).\n>>\n>>  -3::\n>> ---3way::\n>> +--[no-]3way::\n>\n> There's no need to mention --no-3way,...\n\nActually, we prefer to do it this way:\n\n\t-3::\n\t--3way::\n\t--no-3way::\n\t\tDescribe what --3way does here.\n\n\n$ git grep -e '^--no-' -e '^--\\[no-\\]' Documentation/\n\n\n\n\n>>         When the patch does not apply cleanly, fall back on\n>>         3-way merge if the patch records the identity of blobs\n>>         it is supposed to apply to and we have those blobs\n>> -       available locally.\n>> +       available locally.  `am.threeWay` configuration variable\n>> +       can be used to specify the default behaviour.  `--no-3way`\n>> +       is useful to override `am.threeWay`.\n>\n> Usually configuration settings are mentioned in a separate section in\n> the documentation \"CONFIGURATION\" (or not mentioned at all).\n\nI can go either way, actually.  But if the description mentions\nam.threeWay as a way to tweak the default, it also should spell out\nthe default when the configuration is not there at all.\n\n> Also, there's no need to mention that --no-3way can be used to\n> mention the configuration, as its usual (and expected) that the\n> configuration value sets the default behavior, and the\n> command-line switch can override i.\n\nYes.  Also --3way is useful to override `am.threeWay` set to `false` ;-)\n\n> To end off, some off-tangent issues that are not related to the patch\n> series in question, but since I'm looking at git-am.sh....\n>\n> I've noticed that in the block above that initializes all the variables,\n>\n>     sign= utf8=t keep= keepcr= skip= interactive= resolved= rebasing= abort=\n>     messageid= resolvemsg= resume= scissors= no_inbody_headers=\n>     git_apply_opt=\n>     committer_date_is_author_date=\n>     ignore_date=\n>     allow_rerere_autoupdate=\n>     gpg_sign_opt=\n>\n> threeway is not initialized at all, and thus I think running\n> \"threeway=t git am blah\" will affect the behavior of git-am.\n\nCorrect.  I overlooked this when I originally did threeway.  Perhaps\na preparatory bugfix patch is warranted before this one.\n\n> Also, I noticed that we do not check for --no-interactive,\n> --no-signoff, --no-keep, --no-whitespace, etc.\n\nEven though adding support for them would not hurt, lack of these\nare OK, as long as we do not have configuration variables to tweak\ntheir defaults.\n"},{"id":"262355","messageId":"CAPig+cRm6MRaR=Qgjxo2fGGcs522DstVedmq4j_fAQeBwG4ZUg@mail.gmail.com","threadId":"39430","inReplyTo":"CACRoPnQEJHPfoz0LjSs2X1CrW-TdVGb54XSrg6VWXP2tdyu5Uw@mail.gmail.com","subject":"Re: [PATCH 1/3] t4150-am: refactor and clean common setup","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-28T18:15:36Z","receivedAt":"2015-05-28T18:15:36Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, May 28, 2015 at 9:10 AM, Paul Tan <pyokagan@gmail.com> wrote:\n> Take these comments/suggestions with a pinch of salt because I'm not\n> that experienced with the code base as well ;-).\n\nI agree with pretty much all of your review comments. See below for a\nminor addenda.\n\n> On Wed, May 27, 2015 at 5:32 AM, Remi Lespinet\n> <remi.lespinet@ensimag.grenoble-inp.fr> wrote:\n>> +setup_temporary_branch () {\n>\n> Maybe name this checkout_temp_branch() or something, since it more or\n> less is just a wrapper around git-checkout.\n\ncheckout_temp_branch() doesn't give any indication that a new branch\nis being created, so it may not be an improvement over\nsetup_temporary_branch(). A more apt name might be something like\nnew_temp_branch().\n\n>> +       tmp_name=${2-\"temporary\"}\n>\n> I don't think the quotes are required. Also, I don't feel good about\n> swapping the order of the arguments to git-checkout. (or making $2 an\n> optional argument). As the patch stands, if I see\n>\n> setup_temporary_branch lorem\n>\n> I will think: so we are creating a temporary branch \"lorem\". But nope,\n> we are creating a temporary branch \"temporary\" that branches from\n> \"lorem\". I don't think writing setup_temporary_branch \"temporary\"\n> \"lorem\" explicitly is that bad.\n\nIn fact, the second argument is never used in any of the three\npatches, so it seems to be wasted functionality (at this time). If so,\nthen an even more appropriate name might be new_temp_branch_from().\nClearly, then:\n\n    new_temp_branch_from lorem\n\ncreates a throw-away branch based upon 'lorem'.\n"},{"id":"262361","messageId":"CAPig+cQ_PrY8=-iP-FJm_HZ+XKOVoc4NfDu6rtuBj8zfM5oG+w@mail.gmail.com","threadId":"39430","inReplyTo":"1432675975-11020-1-git-send-email-remi.lespinet@ensimag.grenoble-inp.fr","subject":"Re: [PATCH 1/3] t4150-am: refactor and clean common setup","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-28T19:09:52Z","receivedAt":"2015-05-28T19:09:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 26, 2015 at 5:32 PM, Remi Lespinet\n<remi.lespinet@ensimag.grenoble-inp.fr> wrote:\n> Add new functions to keep the setup cleaner:\n>  - setup_temporary_branch: creates a new branch, check it out\n>    and automatically delete it after the test is over\n>  - setup_fixed_branch: creates a fixed branch, which can be re-used\n>    in later tests\n\nSee below for review comments beyond those already provided by Paul...\n\n> Signed-off-by: Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr>\n> ---\n> diff --git a/t/t4150-am.sh b/t/t4150-am.sh\n> index 306e6f3..8370951 100755\n> --- a/t/t4150-am.sh\n> +++ b/t/t4150-am.sh\n> @@ -4,6 +4,20 @@ test_description='git am running'\n>\n>  . ./test-lib.sh\n>\n> +setup_temporary_branch () {\n> +       tmp_name=${2-\"temporary\"}\n> +       git reset --hard &&\n> +       rm -fr .git/rebase-apply &&\n> +       test_when_finished \"git checkout $1 && git branch -D $tmp_name\" &&\n> +       git checkout -b \"$tmp_name\" \"$1\"\n> +}\n> +\n> +setup_fixed_branch () {\n> +       git reset --hard &&\n> +       rm -fr .git/rebase-apply &&\n> +       git checkout -b \"$1\" \"$2\"\n> +}\n> +\n>  test_expect_success 'setup: messages' '\n>         cat >msg <<-\\EOF &&\n>         second\n> @@ -143,9 +157,7 @@ test_expect_success setup '\n>  '\n>\n>  test_expect_success 'am applies patch correctly' '\n> -       rm -fr .git/rebase-apply &&\n> -       git reset --hard &&\n> -       git checkout first &&\n> +       setup_temporary_branch first &&\n\nThis is confusing. The commit message seems to advertise this patch as\nprimarily a refactoring change (pulling boilerplate out of tests and\ninto a new function), but the operation of that new function is\nsurprisingly different from the boilerplate it's replacing: The old\ncode did not create a branch, the new function does.\n\nIf your intention really is to create new branches in tests which\npreviously did not need throwaway branches, then the commit message\nshould state that as its primary purpose and explain why doing so is\ndesirable, since it is not clear from this patch what you gain from\ndoing so.\n\nMoreover, typically, you should only either refactor or change\nbehavior in a single patch, not both. For instance, patch 1 would add\nthe new function and update tests to call it in place of the\nboilerplate; and patch 2 would change behavior (such as creating a\ntemporary branch).\n\nOn the other hand, if these tests really don't benefit from having a\nthrow-away branch, then this change seems suspect and obscures the\nintent of the test rather than clarifying or simplifying.\n\n>         test_tick &&\n>         git am <patch1 &&\n>         test_path_is_missing .git/rebase-apply &&\n> @@ -275,9 +273,7 @@ test_expect_success 'am --keep-non-patch really keeps the non-patch part' '\n>  '\n>\n>  test_expect_success 'am -3 falls back to 3-way merge' '\n> -       rm -fr .git/rebase-apply &&\n> -       git reset --hard &&\n> -       git checkout -b lorem2 master2 &&\n> +       setup_fixed_branch lorem2 master2 &&\n>         sed -n -e \"3,\\$p\" msg >file &&\n>         head -n 9 msg >>file &&\n>         git add file &&\n> @@ -289,9 +285,7 @@ test_expect_success 'am -3 falls back to 3-way merge' '\n>  '\n>\n>  test_expect_success 'am -3 -p0 can read --no-prefix patch' '\n> -       rm -fr .git/rebase-apply &&\n> -       git reset --hard &&\n> -       git checkout -b lorem3 master2 &&\n> +       setup_temporary_branch lorem2 &&\n\nUnlike the test prior to this one which creates a \"fixed\" branch (via\nsetup_fixed_branch) named 'lorem2' which is used by other tests, the\n'lorem3' branch in this test is never used elsewhere, so\nsetup_temporary_branch is used instead. Makes sense.\n\n>         sed -n -e \"3,\\$p\" msg >file &&\n>         head -n 9 msg >>file &&\n>         git add file &&\n> @@ -303,10 +297,8 @@ test_expect_success 'am -3 -p0 can read --no-prefix patch' '\n>  '\n>\n>  test_expect_success 'am can rename a file' '\n> +       setup_temporary_branch lorem &&\n>         grep \"^rename from\" rename.patch &&\n> -       rm -fr .git/rebase-apply &&\n> -       git reset --hard &&\n> -       git checkout lorem^0 &&\n\nIn other cases, you insert the setup_temporary_branch() invocation\nwhere the old code resided. Why the difference in this test (and\nothers following)?\n\n>         git am rename.patch &&\n>         test_path_is_missing .git/rebase-apply &&\n>         git update-index --refresh &&\n> @@ -349,11 +335,9 @@ test_expect_success 'am -3 -q is quiet' '\n>  '\n>\n>  test_expect_success 'am pauses on conflict' '\n> -       rm -fr .git/rebase-apply &&\n> -       git reset --hard &&\n> -       git checkout lorem2^^ &&\n> +       setup_temporary_branch lorem2^^ &&\n>         test_must_fail git am lorem-move.patch &&\n> -       test -d .git/rebase-apply\n> +       test_path_is_dir .git/rebase-apply\n\nSneaking in unrelated change. This sort of cleanup should go in a\npatch of its own.\n\n>  '\n>\n>  test_expect_success 'am --skip works' '\n"},{"id":"262362","messageId":"CAPig+cTdVZKrGM+8eSopCkUYxK08WB3ZriFOcbd+8Zkf2Y27nA@mail.gmail.com","threadId":"39430","inReplyTo":"CAPig+cQ_PrY8=-iP-FJm_HZ+XKOVoc4NfDu6rtuBj8zfM5oG+w@mail.gmail.com","subject":"Re: [PATCH 1/3] t4150-am: refactor and clean common setup","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-28T19:18:28Z","receivedAt":"2015-05-28T19:18:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, May 28, 2015 at 3:09 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Tue, May 26, 2015 at 5:32 PM, Remi Lespinet\n> <remi.lespinet@ensimag.grenoble-inp.fr> wrote:\n>> +setup_temporary_branch () {\n>> +       tmp_name=${2-\"temporary\"}\n\nI forgot to mention the broken &&-chain here. Although the missing &&\ndoesn't actively hurt the function today, someone may someday insert\ncode above the 'tmp_name=' line without noticing the lack of &&, and\nthe test won't notice a failure in that newly added code. Thus, it's\nbetter to keep the &&-chain intact throughout.\n\n>> +       git reset --hard &&\n>> +       rm -fr .git/rebase-apply &&\n>> +       test_when_finished \"git checkout $1 && git branch -D $tmp_name\" &&\n>> +       git checkout -b \"$tmp_name\" \"$1\"\n>> +}\n"},{"id":"262363","messageId":"vpqpp5kpl09.fsf@anie.imag.fr","threadId":"39430","inReplyTo":"xmqqy4k8h9f8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/3] git-am: add am.threeWay config variable","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-05-28T19:20:06Z","receivedAt":"2015-05-28T19:20:06Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> I've noticed that in the block above that initializes all the variables,\n>>\n>>     sign= utf8=t keep= keepcr= skip= interactive= resolved= rebasing= abort=\n>>     messageid= resolvemsg= resume= scissors= no_inbody_headers=\n>>     git_apply_opt=\n>>     committer_date_is_author_date=\n>>     ignore_date=\n>>     allow_rerere_autoupdate=\n>>     gpg_sign_opt=\n>>\n>> threeway is not initialized at all, and thus I think running\n>> \"threeway=t git am blah\" will affect the behavior of git-am.\n>\n> Correct.  I overlooked this when I originally did threeway.  Perhaps\n> a preparatory bugfix patch is warranted before this one.\n>\n>> Also, I noticed that we do not check for --no-interactive,\n>> --no-signoff, --no-keep, --no-whitespace, etc.\n>\n> Even though adding support for them would not hurt, lack of these\n> are OK, as long as we do not have configuration variables to tweak\n> their defaults.\n\nI wouldn't worry about these issues, as the port to C will make them\ndisappear anyway.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"262418","messageId":"871thz4n70.fsf@ensimag.grenoble-inp.fr","threadId":"39430","inReplyTo":"CAPig+cRm6MRaR=Qgjxo2fGGcs522DstVedmq4j_fAQeBwG4ZUg@mail.gmail.com","subject":"[PATCH 1/3] t4150-am: refactor and clean common setup","fromName":"Remi LESPINET","fromEmail":"remi.lespinet@ensimag.grenoble-inp.fr","sentAt":"2015-05-29T11:50:43Z","receivedAt":"2015-05-29T11:50:43Z","isPatch":true,"sender":{"key":"remi.lespinet@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/11941160?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> Paul Tan <pyokagan@gmail.com> writes:\n>\n> >> Remi LESPINET <remi.lespinet@ensimag.grenoble-inp.fr> writes:\n> >> +       tmp_name=${2-\"temporary\"}\n> >\n> > I don't think the quotes are required. Also, I don't feel good about\n> > swapping the order of the arguments to git-checkout. (or making $2 an\n> > optional argument). As the patch stands, if I see\n> >\n> > setup_temporary_branch lorem\n> >\n> > I will think: so we are creating a temporary branch \"lorem\". But nope,\n> > we are creating a temporary branch \"temporary\" that branches from\n> > \"lorem\". I don't think writing setup_temporary_branch \"temporary\"\n> > \"lorem\" explicitly is that bad.\n> \n> In fact, the second argument is never used in any of the three\n> patches, so it seems to be wasted functionality (at this time). If so,\n> then an even more appropriate name might be new_temp_branch_from().\n> Clearly, then:\n> \n>     new_temp_branch_from lorem\n\nConsidering your two comments about the name of the function I think\nremoving the possibility of using the second argument and renaming the\nfunction new_temp_branch_from gets everyone to agree since it has not\nthe possible confusion with git-checkout problem.\n\n> This is confusing. The commit message seems to advertise this patch as\n> primarily a refactoring change (pulling boilerplate out of tests and\n> into a new function), but the operation of that new function is\n> surprisingly different from the boilerplate it's replacing: The old\n> code did not create a branch, the new function does.\n> \n> If your intention really is to create new branches in tests which\n> previously did not need throwaway branches, then the commit message\n> should state that as its primary purpose and explain why doing so is\n> desirable, since it is not clear from this patch what you gain from\n> doing so.\n> \n> Moreover, typically, you should only either refactor or change\n> behavior in a single patch, not both. For instance, patch 1 would add\n> the new function and update tests to call it in place of the\n> boilerplate; and patch 2 would change behavior (such as creating a\n> temporary branch).\n> \n> On the other hand, if these tests really don't benefit from having a\n> throw-away branch, then this change seems suspect and obscures the\n> intent of the test rather than clarifying or simplifying.\n\nThe purpose of this patch was originally to eliminate some test\ndependancies introduced by the checkouts relative to HEAD (which\ncaused \"cascade failure\") and avoid creating branches, whose name\ncan't be reused, but you're right, that may not benefit as much as I\nexpected...  Perhaps I should have make tags from branches used as\nstart points, which would have done the same effect as these temporary\nbranches.\n\n> >         sed -n -e \"3,\\$p\" msg >file &&\n> >         head -n 9 msg >>file &&\n> >         git add file &&\n> > @@ -303,10 +297,8 @@ test_expect_success 'am -3 -p0 can read --no-prefix patch' '\n> >  '\n> >\n> >  test_expect_success 'am can rename a file' '\n> > +       setup_temporary_branch lorem &&\n> >         grep \"^rename from\" rename.patch &&\n> > -       rm -fr .git/rebase-apply &&\n> > -       git reset --hard &&\n> > -       git checkout lorem^0 &&\n> \n> In other cases, you insert the setup_temporary_branch() invocation\n> where the old code resided. Why the difference in this test (and\n> others following)?\n\nThis doesn't affect the result of the test (assuming\nsetup_temporary_branch doesn't fail). I preferred to add the setup\nbefore anything else in the test.  It affects efficiency in case the\nrename patch has not the correct syntax. So it may be better to put the\ngrep as the first instruction to avoid evaluating all the test in case\nthe syntax of the patch is not correct.\n\nThanks a lot for your comments, I'll correct the patch asap !\n"}]}