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

[Elijah Newren <newren@gmail.com>]

On Sun, Sep 5, 2021 at 12:07 PM Junio C Hamano <gitster@pobox.com> 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 <jerry@skydio.com>
> [jc: stolen tests from Jerry's patch]
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  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 <newren@gmail.com>