Re: [PATCH] merge-recursive: introduce merge_options

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Miklos Vajna <vmiklos@xxxxxxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux