apply: fix merge.conflictStyle bug in --3way

PATCH Superseded Denton Liu 3 participants 16 messages Oct 23, 2019 – Oct 24, 2019

lore

Share & tools

lore

Denton Liu

author

2019-10-23 12:03 UTC #0 lore raw

cover apply: fix merge.conflictStyle bug in --3way

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(-)

Signature · 1 line

2.24.0.rc0.197.g0926ab8072

Denton Liu

author

2019-10-23 12:03 UTC #1 lore raw

PATCH 1/5 t4108: replace create_file with test_write_lines

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 <[email protected]>

---
 t/t4108-apply-threeway.sh | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

t/t4108-apply-threeway.sh +10 ~~-17~~

index fa5d4efb89..b109ecbd9f 100755
@@ -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" &&

Signature · 1 line

2.24.0.rc0.197.g0926ab8072

Denton Liu

author

2019-10-23 12:03 UTC #2 lore raw

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

In reply to 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 <[email protected]>

---
 t/t4108-apply-threeway.sh | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

t/t4108-apply-threeway.sh +7 -6

index b109ecbd9f..49739ce8b4 100755
@@ -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 &&

Signature · 1 line

2.24.0.rc0.197.g0926ab8072

Eric Sunshine

2019-10-23 13:32 UTC #3 lore raw

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

Quoted 14 lines · 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 <[email protected]>
---

t/t4108-apply-threeway.sh test_description='git apply --3way' · line 10

Quoted 9 lines

t/t4108-apply-threeway.sh +3 -2

@@ -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".

Denton Liu

author

2019-10-23 17:11 UTC #4 lore raw

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

Quoted 33 lines · Eric Sunshine

Quoted 14 lines · 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 <[email protected]>
---
t/t4108-apply-threeway.sh test_description='git apply --3way' · line 10
Quoted 9 lines
t/t4108-apply-threeway.sh +3 -2
@@ -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 &&
        ...

Quoted 8 lines

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.

Denton Liu

author

2019-10-23 12:04 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---
 t/t4108-apply-threeway.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t4108-apply-threeway.sh +1 -1

index 49739ce8b4..3615256492 100755
@@ -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 &&

Signature · 1 line

2.24.0.rc0.197.g0926ab8072

Denton Liu

author

2019-10-23 12:04 UTC #6 lore raw

PATCH 4/5 t4108: demonstrate bug in apply

In reply to 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 <[email protected]>

---
 t/t4108-apply-threeway.sh | 58 ++++++++++++++++++++++++---------------
 1 file changed, 36 insertions(+), 22 deletions(-)

t/t4108-apply-threeway.sh +36 ~~-22~~

index 3615256492..84347fc178 100755
@@ -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 &&

Signature · 1 line

2.24.0.rc0.197.g0926ab8072

Eric Sunshine

2019-10-23 13:41 UTC #7 lore raw

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

Quoted 20 lines · 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 <[email protected]>
---
t/t4108-apply-threeway.sh +9 -0
@@ -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)" "

t/t4108-apply-threeway.sh test_expect_success 'apply without --3way' ' · line 64

Quoted 10 lines

t/t4108-apply-threeway.sh +9 -0

+               $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
    '

Denton Liu

author

2019-10-23 12:04 UTC #8 lore raw

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

In reply to 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 <[email protected]>

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

apply.c +1 -1

index f8a046a6a5..b9291f5f7b 100644
@@ -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)

t/t4108-apply-threeway.sh +1 -1

index 84347fc178..0e4eeac083 100755
@@ -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 &&

Signature · 1 line

2.24.0.rc0.197.g0926ab8072

Junio C Hamano

2019-10-24 01:20 UTC #9 lore raw

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

Quoted 4 lines · 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.

Quoted 8 lines

Call git_xmerge_config() at the end of git_apply_config() so that the merge.conflictStyle config is read.
Signed-off-by Denton Liu <[email protected]>
---
 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.

Quoted 6 lines

test_apply_with_3way success default true

t/t4108-apply-threeway.sh +1 -1

-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 &&

Denton Liu

author

2019-10-23 23:32 UTC #10 lore raw

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

In reply to 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()

 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
+ '

Signature · 1 line

2.24.0.rc0.197.g0926ab8072

Denton Liu

author

2019-10-23 23:32 UTC #11 lore raw

PATCH v2 1/5 t4108: replace create_file with test_write_lines

Signed-off-by Denton Liu <[email protected]>

---
 t/t4108-apply-threeway.sh | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

t/t4108-apply-threeway.sh +10 ~~-17~~

index fa5d4efb89..b109ecbd9f 100755
@@ -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" &&

Signature · 1 line

2.24.0.rc0.197.g0926ab8072

Denton Liu

author

2019-10-23 23:32 UTC #12 lore raw

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

In reply to Denton Liu

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 <[email protected]>

---
 t/t4108-apply-threeway.sh | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

t/t4108-apply-threeway.sh +7 -6

index b109ecbd9f..3c0ddacddf 100755
@@ -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 &&

Signature · 1 line

2.24.0.rc0.197.g0926ab8072

Denton Liu

author

2019-10-23 23:32 UTC #13 lore raw

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

In reply to 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 <[email protected]>

---
 t/t4108-apply-threeway.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t4108-apply-threeway.sh +1 -1

index 3c0ddacddf..7f96ae9101 100755
@@ -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 &&

Signature · 1 line

2.24.0.rc0.197.g0926ab8072

Denton Liu

author

2019-10-23 23:32 UTC #14 lore raw

PATCH v2 4/5 t4108: demonstrate bug in apply

In reply to Denton Liu

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

Signed-off-by Denton Liu <[email protected]>

---
 t/t4108-apply-threeway.sh | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

t/t4108-apply-threeway.sh +11 -2

index 7f96ae9101..bffe37f1ba 100755
@@ -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' '

Signature · 1 line

2.24.0.rc0.197.g0926ab8072

Denton Liu

author

2019-10-23 23:32 UTC #15 lore raw

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

In reply to 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 <[email protected]>

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

apply.c +1 -1

index f8a046a6a5..b9291f5f7b 100644
@@ -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)

t/t4108-apply-threeway.sh +1 -1

index bffe37f1ba..d7349ced6b 100755
@@ -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
 '

Signature · 1 line

2.24.0.rc0.197.g0926ab8072