Re: [PATCH] merge-recursive: introduce merge_options

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

Miklos Vajna <vmiklos@frugalware.org> writes:

> 1) This applies on top of 1c868d4 (merge-recursive.c: Add more generic
> merge_recursive_generic()). I can rebase this (along with 1c868d4 and
> 1c868d4^) on top of current master, if this is a problem.

It probably is cleaner to treat this as a fresh topic from scratch on top
of 'master', as we do not have anything outstanding in 'next' around this
area.

> 2) I know that this patch is huge, but we want to have the verbosity
> flag in merge_options, so it has to be passed as an argument in many
> places.

Size of the patch that results purely from addition of the merge_options
parameter from top to bottom does not bother me too much.  The look quite
straightforward conversions, and getting rid of these many global
variables is a major step in the right direction.

It might however be a good idea to consistently have this at the same
place (either the beginning or at the end) of the parameter list of
functions that take one.

> @@ -1273,10 +1268,11 @@ int merge_recursive(struct commit *h1,
>  		 * "conflicts" were already resolved.
>  		 */
>  		discard_cache();
> -		merge_recursive(merged_common_ancestors, iter->item,
> -				"Temporary merge branch 1",
> -				"Temporary merge branch 2",
> -				NULL,
> +		memcpy(&opts, o, sizeof(struct merge_options));
> +		opts.branch1 = "Temporary merge branch 1";
> +		opts.branch2 = "Temporary merge branch 2";
> +		merge_recursive(&opts, merged_common_ancestors,
> +				iter->item, NULL,
>  				&merged_common_ancestors);
>  		call_depth--;

After suggesting to keep label in merge_options, I was wondering how this
part should be handled the best.  An alternative would be not to do copy
the structure but stash away only branch1 and branch2 members before
making the recursive call and restore them after it returns, like this:

		const char *saved_b1, *saved_b2;
		...
		saved_b1 = o->branch1;
		saved_b2 = o->branch2;
		o->branch1 = "Temporary merge branch 1";
		o->branch2 = "Temporary merge branch 2";
		merge_recursive(o, ...);
		o->branch1 = saved_b1;
		o->branch2 = saved_b2;
		call_depth--;
		...
		
This might be better in the longer run, as we may want to pass *back*
status from merge_recursive() to the caller in fields of merge_options in
the future.