From: Junio C Hamano <gitster@pobox.com>
To: Jerry Zhang <jerry@skydio.com>
Cc: git@vger.kernel.org, newren@gmail.com, ross@skydio.com,
abe@skydio.com, brian.kubisiak@skydio.com
Subject: Re: [PATCH] git-apply: try threeway first when "--3way" is used
Date: Mon, 05 Apr 2021 23:13:45 -0700 [thread overview]
Message-ID: <xmqqblas2b52.fsf@gitster.g> (raw)
In-Reply-To: <20210406025551.25213-1-jerry@skydio.com> (Jerry Zhang's message of "Mon, 5 Apr 2021 19:55:51 -0700")
Jerry Zhang <jerry@skydio.com> 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.
next prev parent reply other threads:[~2021-04-06 6:13 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-06 2:55 [PATCH] git-apply: try threeway first when "--3way" is used Jerry Zhang
2021-04-06 6:13 ` Junio C Hamano [this message]
2021-04-06 23:13 ` Junio C Hamano
2021-04-06 6:14 ` Junio C Hamano
2021-04-06 23:25 ` Jerry Zhang
2021-04-07 0:19 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqblas2b52.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=abe@skydio.com \
--cc=brian.kubisiak@skydio.com \
--cc=git@vger.kernel.org \
--cc=jerry@skydio.com \
--cc=newren@gmail.com \
--cc=ross@skydio.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).