[PATCH 1/3] t4150-am: refactor and clean common setup

[PATCH 1/3] t4150-am: refactor and clean common setup

[Remi Lespinet]

Add new functions to keep the setup cleaner:
 - setup_temporary_branch: creates a new branch, check it out
   and automatically delete it after the test is over
 - setup_fixed_branch: creates a fixed branch, which can be re-used
   in later tests

Signed-off-by: Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr>
---
 t/t4150-am.sh | 138 ++++++++++++++++++++--------------------------------------
 1 file changed, 47 insertions(+), 91 deletions(-)

diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 306e6f3..8370951 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -4,6 +4,20 @@ test_description='git am running'
 
 . ./test-lib.sh
 
+setup_temporary_branch () {
+	tmp_name=${2-"temporary"}
+	git reset --hard &&
+	rm -fr .git/rebase-apply &&
+	test_when_finished "git checkout $1 && git branch -D $tmp_name" &&
+	git checkout -b "$tmp_name" "$1"
+}
+
+setup_fixed_branch () {
+	git reset --hard &&
+	rm -fr .git/rebase-apply &&
+	git checkout -b "$1" "$2"
+}
+
 test_expect_success 'setup: messages' '
 	cat >msg <<-\EOF &&
 	second
@@ -143,9 +157,7 @@ test_expect_success setup '
 '
 
 test_expect_success 'am applies patch correctly' '
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout first &&
+	setup_temporary_branch first &&
 	test_tick &&
 	git am <patch1 &&
 	test_path_is_missing .git/rebase-apply &&
@@ -155,9 +167,7 @@ test_expect_success 'am applies patch correctly' '
 '
 
 test_expect_success 'am applies patch e-mail not in a mbox' '
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout first &&
+	setup_temporary_branch first &&
 	git am patch1.eml &&
 	test_path_is_missing .git/rebase-apply &&
 	git diff --exit-code second &&
@@ -166,9 +176,7 @@ test_expect_success 'am applies patch e-mail not in a mbox' '
 '
 
 test_expect_success 'am applies patch e-mail not in a mbox with CRLF' '
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout first &&
+	setup_temporary_branch first &&
 	git am patch1-crlf.eml &&
 	test_path_is_missing .git/rebase-apply &&
 	git diff --exit-code second &&
@@ -177,9 +185,7 @@ test_expect_success 'am applies patch e-mail not in a mbox with CRLF' '
 '
 
 test_expect_success 'am applies patch e-mail with preceding whitespace' '
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout first &&
+	setup_temporary_branch first &&
 	git am patch1-ws.eml &&
 	test_path_is_missing .git/rebase-apply &&
 	git diff --exit-code second &&
@@ -203,9 +209,7 @@ compare () {
 
 test_expect_success 'am changes committer and keeps author' '
 	test_tick &&
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout first &&
+	setup_temporary_branch first &&
 	git am patch2 &&
 	test_path_is_missing .git/rebase-apply &&
 	test "$(git rev-parse master^^)" = "$(git rev-parse HEAD^^)" &&
@@ -218,9 +222,7 @@ test_expect_success 'am changes committer and keeps author' '
 '
 
 test_expect_success 'am --signoff adds Signed-off-by: line' '
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout -b master2 first &&
+	setup_fixed_branch master2 first &&
 	git am --signoff <patch2 &&
 	printf "%s\n" "$signoff" >expected &&
 	echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" >>expected &&
@@ -255,9 +257,7 @@ test_expect_success 'am without --keep removes Re: and [PATCH] stuff' '
 '
 
 test_expect_success 'am --keep really keeps the subject' '
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout HEAD^ &&
+	setup_temporary_branch master2^ &&
 	git am --keep patch4 &&
 	test_path_is_missing .git/rebase-apply &&
 	git cat-file commit HEAD >actual &&
@@ -265,9 +265,7 @@ test_expect_success 'am --keep really keeps the subject' '
 '
 
 test_expect_success 'am --keep-non-patch really keeps the non-patch part' '
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout HEAD^ &&
+	setup_temporary_branch master2^ &&
 	git am --keep-non-patch patch4 &&
 	test_path_is_missing .git/rebase-apply &&
 	git cat-file commit HEAD >actual &&
@@ -275,9 +273,7 @@ test_expect_success 'am --keep-non-patch really keeps the non-patch part' '
 '
 
 test_expect_success 'am -3 falls back to 3-way merge' '
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout -b lorem2 master2 &&
+	setup_fixed_branch lorem2 master2 &&
 	sed -n -e "3,\$p" msg >file &&
 	head -n 9 msg >>file &&
 	git add file &&
@@ -289,9 +285,7 @@ test_expect_success 'am -3 falls back to 3-way merge' '
 '
 
 test_expect_success 'am -3 -p0 can read --no-prefix patch' '
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout -b lorem3 master2 &&
+	setup_temporary_branch lorem2 &&
 	sed -n -e "3,\$p" msg >file &&
 	head -n 9 msg >>file &&
 	git add file &&
@@ -303,10 +297,8 @@ test_expect_success 'am -3 -p0 can read --no-prefix patch' '
 '
 
 test_expect_success 'am can rename a file' '
+	setup_temporary_branch lorem &&
 	grep "^rename from" rename.patch &&
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout lorem^0 &&
 	git am rename.patch &&
 	test_path_is_missing .git/rebase-apply &&
 	git update-index --refresh &&
@@ -314,10 +306,8 @@ test_expect_success 'am can rename a file' '
 '
 
 test_expect_success 'am -3 can rename a file' '
+	setup_temporary_branch lorem &&
 	grep "^rename from" rename.patch &&
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout lorem^0 &&
 	git am -3 rename.patch &&
 	test_path_is_missing .git/rebase-apply &&
 	git update-index --refresh &&
@@ -325,10 +315,8 @@ test_expect_success 'am -3 can rename a file' '
 '
 
 test_expect_success 'am -3 can rename a file after falling back to 3-way merge' '
+	setup_temporary_branch lorem &&
 	grep "^rename from" rename-add.patch &&
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout lorem^0 &&
 	git am -3 rename-add.patch &&
 	test_path_is_missing .git/rebase-apply &&
 	git update-index --refresh &&
@@ -336,9 +324,7 @@ test_expect_success 'am -3 can rename a file after falling back to 3-way merge'
 '
 
 test_expect_success 'am -3 -q is quiet' '
-	rm -fr .git/rebase-apply &&
-	git checkout -f lorem2 &&
-	git reset master2 --hard &&
+	setup_temporary_branch lorem2 &&
 	sed -n -e "3,\$p" msg >file &&
 	head -n 9 msg >>file &&
 	git add file &&
@@ -349,11 +335,9 @@ test_expect_success 'am -3 -q is quiet' '
 '
 
 test_expect_success 'am pauses on conflict' '
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout lorem2^^ &&
+	setup_temporary_branch lorem2^^ &&
 	test_must_fail git am lorem-move.patch &&
-	test -d .git/rebase-apply
+	test_path_is_dir .git/rebase-apply
 '
 
 test_expect_success 'am --skip works' '
@@ -371,12 +355,10 @@ test_expect_success 'am --abort removes a stray directory' '
 '
 
 test_expect_success 'am --resolved works' '
+	setup_temporary_branch lorem2^^ &&
 	echo goodbye >expected &&
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout lorem2^^ &&
 	test_must_fail git am lorem-move.patch &&
-	test -d .git/rebase-apply &&
+	test_path_is_dir .git/rebase-apply &&
 	echo resolved >>file &&
 	git add file &&
 	git am --resolved &&
@@ -385,25 +367,21 @@ test_expect_success 'am --resolved works' '
 '
 
 test_expect_success 'am takes patches from a Pine mailbox' '
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout first &&
+	setup_temporary_branch first &&
 	cat pine patch1 | git am &&
 	test_path_is_missing .git/rebase-apply &&
 	git diff --exit-code master^..HEAD
 '
 
 test_expect_success 'am fails on mail without patch' '
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
+	setup_temporary_branch first &&
 	test_must_fail git am <failmail &&
 	git am --abort &&
 	test_path_is_missing .git/rebase-apply
 '
 
 test_expect_success 'am fails on empty patch' '
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
+	setup_temporary_branch first &&
 	echo "---" >>failmail &&
 	test_must_fail git am <failmail &&
 	git am --skip &&
@@ -411,10 +389,8 @@ test_expect_success 'am fails on empty patch' '
 '
 
 test_expect_success 'am works from stdin in subdirectory' '
+	setup_temporary_branch first &&
 	rm -fr subdir &&
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout first &&
 	(
 		mkdir -p subdir &&
 		cd subdir &&
@@ -424,10 +400,8 @@ test_expect_success 'am works from stdin in subdirectory' '
 '
 
 test_expect_success 'am works from file (relative path given) in subdirectory' '
+	setup_temporary_branch first &&
 	rm -fr subdir &&
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout first &&
 	(
 		mkdir -p subdir &&
 		cd subdir &&
@@ -437,10 +411,8 @@ test_expect_success 'am works from file (relative path given) in subdirectory' '
 '
 
 test_expect_success 'am works from file (absolute path given) in subdirectory' '
+	setup_temporary_branch first &&
 	rm -fr subdir &&
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout first &&
 	P=$(pwd) &&
 	(
 		mkdir -p subdir &&
@@ -451,9 +423,7 @@ test_expect_success 'am works from file (absolute path given) in subdirectory' '
 '
 
 test_expect_success 'am --committer-date-is-author-date' '
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout first &&
+	setup_temporary_branch first &&
 	test_tick &&
 	git am --committer-date-is-author-date patch1 &&
 	git cat-file commit HEAD | sed -e "/^\$/q" >head1 &&
@@ -463,9 +433,7 @@ test_expect_success 'am --committer-date-is-author-date' '
 '
 
 test_expect_success 'am without --committer-date-is-author-date' '
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout first &&
+	setup_temporary_branch first &&
 	test_tick &&
 	git am patch1 &&
 	git cat-file commit HEAD | sed -e "/^\$/q" >head1 &&
@@ -479,9 +447,7 @@ test_expect_success 'am without --committer-date-is-author-date' '
 # by test_tick that uses -0700 timezone; if this feature does not
 # work, we will see that instead of +0000.
 test_expect_success 'am --ignore-date' '
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout first &&
+	setup_temporary_branch first &&
 	test_tick &&
 	git am --ignore-date patch1 &&
 	git cat-file commit HEAD | sed -e "/^\$/q" >head1 &&
@@ -490,9 +456,8 @@ test_expect_success 'am --ignore-date' '
 '
 
 test_expect_success 'am into an unborn branch' '
+	setup_temporary_branch first &&
 	git rev-parse first^{tree} >expected &&
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
 	rm -fr subdir &&
 	mkdir subdir &&
 	git format-patch --numbered-files -o subdir -1 first &&
@@ -509,9 +474,7 @@ test_expect_success 'am into an unborn branch' '
 '
 
 test_expect_success 'am newline in subject' '
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout first &&
+	setup_temporary_branch first &&
 	test_tick &&
 	sed -e "s/second/second \\\n foo/" patch1 >patchnl &&
 	git am <patchnl >output.out 2>&1 &&
@@ -519,17 +482,14 @@ test_expect_success 'am newline in subject' '
 '
 
 test_expect_success 'am -q is quiet' '
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout first &&
+	setup_temporary_branch first &&
 	test_tick &&
 	git am -q <patch1 >output.out 2>&1 &&
 	! test -s output.out
 '
 
 test_expect_success 'am empty-file does not infloop' '
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
+	setup_temporary_branch first &&
 	touch empty-file &&
 	test_tick &&
 	test_must_fail git am empty-file 2>actual &&
@@ -538,9 +498,7 @@ test_expect_success 'am empty-file does not infloop' '
 '
 
 test_expect_success 'am --message-id really adds the message id' '
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout HEAD^ &&
+	setup_temporary_branch first &&
 	git am --message-id patch1.eml &&
 	test_path_is_missing .git/rebase-apply &&
 	git cat-file commit HEAD | tail -n1 >actual &&
@@ -549,9 +507,7 @@ test_expect_success 'am --message-id really adds the message id' '
 '
 
 test_expect_success 'am --message-id -s signs off after the message id' '
-	rm -fr .git/rebase-apply &&
-	git reset --hard &&
-	git checkout HEAD^ &&
+	setup_temporary_branch first &&
 	git am -s --message-id patch1.eml &&
 	test_path_is_missing .git/rebase-apply &&
 	git cat-file commit HEAD | tail -n2 | head -n1 >actual &&
-- 
1.9.1

[PATCH 2/3] t4150-am: refactor am -3 tests

[Remi Lespinet]

Move the creation of the file, commit and branch used in git am -3 tests
in a setup test, to avoid creating this setup several time.

Signed-off-by: Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr>
---
 t/t4150-am.sh | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 8370951..8f85098 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -272,13 +272,17 @@ test_expect_success 'am --keep-non-patch really keeps the non-patch part' '
 	grep "^\[foo\] third" actual
 '
 
+test_expect_success 'setup: am -3' '
+	setup_fixed_branch lorem2 master2 &&
+	sed -n -e "3,\$p" msg >file &&
+	head -n 9 msg >>file &&
+	git add file &&
+	test_tick &&
+	git commit -m "copied stuff"
+'
+
 test_expect_success 'am -3 falls back to 3-way merge' '
+	setup_temporary_branch lorem2 &&
-	setup_fixed_branch lorem2 master2 &&
-	sed -n -e "3,\$p" msg >file &&
-	head -n 9 msg >>file &&
-	git add file &&
-	test_tick &&
-	git commit -m "copied stuff" &&
 	git am -3 lorem-move.patch &&
 	test_path_is_missing .git/rebase-apply &&
 	git diff --exit-code lorem
@@ -286,11 +290,6 @@ test_expect_success 'am -3 falls back to 3-way merge' '
 
 test_expect_success 'am -3 -p0 can read --no-prefix patch' '
 	setup_temporary_branch lorem2 &&
-	sed -n -e "3,\$p" msg >file &&
-	head -n 9 msg >>file &&
-	git add file &&
-	test_tick &&
-	git commit -m "copied stuff" &&
 	git am -3 -p0 lorem-zero.patch &&
 	test_path_is_missing .git/rebase-apply &&
 	git diff --exit-code lorem
@@ -325,11 +324,6 @@ test_expect_success 'am -3 can rename a file after falling back to 3-way merge'
 
 test_expect_success 'am -3 -q is quiet' '
 	setup_temporary_branch lorem2 &&
-	sed -n -e "3,\$p" msg >file &&
-	head -n 9 msg >>file &&
-	git add file &&
-	test_tick &&
-	git commit -m "copied stuff" &&
 	git am -3 -q lorem-move.patch >output.out 2>&1 &&
 	! test -s output.out
 '
-- 
1.9.1

[PATCH 3/3] git-am: add am.threeWay config variable

[Remi Lespinet]

Add the am.threeWay configuration variable to use the -3 or --3way
option of git am by default. When am.threeway is set and not desired
for a specific git am command, the --no-3way option can be used to
override it.

Signed-off-by: Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr>
---
 Even if git am will be rewritten soon, the code that will have to be
 ported is not the most important part of the patch and the tests and
 documentation parts can be reused.

 Documentation/config.txt |  7 +++++++
 Documentation/git-am.txt |  6 ++++--
 git-am.sh                |  7 +++++++
 t/t4150-am.sh            | 15 +++++++++++++++
 4 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d44bc85..8e42752 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -769,6 +769,13 @@ am.keepcr::
 	by giving '--no-keep-cr' from the command line.
 	See linkgit:git-am[1], linkgit:git-mailsplit[1].
 
+am.threeWay::
+	If true, git-am will fall back on 3-way merge when the patch
+	cannot be applied cleanly, in the same way as the '-3' or
+	'--3-way' option. Can be overridden by giving '--no-3-way'
+	from the command line.
+	See linkgit:git-am[1].
+
 apply.ignoreWhitespace::
 	When set to 'change', tells 'git apply' to ignore changes in
 	whitespace, in the same way as the '--ignore-space-change'
diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index 0d8ba48..3190c05 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -89,11 +89,13 @@ default.   You can use `--no-utf8` to override this.
 	linkgit:git-mailinfo[1]).
 
 -3::
---3way::
+--[no-]3way::
 	When the patch does not apply cleanly, fall back on
 	3-way merge if the patch records the identity of blobs
 	it is supposed to apply to and we have those blobs
-	available locally.
+	available locally.  `am.threeWay` configuration variable
+	can be used to specify the default behaviour.  `--no-3way`
+	is useful to override `am.threeWay`.
 
 --ignore-space-change::
 --ignore-whitespace::
diff --git a/git-am.sh b/git-am.sh
index 761befb..781507c 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -389,6 +389,11 @@ then
     keepcr=t
 fi
 
+if test "$(git config --bool --get am.threeWay)" = true
+then
+    threeway=t
+fi
+
 while test $# != 0
 do
 	case "$1" in
@@ -400,6 +405,8 @@ it will be removed. Please do not use it anymore."
 		;;
 	-3|--3way)
 		threeway=t ;;
+	--no-3way)
+		threeway=f ;;
 	-s|--signoff)
 		sign=t ;;
 	-u|--utf8)
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 8f85098..e16ef0e 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -288,6 +288,21 @@ test_expect_success 'am -3 falls back to 3-way merge' '
 	git diff --exit-code lorem
 '
 
+test_expect_success 'am with config am.threeWay falls back to 3-way merge' '
+	setup_temporary_branch lorem2 &&
+	test_config am.threeWay 1 &&
+	git am lorem-move.patch &&
+	test_path_is_missing .git/rebase-apply &&
+	git diff --exit-code lorem
+'
+
+test_expect_success 'am with config am.threeWay overridden by --no-3way' '
+	setup_temporary_branch lorem2 &&
+	test_config am.threeWay 1 &&
+	test_must_fail git am --no-3way lorem-move.patch &&
+	test_path_is_dir .git/rebase-apply
+'
+
 test_expect_success 'am -3 -p0 can read --no-prefix patch' '
 	setup_temporary_branch lorem2 &&
 	git am -3 -p0 lorem-zero.patch &&
-- 
1.9.1

Re: [PATCH 1/3] t4150-am: refactor and clean common setup

[Paul Tan]

Hi,

Take these comments/suggestions with a pinch of salt because I'm not
that experienced with the code base as well ;-).

On Wed, May 27, 2015 at 5:32 AM, Remi Lespinet
<remi.lespinet@ensimag.grenoble-inp.fr> wrote:
> Add new functions to keep the setup cleaner:
>  - setup_temporary_branch: creates a new branch, check it out
>    and automatically delete it after the test is over

Agreed. This removes a lot of boilerplate from the tests. Another
positive effect is that we can be sure that any commits made on that
throwaway branch will not affect the other parts of the test suite,
which makes understanding and editing the test suite much easier.

>  - setup_fixed_branch: creates a fixed branch, which can be re-used
>    in later tests

Looking at the patch, I see that setup_fixed_branch() is only used in
2 places, so I don't think it is a common pattern that would require
its own function. Also, see below.

> Signed-off-by: Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr>
> ---
>  t/t4150-am.sh | 138 ++++++++++++++++++++--------------------------------------
>  1 file changed, 47 insertions(+), 91 deletions(-)
>
> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
> index 306e6f3..8370951 100755
> --- a/t/t4150-am.sh
> +++ b/t/t4150-am.sh
> @@ -4,6 +4,20 @@ test_description='git am running'
>
>  . ./test-lib.sh
>
> +setup_temporary_branch () {

Maybe name this checkout_temp_branch() or something, since it more or
less is just a wrapper around git-checkout.

> +       tmp_name=${2-"temporary"}

I don't think the quotes are required. Also, I don't feel good about
swapping the order of the arguments to git-checkout. (or making $2 an
optional argument). As the patch stands, if I see

setup_temporary_branch lorem

I will think: so we are creating a temporary branch "lorem". But nope,
we are creating a temporary branch "temporary" that branches from
"lorem". I don't think writing setup_temporary_branch "temporary"
"lorem" explicitly is that bad.

This is just personal preference though.

> +       git reset --hard &&
> +       rm -fr .git/rebase-apply &&
> +       test_when_finished "git checkout $1 && git branch -D $tmp_name" &&

I think you should use "git checkout -f $1" as if the working tree is
dirty the test will fail at cleanup with no error message at all,
which is annoying for debugging. Furthermore, the test_when_finished
should be shifted below the git checkout -b below, as git branch -D
will fail if the branch does not exist.

> +       git checkout -b "$tmp_name" "$1"

If you use git checkout -f here then there's no need for the git reset
--hard above.

> +}
> +
> +setup_fixed_branch () {
> +       git reset --hard &&
> +       rm -fr .git/rebase-apply &&
> +       git checkout -b "$1" "$2"

Again, by using git checkout -f we can drop the git reset --hard. But
that reduces the function to 2 lines, and thus I don't think that this
usage pattern needs its own function.

> +}
> +
>  test_expect_success 'setup: messages' '
>         cat >msg <<-\EOF &&
>         second
> @@ -143,9 +157,7 @@ test_expect_success setup '
>  '

I haven't looked at the rest of the patch in detail because I'm not
that well-acquainted with t4150 yet, but it looks okay.

Thanks,
Paul

Re: [PATCH 3/3] git-am: add am.threeWay config variable

[Paul Tan]

Hi,

On Wed, May 27, 2015 at 5:32 AM, Remi Lespinet
<remi.lespinet@ensimag.grenoble-inp.fr> wrote:
> Add the am.threeWay configuration variable to use the -3 or --3way
> option of git am by default. When am.threeway is set and not desired
> for a specific git am command, the --no-3way option can be used to
> override it.
>
> Signed-off-by: Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr>
> ---
>  Even if git am will be rewritten soon, the code that will have to be
>  ported is not the most important part of the patch and the tests and
>  documentation parts can be reused.

Yup, there's no problem on my side.

>  Documentation/config.txt |  7 +++++++
>  Documentation/git-am.txt |  6 ++++--
>  git-am.sh                |  7 +++++++
>  t/t4150-am.sh            | 15 +++++++++++++++
>  4 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index d44bc85..8e42752 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -769,6 +769,13 @@ am.keepcr::
>         by giving '--no-keep-cr' from the command line.
>         See linkgit:git-am[1], linkgit:git-mailsplit[1].
>
> +am.threeWay::
> +       If true, git-am will fall back on 3-way merge when the patch
> +       cannot be applied cleanly, in the same way as the '-3' or
> +       '--3-way' option. Can be overridden by giving '--no-3-way'
> +       from the command line.
> +       See linkgit:git-am[1].
> +

Maybe we should start by mentioning the default behavior for git-am.
Something like,

By default, git-am will fail if the patch does not apply cleanly. When
set to true, this setting tells git-am to fall back on 3-way merge if
the patch records the identity of blobs it is supposed to apply to and
we have those blobs available locally (equivalent to giving the --3way
option from the command line).

(I stole most of the text from the git-am and git-config documentation)

>  apply.ignoreWhitespace::
>         When set to 'change', tells 'git apply' to ignore changes in
>         whitespace, in the same way as the '--ignore-space-change'
> diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
> index 0d8ba48..3190c05 100644
> --- a/Documentation/git-am.txt
> +++ b/Documentation/git-am.txt
> @@ -89,11 +89,13 @@ default.   You can use `--no-utf8` to override this.
>         linkgit:git-mailinfo[1]).
>
>  -3::
> ---3way::
> +--[no-]3way::

There's no need to mention --no-3way, it's assumed that most of the
options have the equivalent --no-option because it is implemented
automatically by the option parser.

>         When the patch does not apply cleanly, fall back on
>         3-way merge if the patch records the identity of blobs
>         it is supposed to apply to and we have those blobs
> -       available locally.
> +       available locally.  `am.threeWay` configuration variable
> +       can be used to specify the default behaviour.  `--no-3way`
> +       is useful to override `am.threeWay`.

Usually configuration settings are mentioned in a separate section in
the documentation "CONFIGURATION" (or not mentioned at all). Also,
there's no need to mention that --no-3way can be used to mention the
configuration, as its usual (and expected) that the configuration
value sets the default behavior, and the command-line switch can
override i.

>  --ignore-space-change::
>  --ignore-whitespace::
> diff --git a/git-am.sh b/git-am.sh
> index 761befb..781507c 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -389,6 +389,11 @@ then
>      keepcr=t
>  fi
>
> +if test "$(git config --bool --get am.threeWay)" = true
> +then
> +    threeway=t
> +fi
> +
>  while test $# != 0
>  do
>         case "$1" in
> @@ -400,6 +405,8 @@ it will be removed. Please do not use it anymore."
>                 ;;
>         -3|--3way)
>                 threeway=t ;;
> +       --no-3way)
> +               threeway=f ;;

Okay.

This patch on its own, though, does not handle the case where:

1. am.threeWay is set to "true"
2. we passed "--no-3way" in the command-line
3. git-am failed to apply a patch and quit
4. We git-am --skip or git-am --continue. When continuing, git-am will
not remember the --no-3way (unless we provide it again on the command
line)

They key is to tweak the following lines where the "threeway" setting is loaded:

    if test "$(cat "$dotest/threeway")" = t
    then
        threeway=t
    fi

To something like

    if test "$(cat "$dotest/threeway")" = t
    then
        threeway=t
    else
        threeway=f
    fi

>         -s|--signoff)
>                 sign=t ;;
>         -u|--utf8)
> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
> index 8f85098..e16ef0e 100755
> --- a/t/t4150-am.sh
> +++ b/t/t4150-am.sh
> @@ -288,6 +288,21 @@ test_expect_success 'am -3 falls back to 3-way merge' '
>         git diff --exit-code lorem
>  '
>
> +test_expect_success 'am with config am.threeWay falls back to 3-way merge' '
> +       setup_temporary_branch lorem2 &&
> +       test_config am.threeWay 1 &&
> +       git am lorem-move.patch &&
> +       test_path_is_missing .git/rebase-apply &&
> +       git diff --exit-code lorem
> +'
> +
> +test_expect_success 'am with config am.threeWay overridden by --no-3way' '
> +       setup_temporary_branch lorem2 &&
> +       test_config am.threeWay 1 &&
> +       test_must_fail git am --no-3way lorem-move.patch &&
> +       test_path_is_dir .git/rebase-apply
> +'
> +
>  test_expect_success 'am -3 -p0 can read --no-prefix patch' '
>         setup_temporary_branch lorem2 &&
>         git am -3 -p0 lorem-zero.patch &&

This looks good.

> --
> 1.9.1

To end off, some off-tangent issues that are not related to the patch
series in question, but since I'm looking at git-am.sh....

I've noticed that in the block above that initializes all the variables,

    sign= utf8=t keep= keepcr= skip= interactive= resolved= rebasing= abort=
    messageid= resolvemsg= resume= scissors= no_inbody_headers=
    git_apply_opt=
    committer_date_is_author_date=
    ignore_date=
    allow_rerere_autoupdate=
    gpg_sign_opt=

threeway is not initialized at all, and thus I think running
"threeway=t git am blah" will affect the behavior of git-am.

Also, I noticed that we do not check for --no-interactive,
--no-signoff, --no-keep, --no-whitespace, etc.

Thanks,
Paul

Re: [PATCH 3/3] git-am: add am.threeWay config variable

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

>> diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
>> index 0d8ba48..3190c05 100644
>> --- a/Documentation/git-am.txt
>> +++ b/Documentation/git-am.txt
>> @@ -89,11 +89,13 @@ default.   You can use `--no-utf8` to override this.
>>         linkgit:git-mailinfo[1]).
>>
>>  -3::
>> ---3way::
>> +--[no-]3way::
>
> There's no need to mention --no-3way,...

Actually, we prefer to do it this way:

	-3::
	--3way::
	--no-3way::
		Describe what --3way does here.


$ git grep -e '^--no-' -e '^--\[no-\]' Documentation/




>>         When the patch does not apply cleanly, fall back on
>>         3-way merge if the patch records the identity of blobs
>>         it is supposed to apply to and we have those blobs
>> -       available locally.
>> +       available locally.  `am.threeWay` configuration variable
>> +       can be used to specify the default behaviour.  `--no-3way`
>> +       is useful to override `am.threeWay`.
>
> Usually configuration settings are mentioned in a separate section in
> the documentation "CONFIGURATION" (or not mentioned at all).

I can go either way, actually.  But if the description mentions
am.threeWay as a way to tweak the default, it also should spell out
the default when the configuration is not there at all.

> Also, there's no need to mention that --no-3way can be used to
> mention the configuration, as its usual (and expected) that the
> configuration value sets the default behavior, and the
> command-line switch can override i.

Yes.  Also --3way is useful to override `am.threeWay` set to `false` ;-)

> To end off, some off-tangent issues that are not related to the patch
> series in question, but since I'm looking at git-am.sh....
>
> I've noticed that in the block above that initializes all the variables,
>
>     sign= utf8=t keep= keepcr= skip= interactive= resolved= rebasing= abort=
>     messageid= resolvemsg= resume= scissors= no_inbody_headers=
>     git_apply_opt=
>     committer_date_is_author_date=
>     ignore_date=
>     allow_rerere_autoupdate=
>     gpg_sign_opt=
>
> threeway is not initialized at all, and thus I think running
> "threeway=t git am blah" will affect the behavior of git-am.

Correct.  I overlooked this when I originally did threeway.  Perhaps
a preparatory bugfix patch is warranted before this one.

> Also, I noticed that we do not check for --no-interactive,
> --no-signoff, --no-keep, --no-whitespace, etc.

Even though adding support for them would not hurt, lack of these
are OK, as long as we do not have configuration variables to tweak
their defaults.

Re: [PATCH 1/3] t4150-am: refactor and clean common setup

[Eric Sunshine]

On Thu, May 28, 2015 at 9:10 AM, Paul Tan <pyokagan@gmail.com> wrote:
> Take these comments/suggestions with a pinch of salt because I'm not
> that experienced with the code base as well ;-).

I agree with pretty much all of your review comments. See below for a
minor addenda.

> On Wed, May 27, 2015 at 5:32 AM, Remi Lespinet
> <remi.lespinet@ensimag.grenoble-inp.fr> wrote:
>> +setup_temporary_branch () {
>
> Maybe name this checkout_temp_branch() or something, since it more or
> less is just a wrapper around git-checkout.

checkout_temp_branch() doesn't give any indication that a new branch
is being created, so it may not be an improvement over
setup_temporary_branch(). A more apt name might be something like
new_temp_branch().

>> +       tmp_name=${2-"temporary"}
>
> I don't think the quotes are required. Also, I don't feel good about
> swapping the order of the arguments to git-checkout. (or making $2 an
> optional argument). As the patch stands, if I see
>
> setup_temporary_branch lorem
>
> I will think: so we are creating a temporary branch "lorem". But nope,
> we are creating a temporary branch "temporary" that branches from
> "lorem". I don't think writing setup_temporary_branch "temporary"
> "lorem" explicitly is that bad.

In fact, the second argument is never used in any of the three
patches, so it seems to be wasted functionality (at this time). If so,
then an even more appropriate name might be new_temp_branch_from().
Clearly, then:

    new_temp_branch_from lorem

creates a throw-away branch based upon 'lorem'.

Re: [PATCH 1/3] t4150-am: refactor and clean common setup

[Eric Sunshine]

On Tue, May 26, 2015 at 5:32 PM, Remi Lespinet
<remi.lespinet@ensimag.grenoble-inp.fr> wrote:
> Add new functions to keep the setup cleaner:
>  - setup_temporary_branch: creates a new branch, check it out
>    and automatically delete it after the test is over
>  - setup_fixed_branch: creates a fixed branch, which can be re-used
>    in later tests

See below for review comments beyond those already provided by Paul...

> Signed-off-by: Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr>
> ---
> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
> index 306e6f3..8370951 100755
> --- a/t/t4150-am.sh
> +++ b/t/t4150-am.sh
> @@ -4,6 +4,20 @@ test_description='git am running'
>
>  . ./test-lib.sh
>
> +setup_temporary_branch () {
> +       tmp_name=${2-"temporary"}
> +       git reset --hard &&
> +       rm -fr .git/rebase-apply &&
> +       test_when_finished "git checkout $1 && git branch -D $tmp_name" &&
> +       git checkout -b "$tmp_name" "$1"
> +}
> +
> +setup_fixed_branch () {
> +       git reset --hard &&
> +       rm -fr .git/rebase-apply &&
> +       git checkout -b "$1" "$2"
> +}
> +
>  test_expect_success 'setup: messages' '
>         cat >msg <<-\EOF &&
>         second
> @@ -143,9 +157,7 @@ test_expect_success setup '
>  '
>
>  test_expect_success 'am applies patch correctly' '
> -       rm -fr .git/rebase-apply &&
> -       git reset --hard &&
> -       git checkout first &&
> +       setup_temporary_branch first &&

This is confusing. The commit message seems to advertise this patch as
primarily a refactoring change (pulling boilerplate out of tests and
into a new function), but the operation of that new function is
surprisingly different from the boilerplate it's replacing: The old
code did not create a branch, the new function does.

If your intention really is to create new branches in tests which
previously did not need throwaway branches, then the commit message
should state that as its primary purpose and explain why doing so is
desirable, since it is not clear from this patch what you gain from
doing so.

Moreover, typically, you should only either refactor or change
behavior in a single patch, not both. For instance, patch 1 would add
the new function and update tests to call it in place of the
boilerplate; and patch 2 would change behavior (such as creating a
temporary branch).

On the other hand, if these tests really don't benefit from having a
throw-away branch, then this change seems suspect and obscures the
intent of the test rather than clarifying or simplifying.

>         test_tick &&
>         git am <patch1 &&
>         test_path_is_missing .git/rebase-apply &&
> @@ -275,9 +273,7 @@ test_expect_success 'am --keep-non-patch really keeps the non-patch part' '
>  '
>
>  test_expect_success 'am -3 falls back to 3-way merge' '
> -       rm -fr .git/rebase-apply &&
> -       git reset --hard &&
> -       git checkout -b lorem2 master2 &&
> +       setup_fixed_branch lorem2 master2 &&
>         sed -n -e "3,\$p" msg >file &&
>         head -n 9 msg >>file &&
>         git add file &&
> @@ -289,9 +285,7 @@ test_expect_success 'am -3 falls back to 3-way merge' '
>  '
>
>  test_expect_success 'am -3 -p0 can read --no-prefix patch' '
> -       rm -fr .git/rebase-apply &&
> -       git reset --hard &&
> -       git checkout -b lorem3 master2 &&
> +       setup_temporary_branch lorem2 &&

Unlike the test prior to this one which creates a "fixed" branch (via
setup_fixed_branch) named 'lorem2' which is used by other tests, the
'lorem3' branch in this test is never used elsewhere, so
setup_temporary_branch is used instead. Makes sense.

>         sed -n -e "3,\$p" msg >file &&
>         head -n 9 msg >>file &&
>         git add file &&
> @@ -303,10 +297,8 @@ test_expect_success 'am -3 -p0 can read --no-prefix patch' '
>  '
>
>  test_expect_success 'am can rename a file' '
> +       setup_temporary_branch lorem &&
>         grep "^rename from" rename.patch &&
> -       rm -fr .git/rebase-apply &&
> -       git reset --hard &&
> -       git checkout lorem^0 &&

In other cases, you insert the setup_temporary_branch() invocation
where the old code resided. Why the difference in this test (and
others following)?

>         git am rename.patch &&
>         test_path_is_missing .git/rebase-apply &&
>         git update-index --refresh &&
> @@ -349,11 +335,9 @@ test_expect_success 'am -3 -q is quiet' '
>  '
>
>  test_expect_success 'am pauses on conflict' '
> -       rm -fr .git/rebase-apply &&
> -       git reset --hard &&
> -       git checkout lorem2^^ &&
> +       setup_temporary_branch lorem2^^ &&
>         test_must_fail git am lorem-move.patch &&
> -       test -d .git/rebase-apply
> +       test_path_is_dir .git/rebase-apply

Sneaking in unrelated change. This sort of cleanup should go in a
patch of its own.

>  '
>
>  test_expect_success 'am --skip works' '

Re: [PATCH 1/3] t4150-am: refactor and clean common setup

[Eric Sunshine]

On Thu, May 28, 2015 at 3:09 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, May 26, 2015 at 5:32 PM, Remi Lespinet
> <remi.lespinet@ensimag.grenoble-inp.fr> wrote:
>> +setup_temporary_branch () {
>> +       tmp_name=${2-"temporary"}

I forgot to mention the broken &&-chain here. Although the missing &&
doesn't actively hurt the function today, someone may someday insert
code above the 'tmp_name=' line without noticing the lack of &&, and
the test won't notice a failure in that newly added code. Thus, it's
better to keep the &&-chain intact throughout.

>> +       git reset --hard &&
>> +       rm -fr .git/rebase-apply &&
>> +       test_when_finished "git checkout $1 && git branch -D $tmp_name" &&
>> +       git checkout -b "$tmp_name" "$1"
>> +}

Re: [PATCH 3/3] git-am: add am.threeWay config variable

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

>> I've noticed that in the block above that initializes all the variables,
>>
>>     sign= utf8=t keep= keepcr= skip= interactive= resolved= rebasing= abort=
>>     messageid= resolvemsg= resume= scissors= no_inbody_headers=
>>     git_apply_opt=
>>     committer_date_is_author_date=
>>     ignore_date=
>>     allow_rerere_autoupdate=
>>     gpg_sign_opt=
>>
>> threeway is not initialized at all, and thus I think running
>> "threeway=t git am blah" will affect the behavior of git-am.
>
> Correct.  I overlooked this when I originally did threeway.  Perhaps
> a preparatory bugfix patch is warranted before this one.
>
>> Also, I noticed that we do not check for --no-interactive,
>> --no-signoff, --no-keep, --no-whitespace, etc.
>
> Even though adding support for them would not hurt, lack of these
> are OK, as long as we do not have configuration variables to tweak
> their defaults.

I wouldn't worry about these issues, as the port to C will make them
disappear anyway.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH 1/3] t4150-am: refactor and clean common setup

[Remi LESPINET]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Paul Tan <pyokagan@gmail.com> writes:
>
> >> Remi LESPINET <remi.lespinet@ensimag.grenoble-inp.fr> writes:
> >> +       tmp_name=${2-"temporary"}
> >
> > I don't think the quotes are required. Also, I don't feel good about
> > swapping the order of the arguments to git-checkout. (or making $2 an
> > optional argument). As the patch stands, if I see
> >
> > setup_temporary_branch lorem
> >
> > I will think: so we are creating a temporary branch "lorem". But nope,
> > we are creating a temporary branch "temporary" that branches from
> > "lorem". I don't think writing setup_temporary_branch "temporary"
> > "lorem" explicitly is that bad.
> 
> In fact, the second argument is never used in any of the three
> patches, so it seems to be wasted functionality (at this time). If so,
> then an even more appropriate name might be new_temp_branch_from().
> Clearly, then:
> 
>     new_temp_branch_from lorem

Considering your two comments about the name of the function I think
removing the possibility of using the second argument and renaming the
function new_temp_branch_from gets everyone to agree since it has not
the possible confusion with git-checkout problem.

> This is confusing. The commit message seems to advertise this patch as
> primarily a refactoring change (pulling boilerplate out of tests and
> into a new function), but the operation of that new function is
> surprisingly different from the boilerplate it's replacing: The old
> code did not create a branch, the new function does.
> 
> If your intention really is to create new branches in tests which
> previously did not need throwaway branches, then the commit message
> should state that as its primary purpose and explain why doing so is
> desirable, since it is not clear from this patch what you gain from
> doing so.
> 
> Moreover, typically, you should only either refactor or change
> behavior in a single patch, not both. For instance, patch 1 would add
> the new function and update tests to call it in place of the
> boilerplate; and patch 2 would change behavior (such as creating a
> temporary branch).
> 
> On the other hand, if these tests really don't benefit from having a
> throw-away branch, then this change seems suspect and obscures the
> intent of the test rather than clarifying or simplifying.

The purpose of this patch was originally to eliminate some test
dependancies introduced by the checkouts relative to HEAD (which
caused "cascade failure") and avoid creating branches, whose name
can't be reused, but you're right, that may not benefit as much as I
expected...  Perhaps I should have make tags from branches used as
start points, which would have done the same effect as these temporary
branches.

> >         sed -n -e "3,\$p" msg >file &&
> >         head -n 9 msg >>file &&
> >         git add file &&
> > @@ -303,10 +297,8 @@ test_expect_success 'am -3 -p0 can read --no-prefix patch' '
> >  '
> >
> >  test_expect_success 'am can rename a file' '
> > +       setup_temporary_branch lorem &&
> >         grep "^rename from" rename.patch &&
> > -       rm -fr .git/rebase-apply &&
> > -       git reset --hard &&
> > -       git checkout lorem^0 &&
> 
> In other cases, you insert the setup_temporary_branch() invocation
> where the old code resided. Why the difference in this test (and
> others following)?

This doesn't affect the result of the test (assuming
setup_temporary_branch doesn't fail). I preferred to add the setup
before anything else in the test.  It affects efficiency in case the
rename patch has not the correct syntax. So it may be better to put the
grep as the first instruction to avoid evaluating all the test in case
the syntax of the patch is not correct.

Thanks a lot for your comments, I'll correct the patch asap !