Jerry Zhang <jerry@xxxxxxxxxx> 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.
> 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?
> @@ -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.
> @@ -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.
