diff mbox series

apply: support --ours, --theirs, and --union for three-way merges

Message ID 20240906044222.4881-1-alexhenrie24@gmail.com (mailing list archive)
State Superseded
Headers show
Series apply: support --ours, --theirs, and --union for three-way merges | expand

Commit Message

Alex Henrie Sept. 6, 2024, 4:22 a.m. UTC 
--ours, --theirs, and --union are already supported in `git merge-file`
for automatically resolving conflicts in favor of one version or the
other, instead of leaving conflict markers in the file. Support them in
`git apply -3` as well because the two commands do the same kind of
file-level merges.

In case in the future --ours, --theirs, and --union gain a meaning
outside of three-way-merges, they do not imply --3way but rather must be
specified alongside it.

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 Documentation/git-apply.txt |  9 ++++++-
 apply.c                     | 49 ++++++++++++++++++++++++++++++++++---
 apply.h                     |  2 ++
 t/t4108-apply-threeway.sh   | 40 ++++++++++++++++++++++++++++++
 4 files changed, 96 insertions(+), 4 deletions(-)

Comments

Junio C Hamano Sept. 6, 2024, 8:49 p.m. UTC | #1 
Alex Henrie <alexhenrie24@gmail.com> writes:

> --ours, --theirs, and --union are already supported in `git merge-file`
> for automatically resolving conflicts in favor of one version or the
> other, instead of leaving conflict markers in the file. Support them in
> `git apply -3` as well because the two commands do the same kind of
> file-level merges.

An unrelated #leftoverbits tangent.  

We probably should teach add "union" as a valid choice in
.recursive_variant in merge-recursive.c:parse_merge_opt(), together
with "ours" and "theirs" that are already supported.

> In case in the future --ours, --theirs, and --union gain a meaning
> outside of three-way-merges, they do not imply --3way but rather must be
> specified alongside it.

OK.  At least the code insists on having --3way specified, instead
of silently ignoring --ours given without --3way, so it is good.

> +static int apply_option_parse_favorite(const struct option *opt,
> +				       const char *arg, int unset)
> +{
> +	struct apply_state *state = opt->value;
> +
> +	BUG_ON_OPT_ARG(arg);
> +	BUG_ON_OPT_NEG(unset);
> +
> +	if (!strcmp(opt->long_name, "ours"))
> +		state->merge_opts.variant = XDL_MERGE_FAVOR_OURS;
> +	else if (!strcmp(opt->long_name, "theirs"))
> +		state->merge_opts.variant = XDL_MERGE_FAVOR_THEIRS;
> +	else
> +		state->merge_opts.variant = XDL_MERGE_FAVOR_UNION;
> +	return 0;
> +}

If you MUST use a opt-callback, then do not assume anything that is
not ours or theirs will always be union.  Help future developers by
making your assumption more explicit, i.e.

	if (..."ours"...)
		do ours thing;
	else if (... "theirs" ...)
		do theirs thing;
	else if (... "union" ...)
		do union thing;
	else
		BUG("unexpected option '--%s'", opt->long_name);

Having said that, I do not think you want or need a callback in this
case to begin with.

>  static int apply_option_parse_whitespace(const struct option *opt,
>  					 const char *arg, int unset)
>  {
> @@ -5151,6 +5177,18 @@ int apply_parse_options(int argc, const char **argv,
>  			N_("also apply the patch (use with --stat/--summary/--check)")),
>  		OPT_BOOL('3', "3way", &state->threeway,
>  			 N_( "attempt three-way merge, fall back on normal patch if that fails")),
> +		OPT_CALLBACK_F(0, "ours", state, NULL,
> +			N_("for conflicts, use our version"),
> +			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
> +			apply_option_parse_favorite),
> +		OPT_CALLBACK_F(0, "theirs", state, NULL,
> +			N_("for conflicts, use their version"),
> +			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
> +			apply_option_parse_favorite),
> +		OPT_CALLBACK_F(0, "union", state, NULL,
> +			N_("for conflicts, use a union version"),
> +			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
> +			apply_option_parse_favorite),

Instead of embedding the whole ll_merge_options in apply_state, just
define a new integer member "merge_variant", and use OPT_SET_INT()
on that field.  That way, you won't have to do the callback interface
and worry about keeping the function's if/elseif cascade in sync
with these options that call the same function.

Then, instead of passing state->merge_opts, you can initialize
ll_merge_options instance in three_way_merge() with whatever is
needed from the apply_state (like the merge_variant mentioned
above).

> -	if (status) {
> +	if (state->merge_opts.variant) {
> +		/*
> +		 * XDL_MERGE_FAVOR_(OURS|THEIRS|UNION) automatically resolves
> +		 * conflicts, but the ll_merge function is not yet smart enough
> +		 * to report whether or not there were conflicts, so just print
> +		 * a generic message.
> +		 */
> +		fprintf(stderr, _("Applied patch to '%s'.\n"), patch->new_name);

I do not think this extra message or the comment is warranted.  When
you said "--ours" you told the machinery that you favor our version,
so there is no place to be "smart enough to report".  Instead of
normal 3-way merge, you told it there won't be any conflict.  There
is nothing to report.

The else clause of the original "if (status)" does report "applied
patch cleanly" when apply_verbosity is set to report it.  As far as
the command is concerned, if you told it to use "ours" and got a
merge result, that was also applied cleanly.  So you can just drop
this hunk and let the existing code take care of the rest (including
honoring the verbosity settings).

Other than that, looks fairly straight-forward.

I didn't read the tests, though.

Thanks.
Alex Henrie Sept. 9, 2024, 2:10 p.m. UTC | #2 
On Fri, Sep 6, 2024 at 2:49 PM Junio C Hamano <gitster@pobox.com> wrote:

> An unrelated #leftoverbits tangent.
>
> We probably should teach add "union" as a valid choice in
> .recursive_variant in merge-recursive.c:parse_merge_opt(), together
> with "ours" and "theirs" that are already supported.

I agree, that would be really nice, and it would simplify the tests
for `git apply -3 --union`.

The other changes you requested are done in v2. Thanks for the feedback!

-Alex
diff mbox series

Patch

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index 9cce68a38b..dd4a61ef28 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -9,7 +9,8 @@  git-apply - Apply a patch to files and/or to the index
 SYNOPSIS
 --------
 [verse]
-'git apply' [--stat] [--numstat] [--summary] [--check] [--index | --intent-to-add] [--3way]
+'git apply' [--stat] [--numstat] [--summary] [--check]
+	  [--index | --intent-to-add] [--3way] [--ours | --theirs | --union]
 	  [--apply] [--no-add] [--build-fake-ancestor=<file>] [-R | --reverse]
 	  [--allow-binary-replacement | --binary] [--reject] [-z]
 	  [-p<n>] [-C<n>] [--inaccurate-eof] [--recount] [--cached]
@@ -92,6 +93,12 @@  OPTIONS
 	When used with the `--cached` option, any conflicts are left at higher stages
 	in the cache.
 
+--ours::
+--theirs::
+--union::
+	Instead of leaving conflicts in the file, resolve conflicts favouring
+	our (or their or both) side of the lines. Requires --3way.
+
 --build-fake-ancestor=<file>::
 	Newer 'git diff' output has embedded 'index information'
 	for each blob to help identify the original version that
diff --git a/apply.c b/apply.c
index 6e1060a952..e47ec6dc56 100644
--- a/apply.c
+++ b/apply.c
@@ -110,6 +110,7 @@  int init_apply_state(struct apply_state *state,
 	state->prefix = prefix;
 	state->repo = repo;
 	state->apply = 1;
+	state->merge_opts.conflict_style = -1;
 	state->line_termination = '\n';
 	state->p_value = 1;
 	state->p_context = UINT_MAX;
@@ -3578,7 +3579,7 @@  static int three_way_merge(struct apply_state *state,
 			  &our_file, "ours",
 			  &their_file, "theirs",
 			  state->repo->index,
-			  NULL);
+			  &state->merge_opts);
 	if (status == LL_MERGE_BINARY_CONFLICT)
 		warning("Cannot merge binary files: %s (%s vs. %s)",
 			path, "ours", "theirs");
@@ -3704,7 +3705,15 @@  static int try_threeway(struct apply_state *state,
 		return status;
 	}
 
-	if (status) {
+	if (state->merge_opts.variant) {
+		/*
+		 * XDL_MERGE_FAVOR_(OURS|THEIRS|UNION) automatically resolves
+		 * conflicts, but the ll_merge function is not yet smart enough
+		 * to report whether or not there were conflicts, so just print
+		 * a generic message.
+		 */
+		fprintf(stderr, _("Applied patch to '%s'.\n"), patch->new_name);
+	} else if (status) {
 		patch->conflicted_threeway = 1;
 		if (patch->is_new)
 			oidclr(&patch->threeway_stage[0], the_repository->hash_algo);
@@ -4980,6 +4989,23 @@  static int apply_option_parse_space_change(const struct option *opt,
 	return 0;
 }
 
+static int apply_option_parse_favorite(const struct option *opt,
+				       const char *arg, int unset)
+{
+	struct apply_state *state = opt->value;
+
+	BUG_ON_OPT_ARG(arg);
+	BUG_ON_OPT_NEG(unset);
+
+	if (!strcmp(opt->long_name, "ours"))
+		state->merge_opts.variant = XDL_MERGE_FAVOR_OURS;
+	else if (!strcmp(opt->long_name, "theirs"))
+		state->merge_opts.variant = XDL_MERGE_FAVOR_THEIRS;
+	else
+		state->merge_opts.variant = XDL_MERGE_FAVOR_UNION;
+	return 0;
+}
+
 static int apply_option_parse_whitespace(const struct option *opt,
 					 const char *arg, int unset)
 {
@@ -5151,6 +5177,18 @@  int apply_parse_options(int argc, const char **argv,
 			N_("also apply the patch (use with --stat/--summary/--check)")),
 		OPT_BOOL('3', "3way", &state->threeway,
 			 N_( "attempt three-way merge, fall back on normal patch if that fails")),
+		OPT_CALLBACK_F(0, "ours", state, NULL,
+			N_("for conflicts, use our version"),
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
+			apply_option_parse_favorite),
+		OPT_CALLBACK_F(0, "theirs", state, NULL,
+			N_("for conflicts, use their version"),
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
+			apply_option_parse_favorite),
+		OPT_CALLBACK_F(0, "union", state, NULL,
+			N_("for conflicts, use a union version"),
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
+			apply_option_parse_favorite),
 		OPT_FILENAME(0, "build-fake-ancestor", &state->fake_ancestor,
 			N_("build a temporary index based on embedded index information")),
 		/* Think twice before adding "--nul" synonym to this */
@@ -5190,5 +5228,10 @@  int apply_parse_options(int argc, const char **argv,
 		OPT_END()
 	};
 
-	return parse_options(argc, argv, state->prefix, builtin_apply_options, apply_usage, 0);
+	argc = parse_options(argc, argv, state->prefix, builtin_apply_options, apply_usage, 0);
+
+	if (state->merge_opts.variant && !state->threeway)
+		die(_("--ours, --theirs, and --union require --3way"));
+
+	return argc;
 }
diff --git a/apply.h b/apply.h
index cd25d24cc4..096f4472b4 100644
--- a/apply.h
+++ b/apply.h
@@ -3,6 +3,7 @@ 
 
 #include "hash.h"
 #include "lockfile.h"
+#include "merge-ll.h"
 #include "string-list.h"
 #include "strmap.h"
 
@@ -59,6 +60,7 @@  struct apply_state {
 	struct repository *repo;
 	const char *index_file;
 	enum apply_verbosity apply_verbosity;
+	struct ll_merge_options merge_opts;
 	char *fake_ancestor;
 	const char *patch_input_file;
 	int line_termination;
diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
index 3211e1e65f..c6302163d8 100755
--- a/t/t4108-apply-threeway.sh
+++ b/t/t4108-apply-threeway.sh
@@ -82,6 +82,46 @@  test_expect_success 'apply with --3way with merge.conflictStyle = diff3' '
 	test_apply_with_3way
 '
 
+test_apply_with_3way_favoritism () {
+	apply_arg=$1
+	merge_arg=$2
+
+	# Merging side should be similar to applying this patch
+	git diff ...side >P.diff &&
+
+	# The corresponding conflicted merge
+	git reset --hard &&
+	git checkout main^0 &&
+	git merge --no-commit $merge_arg side &&
+	git ls-files -s >expect.ls &&
+	print_sanitized_conflicted_diff >expect.diff &&
+
+	# should apply successfully
+	git reset --hard &&
+	git checkout main^0 &&
+	git apply --index --3way $apply_arg P.diff &&
+	git ls-files -s >actual.ls &&
+	print_sanitized_conflicted_diff >actual.diff &&
+
+	# The result should resemble the corresponding merge
+	test_cmp expect.ls actual.ls &&
+	test_cmp expect.diff actual.diff
+}
+
+test_expect_success 'apply with --3way --ours' '
+	test_apply_with_3way_favoritism --ours -Xours
+'
+
+test_expect_success 'apply with --3way --theirs' '
+	test_apply_with_3way_favoritism --theirs -Xtheirs
+'
+
+test_expect_success 'apply with --3way --union' '
+	echo "* merge=union" >.gitattributes &&
+	test_apply_with_3way_favoritism --union &&
+	rm .gitattributes
+'
+
 test_expect_success 'apply with --3way with rerere enabled' '
 	test_config rerere.enabled true &&