Re: [PATCH] merge-recursive: introduce merge_options

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 25, 2008, 06:06 UTC
Message-ID: <7v7ia5iq7l.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1219628677-23903-1-git-send-email-vmiklos@frugalware.org>

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.

Show 15 quoted lines

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