@matheusbsbs / matheus-bagliano / commits / 923cd87ac8

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>

Jerry Zhang committed Apr 6, 2021 at 16:25 UTC 923cd87ac8550a8e277bfeb19198a11b6a8ed854
3 files changed +28 -10
Documentation/git-apply.txt
+2 -3
@@ -84,9 +84,8 @@ OPTIONS
84
85 -3::
86 --3way::
87 - When the patch does not apply cleanly, fall back on 3-way merge if
88 - the patch records the identity of blobs it is supposed to apply to,
89 - and we have those blobs available locally, possibly leaving the
87 + Attempt 3-way merge if the patch records the identity of blobs it is supposed
88 + to apply to and we have those blobs available locally, possibly leaving the
89 conflict markers in the files in the working tree for the user to
90 resolve. This option implies the `--index` option, and is incompatible
91 with the `--reject` and the `--cached` options.
apply.c
+6 -7
@@ -3570,10 +3570,10 @@ static int try_threeway(struct apply_state *state,
3570 write_object_file("", 0, blob_type, &pre_oid);
3571 else if (get_oid(patch->old_oid_prefix, &pre_oid) ||
3572 read_blob_object(&buf, &pre_oid, patch->old_mode))
3573 - return error(_("repository lacks the necessary blob to fall back on 3-way merge."));
3573 + return error(_("repository lacks the necessary blob to perform 3-way merge."));
3574
3575 if (state->apply_verbosity > verbosity_silent)
3576 - fprintf(stderr, _("Falling back to three-way merge...\n"));
3576 + fprintf(stderr, _("Performing three-way merge...\n"));
3577
3578 img = strbuf_detach(&buf, &len);
3579 prepare_image(&tmp_image, img, len, 1);
@@ -3605,7 +3605,7 @@ static int try_threeway(struct apply_state *state,
3605 if (status < 0) {
3606 if (state->apply_verbosity > verbosity_silent)
3607 fprintf(stderr,
3608 - _("Failed to fall back on three-way merge...\n"));
3608 + _("Failed to perform three-way merge...\n"));
3609 return status;
3610 }
3611
@@ -3638,10 +3638,9 @@ static int apply_data(struct apply_state *state, struct patch *patch,
3638 if (load_preimage(state, &image, patch, st, ce) < 0)
3639 return -1;
3640
3641 - if (patch->direct_to_threeway ||
3642 - apply_fragments(state, &image, patch) < 0) {
3641 + if (!state->threeway || try_threeway(state, &image, patch, st, ce) < 0) {
3642 /* Note: with --reject, apply_fragments() returns 0 */
3644 - if (!state->threeway || try_threeway(state, &image, patch, st, ce) < 0)
3643 + if (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0)
3644 return -1;
3645 }
3646 patch->result = image.buf;
@@ -5018,7 +5017,7 @@ int apply_parse_options(int argc, const char **argv,
5017 OPT_BOOL(0, "apply", force_apply,
5018 N_("also apply the patch (use with --stat/--summary/--check)")),
5019 OPT_BOOL('3', "3way", &state->threeway,
5021 - N_( "attempt three-way merge if a patch does not apply")),
5020 + N_( "attempt three-way merge, fall back on normal patch if that fails")),
5021 OPT_FILENAME(0, "build-fake-ancestor", &state->fake_ancestor,
5022 N_("build a temporary index based on embedded index information")),
5023 /* Think twice before adding "--nul" synonym to this */
t/t4108-apply-threeway.sh
+20
@@ -160,4 +160,24 @@ test_expect_success 'apply -3 with add/add conflict (dirty working tree)' '
160 test_cmp three.save three
161 '
162
163 +test_expect_success 'apply -3 with ambiguous repeating file' '
164 + git reset --hard &&
165 + test_write_lines 1 2 1 2 1 2 1 2 1 2 1 >one_two_repeat &&
166 + git add one_two_repeat &&
167 + git commit -m "init one" &&
168 + test_write_lines 1 2 1 2 1 2 1 2 one 2 1 >one_two_repeat &&
169 + git commit -a -m "change one" &&
170 +
171 + git diff HEAD~ >Repeat.diff &&
172 + git reset --hard HEAD~ &&
173 +
174 + test_write_lines 1 2 1 2 1 2 one 2 1 2 one >one_two_repeat &&
175 + git commit -a -m "change surrounding one" &&
176 +
177 + git apply --index --3way Repeat.diff &&
178 + test_write_lines 1 2 1 2 1 2 one 2 one 2 one >expect &&
179 +
180 + test_cmp expect one_two_repeat
181 +'
182 +
183 test_done