Re: [PATCH] git-apply: try threeway first when "--3way" is used

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 6, 2021, 06:13 UTC
Message-ID: <xmqqblas2b52.fsf@gitster.g>
In-Reply-To: <20210406025551.25213-1-jerry@skydio.com>

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"?

Show 11 quoted lines

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

Show 7 quoted lines

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

Show 10 quoted lines

> 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?

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 6 quoted lines

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