[PATCH 0/5] apply: fix merge.conflictStyle bug in --3way

[PATCH 0/5] apply: fix merge.conflictStyle bug in --3way

[Denton Liu]

This fixes a bug where even if `merge.conflictStyle = diff3`, running
`git apply --3way` would not output the base.

Patches 1-3 are general cleanup for t4108, 4 demonstrates the bug and 5
actually fixes it.

Denton Liu (5):
  t4108: replace create_file with test_write_lines
  t4108: remove git command upstream of pipe
  t4108: use `test_config` instead of `git config`
  t4108: demonstrate bug in apply
  apply: respect merge.conflictStyle in --3way

 apply.c                   |  2 +-
 t/t4108-apply-threeway.sh | 96 +++++++++++++++++++++------------------
 2 files changed, 53 insertions(+), 45 deletions(-)

-- 
2.24.0.rc0.197.g0926ab8072

[PATCH 1/5] t4108: replace create_file with test_write_lines

[Denton Liu]

Since the locally defined create_file() duplicates the functionality of
the test_write_lines() helper function, remove create_file() and replace
all instances with test_write_lines(). While we're at it, move
redirection operators to the end of the command which is the more
conventional place to put it.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t4108-apply-threeway.sh | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
index fa5d4efb89..b109ecbd9f 100755
--- a/t/t4108-apply-threeway.sh
+++ b/t/t4108-apply-threeway.sh
@@ -4,13 +4,6 @@ test_description='git apply --3way'
 
 . ./test-lib.sh
 
-create_file () {
-	for i
-	do
-		echo "$i"
-	done
-}
-
 sanitize_conflicted_diff () {
 	sed -e '
 		/^index /d
@@ -20,7 +13,7 @@ sanitize_conflicted_diff () {
 
 test_expect_success setup '
 	test_tick &&
-	create_file >one 1 2 3 4 5 6 7 &&
+	test_write_lines 1 2 3 4 5 6 7 >one &&
 	cat one >two &&
 	git add one two &&
 	git commit -m initial &&
@@ -28,13 +21,13 @@ test_expect_success setup '
 	git branch side &&
 
 	test_tick &&
-	create_file >one 1 two 3 4 5 six 7 &&
-	create_file >two 1 two 3 4 5 6 7 &&
+	test_write_lines 1 two 3 4 5 six 7 >one &&
+	test_write_lines 1 two 3 4 5 6 7 >two &&
 	git commit -a -m master &&
 
 	git checkout side &&
-	create_file >one 1 2 3 4 five 6 7 &&
-	create_file >two 1 2 3 4 five 6 7 &&
+	test_write_lines 1 2 3 4 five 6 7 >one &&
+	test_write_lines 1 2 3 4 five 6 7 >two &&
 	git commit -a -m side &&
 
 	git checkout master
@@ -87,7 +80,7 @@ test_expect_success 'apply with --3way with rerere enabled' '
 	test_must_fail git merge --no-commit side &&
 
 	# Manually resolve and record the resolution
-	create_file 1 two 3 4 five six 7 >one &&
+	test_write_lines 1 two 3 4 five six 7 >one &&
 	git rerere &&
 	cat one >expect &&
 
@@ -104,14 +97,14 @@ test_expect_success 'apply -3 with add/add conflict setup' '
 	git reset --hard &&
 
 	git checkout -b adder &&
-	create_file 1 2 3 4 5 6 7 >three &&
-	create_file 1 2 3 4 5 6 7 >four &&
+	test_write_lines 1 2 3 4 5 6 7 >three &&
+	test_write_lines 1 2 3 4 5 6 7 >four &&
 	git add three four &&
 	git commit -m "add three and four" &&
 
 	git checkout -b another adder^ &&
-	create_file 1 2 3 4 5 6 7 >three &&
-	create_file 1 2 3 four 5 6 7 >four &&
+	test_write_lines 1 2 3 4 5 6 7 >three &&
+	test_write_lines 1 2 3 four 5 6 7 >four &&
 	git add three four &&
 	git commit -m "add three and four" &&
 
-- 
2.24.0.rc0.197.g0926ab8072

[PATCH 2/5] t4108: remove git command upstream of pipe

[Denton Liu]

Before, the output of `git diff HEAD` would always be piped to
sanitize_conflicted_diff(). However, since the Git command was upstream
of the pipe, in case the Git command fails, the return code would be
lost. Rewrite into separate statements so that the return code is no
longer lost.

Since only the command `git diff HEAD` was being piped to
sanitize_conflicted_diff(), move the command into the function and rename
it to print_sanitized_diff().

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t4108-apply-threeway.sh | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
index b109ecbd9f..49739ce8b4 100755
--- a/t/t4108-apply-threeway.sh
+++ b/t/t4108-apply-threeway.sh
@@ -4,11 +4,12 @@ test_description='git apply --3way'
 
 . ./test-lib.sh
 
-sanitize_conflicted_diff () {
+print_sanitized_diff () {
+	git diff HEAD >diff.raw &&
 	sed -e '
 		/^index /d
 		s/^\(+[<>][<>][<>][<>]*\) .*/\1/
-	'
+	' diff.raw
 }
 
 test_expect_success setup '
@@ -54,14 +55,14 @@ test_expect_success 'apply with --3way' '
 	git checkout master^0 &&
 	test_must_fail git merge --no-commit side &&
 	git ls-files -s >expect.ls &&
-	git diff HEAD | sanitize_conflicted_diff >expect.diff &&
+	print_sanitized_diff >expect.diff &&
 
 	# should fail to apply
 	git reset --hard &&
 	git checkout master^0 &&
 	test_must_fail git apply --index --3way P.diff &&
 	git ls-files -s >actual.ls &&
-	git diff HEAD | sanitize_conflicted_diff >actual.diff &&
+	print_sanitized_diff >actual.diff &&
 
 	# The result should resemble the corresponding merge
 	test_cmp expect.ls actual.ls &&
@@ -114,7 +115,7 @@ test_expect_success 'apply -3 with add/add conflict setup' '
 	git checkout adder^0 &&
 	test_must_fail git merge --no-commit another &&
 	git ls-files -s >expect.ls &&
-	git diff HEAD | sanitize_conflicted_diff >expect.diff
+	print_sanitized_diff >expect.diff
 '
 
 test_expect_success 'apply -3 with add/add conflict' '
@@ -124,7 +125,7 @@ test_expect_success 'apply -3 with add/add conflict' '
 	test_must_fail git apply --index --3way P.diff &&
 	# ... and leave conflicts in the index and in the working tree
 	git ls-files -s >actual.ls &&
-	git diff HEAD | sanitize_conflicted_diff >actual.diff &&
+	print_sanitized_diff >actual.diff &&
 
 	# The result should resemble the corresponding merge
 	test_cmp expect.ls actual.ls &&
-- 
2.24.0.rc0.197.g0926ab8072

[PATCH 3/5] t4108: use `test_config` instead of `git config`

[Denton Liu]

Since `git config` leaves the configurations set even after the test
case completes, use `test_config` instead so that the configurations are
reset once the test case finishes.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t4108-apply-threeway.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
index 49739ce8b4..3615256492 100755
--- a/t/t4108-apply-threeway.sh
+++ b/t/t4108-apply-threeway.sh
@@ -70,7 +70,7 @@ test_expect_success 'apply with --3way' '
 '
 
 test_expect_success 'apply with --3way with rerere enabled' '
-	git config rerere.enabled true &&
+	test_config rerere.enabled true &&
 
 	# Merging side should be similar to applying this patch
 	git diff ...side >P.diff &&
-- 
2.24.0.rc0.197.g0926ab8072

[PATCH 4/5] t4108: demonstrate bug in apply

[Denton Liu]

Currently, apply does not respect the merge.conflictStyle setting.
Demonstrate this by making the 'apply with --3way' test case generic and
extending it to show that the configuration of
merge.conflictStyle = diff3 causes a breakage.

Change print_sanitized_diff() to also sanitize `|||||||` conflict markers.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t4108-apply-threeway.sh | 58 ++++++++++++++++++++++++---------------
 1 file changed, 36 insertions(+), 22 deletions(-)

diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
index 3615256492..84347fc178 100755
--- a/t/t4108-apply-threeway.sh
+++ b/t/t4108-apply-threeway.sh
@@ -8,7 +8,7 @@ print_sanitized_diff () {
 	git diff HEAD >diff.raw &&
 	sed -e '
 		/^index /d
-		s/^\(+[<>][<>][<>][<>]*\) .*/\1/
+		s/^\(+[<>|][<>|][<>|][<>|]*\) .*/\1/
 	' diff.raw
 }
 
@@ -46,28 +46,42 @@ test_expect_success 'apply without --3way' '
 	git diff-index --exit-code --cached HEAD
 '
 
-test_expect_success 'apply with --3way' '
-	# Merging side should be similar to applying this patch
-	git diff ...side >P.diff &&
-
-	# The corresponding conflicted merge
-	git reset --hard &&
-	git checkout master^0 &&
-	test_must_fail git merge --no-commit side &&
-	git ls-files -s >expect.ls &&
-	print_sanitized_diff >expect.diff &&
-
-	# should fail to apply
-	git reset --hard &&
-	git checkout master^0 &&
-	test_must_fail git apply --index --3way P.diff &&
-	git ls-files -s >actual.ls &&
-	print_sanitized_diff >actual.diff &&
+test_apply_with_3way () {
+	status="$1" &&
+	shift &&
+	description="$1" &&
+	shift &&
+	preamble="$1" &&
+	shift &&
+
+	test_expect_$status "apply with --3way ($description)" "
+		$preamble &&
+
+		# Merging side should be similar to applying this patch
+		git diff ...side >P.diff &&
+
+		# The corresponding conflicted merge
+		git reset --hard &&
+		git checkout master^0 &&
+		test_must_fail git merge --no-commit side &&
+		git ls-files -s >expect.ls &&
+		print_sanitized_diff >expect.diff &&
+
+		# should fail to apply
+		git reset --hard &&
+		git checkout master^0 &&
+		test_must_fail git apply --index --3way P.diff &&
+		git ls-files -s >actual.ls &&
+		print_sanitized_diff >actual.diff &&
+
+		# The result should resemble the corresponding merge
+		test_cmp expect.ls actual.ls &&
+		test_cmp expect.diff actual.diff
+	"
+}
 
-	# The result should resemble the corresponding merge
-	test_cmp expect.ls actual.ls &&
-	test_cmp expect.diff actual.diff
-'
+test_apply_with_3way success default true
+test_apply_with_3way failure 'merge.conflictStyle = diff3' 'test_config merge.conflictStyle diff3'
 
 test_expect_success 'apply with --3way with rerere enabled' '
 	test_config rerere.enabled true &&
-- 
2.24.0.rc0.197.g0926ab8072

[PATCH 5/5] apply: respect merge.conflictStyle in --3way

[Denton Liu]

Before, when doing a 3-way merge, the merge.conflictStyle option was not
respected and the "merge" style was always used, even if "diff3" was
specified.

Call git_xmerge_config() at the end of git_apply_config() so that the
merge.conflictStyle config is read.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 apply.c                   | 2 +-
 t/t4108-apply-threeway.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/apply.c b/apply.c
index f8a046a6a5..b9291f5f7b 100644
--- a/apply.c
+++ b/apply.c
@@ -32,7 +32,7 @@ static void git_apply_config(void)
 {
 	git_config_get_string_const("apply.whitespace", &apply_default_whitespace);
 	git_config_get_string_const("apply.ignorewhitespace", &apply_default_ignorewhitespace);
-	git_config(git_default_config, NULL);
+	git_config(git_xmerge_config, NULL);
 }
 
 static int parse_whitespace_option(struct apply_state *state, const char *option)
diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
index 84347fc178..0e4eeac083 100755
--- a/t/t4108-apply-threeway.sh
+++ b/t/t4108-apply-threeway.sh
@@ -81,7 +81,7 @@ test_apply_with_3way () {
 }
 
 test_apply_with_3way success default true
-test_apply_with_3way failure 'merge.conflictStyle = diff3' 'test_config merge.conflictStyle diff3'
+test_apply_with_3way success 'merge.conflictStyle = diff3' 'test_config merge.conflictStyle diff3'
 
 test_expect_success 'apply with --3way with rerere enabled' '
 	test_config rerere.enabled true &&
-- 
2.24.0.rc0.197.g0926ab8072

Re: [PATCH 2/5] t4108: remove git command upstream of pipe

[Eric Sunshine]

On Wed, Oct 23, 2019 at 8:04 AM Denton Liu <liu.denton@gmail.com> wrote:
> Before, the output of `git diff HEAD` would always be piped to
> sanitize_conflicted_diff(). However, since the Git command was upstream
> of the pipe, in case the Git command fails, the return code would be
> lost. Rewrite into separate statements so that the return code is no
> longer lost.
>
> Since only the command `git diff HEAD` was being piped to
> sanitize_conflicted_diff(), move the command into the function and rename
> it to print_sanitized_diff().
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
> @@ -4,11 +4,12 @@ test_description='git apply --3way'
> -sanitize_conflicted_diff () {
> +print_sanitized_diff () {
> +       git diff HEAD >diff.raw &&
>         sed -e '
>                 /^index /d
>                 s/^\(+[<>][<>][<>][<>]*\) .*/\1/
> -       '
> +       ' diff.raw
>  }

Nit: By hard-coding "HEAD" in this function, you lose the flexibility
of the original. An alternative would have been to accept the ref
against which to diff as an argument to this function:

    print_sanitized_diff () {
        git diff "$@" >diff.raw &&
        ...

Or, better yet, keep the original design and pass the diff in as the
shell function's input, so a caller would say:

    git diff HEAD >diff.raw &&
    sanitize_conflicted_diff <diff.raw >expect.diff &&

However, not necessarily worth a re-roll if we never expect anyone to
pass anything other than "HEAD".

Re: [PATCH 4/5] t4108: demonstrate bug in apply

[Eric Sunshine]

On Wed, Oct 23, 2019 at 8:04 AM Denton Liu <liu.denton@gmail.com> wrote:
> Currently, apply does not respect the merge.conflictStyle setting.
> Demonstrate this by making the 'apply with --3way' test case generic and
> extending it to show that the configuration of
> merge.conflictStyle = diff3 causes a breakage.
>
> Change print_sanitized_diff() to also sanitize `|||||||` conflict markers.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
> @@ -46,28 +46,42 @@ test_expect_success 'apply without --3way' '
> +test_apply_with_3way () {
> +       status="$1" &&
> +       shift &&
> +       description="$1" &&
> +       shift &&
> +       preamble="$1" &&
> +       shift &&
> +
> +       test_expect_$status "apply with --3way ($description)" "
> +               $preamble &&
> +
> +               # Merging side should be similar to applying this patch
> +               git diff ...side >P.diff &&
> +               [...]
> +       "
> +}
>
> +test_apply_with_3way success default true
> +test_apply_with_3way failure 'merge.conflictStyle = diff3' 'test_config merge.conflictStyle diff3'

This isn't the prettiest way to achieve this; the "success"/"failure"
arguments, especially, are somewhat ugly. A perhaps more palatable
alternative which gives you the same benefit of re-using the common
code but avoids the ugliness while still allowing customization (now
and in the future):

    test_apply_with_3way () {
        # Merging side should be similar to applying this patch
        git diff ...side >P.diff &&
        [...]
    }

    test_expect_success 'apply with --3way (default)' '
        test_apply_with_3way
    '

    test_expect_failure 'apply with --3way (merge.conflictStyle = diff3)' '
        test_config merge.conflictStyle diff3 &&
        test_apply_with_3way
    '

Re: [PATCH 2/5] t4108: remove git command upstream of pipe

[Denton Liu]

On Wed, Oct 23, 2019 at 09:32:26AM -0400, Eric Sunshine wrote:
> On Wed, Oct 23, 2019 at 8:04 AM Denton Liu <liu.denton@gmail.com> wrote:
> > Before, the output of `git diff HEAD` would always be piped to
> > sanitize_conflicted_diff(). However, since the Git command was upstream
> > of the pipe, in case the Git command fails, the return code would be
> > lost. Rewrite into separate statements so that the return code is no
> > longer lost.
> >
> > Since only the command `git diff HEAD` was being piped to
> > sanitize_conflicted_diff(), move the command into the function and rename
> > it to print_sanitized_diff().
> >
> > Signed-off-by: Denton Liu <liu.denton@gmail.com>
> > ---
> > diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
> > @@ -4,11 +4,12 @@ test_description='git apply --3way'
> > -sanitize_conflicted_diff () {
> > +print_sanitized_diff () {
> > +       git diff HEAD >diff.raw &&
> >         sed -e '
> >                 /^index /d
> >                 s/^\(+[<>][<>][<>][<>]*\) .*/\1/
> > -       '
> > +       ' diff.raw
> >  }
> 
> Nit: By hard-coding "HEAD" in this function, you lose the flexibility
> of the original. An alternative would have been to accept the ref
> against which to diff as an argument to this function:
> 
>     print_sanitized_diff () {
>         git diff "$@" >diff.raw &&
>         ...
> 
> Or, better yet, keep the original design and pass the diff in as the
> shell function's input, so a caller would say:
> 
>     git diff HEAD >diff.raw &&
>     sanitize_conflicted_diff <diff.raw >expect.diff &&
> 
> However, not necessarily worth a re-roll if we never expect anyone to
> pass anything other than "HEAD".

Since it doesn't really make sense to commmit conflicts, I decided to
hardcode it to be a diff against HEAD and the worktree since that's the
only sensible place where the conflict should live.

Speaking of conflicts, I dropped the "conflicted" part of the old
function name. I think that removes a lot of clarity so I'll reroll
renaming the function to print_sanitized_conflicted_diff() or something
like that.

[PATCH v2 0/5] This fixes a bug where even if `merge.conflictStyle = diff3`, running `git apply --3way` would not output the base.

[Denton Liu]

Patches 1-3 are general cleanup for t4108, 4 demonstrates the bug and 5
actually fixes it.

Changes since v1:

* Pretty up 4/5 using Eric's suggestion

* Rename print_sanitized_diff() to print_santized_conflicted_diff()

Denton Liu (5):
  t4108: replace create_file with test_write_lines
  t4108: remove git command upstream of pipe
  t4108: use `test_config` instead of `git config`
  t4108: demonstrate bug in apply
  apply: respect merge.conflictStyle in --3way

 apply.c                   |  2 +-
 t/t4108-apply-threeway.sh | 55 +++++++++++++++++++++------------------
 2 files changed, 30 insertions(+), 27 deletions(-)

Range-diff against v1:
1:  9d915748c1 ! 1:  fd1b770c0c t4108: remove git command upstream of pipe
    @@ Commit message
     
         Since only the command `git diff HEAD` was being piped to
         sanitize_conflicted_diff(), move the command into the function and rename
    -    it to print_sanitized_diff().
    +    it to print_sanitized_conflicted_diff().
     
      ## t/t4108-apply-threeway.sh ##
     @@ t/t4108-apply-threeway.sh: test_description='git apply --3way'
    @@ t/t4108-apply-threeway.sh: test_description='git apply --3way'
      . ./test-lib.sh
      
     -sanitize_conflicted_diff () {
    -+print_sanitized_diff () {
    ++print_sanitized_conflicted_diff () {
     +	git diff HEAD >diff.raw &&
      	sed -e '
      		/^index /d
    @@ t/t4108-apply-threeway.sh: test_expect_success 'apply with --3way' '
      	test_must_fail git merge --no-commit side &&
      	git ls-files -s >expect.ls &&
     -	git diff HEAD | sanitize_conflicted_diff >expect.diff &&
    -+	print_sanitized_diff >expect.diff &&
    ++	print_sanitized_conflicted_diff >expect.diff &&
      
      	# should fail to apply
      	git reset --hard &&
    @@ t/t4108-apply-threeway.sh: test_expect_success 'apply with --3way' '
      	test_must_fail git apply --index --3way P.diff &&
      	git ls-files -s >actual.ls &&
     -	git diff HEAD | sanitize_conflicted_diff >actual.diff &&
    -+	print_sanitized_diff >actual.diff &&
    ++	print_sanitized_conflicted_diff >actual.diff &&
      
      	# The result should resemble the corresponding merge
      	test_cmp expect.ls actual.ls &&
    @@ t/t4108-apply-threeway.sh: test_expect_success 'apply -3 with add/add conflict s
      	test_must_fail git merge --no-commit another &&
      	git ls-files -s >expect.ls &&
     -	git diff HEAD | sanitize_conflicted_diff >expect.diff
    -+	print_sanitized_diff >expect.diff
    ++	print_sanitized_conflicted_diff >expect.diff
      '
      
      test_expect_success 'apply -3 with add/add conflict' '
    @@ t/t4108-apply-threeway.sh: test_expect_success 'apply -3 with add/add conflict'
      	# ... and leave conflicts in the index and in the working tree
      	git ls-files -s >actual.ls &&
     -	git diff HEAD | sanitize_conflicted_diff >actual.diff &&
    -+	print_sanitized_diff >actual.diff &&
    ++	print_sanitized_conflicted_diff >actual.diff &&
      
      	# The result should resemble the corresponding merge
      	test_cmp expect.ls actual.ls &&
2:  d77c5f4199 = 2:  43c42b299e t4108: use `test_config` instead of `git config`
3:  5feddf1597 < -:  ---------- t4108: demonstrate bug in apply
-:  ---------- > 3:  58d32e2618 t4108: demonstrate bug in apply
4:  56c31310db ! 4:  5412dc9153 apply: respect merge.conflictStyle in --3way
    @@ apply.c: static void git_apply_config(void)
      static int parse_whitespace_option(struct apply_state *state, const char *option)
     
      ## t/t4108-apply-threeway.sh ##
    -@@ t/t4108-apply-threeway.sh: test_apply_with_3way () {
    - }
    +@@ t/t4108-apply-threeway.sh: test_expect_success 'apply with --3way' '
    + 	test_apply_with_3way
    + '
      
    - test_apply_with_3way success default true
    --test_apply_with_3way failure 'merge.conflictStyle = diff3' 'test_config merge.conflictStyle diff3'
    -+test_apply_with_3way success 'merge.conflictStyle = diff3' 'test_config merge.conflictStyle diff3'
    - 
    - test_expect_success 'apply with --3way with rerere enabled' '
    - 	test_config rerere.enabled true &&
    +-test_expect_failure 'apply with --3way with merge.conflictStyle = diff3' '
    ++test_expect_success 'apply with --3way with merge.conflictStyle = diff3' '
    + 	test_config merge.conflictStyle diff3 &&
    + 	test_apply_with_3way
    + '
-- 
2.24.0.rc0.197.g0926ab8072

[PATCH v2 1/5] t4108: replace create_file with test_write_lines

[Denton Liu]

Since the locally defined create_file() duplicates the functionality of
the test_write_lines() helper function, remove create_file() and replace
all instances with test_write_lines(). While we're at it, move
redirection operators to the end of the command which is the more
conventional place to put it.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t4108-apply-threeway.sh | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
index fa5d4efb89..b109ecbd9f 100755
--- a/t/t4108-apply-threeway.sh
+++ b/t/t4108-apply-threeway.sh
@@ -4,13 +4,6 @@ test_description='git apply --3way'
 
 . ./test-lib.sh
 
-create_file () {
-	for i
-	do
-		echo "$i"
-	done
-}
-
 sanitize_conflicted_diff () {
 	sed -e '
 		/^index /d
@@ -20,7 +13,7 @@ sanitize_conflicted_diff () {
 
 test_expect_success setup '
 	test_tick &&
-	create_file >one 1 2 3 4 5 6 7 &&
+	test_write_lines 1 2 3 4 5 6 7 >one &&
 	cat one >two &&
 	git add one two &&
 	git commit -m initial &&
@@ -28,13 +21,13 @@ test_expect_success setup '
 	git branch side &&
 
 	test_tick &&
-	create_file >one 1 two 3 4 5 six 7 &&
-	create_file >two 1 two 3 4 5 6 7 &&
+	test_write_lines 1 two 3 4 5 six 7 >one &&
+	test_write_lines 1 two 3 4 5 6 7 >two &&
 	git commit -a -m master &&
 
 	git checkout side &&
-	create_file >one 1 2 3 4 five 6 7 &&
-	create_file >two 1 2 3 4 five 6 7 &&
+	test_write_lines 1 2 3 4 five 6 7 >one &&
+	test_write_lines 1 2 3 4 five 6 7 >two &&
 	git commit -a -m side &&
 
 	git checkout master
@@ -87,7 +80,7 @@ test_expect_success 'apply with --3way with rerere enabled' '
 	test_must_fail git merge --no-commit side &&
 
 	# Manually resolve and record the resolution
-	create_file 1 two 3 4 five six 7 >one &&
+	test_write_lines 1 two 3 4 five six 7 >one &&
 	git rerere &&
 	cat one >expect &&
 
@@ -104,14 +97,14 @@ test_expect_success 'apply -3 with add/add conflict setup' '
 	git reset --hard &&
 
 	git checkout -b adder &&
-	create_file 1 2 3 4 5 6 7 >three &&
-	create_file 1 2 3 4 5 6 7 >four &&
+	test_write_lines 1 2 3 4 5 6 7 >three &&
+	test_write_lines 1 2 3 4 5 6 7 >four &&
 	git add three four &&
 	git commit -m "add three and four" &&
 
 	git checkout -b another adder^ &&
-	create_file 1 2 3 4 5 6 7 >three &&
-	create_file 1 2 3 four 5 6 7 >four &&
+	test_write_lines 1 2 3 4 5 6 7 >three &&
+	test_write_lines 1 2 3 four 5 6 7 >four &&
 	git add three four &&
 	git commit -m "add three and four" &&
 
-- 
2.24.0.rc0.197.g0926ab8072

[PATCH v2 2/5] t4108: remove git command upstream of pipe

[Denton Liu]

Before, the output of `git diff HEAD` would always be piped to
sanitize_conflicted_diff(). However, since the Git command was upstream
of the pipe, in case the Git command fails, the return code would be
lost. Rewrite into separate statements so that the return code is no
longer lost.

Since only the command `git diff HEAD` was being piped to
sanitize_conflicted_diff(), move the command into the function and rename
it to print_sanitized_conflicted_diff().

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t4108-apply-threeway.sh | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
index b109ecbd9f..3c0ddacddf 100755
--- a/t/t4108-apply-threeway.sh
+++ b/t/t4108-apply-threeway.sh
@@ -4,11 +4,12 @@ test_description='git apply --3way'
 
 . ./test-lib.sh
 
-sanitize_conflicted_diff () {
+print_sanitized_conflicted_diff () {
+	git diff HEAD >diff.raw &&
 	sed -e '
 		/^index /d
 		s/^\(+[<>][<>][<>][<>]*\) .*/\1/
-	'
+	' diff.raw
 }
 
 test_expect_success setup '
@@ -54,14 +55,14 @@ test_expect_success 'apply with --3way' '
 	git checkout master^0 &&
 	test_must_fail git merge --no-commit side &&
 	git ls-files -s >expect.ls &&
-	git diff HEAD | sanitize_conflicted_diff >expect.diff &&
+	print_sanitized_conflicted_diff >expect.diff &&
 
 	# should fail to apply
 	git reset --hard &&
 	git checkout master^0 &&
 	test_must_fail git apply --index --3way P.diff &&
 	git ls-files -s >actual.ls &&
-	git diff HEAD | sanitize_conflicted_diff >actual.diff &&
+	print_sanitized_conflicted_diff >actual.diff &&
 
 	# The result should resemble the corresponding merge
 	test_cmp expect.ls actual.ls &&
@@ -114,7 +115,7 @@ test_expect_success 'apply -3 with add/add conflict setup' '
 	git checkout adder^0 &&
 	test_must_fail git merge --no-commit another &&
 	git ls-files -s >expect.ls &&
-	git diff HEAD | sanitize_conflicted_diff >expect.diff
+	print_sanitized_conflicted_diff >expect.diff
 '
 
 test_expect_success 'apply -3 with add/add conflict' '
@@ -124,7 +125,7 @@ test_expect_success 'apply -3 with add/add conflict' '
 	test_must_fail git apply --index --3way P.diff &&
 	# ... and leave conflicts in the index and in the working tree
 	git ls-files -s >actual.ls &&
-	git diff HEAD | sanitize_conflicted_diff >actual.diff &&
+	print_sanitized_conflicted_diff >actual.diff &&
 
 	# The result should resemble the corresponding merge
 	test_cmp expect.ls actual.ls &&
-- 
2.24.0.rc0.197.g0926ab8072

[PATCH v2 3/5] t4108: use `test_config` instead of `git config`

[Denton Liu]

Since `git config` leaves the configurations set even after the test
case completes, use `test_config` instead so that the configurations are
reset once the test case finishes.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t4108-apply-threeway.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
index 3c0ddacddf..7f96ae9101 100755
--- a/t/t4108-apply-threeway.sh
+++ b/t/t4108-apply-threeway.sh
@@ -70,7 +70,7 @@ test_expect_success 'apply with --3way' '
 '
 
 test_expect_success 'apply with --3way with rerere enabled' '
-	git config rerere.enabled true &&
+	test_config rerere.enabled true &&
 
 	# Merging side should be similar to applying this patch
 	git diff ...side >P.diff &&
-- 
2.24.0.rc0.197.g0926ab8072

[PATCH v2 4/5] t4108: demonstrate bug in apply

[Denton Liu]

Currently, apply does not respect the merge.conflictStyle setting.
Demonstrate this by making the 'apply with --3way' test case generic and
extending it to show that the configuration of
merge.conflictStyle = diff3 causes a breakage.

Change print_sanitized_conflicted_diff() to also sanitize `|||||||`
conflict markers.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t4108-apply-threeway.sh | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
index 7f96ae9101..bffe37f1ba 100755
--- a/t/t4108-apply-threeway.sh
+++ b/t/t4108-apply-threeway.sh
@@ -8,7 +8,7 @@ print_sanitized_conflicted_diff () {
 	git diff HEAD >diff.raw &&
 	sed -e '
 		/^index /d
-		s/^\(+[<>][<>][<>][<>]*\) .*/\1/
+		s/^\(+[<>|][<>|][<>|][<>|]*\) .*/\1/
 	' diff.raw
 }
 
@@ -46,7 +46,7 @@ test_expect_success 'apply without --3way' '
 	git diff-index --exit-code --cached HEAD
 '
 
-test_expect_success 'apply with --3way' '
+test_apply_with_3way () {
 	# Merging side should be similar to applying this patch
 	git diff ...side >P.diff &&
 
@@ -67,6 +67,15 @@ test_expect_success 'apply with --3way' '
 	# The result should resemble the corresponding merge
 	test_cmp expect.ls actual.ls &&
 	test_cmp expect.diff actual.diff
+}
+
+test_expect_success 'apply with --3way' '
+	test_apply_with_3way
+'
+
+test_expect_failure 'apply with --3way with merge.conflictStyle = diff3' '
+	test_config merge.conflictStyle diff3 &&
+	test_apply_with_3way
 '
 
 test_expect_success 'apply with --3way with rerere enabled' '
-- 
2.24.0.rc0.197.g0926ab8072

[PATCH v2 5/5] apply: respect merge.conflictStyle in --3way

[Denton Liu]

Before, when doing a 3-way merge, the merge.conflictStyle option was not
respected and the "merge" style was always used, even if "diff3" was
specified.

Call git_xmerge_config() at the end of git_apply_config() so that the
merge.conflictStyle config is read.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 apply.c                   | 2 +-
 t/t4108-apply-threeway.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/apply.c b/apply.c
index f8a046a6a5..b9291f5f7b 100644
--- a/apply.c
+++ b/apply.c
@@ -32,7 +32,7 @@ static void git_apply_config(void)
 {
 	git_config_get_string_const("apply.whitespace", &apply_default_whitespace);
 	git_config_get_string_const("apply.ignorewhitespace", &apply_default_ignorewhitespace);
-	git_config(git_default_config, NULL);
+	git_config(git_xmerge_config, NULL);
 }
 
 static int parse_whitespace_option(struct apply_state *state, const char *option)
diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
index bffe37f1ba..d7349ced6b 100755
--- a/t/t4108-apply-threeway.sh
+++ b/t/t4108-apply-threeway.sh
@@ -73,7 +73,7 @@ test_expect_success 'apply with --3way' '
 	test_apply_with_3way
 '
 
-test_expect_failure 'apply with --3way with merge.conflictStyle = diff3' '
+test_expect_success 'apply with --3way with merge.conflictStyle = diff3' '
 	test_config merge.conflictStyle diff3 &&
 	test_apply_with_3way
 '
-- 
2.24.0.rc0.197.g0926ab8072

Re: [PATCH 5/5] apply: respect merge.conflictStyle in --3way

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

> Before, when doing a 3-way merge, the merge.conflictStyle option was not
> respected and the "merge" style was always used, even if "diff3" was
> specified.
>
> Call git_xmerge_config() at the end of git_apply_config() so that the
> merge.conflictStyle config is read.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  apply.c                   | 2 +-
>  t/t4108-apply-threeway.sh | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Interesting.

I do not recall why I did not add this when I introduced the "diff3"
style.  If it were because I knew other parts of "git apply" were
not prepared to see the common ancestor hunk and using the "diff3"
style would have triggered breakage in them, I would remember, so
hopefully there is no subtle and hidden gotchas like it.

The implementation to trigger the "diff3" style conflict
presentation obviously is correct ;-)

Thanks.

>  test_apply_with_3way success default true
> -test_apply_with_3way failure 'merge.conflictStyle = diff3' 'test_config merge.conflictStyle diff3'
> +test_apply_with_3way success 'merge.conflictStyle = diff3' 'test_config merge.conflictStyle diff3'
>  
>  test_expect_success 'apply with --3way with rerere enabled' '
>  	test_config rerere.enabled true &&