| Message ID | 20210408021344.8053-1-jerry@skydio.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] git-apply: allow simultaneous --cached and --3way options | expand |
Jerry Zhang <jerry@skydio.com> writes: > Documentation/git-apply.txt | 6 +++-- > apply.c | 9 ++++--- > t/t4108-apply-threeway.sh | 50 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 59 insertions(+), 6 deletions(-) Nicely done. Will queue. Elijah, how does this round look to you? > diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt > index 9144575299c264dd299b542b7b5948eef35f211c..aa1ae56a25e0428cabcfa2539900ef2a09abcb7c 100644 > --- a/Documentation/git-apply.txt > +++ b/Documentation/git-apply.txt > @@ -87,8 +87,10 @@ OPTIONS > 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. > + resolve. This option implies the `--index` option unless the > + `--cached` option is used, and is incompatible with the `--reject` option. > + When used with the `--cached` option, any conflicts are left at higher stages > + in the cache. > > --build-fake-ancestor=<file>:: > Newer 'git diff' output has embedded 'index information' > diff --git a/apply.c b/apply.c > index 9bd4efcbced842d2c5c030a0f2178ddb36114600..dadab80ec967357b031657d4e3d0ae52fac11411 100644 > --- a/apply.c > +++ b/apply.c > @@ -133,8 +133,6 @@ int check_apply_state(struct apply_state *state, int force_apply) > > if (state->apply_with_reject && state->threeway) > return error(_("--reject and --3way cannot be used together.")); > - if (state->cached && state->threeway) > - return error(_("--cached and --3way cannot be used together.")); > if (state->threeway) { > if (is_not_gitdir) > return error(_("--3way outside a repository")); > @@ -4644,8 +4642,11 @@ static int write_out_results(struct apply_state *state, struct patch *list) > fprintf(stderr, "U %s\n", item->string); > } > string_list_clear(&cpath, 0); > - > - repo_rerere(state->repo, 0); > + /* Rerere relies on the partially merged result being in the working tree > + * with conflict markers, but that isn't written with --cached. > + */ > + if (!state->cached) > + repo_rerere(state->repo, 0); > } > > return errs; > diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh > index 9ff313f976422f9c12dc8032d14567b54cfe3765..65147efdea9a00e30d156e6f4d5d72a3987f230d 100755 > --- a/t/t4108-apply-threeway.sh > +++ b/t/t4108-apply-threeway.sh > @@ -180,4 +180,54 @@ test_expect_success 'apply -3 with ambiguous repeating file' ' > test_cmp expect one_two_repeat > ' > > +test_expect_success 'apply with --3way --cached clean apply' ' > + # Merging side should be similar to applying this patch > + git diff ...side >P.diff && > + > + # The corresponding cleanly applied merge > + git reset --hard && > + git checkout main~ && > + git merge --no-commit side && > + git ls-files -s >expect.ls && > + > + # should succeed > + git reset --hard && > + git checkout main~ && > + git apply --cached --3way P.diff && > + git ls-files -s >actual.ls && > + print_sanitized_conflicted_diff >actual.diff && > + > + # The cache should resemble the corresponding merge > + # (both files at stage #0) > + test_cmp expect.ls actual.ls && > + # However the working directory should not change > + >expect.diff && > + test_cmp expect.diff actual.diff > +' > + > +test_expect_success 'apply with --3way --cached and conflicts' ' > + # Merging side should be similar to applying this patch > + git diff ...side >P.diff && > + > + # The corresponding conflicted merge > + git reset --hard && > + git checkout main^0 && > + test_must_fail git merge --no-commit side && > + git ls-files -s >expect.ls && > + > + # should fail to apply > + git reset --hard && > + git checkout main^0 && > + test_must_fail git apply --cached --3way P.diff && > + git ls-files -s >actual.ls && > + print_sanitized_conflicted_diff >actual.diff && > + > + # The cache should resemble the corresponding merge > + # (one file at stage #0, one file at stages #1 #2 #3) > + test_cmp expect.ls actual.ls && > + # However the working directory should not change > + >expect.diff && > + test_cmp expect.diff actual.diff > +' > + > test_done
On Wed, Apr 7, 2021 at 7:13 PM Jerry Zhang <jerry@skydio.com> wrote: > > "git apply" does not allow "--cached" and > "--3way" to be used together, since "--3way" > writes conflict markers into the working tree. > > Allow "git apply" to accept "--cached" and > "--3way" at the same time. When a single file > auto-resolves cleanly, the result is placed in the > index at stage #0 and the command exits with 0 > status. Should this instead read: "...placed in the index at stage #0. If all files auto-resolve cleanly, the command exits with 0 status." or something like that? > For a file that has a conflict which > cannot be cleanly auto-resolved, the original > contents from common ancestor (stage #1), our > version (stage #2) and the contents from the > patch (stage #3) are left at separate stages. > No attempt is made to resolve the conflict at > the content level, and the command exists with s/exists/exits/ > non-zero status, because there is no place > (like the working tree) to leave a half-resolved > merge for the user to resolve. > > The user can use `git diff` to view the contents > of the conflict, or `git checkout -m -- .` to > regenerate the conflict markers in the working > directory. > > Don't attempt rerere in this case since it depends > on conflict markers written to file for its database > storage and lookup. There would be two main changes > required to get rerere working: > 1. Allow the rerere api to accept in memory object > rather than files, which would allow us to pass in > the conflict markers contained in the result from > ll_merge(). > 2. Rerere can't write to the working directory, so > it would have to apply the result to cache stage #0 > directly. A flag would be needed to control this. > > Signed-off-by: Jerry Zhang <jerry@skydio.com> > --- > Patch applies on top of > "[PATCH v2] git-apply: try threeway first when "--3way"" > Main merge conflict is the addition of multiple > tests at the bottom of the file. > > v4->v5: > > Updated in file comment about rerere > Added test for cleanly applying patch (should return 0) > Previous test captured case where patch fails to apply > due to 1 conflicting file and 1 cleanly applying file > (returns 1). > > Documentation/git-apply.txt | 6 +++-- > apply.c | 9 ++++--- > t/t4108-apply-threeway.sh | 50 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 59 insertions(+), 6 deletions(-) > > diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt > index 9144575299c264dd299b542b7b5948eef35f211c..aa1ae56a25e0428cabcfa2539900ef2a09abcb7c 100644 > --- a/Documentation/git-apply.txt > +++ b/Documentation/git-apply.txt > @@ -87,8 +87,10 @@ OPTIONS > 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. > + resolve. This option implies the `--index` option unless the > + `--cached` option is used, and is incompatible with the `--reject` option. > + When used with the `--cached` option, any conflicts are left at higher stages > + in the cache. > > --build-fake-ancestor=<file>:: > Newer 'git diff' output has embedded 'index information' > diff --git a/apply.c b/apply.c > index 9bd4efcbced842d2c5c030a0f2178ddb36114600..dadab80ec967357b031657d4e3d0ae52fac11411 100644 > --- a/apply.c > +++ b/apply.c > @@ -133,8 +133,6 @@ int check_apply_state(struct apply_state *state, int force_apply) > > if (state->apply_with_reject && state->threeway) > return error(_("--reject and --3way cannot be used together.")); > - if (state->cached && state->threeway) > - return error(_("--cached and --3way cannot be used together.")); > if (state->threeway) { > if (is_not_gitdir) > return error(_("--3way outside a repository")); > @@ -4644,8 +4642,11 @@ static int write_out_results(struct apply_state *state, struct patch *list) > fprintf(stderr, "U %s\n", item->string); > } > string_list_clear(&cpath, 0); > - > - repo_rerere(state->repo, 0); > + /* Rerere relies on the partially merged result being in the working tree > + * with conflict markers, but that isn't written with --cached. > + */ > + if (!state->cached) > + repo_rerere(state->repo, 0); > } > > return errs; > diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh > index 9ff313f976422f9c12dc8032d14567b54cfe3765..65147efdea9a00e30d156e6f4d5d72a3987f230d 100755 > --- a/t/t4108-apply-threeway.sh > +++ b/t/t4108-apply-threeway.sh > @@ -180,4 +180,54 @@ test_expect_success 'apply -3 with ambiguous repeating file' ' > test_cmp expect one_two_repeat > ' > > +test_expect_success 'apply with --3way --cached clean apply' ' > + # Merging side should be similar to applying this patch > + git diff ...side >P.diff && > + > + # The corresponding cleanly applied merge > + git reset --hard && > + git checkout main~ && > + git merge --no-commit side && > + git ls-files -s >expect.ls && > + > + # should succeed > + git reset --hard && > + git checkout main~ && > + git apply --cached --3way P.diff && > + git ls-files -s >actual.ls && > + print_sanitized_conflicted_diff >actual.diff && > + > + # The cache should resemble the corresponding merge > + # (both files at stage #0) > + test_cmp expect.ls actual.ls && > + # However the working directory should not change > + >expect.diff && > + test_cmp expect.diff actual.diff > +' > + > +test_expect_success 'apply with --3way --cached and conflicts' ' > + # Merging side should be similar to applying this patch > + git diff ...side >P.diff && > + > + # The corresponding conflicted merge > + git reset --hard && > + git checkout main^0 && > + test_must_fail git merge --no-commit side && > + git ls-files -s >expect.ls && > + > + # should fail to apply > + git reset --hard && > + git checkout main^0 && > + test_must_fail git apply --cached --3way P.diff && > + git ls-files -s >actual.ls && > + print_sanitized_conflicted_diff >actual.diff && > + > + # The cache should resemble the corresponding merge > + # (one file at stage #0, one file at stages #1 #2 #3) > + test_cmp expect.ls actual.ls && > + # However the working directory should not change > + >expect.diff && > + test_cmp expect.diff actual.diff > +' > + > test_done > -- > 2.29.0 Otherwise, looks good to me.
On Thu, Apr 8, 2021 at 6:33 AM Junio C Hamano <gitster@pobox.com> wrote: > > Jerry Zhang <jerry@skydio.com> writes: > > > Documentation/git-apply.txt | 6 +++-- > > apply.c | 9 ++++--- > > t/t4108-apply-threeway.sh | 50 +++++++++++++++++++++++++++++++++++++ > > 3 files changed, 59 insertions(+), 6 deletions(-) > > Nicely done. Will queue. > > Elijah, how does this round look to you? Sorry for the delay; modulo two minor issues with the commit message it looks good to me. This change won't allow git-apply to handle upstream renames, and makes me wonder if we should lift the fall_back_threeway() logic out of builtin/am.c and use it here. This change also makes me wonder if we should change git-am's --3way flag to make it not be treated as a fallback to be consistent with what we are doing here. But neither of those changes need to be part of this patch. > > diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt > > index 9144575299c264dd299b542b7b5948eef35f211c..aa1ae56a25e0428cabcfa2539900ef2a09abcb7c 100644 > > --- a/Documentation/git-apply.txt > > +++ b/Documentation/git-apply.txt > > @@ -87,8 +87,10 @@ OPTIONS > > 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. > > + resolve. This option implies the `--index` option unless the > > + `--cached` option is used, and is incompatible with the `--reject` option. > > + When used with the `--cached` option, any conflicts are left at higher stages > > + in the cache. > > > > --build-fake-ancestor=<file>:: > > Newer 'git diff' output has embedded 'index information' > > diff --git a/apply.c b/apply.c > > index 9bd4efcbced842d2c5c030a0f2178ddb36114600..dadab80ec967357b031657d4e3d0ae52fac11411 100644 > > --- a/apply.c > > +++ b/apply.c > > @@ -133,8 +133,6 @@ int check_apply_state(struct apply_state *state, int force_apply) > > > > if (state->apply_with_reject && state->threeway) > > return error(_("--reject and --3way cannot be used together.")); > > - if (state->cached && state->threeway) > > - return error(_("--cached and --3way cannot be used together.")); > > if (state->threeway) { > > if (is_not_gitdir) > > return error(_("--3way outside a repository")); > > @@ -4644,8 +4642,11 @@ static int write_out_results(struct apply_state *state, struct patch *list) > > fprintf(stderr, "U %s\n", item->string); > > } > > string_list_clear(&cpath, 0); > > - > > - repo_rerere(state->repo, 0); > > + /* Rerere relies on the partially merged result being in the working tree > > + * with conflict markers, but that isn't written with --cached. > > + */ > > + if (!state->cached) > > + repo_rerere(state->repo, 0); > > } > > > > return errs; > > diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh > > index 9ff313f976422f9c12dc8032d14567b54cfe3765..65147efdea9a00e30d156e6f4d5d72a3987f230d 100755 > > --- a/t/t4108-apply-threeway.sh > > +++ b/t/t4108-apply-threeway.sh > > @@ -180,4 +180,54 @@ test_expect_success 'apply -3 with ambiguous repeating file' ' > > test_cmp expect one_two_repeat > > ' > > > > +test_expect_success 'apply with --3way --cached clean apply' ' > > + # Merging side should be similar to applying this patch > > + git diff ...side >P.diff && > > + > > + # The corresponding cleanly applied merge > > + git reset --hard && > > + git checkout main~ && > > + git merge --no-commit side && > > + git ls-files -s >expect.ls && > > + > > + # should succeed > > + git reset --hard && > > + git checkout main~ && > > + git apply --cached --3way P.diff && > > + git ls-files -s >actual.ls && > > + print_sanitized_conflicted_diff >actual.diff && > > + > > + # The cache should resemble the corresponding merge > > + # (both files at stage #0) > > + test_cmp expect.ls actual.ls && > > + # However the working directory should not change > > + >expect.diff && > > + test_cmp expect.diff actual.diff > > +' > > + > > +test_expect_success 'apply with --3way --cached and conflicts' ' > > + # Merging side should be similar to applying this patch > > + git diff ...side >P.diff && > > + > > + # The corresponding conflicted merge > > + git reset --hard && > > + git checkout main^0 && > > + test_must_fail git merge --no-commit side && > > + git ls-files -s >expect.ls && > > + > > + # should fail to apply > > + git reset --hard && > > + git checkout main^0 && > > + test_must_fail git apply --cached --3way P.diff && > > + git ls-files -s >actual.ls && > > + print_sanitized_conflicted_diff >actual.diff && > > + > > + # The cache should resemble the corresponding merge > > + # (one file at stage #0, one file at stages #1 #2 #3) > > + test_cmp expect.ls actual.ls && > > + # However the working directory should not change > > + >expect.diff && > > + test_cmp expect.diff actual.diff > > +' > > + > > test_done
Elijah Newren <newren@gmail.com> writes: > Sorry for the delay; modulo two minor issues with the commit message > it looks good to me. Thanks.
Elijah Newren <newren@gmail.com> writes: > On Wed, Apr 7, 2021 at 7:13 PM Jerry Zhang <jerry@skydio.com> wrote: >> >> "git apply" does not allow "--cached" and >> "--3way" to be used together, since "--3way" >> writes conflict markers into the working tree. >> >> Allow "git apply" to accept "--cached" and >> "--3way" at the same time. When a single file >> auto-resolves cleanly, the result is placed in the >> index at stage #0 and the command exits with 0 >> status. > > Should this instead read: > "...placed in the index at stage #0. If all files auto-resolve > cleanly, the command exits with 0 status." > or something like that? Perhaps. >> For a file that has a conflict which >> cannot be cleanly auto-resolved, the original >> contents from common ancestor (stage #1), our >> version (stage #2) and the contents from the >> patch (stage #3) are left at separate stages. >> No attempt is made to resolve the conflict at >> the content level, and the command exists with > > s/exists/exits/ Will squash it in.
diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt index 9144575299c264dd299b542b7b5948eef35f211c..aa1ae56a25e0428cabcfa2539900ef2a09abcb7c 100644 --- a/Documentation/git-apply.txt +++ b/Documentation/git-apply.txt @@ -87,8 +87,10 @@ OPTIONS 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. + resolve. This option implies the `--index` option unless the + `--cached` option is used, and is incompatible with the `--reject` option. + When used with the `--cached` option, any conflicts are left at higher stages + in the cache. --build-fake-ancestor=<file>:: Newer 'git diff' output has embedded 'index information' diff --git a/apply.c b/apply.c index 9bd4efcbced842d2c5c030a0f2178ddb36114600..dadab80ec967357b031657d4e3d0ae52fac11411 100644 --- a/apply.c +++ b/apply.c @@ -133,8 +133,6 @@ int check_apply_state(struct apply_state *state, int force_apply) if (state->apply_with_reject && state->threeway) return error(_("--reject and --3way cannot be used together.")); - if (state->cached && state->threeway) - return error(_("--cached and --3way cannot be used together.")); if (state->threeway) { if (is_not_gitdir) return error(_("--3way outside a repository")); @@ -4644,8 +4642,11 @@ static int write_out_results(struct apply_state *state, struct patch *list) fprintf(stderr, "U %s\n", item->string); } string_list_clear(&cpath, 0); - - repo_rerere(state->repo, 0); + /* Rerere relies on the partially merged result being in the working tree + * with conflict markers, but that isn't written with --cached. + */ + if (!state->cached) + repo_rerere(state->repo, 0); } return errs; diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh index 9ff313f976422f9c12dc8032d14567b54cfe3765..65147efdea9a00e30d156e6f4d5d72a3987f230d 100755 --- a/t/t4108-apply-threeway.sh +++ b/t/t4108-apply-threeway.sh @@ -180,4 +180,54 @@ test_expect_success 'apply -3 with ambiguous repeating file' ' test_cmp expect one_two_repeat ' +test_expect_success 'apply with --3way --cached clean apply' ' + # Merging side should be similar to applying this patch + git diff ...side >P.diff && + + # The corresponding cleanly applied merge + git reset --hard && + git checkout main~ && + git merge --no-commit side && + git ls-files -s >expect.ls && + + # should succeed + git reset --hard && + git checkout main~ && + git apply --cached --3way P.diff && + git ls-files -s >actual.ls && + print_sanitized_conflicted_diff >actual.diff && + + # The cache should resemble the corresponding merge + # (both files at stage #0) + test_cmp expect.ls actual.ls && + # However the working directory should not change + >expect.diff && + test_cmp expect.diff actual.diff +' + +test_expect_success 'apply with --3way --cached and conflicts' ' + # Merging side should be similar to applying this patch + git diff ...side >P.diff && + + # The corresponding conflicted merge + git reset --hard && + git checkout main^0 && + test_must_fail git merge --no-commit side && + git ls-files -s >expect.ls && + + # should fail to apply + git reset --hard && + git checkout main^0 && + test_must_fail git apply --cached --3way P.diff && + git ls-files -s >actual.ls && + print_sanitized_conflicted_diff >actual.diff && + + # The cache should resemble the corresponding merge + # (one file at stage #0, one file at stages #1 #2 #3) + test_cmp expect.ls actual.ls && + # However the working directory should not change + >expect.diff && + test_cmp expect.diff actual.diff +' + test_done
"git apply" does not allow "--cached" and "--3way" to be used together, since "--3way" writes conflict markers into the working tree. Allow "git apply" to accept "--cached" and "--3way" at the same time. When a single file auto-resolves cleanly, the result is placed in the index at stage #0 and the command exits with 0 status. For a file that has a conflict which cannot be cleanly auto-resolved, the original contents from common ancestor (stage #1), our version (stage #2) and the contents from the patch (stage #3) are left at separate stages. No attempt is made to resolve the conflict at the content level, and the command exists with non-zero status, because there is no place (like the working tree) to leave a half-resolved merge for the user to resolve. The user can use `git diff` to view the contents of the conflict, or `git checkout -m -- .` to regenerate the conflict markers in the working directory. Don't attempt rerere in this case since it depends on conflict markers written to file for its database storage and lookup. There would be two main changes required to get rerere working: 1. Allow the rerere api to accept in memory object rather than files, which would allow us to pass in the conflict markers contained in the result from ll_merge(). 2. Rerere can't write to the working directory, so it would have to apply the result to cache stage #0 directly. A flag would be needed to control this. Signed-off-by: Jerry Zhang <jerry@skydio.com> --- Patch applies on top of "[PATCH v2] git-apply: try threeway first when "--3way"" Main merge conflict is the addition of multiple tests at the bottom of the file. v4->v5: Updated in file comment about rerere Added test for cleanly applying patch (should return 0) Previous test captured case where patch fails to apply due to 1 conflicting file and 1 cleanly applying file (returns 1). Documentation/git-apply.txt | 6 +++-- apply.c | 9 ++++--- t/t4108-apply-threeway.sh | 50 +++++++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 6 deletions(-)