threads / patch / 55532

patchgit-apply: silence errors for success cases

Subject: [PATCH] git-apply: silence errors for success cases

## tl;dr

5 messages between Apr 21, 2021 and Dec 11, 2021. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Jerry Zhang· Apr 21, 2021, 00:47 UTC · lore

Certain invocations of "git apply --3way" will print error messages even though git is able to fall back on apply_fragments and apply the patch successfully with a return value of 0. To fix, return early from try_threeway() in the following cases:

When the patch is a rename and no lines have changed. In this case, "git diff" doesn't record the blob info, so 3way is neither possible nor necessary.

When the patch is an addition and there is no add/add conflict, i.e. direct_to_threeway is false. In this case, threeway will fail since the preimage is not in cache, but isn't necessary anyway since there is no conflict.

Only error messaging is affected and other behavior does not change.

Signed-off-by: Jerry Zhang <jerry@skydio.com>
---
 apply.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Show changes to apply.c +3 −1

diff --git a/apply.c b/apply.c
index 8c5b29809b..a36d4002ca 100644
--- a/apply.c
+++ b/apply.c
@@ -3560,7 +3560,9 @@ static int try_threeway(struct apply_state *state,
 
 	/* No point falling back to 3-way merge in these cases */
 	if (patch->is_delete ||
-	    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode))
+	    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode) ||
+	    (patch->is_new && !patch->direct_to_threeway) ||
+	    (patch->is_rename && !patch->lines_added && !patch->lines_deleted))
 		return -1;
 
 	/* Preimage the patch was prepared for */

-- 
2.29.0

Junio C Hamano· Apr 21, 2021, 02:14 UTC · re: Jerry Zhang · lore

Re: [PATCH] git-apply: silence errors for success cases

Jerry Zhang <jerry@skydio.com> writes:

Show 6 quoted lines

> Certain invocations of "git apply --3way"
> will print error messages even though git
> is able to fall back on apply_fragments and
> apply the patch successfully with a return
> value of 0. To fix, return early from
> try_threeway() in the following cases:

I suspect that this is a recent breakage after we swapped the order of 3way fallback. It used to be that we tried the straight application first (while suppressing the error messages) and then fell back on 3way (while fully exposing the error messages).

It is understandable if we just swapped the order without changing when errors are squelched, we may leak unnecessary error messages while trying 3way and failing.

After having written all of the above, I just realized that the swapping of the order was _your_ topic and you didn't have to be explained any of the above ;-) Just consider it as my thinking aloud for the benefit of those who are reading from the sideline (i.e. those not on the To/Cc list but are reading because this message is sent to git@ mailing list).

Junio C Hamano· Apr 21, 2021, 23:53 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-apply: silence errors for success cases

Junio C Hamano <gitster@pobox.com> writes:

Show 13 quoted lines

> Jerry Zhang <jerry@skydio.com> writes:
>
>> Certain invocations of "git apply --3way"
>> will print error messages even though git
>> is able to fall back on apply_fragments and
>> apply the patch successfully with a return
>> value of 0. To fix, return early from
>> try_threeway() in the following cases:
>
> I suspect that this is a recent breakage after we swapped the order
> of 3way fallback.  It used to be that we tried the straight
> application first (while suppressing the error messages) and then
> fell back on 3way (while fully exposing the error messages).

The other side of the coin is that we should say, before falling back to non-3way codepath, that we saw 3way application failed, and falling back to the straight patch, if we don't say so already with the recent change, just like we used to say the opposite after seeing the patch application to fail and then fell back to 3way.

In other words, we may have said, under the verbose mode, that we are falling back to threeway only after seeing apply-fragments failed, but if we are doing 3-way first, telling that we are doing three-way application in try_threeway() function is actively wrong. The message was there only because try_threeway() was a fallback and sometimes did not happen even when the user asked to do --3way (hence deserved a message). Now we always try 3way first when the user asked, the extra message should be given only after try_threeway() failed and should talk about falling back to the straight application.

IOW, a fix something along this line, perhaps, is needed to complete the "swap the order between 3way and direct application" series we earlier worked on.

 apply.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Show changes to diff +4 −1

diff --git i/apply.c w/apply.c
index 8c5b29809b..6634305e9e 100644
--- i/apply.c
+++ w/apply.c
@@ -3570,7 +3570,7 @@ static int try_threeway(struct apply_state *state,
 		 read_blob_object(&buf, &pre_oid, patch->old_mode))
 		return error(_("repository lacks the necessary blob to perform 3-way merge."));
 
-	if (state->apply_verbosity > verbosity_silent)
+	if (state->apply_verbosity > verbosity_silent && patch->direct_to_threeway)
 		fprintf(stderr, _("Performing three-way merge...\n"));
 
 	img = strbuf_detach(&buf, &len);
@@ -3637,6 +3637,9 @@ static int apply_data(struct apply_state *state, struct patch *patch,
 		return -1;
 
 	if (!state->threeway || try_threeway(state, &image, patch, st, ce) < 0) {
+		if (state->threeway && !patch->direct_to_threeway)
+			fprintf(stderr, _("Falling back to direct application...\n"));
+
 		/* Note: with --reject, apply_fragments() returns 0 */
 		if (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0)
 			return -1;

Jerry Zhang· Jul 28, 2021, 03:14 UTC · re: Junio C Hamano · lore

[PATCH V2] git-apply: silence errors for success cases

When the patch is a rename and no lines have changed. In this case, "git diff" doesn't record the blob info, so 3way is neither possible nor necessary.

Only error messaging is affected and other behavior does not change.

Signed-off-by: Jerry Zhang <jerry@skydio.com>
---
V1->V2: rebase onto master and rerun tests

I think I addressed previous comments. What are the next steps for this patch?

 apply.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Show changes to apply.c +3 −1

diff --git a/apply.c b/apply.c
index 44bc31d6eb..fb321c707b 100644
--- a/apply.c
+++ b/apply.c
@@ -3560,7 +3560,9 @@ static int try_threeway(struct apply_state *state,
 
 	/* No point falling back to 3-way merge in these cases */
 	if (patch->is_delete ||
-	    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode))
+	    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode) ||
+	    (patch->is_new && !patch->direct_to_threeway) ||
+	    (patch->is_rename && !patch->lines_added && !patch->lines_deleted))
 		return -1;
 
 	/* Preimage the patch was prepared for */

-- 
2.32.0.1314.g6ed4fcc4cc

Jerry Zhang· Dec 11, 2021, 02:05 UTC · re: Jerry Zhang · lore

Re: [PATCH V2] git-apply: silence errors for success cases

On Tue, Jul 27, 2021 at 8:14 PM Jerry Zhang <jerry@skydio.com> wrote:

Show 50 quoted lines

>
> Certain invocations of "git apply --3way"
> will print error messages even though git
> is able to fall back on apply_fragments and
> apply the patch successfully with a return
> value of 0. To fix, return early from
> try_threeway() in the following cases:
>
> When the patch is a rename and no lines have
> changed. In this case, "git diff" doesn't
> record the blob info, so 3way is neither
> possible nor necessary.
>
> When the patch is an addition and there is
> no add/add conflict, i.e. direct_to_threeway
> is false. In this case, threeway will fail
> since the preimage is not in cache, but isn't
> necessary anyway since there is no conflict.
>
> Only error messaging is affected and other
> behavior does not change.
>
> Signed-off-by: Jerry Zhang <jerry@skydio.com>
> ---
> V1->V2: rebase onto master and rerun tests
>
> I think I addressed previous comments. What
> are the next steps for this patch?
>
>  apply.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/apply.c b/apply.c
> index 44bc31d6eb..fb321c707b 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -3560,7 +3560,9 @@ static int try_threeway(struct apply_state *state,
>
>         /* No point falling back to 3-way merge in these cases */
>         if (patch->is_delete ||
> -           S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode))
> +           S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode) ||
> +           (patch->is_new && !patch->direct_to_threeway) ||
> +           (patch->is_rename && !patch->lines_added && !patch->lines_deleted))
>                 return -1;
>
>         /* Preimage the patch was prepared for */
> --
> 2.32.0.1314.g6ed4fcc4cc
>

Any updates on this patch? I had addressed the previous comment in this thread in a different patch "apply: adjust messages to account for --3way changes" that was already merged in a while ago.

I originally intended this to mainly fix print message behavior, but while testing I found that it also fixed a separately reported issue from this thread: https://www.spinics.net/lists/git/msg421481.html.

I probably owe an update / refresher on why I originally submitted these features in the first place. I've written a tool for branchless / patch stack based code review and development w/github. The core of this is a mechanism that can cherry-pick commits without touching the cache or working tree by using "git apply --cached --3way" with a custom index file. We've been using this internally with a custom built git for a while now and gotten pretty good feedback. We'd like to open source this tool soon, and it'd be nice if our open-source users (eventually) don't need to build git themselves :).