Re: [PATCH] apply: support --ours, --theirs, and --union for three-way merges

[Junio C Hamano <gitster@pobox.com>]

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.