{"thread":{"id":"55532","subject":"[PATCH] git-apply: silence errors for success cases","startedAt":"2021-04-21T00:47:48Z","lastAt":"2021-12-11T02:06:05Z","messageCount":5,"participants":["Jerry Zhang","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"422511","messageId":"20210421004733.22395-1-jerry@skydio.com","threadId":"55532","inReplyTo":null,"subject":"[PATCH] git-apply: silence errors for success cases","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-04-21T00:47:33Z","receivedAt":"2021-04-21T00:47:48Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"Certain invocations of \"git apply --3way\"\nwill print error messages even though git\nis able to fall back on apply_fragments and\napply the patch successfully with a return\nvalue of 0. To fix, return early from\ntry_threeway() in the following cases:\n\nWhen the patch is a rename and no lines have\nchanged. In this case, \"git diff\" doesn't\nrecord the blob info, so 3way is neither\npossible nor necessary.\n\nWhen the patch is an addition and there is\nno add/add conflict, i.e. direct_to_threeway\nis false. In this case, threeway will fail\nsince the preimage is not in cache, but isn't\nnecessary anyway since there is no conflict.\n\nOnly error messaging is affected and other\nbehavior does not change.\n\nSigned-off-by: Jerry Zhang <jerry@skydio.com>\n---\n apply.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex 8c5b29809b..a36d4002ca 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3560,7 +3560,9 @@ static int try_threeway(struct apply_state *state,\n \n \t/* No point falling back to 3-way merge in these cases */\n \tif (patch->is_delete ||\n-\t    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode))\n+\t    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode) ||\n+\t    (patch->is_new && !patch->direct_to_threeway) ||\n+\t    (patch->is_rename && !patch->lines_added && !patch->lines_deleted))\n \t\treturn -1;\n \n \t/* Preimage the patch was prepared for */\n-- \n2.29.0\n\n"},{"id":"422519","messageId":"xmqqtuo0z8mc.fsf@gitster.g","threadId":"55532","inReplyTo":"20210421004733.22395-1-jerry@skydio.com","subject":"Re: [PATCH] git-apply: silence errors for success cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-21T02:14:51Z","receivedAt":"2021-04-21T02:14:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jerry Zhang <jerry@skydio.com> writes:\n\n> Certain invocations of \"git apply --3way\"\n> will print error messages even though git\n> is able to fall back on apply_fragments and\n> apply the patch successfully with a return\n> value of 0. To fix, return early from\n> try_threeway() in the following cases:\n\nI suspect that this is a recent breakage after we swapped the order\nof 3way fallback.  It used to be that we tried the straight\napplication first (while suppressing the error messages) and then\nfell back on 3way (while fully exposing the error messages).\n\nIt is understandable if we just swapped the order without changing\nwhen errors are squelched, we may leak unnecessary error messages\nwhile trying 3way and failing.\n\nAfter having written all of the above, I just realized that the\nswapping of the order was _your_ topic and you didn't have to be\nexplained any of the above ;-)  Just consider it as my thinking\naloud for the benefit of those who are reading from the sideline\n(i.e. those not on the To/Cc list but are reading because this\nmessage is sent to git@ mailing list).\n\n"},{"id":"422647","messageId":"xmqqwnsvw5xi.fsf@gitster.g","threadId":"55532","inReplyTo":"xmqqtuo0z8mc.fsf@gitster.g","subject":"Re: [PATCH] git-apply: silence errors for success cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-21T23:53:29Z","receivedAt":"2021-04-21T23:53:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jerry Zhang <jerry@skydio.com> writes:\n>\n>> Certain invocations of \"git apply --3way\"\n>> will print error messages even though git\n>> is able to fall back on apply_fragments and\n>> apply the patch successfully with a return\n>> value of 0. To fix, return early from\n>> try_threeway() in the following cases:\n>\n> I suspect that this is a recent breakage after we swapped the order\n> of 3way fallback.  It used to be that we tried the straight\n> application first (while suppressing the error messages) and then\n> fell back on 3way (while fully exposing the error messages).\n\nThe other side of the coin is that we should say, before falling\nback to non-3way codepath, that we saw 3way application failed, and\nfalling back to the straight patch, if we don't say so already with\nthe recent change, just like we used to say the opposite after seeing\nthe patch application to fail and then fell back to 3way.\n\nIn other words, we may have said, under the verbose mode, that we\nare falling back to threeway only after seeing apply-fragments\nfailed, but if we are doing 3-way first, telling that we are doing\nthree-way application in try_threeway() function is actively wrong.\nThe message was there only because try_threeway() was a fallback and\nsometimes did not happen even when the user asked to do --3way (hence\ndeserved a message).  Now we always try 3way first when the user asked,\nthe extra message should be given only after try_threeway() failed\nand should talk about falling back to the straight application.\n\nIOW, a fix something along this line, perhaps, is needed to complete\nthe \"swap the order between 3way and direct application\" series we\nearlier worked on.\n\n\n apply.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git i/apply.c w/apply.c\nindex 8c5b29809b..6634305e9e 100644\n--- i/apply.c\n+++ w/apply.c\n@@ -3570,7 +3570,7 @@ static int try_threeway(struct apply_state *state,\n \t\t read_blob_object(&buf, &pre_oid, patch->old_mode))\n \t\treturn error(_(\"repository lacks the necessary blob to perform 3-way merge.\"));\n \n-\tif (state->apply_verbosity > verbosity_silent)\n+\tif (state->apply_verbosity > verbosity_silent && patch->direct_to_threeway)\n \t\tfprintf(stderr, _(\"Performing three-way merge...\\n\"));\n \n \timg = strbuf_detach(&buf, &len);\n@@ -3637,6 +3637,9 @@ static int apply_data(struct apply_state *state, struct patch *patch,\n \t\treturn -1;\n \n \tif (!state->threeway || try_threeway(state, &image, patch, st, ce) < 0) {\n+\t\tif (state->threeway && !patch->direct_to_threeway)\n+\t\t\tfprintf(stderr, _(\"Falling back to direct application...\\n\"));\n+\n \t\t/* Note: with --reject, apply_fragments() returns 0 */\n \t\tif (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0)\n \t\t\treturn -1;\n\n\n\n\n"},{"id":"431377","messageId":"20210728031437.14257-1-jerry@skydio.com","threadId":"55532","inReplyTo":"xmqqwnsvw5xi.fsf@gitster.g","subject":"[PATCH V2] git-apply: silence errors for success cases","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-07-28T03:14:37Z","receivedAt":"2021-07-28T03:14:47Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"Certain invocations of \"git apply --3way\"\nwill print error messages even though git\nis able to fall back on apply_fragments and\napply the patch successfully with a return\nvalue of 0. To fix, return early from\ntry_threeway() in the following cases:\n\nWhen the patch is a rename and no lines have\nchanged. In this case, \"git diff\" doesn't\nrecord the blob info, so 3way is neither\npossible nor necessary.\n\nWhen the patch is an addition and there is\nno add/add conflict, i.e. direct_to_threeway\nis false. In this case, threeway will fail\nsince the preimage is not in cache, but isn't\nnecessary anyway since there is no conflict.\n\nOnly error messaging is affected and other\nbehavior does not change.\n\nSigned-off-by: Jerry Zhang <jerry@skydio.com>\n---\nV1->V2: rebase onto master and rerun tests\n\nI think I addressed previous comments. What\nare the next steps for this patch?\n\n apply.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex 44bc31d6eb..fb321c707b 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3560,7 +3560,9 @@ static int try_threeway(struct apply_state *state,\n \n \t/* No point falling back to 3-way merge in these cases */\n \tif (patch->is_delete ||\n-\t    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode))\n+\t    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode) ||\n+\t    (patch->is_new && !patch->direct_to_threeway) ||\n+\t    (patch->is_rename && !patch->lines_added && !patch->lines_deleted))\n \t\treturn -1;\n \n \t/* Preimage the patch was prepared for */\n-- \n2.32.0.1314.g6ed4fcc4cc\n\n"},{"id":"443854","messageId":"CAMKO5CvETNfab9KLU0DDchvdM7JEJupifyKKPbWwz4QqiqcqPg@mail.gmail.com","threadId":"55532","inReplyTo":"20210728031437.14257-1-jerry@skydio.com","subject":"Re: [PATCH V2] git-apply: silence errors for success cases","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-12-11T02:05:51Z","receivedAt":"2021-12-11T02:06:05Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"On Tue, Jul 27, 2021 at 8:14 PM Jerry Zhang <jerry@skydio.com> wrote:\n>\n> Certain invocations of \"git apply --3way\"\n> will print error messages even though git\n> is able to fall back on apply_fragments and\n> apply the patch successfully with a return\n> value of 0. To fix, return early from\n> try_threeway() in the following cases:\n>\n> When the patch is a rename and no lines have\n> changed. In this case, \"git diff\" doesn't\n> record the blob info, so 3way is neither\n> possible nor necessary.\n>\n> When the patch is an addition and there is\n> no add/add conflict, i.e. direct_to_threeway\n> is false. In this case, threeway will fail\n> since the preimage is not in cache, but isn't\n> necessary anyway since there is no conflict.\n>\n> Only error messaging is affected and other\n> behavior does not change.\n>\n> Signed-off-by: Jerry Zhang <jerry@skydio.com>\n> ---\n> V1->V2: rebase onto master and rerun tests\n>\n> I think I addressed previous comments. What\n> are the next steps for this patch?\n>\n>  apply.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/apply.c b/apply.c\n> index 44bc31d6eb..fb321c707b 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -3560,7 +3560,9 @@ static int try_threeway(struct apply_state *state,\n>\n>         /* No point falling back to 3-way merge in these cases */\n>         if (patch->is_delete ||\n> -           S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode))\n> +           S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode) ||\n> +           (patch->is_new && !patch->direct_to_threeway) ||\n> +           (patch->is_rename && !patch->lines_added && !patch->lines_deleted))\n>                 return -1;\n>\n>         /* Preimage the patch was prepared for */\n> --\n> 2.32.0.1314.g6ed4fcc4cc\n>\nAny updates on this patch? I had addressed the previous comment in this\nthread in a different patch \"apply: adjust messages to account for\n--3way changes\"\nthat was already merged in a while ago.\n\nI originally intended this to mainly fix print message behavior, but\nwhile testing\nI found that it also fixed a separately reported issue from this thread:\nhttps://www.spinics.net/lists/git/msg421481.html.\n\nI probably owe an update / refresher on why I originally submitted these\nfeatures in the first place. I've written a tool for branchless / patch stack\nbased code review and development w/github. The core of this is a mechanism\nthat can cherry-pick commits without touching the cache or working tree\nby using \"git apply --cached --3way\" with a custom index file. We've been\nusing this internally with a custom built git for a while now and gotten pretty\ngood feedback. We'd like to open source this tool soon, and it'd be nice\nif our open-source users (eventually) don't need to build git themselves :).\n"}]}