Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] git-apply: silence errors for success cases
@ 2021-04-21  0:47 Jerry Zhang
  2021-04-21  2:14 ` Junio C Hamano
  0 siblings, 1 reply; 3+ messages in thread
From: Jerry Zhang @ 2021-04-21  0:47 UTC (permalink / raw)
  To: git, gitster; +Cc: ross, abe, brian.kubisiak, Jerry Zhang

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(-)

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] git-apply: silence errors for success cases
  2021-04-21  0:47 [PATCH] git-apply: silence errors for success cases Jerry Zhang
@ 2021-04-21  2:14 ` Junio C Hamano
  2021-04-21 23:53   ` Junio C Hamano
  0 siblings, 1 reply; 3+ messages in thread
From: Junio C Hamano @ 2021-04-21  2:14 UTC (permalink / raw)
  To: Jerry Zhang; +Cc: git, ross, abe, brian.kubisiak

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).

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).


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] git-apply: silence errors for success cases
  2021-04-21  2:14 ` Junio C Hamano
@ 2021-04-21 23:53   ` Junio C Hamano
  0 siblings, 0 replies; 3+ messages in thread
From: Junio C Hamano @ 2021-04-21 23:53 UTC (permalink / raw)
  To: Jerry Zhang; +Cc: git, ross, abe, brian.kubisiak

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

> 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(-)

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;





^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-21  0:47 [PATCH] git-apply: silence errors for success cases Jerry Zhang
2021-04-21  2:14 ` Junio C Hamano
2021-04-21 23:53   ` Junio C Hamano

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git