From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1B08CC433B4 for ; Mon, 12 Apr 2021 15:41:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id ECDEE6124C for ; Mon, 12 Apr 2021 15:41:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S242174AbhDLPlY (ORCPT ); Mon, 12 Apr 2021 11:41:24 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42480 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238815AbhDLPlX (ORCPT ); Mon, 12 Apr 2021 11:41:23 -0400 Received: from mail-oi1-x22c.google.com (mail-oi1-x22c.google.com [IPv6:2607:f8b0:4864:20::22c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D9E72C061574 for ; Mon, 12 Apr 2021 08:41:03 -0700 (PDT) Received: by mail-oi1-x22c.google.com with SMTP id d12so13804301oiw.12 for ; Mon, 12 Apr 2021 08:41:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=l2dTZ/YtjKOJa0z6KfANiU1owYYpjeXP076jU8TJ+tw=; b=BM8zioePJX13uTJU7GS0fJ1K5s0SM/PFlhT56oWiS7Nm2XlVTAj2GadWK4a2HY808x X+KOkkf5Ni0TRR4zP3CV1WL+YieR0VX6qde4Pfk1EMZC2YHA/vikAL5L7JGjgFjtfvy2 pVoOoSzdeI46rNRRTgZfvyJ8/cNY1yIWaSSh367oD8FSkC6djt8m/1cHMGcfMEfsQ9Vq 0iFJ0J1jm/nTQ7hE130bJOWxtS/cDD8SYVHmsh/vPSB/h6agKCcB6zAUXDJwI3C07lhN xbMOnWj9Nz/Iu36OwJfVt1RVLqHN2FOlgf5BWKKXdCInONVd3m3xtctYShUXRqEKEASA oNKA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=l2dTZ/YtjKOJa0z6KfANiU1owYYpjeXP076jU8TJ+tw=; b=sNkeW1FyLjtXceX65TYyGrKwlFf85abajr8J7+sLea0fuKF1oATmDce0YstXnu38oy IrVwBgxZSPUCtSe8ZP4WerDaFmOA+XVgUsZABZmzDGwz9k+FyTcMB3ResjsDtx69BSCi md6nUVUjuOg7X4dT6OVGw5cnONjSpFDnOrI0OO6zLwY8grxGef758zR5gbdY6iyjkBys Kw9Ax91+KD423GPwbWWi3m3mCnSJJOxs7X9+9nlK/tVXVjNK0O484r51ZBGU39ZPvqEl sbOHN1CUiGPKFp2NLJN0snluVx7ms4RykhgJG5NVpMlZiKcRcLK7ruyqT37GULRD5LQI k2PQ== X-Gm-Message-State: AOAM5332Tx2fioiBKe75mt9ZBHEmEuD9CWODSGQ7DuTLnhKdXYkm8SMX /sTi/g1Ri9Lv7Z6+dC5b8nJ6zA29hnlofUI8dOQ= X-Google-Smtp-Source: ABdhPJxW3YRxPhL8s/E6/hh9JvRLZlez4N8Nz2x4xsSyjIo5j23vI1SgA4Fm6O8D2oLwfJg6Y2RKfWiuIqT54TyaMrA= X-Received: by 2002:a05:6808:a8a:: with SMTP id q10mr19604705oij.167.1618242063184; Mon, 12 Apr 2021 08:41:03 -0700 (PDT) MIME-Version: 1.0 References: <20210407180349.10173-1-jerry@skydio.com> <20210408021344.8053-1-jerry@skydio.com> In-Reply-To: <20210408021344.8053-1-jerry@skydio.com> From: Elijah Newren Date: Mon, 12 Apr 2021 08:40:52 -0700 Message-ID: Subject: Re: [PATCH v5] git-apply: allow simultaneous --cached and --3way options To: Jerry Zhang Cc: Git Mailing List , Junio C Hamano , ross@skydio.com, Abraham Bachrach , brian.kubisiak@skydio.com Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Wed, Apr 7, 2021 at 7:13 PM Jerry Zhang 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 > --- > 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=:: > 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.