git-apply: try threeway first when "--3way" is used

The apply_fragments() method of "git apply"
can silently apply patches incorrectly if
a file has repeating contents. In these
cases a three-way merge is capable of applying
it correctly in more situations, and will
show a conflict rather than applying it
incorrectly. However, because the patches
apply "successfully" using apply_fragments(),
git will never fall back to the merge, even
if the "--3way" flag is used, and the user has
no way to ensure correctness by forcing the
three-way merge method.

Change the behavior so that when "--3way" is used,
git will always try the three-way merge first and
will only fall back to apply_fragments() in cases
where blobs are not available or some other error
(but not in the case of a merge conflict).

Since user-facing results will be different,
this has backwards compatibility implications
for users depending on the old behavior. In
addition, the three-way merge will be slower
than direct patch application.

Signed-off-by: Jerry Zhang <jerry@skydio.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index 91d9a8601c8c316d4649c405af42e531c39991a8..9144575299c264dd299b542b7b5948eef35f211c 100644 (file)
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -84,9 +84,8 @@ OPTIONS
 
 -3::
 --3way::
-       When the patch does not apply cleanly, fall back on 3-way merge if
-       the patch records the identity of blobs it is supposed to apply to,
-       and we have those blobs available locally, possibly leaving the
+       Attempt 3-way merge if the patch records the identity of blobs it is supposed
+       to apply to and we have those blobs available locally, possibly leaving the
        conflict markers in the files in the working tree for the user to
        resolve.  This option implies the `--index` option, and is incompatible
        with the `--reject` and the `--cached` options.

diff --git a/apply.c b/apply.c
index 466f880d737dbdbff8259e89ec9767643fb248e1..69197268ccccbc18d70b865e47d5e3f062c0771a 100644 (file)
--- a/apply.c
+++ b/apply.c
@@ -3570,10 +3570,10 @@ static int try_threeway(struct apply_state *state,
                write_object_file("", 0, blob_type, &pre_oid);
        else if (get_oid(patch->old_oid_prefix, &pre_oid) ||
                 read_blob_object(&buf, &pre_oid, patch->old_mode))
-               return error(_("repository lacks the necessary blob to fall back on 3-way merge."));
+               return error(_("repository lacks the necessary blob to perform 3-way merge."));
 
        if (state->apply_verbosity > verbosity_silent)
-               fprintf(stderr, _("Falling back to three-way merge...\n"));
+               fprintf(stderr, _("Performing three-way merge...\n"));
 
        img = strbuf_detach(&buf, &len);
        prepare_image(&tmp_image, img, len, 1);
@@ -3605,7 +3605,7 @@ static int try_threeway(struct apply_state *state,
        if (status < 0) {
                if (state->apply_verbosity > verbosity_silent)
                        fprintf(stderr,
-                               _("Failed to fall back on three-way merge...\n"));
+                               _("Failed to perform three-way merge...\n"));
                return status;
        }
 
@@ -3638,10 +3638,9 @@ static int apply_data(struct apply_state *state, struct patch *patch,
        if (load_preimage(state, &image, patch, st, ce) < 0)
                return -1;
 
-       if (patch->direct_to_threeway ||
-           apply_fragments(state, &image, patch) < 0) {
+       if (!state->threeway || try_threeway(state, &image, patch, st, ce) < 0) {
                /* Note: with --reject, apply_fragments() returns 0 */
-               if (!state->threeway || try_threeway(state, &image, patch, st, ce) < 0)
+               if (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0)
                        return -1;
        }
        patch->result = image.buf;
@@ -5018,7 +5017,7 @@ int apply_parse_options(int argc, const char **argv,
                OPT_BOOL(0, "apply", force_apply,
                        N_("also apply the patch (use with --stat/--summary/--check)")),
                OPT_BOOL('3', "3way", &state->threeway,
-                        N_( "attempt three-way merge if a patch does not apply")),
+                        N_( "attempt three-way merge, fall back on normal patch if that fails")),
                OPT_FILENAME(0, "build-fake-ancestor", &state->fake_ancestor,
                        N_("build a temporary index based on embedded index information")),
                /* Think twice before adding "--nul" synonym to this */

diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
index d62db3fbe16f35a625a4a14eebb70034f695d3eb..9ff313f976422f9c12dc8032d14567b54cfe3765 100755 (executable)
--- a/t/t4108-apply-threeway.sh
+++ b/t/t4108-apply-threeway.sh
@@ -160,4 +160,24 @@ test_expect_success 'apply -3 with add/add conflict (dirty working tree)' '
        test_cmp three.save three
 '
 
+test_expect_success 'apply -3 with ambiguous repeating file' '
+       git reset --hard &&
+       test_write_lines 1 2 1 2 1 2 1 2 1 2 1 >one_two_repeat &&
+       git add one_two_repeat &&
+       git commit -m "init one" &&
+       test_write_lines 1 2 1 2 1 2 1 2 one 2 1 >one_two_repeat &&
+       git commit -a -m "change one" &&
+
+       git diff HEAD~ >Repeat.diff &&
+       git reset --hard HEAD~ &&
+
+       test_write_lines 1 2 1 2 1 2 one 2 1 2 one >one_two_repeat &&
+       git commit -a -m "change surrounding one" &&
+
+       git apply --index --3way Repeat.diff &&
+       test_write_lines 1 2 1 2 1 2 one 2 one 2 one >expect &&
+
+       test_cmp expect one_two_repeat
+'
+
 test_done