git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jerry Zhang <jerry@skydio.com>
Cc: git@vger.kernel.org, newren@gmail.com, ross@skydio.com,
	abe@skydio.com, brian.kubisiak@skydio.com
Subject: Re: [PATCH] git-apply: try threeway first when "--3way" is used
Date: Mon, 05 Apr 2021 23:13:45 -0700	[thread overview]
Message-ID: <xmqqblas2b52.fsf@gitster.g> (raw)
In-Reply-To: <20210406025551.25213-1-jerry@skydio.com> (Jerry Zhang's message of "Mon, 5 Apr 2021 19:55:51 -0700")

Jerry Zhang <jerry@skydio.com> writes:

> The apply_fragments() method of "git apply"
> can silently apply patches incorrectly if
> a file has repeating contents. In these
> cases a three-way merge can apply it correctly

Is that "can apply"?  Isn't it "has a better chance to correctly
apply"?

> or show a conflict. However, because the patches
> apply "successfully" using apply_fragments(),
> git will never fall back to the merge, even
> if the "--3way" flag is used, and the user has
> no way to ensure correctness by forcing the
> three-way merge method.
>
> Change the behavior so that when "--3way" is
> used, git will always try the three-way merge
> first and will only fall back to apply_fragments()
> in caseswhere blobs are not available or some other

Missing SP before two words.

> error (but not in the case of a merge conflict).

We may want to note a possible backward compatibility fallout to
warn reviewers here in the proposed log message.

>  -3::
>  --3way::
> +	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.

OK.  This patch obviously expects it to graduate before the other
"--3way and --cached at the same time" patch.

> diff --git a/apply.c b/apply.c
> index 6695a931e979a968b28af88d425d0c76ba17d0d4..62d65ef8d9c0b68857db55198c73db1f41589df1 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -3569,10 +3569,10 @@ static int try_threeway(struct apply_state *state,
>  		write_object_file("", 0, blob_type, &pre_oid);
>  	else if (get_oid(patch->old_oid_prefix, &pre_oid) ||
>  		 read_blob_object(&buf, &pre_oid, patch->old_mode))
> -		return error(_("repository lacks the necessary blob to fall back on 3-way merge."));
> +		return error(_("repository lacks the necessary blob to do 3-way merge."));

s/do/perform/ perhaps?

> @@ -3637,10 +3637,9 @@ static int apply_data(struct apply_state *state, struct patch *patch,
>  	if (load_preimage(state, &image, patch, st, ce) < 0)
>  		return -1;
>  
> -	if (patch->direct_to_threeway ||
> -	    apply_fragments(state, &image, patch) < 0) {

The original was "If the logic flow that came before us already
decided we should skip the straight application of the patch and
jump directly to the three-way codepath.  Otherwise try the straight
application and perform 3-way only when it fails".

The "direct-to-threeway" logic was introduced by 099f3c42 (apply:
--3way with add/add conflict, 2012-06-07).

> +	if (!state->threeway || try_threeway(state, &image, patch, st, ce) < 0) {
>  		/* Note: with --reject, apply_fragments() returns 0 */
> -		if (!state->threeway || try_threeway(state, &image, patch, st, ce) < 0)
> +		if (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0)
>  			return -1;

This says something different.  "If 3-way was not asked, jump
directly to inside the block.  Otherwise, try 3-way first, and go
inside the block only if 3-way did not work."  And the inside the
block is the straight patch application.  It says "if we have
already decided we should do the 3-way and nothing else, just fail.
Otherwise try the straight patch application and if it fails, then
fail the whole thing."

This looks like a correct "inversion" of the fallback codepath.

> @@ -5017,7 +5016,7 @@ int apply_parse_options(int argc, const char **argv,
>  		OPT_BOOL(0, "apply", force_apply,
>  			N_("also apply the patch (use with --stat/--summary/--check)")),
>  		OPT_BOOL('3', "3way", &state->threeway,
> -			 N_( "attempt three-way merge if a patch does not apply")),
> +			 N_( "attempt three-way merge, fall back on normal patch if that fails")),

OK.

Overall, the change is very cleanly done.

Will queue.  Thanks.



  reply	other threads:[~2021-04-06  6:13 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-06  2:55 Jerry Zhang
2021-04-06  6:13 ` Junio C Hamano [this message]
2021-04-06 23:13   ` Junio C Hamano
2021-04-06  6:14 ` Junio C Hamano
2021-04-06 23:25 ` Jerry Zhang
2021-04-07  0:19   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqblas2b52.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=abe@skydio.com \
    --cc=brian.kubisiak@skydio.com \
    --cc=git@vger.kernel.org \
    --cc=jerry@skydio.com \
    --cc=newren@gmail.com \
    --cc=ross@skydio.com \
    --subject='Re: [PATCH] git-apply: try threeway first when "--3way" is used' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).