[v2] apply: resolve trivial merge without hitting ll-merge with "--3way"

Commit Message

Junio C Hamano Sept. 5, 2021, 7:06 p.m. UTC

The ll_binary_merge() function assumes that the ancestor blob is
different from either side of the new versions, and always fails
the merge in conflict, unless -Xours or -Xtheirs is in effect.

The normal "merge" machineries all resolve the trivial cases
(e.g. if our side changed while their side did not, the result
is ours) without triggering the file-level merge drivers, so the
assumption is warranted.

The code path in "git apply --3way", however, does not check for
the trivial three-way merge situation and always calls the
file-level merge drivers.  This used to be perfectly OK back
when we always first attempted a straight patch application and
used the three-way code path only as a fallback.  Any binary
patch that can be applied as a trivial three-way merge (e.g. the
patch is based exactly on the version we happen to have) would
always cleanly apply, so the ll_binary_merge() that is not
prepared to see the trivial case would not have to handle such a
case.

This no longer is true after we made "--3way" to mean "first try
three-way and then fall back to straight application", and made
"git apply -3" on a binary patch that is based on the current
version no longer apply.

Teach "git apply -3" to first check for the trivial merge cases
and resolve them without hitting the file-level merge drivers.

Signed-off-by: Jerry Zhang <[e-mail]>
[jc: stolen tests from Jerry's patch]
Signed-off-by: Junio C Hamano <[e-mail]>
---

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

Comments

Elijah Newren Sept. 6, 2021, 6:57 p.m. UTC | #1

On Sun, Sep 5, 2021 at 12:07 PM Junio C Hamano <[e-mail]> wrote:
>
> The ll_binary_merge() function assumes that the ancestor blob is
> different from either side of the new versions, and always fails
> the merge in conflict, unless -Xours or -Xtheirs is in effect.
>
> The normal "merge" machineries all resolve the trivial cases
> (e.g. if our side changed while their side did not, the result
> is ours) without triggering the file-level merge drivers, so the
> assumption is warranted.
>
> The code path in "git apply --3way", however, does not check for
> the trivial three-way merge situation and always calls the
> file-level merge drivers.  This used to be perfectly OK back
> when we always first attempted a straight patch application and
> used the three-way code path only as a fallback.  Any binary
> patch that can be applied as a trivial three-way merge (e.g. the
> patch is based exactly on the version we happen to have) would
> always cleanly apply, so the ll_binary_merge() that is not
> prepared to see the trivial case would not have to handle such a
> case.
>
> This no longer is true after we made "--3way" to mean "first try
> three-way and then fall back to straight application", and made
> "git apply -3" on a binary patch that is based on the current
> version no longer apply.
>
> Teach "git apply -3" to first check for the trivial merge cases
> and resolve them without hitting the file-level merge drivers.
>
> Signed-off-by: Jerry Zhang <[e-mail]>
> [jc: stolen tests from Jerry's patch]
> Signed-off-by: Junio C Hamano <[e-mail]>
> ---
>
>  apply.c                   | 21 ++++++++++++++++++
>  t/t4108-apply-threeway.sh | 45 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+)
>
> diff --git a/apply.c b/apply.c
> index 44bc31d6eb..c9f9503e90 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -3467,6 +3467,21 @@ static int load_preimage(struct apply_state *state,
>         return 0;
>  }
>
> +static int resolve_to(struct image *image, const struct object_id *result_id)
> +{
> +       unsigned long size;
> +       enum object_type type;
> +
> +       clear_image(image);
> +
> +       image->buf = read_object_file(result_id, &type, &size);
> +       if (!image->buf || type != OBJ_BLOB)
> +               die("unable to read blob object %s", oid_to_hex(result_id));
> +       image->len = size;
> +
> +       return 0;
> +}
> +
>  static int three_way_merge(struct apply_state *state,
>                            struct image *image,
>                            char *path,
> @@ -3478,6 +3493,12 @@ static int three_way_merge(struct apply_state *state,
>         mmbuffer_t result = { NULL };
>         int status;
>
> +       /* resolve trivial cases first */
> +       if (oideq(base, ours))
> +               return resolve_to(image, theirs);
> +       else if (oideq(base, theirs) || oideq(ours, theirs))
> +               return resolve_to(image, ours);
> +
>         read_mmblob(&base_file, base);
>         read_mmblob(&our_file, ours);
>         read_mmblob(&their_file, theirs);
> diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
> index 65147efdea..cc3aa3314a 100755
> --- a/t/t4108-apply-threeway.sh
> +++ b/t/t4108-apply-threeway.sh
> @@ -230,4 +230,49 @@ test_expect_success 'apply with --3way --cached and conflicts' '
>         test_cmp expect.diff actual.diff
>  '
>
> +test_expect_success 'apply binary file patch' '
> +       git reset --hard main &&
> +       cp "$TEST_DIRECTORY/test-binary-1.png" bin.png &&
> +       git add bin.png &&
> +       git commit -m "add binary file" &&
> +
> +       cp "$TEST_DIRECTORY/test-binary-2.png" bin.png &&
> +
> +       git diff --binary >bin.diff &&
> +       git reset --hard &&
> +
> +       # Apply must succeed.
> +       git apply bin.diff
> +'
> +
> +test_expect_success 'apply binary file patch with 3way' '
> +       git reset --hard main &&
> +       cp "$TEST_DIRECTORY/test-binary-1.png" bin.png &&
> +       git add bin.png &&
> +       git commit -m "add binary file" &&
> +
> +       cp "$TEST_DIRECTORY/test-binary-2.png" bin.png &&
> +
> +       git diff --binary >bin.diff &&
> +       git reset --hard &&
> +
> +       # Apply must succeed.
> +       git apply --3way --index bin.diff
> +'
> +
> +test_expect_success 'apply full-index patch with 3way' '
> +       git reset --hard main &&
> +       cp "$TEST_DIRECTORY/test-binary-1.png" bin.png &&
> +       git add bin.png &&
> +       git commit -m "add binary file" &&
> +
> +       cp "$TEST_DIRECTORY/test-binary-2.png" bin.png &&
> +
> +       git diff --full-index >bin.diff &&
> +       git reset --hard &&
> +
> +       # Apply must succeed.
> +       git apply --3way --index bin.diff
> +'
> +
>  test_done
> --
> 2.33.0-408-g8e1aa136b3

Reviewed-by: Elijah Newren <[e-mail]>

Ævar Arnfjörð Bjarmason Sept. 6, 2021, 9:59 p.m. UTC | #2

On Sun, Sep 05 2021, Junio C Hamano wrote:

> +	if (!image->buf || type != OBJ_BLOB)
> +		die("unable to read blob object %s", oid_to_hex(result_id));

This die() message seems to only be applicable to the first condition
here, shouldn't this be:

    if (!image->buf)
        die(_("unable to read blob object %s"), oid_to_hex(result_id));
    if (type != OBJ_BLOB)
        die(_("object %s is %s, expected blob"), oid_to_hex(result_id), type_name(type));

Also as shown there, missing _() for marking the translation.

> [...]
> +test_expect_success 'apply binary file patch' '
> +	git reset --hard main &&

Partly this is cleaning up a mess after an existing test, but here
there's no reason we can't use test_when_finished() for all the new
tests to make them clean up after themselves:

diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
index cc3aa3314a3..c3c9b52e30d 100755
--- a/t/t4108-apply-threeway.sh
+++ b/t/t4108-apply-threeway.sh
@@ -232,6 +232,8 @@ test_expect_success 'apply with --3way --cached and conflicts' '
 
 test_expect_success 'apply binary file patch' '
 	git reset --hard main &&
+	test_when_finished "git reset --hard main" &&
+
 	cp "$TEST_DIRECTORY/test-binary-1.png" bin.png &&
 	git add bin.png &&
 	git commit -m "add binary file" &&
@@ -246,7 +248,8 @@ test_expect_success 'apply binary file patch' '
 '
 
 test_expect_success 'apply binary file patch with 3way' '
-	git reset --hard main &&
+	test_when_finished "git reset --hard main" &&
+
 	cp "$TEST_DIRECTORY/test-binary-1.png" bin.png &&
 	git add bin.png &&
 	git commit -m "add binary file" &&
@@ -261,7 +264,8 @@ test_expect_success 'apply binary file patch with 3way' '
 '
 
 test_expect_success 'apply full-index patch with 3way' '
-	git reset --hard main &&
+	test_when_finished "git reset --hard main" &&
+
 	cp "$TEST_DIRECTORY/test-binary-1.png" bin.png &&
 	git add bin.png &&
 	git commit -m "add binary file" &&

Junio C Hamano Sept. 7, 2021, 2:32 a.m. UTC | #3

Ævar Arnfjörð Bjarmason <[e-mail]> writes:

> On Sun, Sep 05 2021, Junio C Hamano wrote:
>
>> +	if (!image->buf || type != OBJ_BLOB)
>> +		die("unable to read blob object %s", oid_to_hex(result_id));
>
> This die() message seems to only be applicable to the first condition
> here, shouldn't this be:

As this directly was lifted from read_mmblob(), it should be exactly
spelled as I wrote.

Junio C Hamano Sept. 7, 2021, 8:15 p.m. UTC | #4

Ævar Arnfjörð Bjarmason <[e-mail]> writes:

> Partly this is cleaning up a mess after an existing test, but here
> there's no reason we can't use test_when_finished() for all the new
> tests to make them clean up after themselves:

I do not mind if somebody wants to send in a janitorial patch after
the dust settles, but adding "test_when_finished reset --hard" after
each "refs --hard" at the beginning of each test is not something I
would expect to see.  Such a patch should first choose between "each
test cleans after itself" and "expect previous ones may have left a
mess, so each test clears the slate sufficiently before it starts"
and then stick to the approach, not mixture of both, I would think.

Thanks.

Patch

diff --git a/apply.c b/apply.c
index 44bc31d6eb..c9f9503e90 100644
--- a/apply.c
+++ b/apply.c
@@ -3467,6 +3467,21 @@  static int load_preimage(struct apply_state *state,
 	return 0;
 }
 
+static int resolve_to(struct image *image, const struct object_id *result_id)
+{
+	unsigned long size;
+	enum object_type type;
+
+	clear_image(image);
+
+	image->buf = read_object_file(result_id, &type, &size);
+	if (!image->buf || type != OBJ_BLOB)
+		die("unable to read blob object %s", oid_to_hex(result_id));
+	image->len = size;
+
+	return 0;
+}
+
 static int three_way_merge(struct apply_state *state,
 			   struct image *image,
 			   char *path,
@@ -3478,6 +3493,12 @@  static int three_way_merge(struct apply_state *state,
 	mmbuffer_t result = { NULL };
 	int status;
 
+	/* resolve trivial cases first */
+	if (oideq(base, ours))
+		return resolve_to(image, theirs);
+	else if (oideq(base, theirs) || oideq(ours, theirs))
+		return resolve_to(image, ours);
+
 	read_mmblob(&base_file, base);
 	read_mmblob(&our_file, ours);
 	read_mmblob(&their_file, theirs);
diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
index 65147efdea..cc3aa3314a 100755
--- a/t/t4108-apply-threeway.sh
+++ b/t/t4108-apply-threeway.sh
@@ -230,4 +230,49 @@  test_expect_success 'apply with --3way --cached and conflicts' '
 	test_cmp expect.diff actual.diff
 '
 
+test_expect_success 'apply binary file patch' '
+	git reset --hard main &&
+	cp "$TEST_DIRECTORY/test-binary-1.png" bin.png &&
+	git add bin.png &&
+	git commit -m "add binary file" &&
+
+	cp "$TEST_DIRECTORY/test-binary-2.png" bin.png &&
+
+	git diff --binary >bin.diff &&
+	git reset --hard &&
+
+	# Apply must succeed.
+	git apply bin.diff
+'
+
+test_expect_success 'apply binary file patch with 3way' '
+	git reset --hard main &&
+	cp "$TEST_DIRECTORY/test-binary-1.png" bin.png &&
+	git add bin.png &&
+	git commit -m "add binary file" &&
+
+	cp "$TEST_DIRECTORY/test-binary-2.png" bin.png &&
+
+	git diff --binary >bin.diff &&
+	git reset --hard &&
+
+	# Apply must succeed.
+	git apply --3way --index bin.diff
+'
+
+test_expect_success 'apply full-index patch with 3way' '
+	git reset --hard main &&
+	cp "$TEST_DIRECTORY/test-binary-1.png" bin.png &&
+	git add bin.png &&
+	git commit -m "add binary file" &&
+
+	cp "$TEST_DIRECTORY/test-binary-2.png" bin.png &&
+
+	git diff --full-index >bin.diff &&
+	git reset --hard &&
+
+	# Apply must succeed.
+	git apply --3way --index bin.diff
+'
+
 test_done