Re: [PATCH] git-apply: try threeway first when "--3way" is used
From: Junio C Hamano <hidden>
Date: 2021-04-06 06:13:50
Jerry Zhang [off-list ref] writes:
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 can apply it correctly
Is that "can apply"? Isn't it "has a better chance to correctly apply"?
or show a conflict. 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 caseswhere blobs are not available or some other
Missing SP before two words.
error (but not in the case of a merge conflict).
We may want to note a possible backward compatibility fallout to warn reviewers here in the proposed log message.
-3:: --3way:: + 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.
OK. This patch obviously expects it to graduate before the other "--3way and --cached at the same time" patch.
quoted hunk
diff --git a/apply.c b/apply.c index 6695a931e979a968b28af88d425d0c76ba17d0d4..62d65ef8d9c0b68857db55198c73db1f41589df1 100644 --- a/apply.c +++ b/apply.c@@ -3569,10 +3569,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 do 3-way merge."));
s/do/perform/ perhaps?
quoted hunk
@@ -3637,10 +3637,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) {
The original was "If the logic flow that came before us already decided we should skip the straight application of the patch and jump directly to the three-way codepath. Otherwise try the straight application and perform 3-way only when it fails". The "direct-to-threeway" logic was introduced by 099f3c42 (apply: --3way with add/add conflict, 2012-06-07).
+ 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;This says something different. "If 3-way was not asked, jump directly to inside the block. Otherwise, try 3-way first, and go inside the block only if 3-way did not work." And the inside the block is the straight patch application. It says "if we have already decided we should do the 3-way and nothing else, just fail. Otherwise try the straight patch application and if it fails, then fail the whole thing." This looks like a correct "inversion" of the fallback codepath.
quoted hunk
@@ -5017,7 +5016,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")),
OK. Overall, the change is very cleanly done. Will queue. Thanks.