git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] apply: fix merge.conflictStyle bug in --3way
@ 2019-10-23 12:03 Denton Liu
  2019-10-23 12:03 ` [PATCH 1/5] t4108: replace create_file with test_write_lines Denton Liu
                   ` (5 more replies)
  0 siblings, 6 replies; 16+ messages in thread
From: Denton Liu @ 2019-10-23 12:03 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/5] t4108: replace create_file with test_write_lines
  2019-10-23 12:03 [PATCH 0/5] apply: fix merge.conflictStyle bug in --3way Denton Liu
@ 2019-10-23 12:03 ` Denton Liu
  2019-10-23 12:03 ` [PATCH 2/5] t4108: remove git command upstream of pipe Denton Liu
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: Denton Liu @ 2019-10-23 12:03 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/5] t4108: remove git command upstream of pipe
  2019-10-23 12:03 [PATCH 0/5] apply: fix merge.conflictStyle bug in --3way Denton Liu
  2019-10-23 12:03 ` [PATCH 1/5] t4108: replace create_file with test_write_lines Denton Liu
@ 2019-10-23 12:03 ` Denton Liu
  2019-10-23 13:32   ` Eric Sunshine
  2019-10-23 12:03 ` [PATCH 3/5] t4108: use `test_config` instead of `git config` Denton Liu
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 16+ messages in thread
From: Denton Liu @ 2019-10-23 12:03 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 3/5] t4108: use `test_config` instead of `git config`
  2019-10-23 12:03 [PATCH 0/5] apply: fix merge.conflictStyle bug in --3way Denton Liu
  2019-10-23 12:03 ` [PATCH 1/5] t4108: replace create_file with test_write_lines Denton Liu
  2019-10-23 12:03 ` [PATCH 2/5] t4108: remove git command upstream of pipe Denton Liu
@ 2019-10-23 12:03 ` Denton Liu
  2019-10-23 12:03 ` [PATCH 4/5] t4108: demonstrate bug in apply Denton Liu
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: Denton Liu @ 2019-10-23 12:03 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 4/5] t4108: demonstrate bug in apply
  2019-10-23 12:03 [PATCH 0/5] apply: fix merge.conflictStyle bug in --3way Denton Liu
                   ` (2 preceding siblings ...)
  2019-10-23 12:03 ` [PATCH 3/5] t4108: use `test_config` instead of `git config` Denton Liu
@ 2019-10-23 12:03 ` Denton Liu
  2019-10-23 13:40   ` Eric Sunshine
  2019-10-23 12:03 ` [PATCH 5/5] apply: respect merge.conflictStyle in --3way Denton Liu
  2019-10-23 23:32 ` [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
  5 siblings, 1 reply; 16+ messages in thread
From: Denton Liu @ 2019-10-23 12:03 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 5/5] apply: respect merge.conflictStyle in --3way
  2019-10-23 12:03 [PATCH 0/5] apply: fix merge.conflictStyle bug in --3way Denton Liu
                   ` (3 preceding siblings ...)
  2019-10-23 12:03 ` [PATCH 4/5] t4108: demonstrate bug in apply Denton Liu
@ 2019-10-23 12:03 ` Denton Liu
  2019-10-24  1:20   ` Junio C Hamano
  2019-10-23 23:32 ` [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
  5 siblings, 1 reply; 16+ messages in thread
From: Denton Liu @ 2019-10-23 12:03 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/5] t4108: remove git command upstream of pipe
  2019-10-23 12:03 ` [PATCH 2/5] t4108: remove git command upstream of pipe Denton Liu
@ 2019-10-23 13:32   ` Eric Sunshine
  2019-10-23 17:11     ` Denton Liu
  0 siblings, 1 reply; 16+ messages in thread
From: Eric Sunshine @ 2019-10-23 13:32 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/5] t4108: demonstrate bug in apply
  2019-10-23 12:03 ` [PATCH 4/5] t4108: demonstrate bug in apply Denton Liu
@ 2019-10-23 13:40   ` Eric Sunshine
  0 siblings, 0 replies; 16+ messages in thread
From: Eric Sunshine @ 2019-10-23 13:40 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List, Junio C Hamano

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
    '

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/5] t4108: remove git command upstream of pipe
  2019-10-23 13:32   ` Eric Sunshine
@ 2019-10-23 17:11     ` Denton Liu
  0 siblings, 0 replies; 16+ messages in thread
From: Denton Liu @ 2019-10-23 17:11 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Git Mailing List, Junio C Hamano

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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 0/5] This fixes a bug where even if `merge.conflictStyle = diff3`, running `git apply --3way` would not output the base.
  2019-10-23 12:03 [PATCH 0/5] apply: fix merge.conflictStyle bug in --3way Denton Liu
                   ` (4 preceding siblings ...)
  2019-10-23 12:03 ` [PATCH 5/5] apply: respect merge.conflictStyle in --3way Denton Liu
@ 2019-10-23 23:32 ` Denton Liu
  2019-10-23 23:32   ` [PATCH v2 1/5] t4108: replace create_file with test_write_lines Denton Liu
                     ` (4 more replies)
  5 siblings, 5 replies; 16+ messages in thread
From: Denton Liu @ 2019-10-23 23:32 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Eric Sunshine

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 1/5] t4108: replace create_file with test_write_lines
  2019-10-23 23:32 ` [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
@ 2019-10-23 23:32   ` Denton Liu
  2019-10-23 23:32   ` [PATCH v2 2/5] t4108: remove git command upstream of pipe Denton Liu
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 16+ messages in thread
From: Denton Liu @ 2019-10-23 23:32 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Eric Sunshine

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 2/5] t4108: remove git command upstream of pipe
  2019-10-23 23:32 ` [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
  2019-10-23 23:32   ` [PATCH v2 1/5] t4108: replace create_file with test_write_lines Denton Liu
@ 2019-10-23 23:32   ` Denton Liu
  2019-10-23 23:32   ` [PATCH v2 3/5] t4108: use `test_config` instead of `git config` Denton Liu
                     ` (2 subsequent siblings)
  4 siblings, 0 replies; 16+ messages in thread
From: Denton Liu @ 2019-10-23 23:32 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Eric Sunshine

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 3/5] t4108: use `test_config` instead of `git config`
  2019-10-23 23:32 ` [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
  2019-10-23 23:32   ` [PATCH v2 1/5] t4108: replace create_file with test_write_lines Denton Liu
  2019-10-23 23:32   ` [PATCH v2 2/5] t4108: remove git command upstream of pipe Denton Liu
@ 2019-10-23 23:32   ` Denton Liu
  2019-10-23 23:32   ` [PATCH v2 4/5] t4108: demonstrate bug in apply Denton Liu
  2019-10-23 23:32   ` [PATCH v2 5/5] apply: respect merge.conflictStyle in --3way Denton Liu
  4 siblings, 0 replies; 16+ messages in thread
From: Denton Liu @ 2019-10-23 23:32 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Eric Sunshine

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 4/5] t4108: demonstrate bug in apply
  2019-10-23 23:32 ` [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
                     ` (2 preceding siblings ...)
  2019-10-23 23:32   ` [PATCH v2 3/5] t4108: use `test_config` instead of `git config` Denton Liu
@ 2019-10-23 23:32   ` Denton Liu
  2019-10-23 23:32   ` [PATCH v2 5/5] apply: respect merge.conflictStyle in --3way Denton Liu
  4 siblings, 0 replies; 16+ messages in thread
From: Denton Liu @ 2019-10-23 23:32 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Eric Sunshine

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 5/5] apply: respect merge.conflictStyle in --3way
  2019-10-23 23:32 ` [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
                     ` (3 preceding siblings ...)
  2019-10-23 23:32   ` [PATCH v2 4/5] t4108: demonstrate bug in apply Denton Liu
@ 2019-10-23 23:32   ` Denton Liu
  4 siblings, 0 replies; 16+ messages in thread
From: Denton Liu @ 2019-10-23 23:32 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Eric Sunshine

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 5/5] apply: respect merge.conflictStyle in --3way
  2019-10-23 12:03 ` [PATCH 5/5] apply: respect merge.conflictStyle in --3way Denton Liu
@ 2019-10-24  1:20   ` Junio C Hamano
  0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2019-10-24  1:20 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2019-10-24  1:20 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-23 12:03 [PATCH 0/5] apply: fix merge.conflictStyle bug in --3way Denton Liu
2019-10-23 12:03 ` [PATCH 1/5] t4108: replace create_file with test_write_lines Denton Liu
2019-10-23 12:03 ` [PATCH 2/5] t4108: remove git command upstream of pipe Denton Liu
2019-10-23 13:32   ` Eric Sunshine
2019-10-23 17:11     ` Denton Liu
2019-10-23 12:03 ` [PATCH 3/5] t4108: use `test_config` instead of `git config` Denton Liu
2019-10-23 12:03 ` [PATCH 4/5] t4108: demonstrate bug in apply Denton Liu
2019-10-23 13:40   ` Eric Sunshine
2019-10-23 12:03 ` [PATCH 5/5] apply: respect merge.conflictStyle in --3way Denton Liu
2019-10-24  1:20   ` Junio C Hamano
2019-10-23 23:32 ` [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
2019-10-23 23:32   ` [PATCH v2 1/5] t4108: replace create_file with test_write_lines Denton Liu
2019-10-23 23:32   ` [PATCH v2 2/5] t4108: remove git command upstream of pipe Denton Liu
2019-10-23 23:32   ` [PATCH v2 3/5] t4108: use `test_config` instead of `git config` Denton Liu
2019-10-23 23:32   ` [PATCH v2 4/5] t4108: demonstrate bug in apply Denton Liu
2019-10-23 23:32   ` [PATCH v2 5/5] apply: respect merge.conflictStyle in --3way Denton Liu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).