* [PATCH] git-apply: try threeway first when "--3way" is used
@ 2021-04-06 2:55 Jerry Zhang
2021-04-06 6:13 ` Junio C Hamano
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jerry Zhang @ 2021-04-06 2:55 UTC (permalink / raw)
To: git, newren, gitster; +Cc: ross, abe, brian.kubisiak, Jerry Zhang
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
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
error (but not in the case of a merge conflict).
Signed-off-by: Jerry Zhang <jerry@skydio.com>
---
Documentation/git-apply.txt | 5 ++---
apply.c | 13 ++++++-------
t/t4108-apply-threeway.sh | 20 ++++++++++++++++++++
3 files changed, 28 insertions(+), 10 deletions(-)
diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index 91d9a8601c8c316d4649c405af42e531c39991a8..9144575299c264dd299b542b7b5948eef35f211c 100644
--- 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 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."));
if (state->apply_verbosity > verbosity_silent)
- fprintf(stderr, _("Falling back to three-way merge...\n"));
+ fprintf(stderr, _("Doing three-way merge...\n"));
img = strbuf_detach(&buf, &len);
prepare_image(&tmp_image, img, len, 1);
@@ -3604,7 +3604,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 do three-way merge...\n"));
return status;
}
@@ -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) {
+ 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;
@@ -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")),
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..0a7332fed5f60a8a2c9c25fc6713d513c3f0ace1 100755
--- 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
--
2.29.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] git-apply: try threeway first when "--3way" is used
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
2021-04-06 23:13 ` Junio C Hamano
2021-04-06 6:14 ` Junio C Hamano
2021-04-06 23:25 ` Jerry Zhang
2 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2021-04-06 6:13 UTC (permalink / raw)
To: Jerry Zhang; +Cc: git, newren, ross, abe, brian.kubisiak
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] git-apply: try threeway first when "--3way" is used
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
@ 2021-04-06 6:14 ` Junio C Hamano
2021-04-06 23:25 ` Jerry Zhang
2 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2021-04-06 6:14 UTC (permalink / raw)
To: Jerry Zhang; +Cc: git, newren, ross, abe, brian.kubisiak
Jerry Zhang <jerry@skydio.com> writes:
> diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
> index d62db3fbe16f35a625a4a14eebb70034f695d3eb..0a7332fed5f60a8a2c9c25fc6713d513c3f0ace1 100755
> --- 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 &&
Missing SP before '>' (same issue on other redirections below).
> + 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] git-apply: try threeway first when "--3way" is used
2021-04-06 6:13 ` Junio C Hamano
@ 2021-04-06 23:13 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2021-04-06 23:13 UTC (permalink / raw)
To: Jerry Zhang; +Cc: git, newren, ross, abe, brian.kubisiak
Junio C Hamano <gitster@pobox.com> writes:
> Will queue. Thanks.
Just to avoid confusion, "Will queue" does not mean "No further
updates are necessary from you".
It is a short-hand to say "Even though the version I just reviewed
may still want to be improved, it is in good enough shape to be
tested with other topics on the 'seen' branch, so I'll do so
primarily to see if there are any funny interactions with them".
IOW, I expect the patch to be rerolled before it is ready to be
merged to 'next' and below.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] git-apply: try threeway first when "--3way" is used
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
2021-04-06 6:14 ` Junio C Hamano
@ 2021-04-06 23:25 ` Jerry Zhang
2021-04-07 0:19 ` Junio C Hamano
2 siblings, 1 reply; 6+ messages in thread
From: Jerry Zhang @ 2021-04-06 23:25 UTC (permalink / raw)
To: git, newren, gitster; +Cc: ross, abe, brian.kubisiak, Jerry Zhang
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>
---
Documentation/git-apply.txt | 5 ++---
apply.c | 13 ++++++-------
t/t4108-apply-threeway.sh | 20 ++++++++++++++++++++
3 files changed, 28 insertions(+), 10 deletions(-)
diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index 91d9a8601c8c316d4649c405af42e531c39991a8..9144575299c264dd299b542b7b5948eef35f211c 100644
--- 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 6695a931e979a968b28af88d425d0c76ba17d0d4..9bd4efcbced842d2c5c030a0f2178ddb36114600 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 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);
@@ -3604,7 +3604,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;
}
@@ -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) {
+ 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;
@@ -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")),
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
--- 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
--
2.29.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] git-apply: try threeway first when "--3way" is used
2021-04-06 23:25 ` Jerry Zhang
@ 2021-04-07 0:19 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2021-04-07 0:19 UTC (permalink / raw)
To: Jerry Zhang; +Cc: git, newren, ross, abe
Jerry Zhang <jerry@skydio.com> writes:
> Subject: Re: [PATCH] git-apply: try threeway first when "--3way" is used
Just for future reference, it is customery to start with [PATCH v2],
[PATCH v3], etc. when sending an updated patch to make sure it is
obvious to readers of the list which one is the latest.
> 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.
I think this version addresses all issues I noticed in the previous
version. Unless somebody else finds some more issues in a coming
few days, let's declare victory and merge it down to 'next'.
By the way, as my last response bounced for the address
brian.kubisiak@skydio.com you had on the CC list, I'm excluding it
from the Cc list of this message.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-07 0:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
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).