Re: [PATCH 2/2] git-merge-recursive-{ours,theirs}

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Fri, 20 Jun 2008, Junio C Hamano wrote:

> diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c
> index 4aa28a1..a355e7a 100644
> --- a/builtin-merge-recursive.c
> +++ b/builtin-merge-recursive.c
> @@ -650,9 +655,26 @@ static int merge_3way(mmbuffer_t *result_buf,
>  	fill_mm(a->sha1, &src1);
>  	fill_mm(b->sha1, &src2);
>  
> +	if (index_only)
> +		favor = 0;
> +	else {
> +		switch (merge_recursive_variants) {
> +		case MERGE_RECURSIVE_OURS:
> +			favor = XDL_MERGE_FAVOR_OURS;
> +			break;
> +		case MERGE_RECURSIVE_THEIRS:
> +			favor = XDL_MERGE_FAVOR_THEIRS;
> +			break;
> +		default:
> +			favor = 0;
> +			break;
> +		}

Hrm.  I would have preferred something like this:

	if (!index_only && merge_recursive_variants == MERGE_RECURSIVE_OURS)
		favor = XDL_MERGE_FAVOR_OURS;
	if (!index_only && merge_recursive_variants == MERGE_RECURSIVE_THEIRS)
		favor = XDL_MERGE_FAVOR_THEIRS;
	else
		favor = 0;

> +	}
> +	flag = LL_MERGE_FLAGS(index_only, favor);
> +
>  	merge_status = ll_merge(result_buf, a->path, &orig,
>  				&src1, name1, &src2, name2,
> -				index_only);
> +				flag);

Sorry, but in my opinion this flag mangling makes the whole code uglier.  
Why not just add another parameter?

Or if you are really concerned about future enhancements to ll_merge(), 
use a struct.

> @@ -1379,11 +1401,18 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
>  	struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
>  	int index_fd;
>  
> +	merge_recursive_variants = 0;
>  	if (argv[0]) {
>  		int namelen = strlen(argv[0]);
>  		if (8 < namelen &&
>  		    !strcmp(argv[0] + namelen - 8, "-subtree"))
> -			subtree_merge = 1;
> +			merge_recursive_variants = MERGE_RECURSIVE_SUBTREE;
> +		else if (5 < namelen &&
> +			 !strcmp(argv[0] + namelen - 5, "-ours"))
> +			merge_recursive_variants = MERGE_RECURSIVE_OURS;
> +		else if (7 < namelen &&
> +			 !strcmp(argv[0] + namelen - 7, "-theirs"))
> +			merge_recursive_variants = MERGE_RECURSIVE_THEIRS;

This just cries out loud for a new function suffixcmp().

I will not say anything about the long lines in git-merge.sh, since I 
fully expect builtin-merge to happen Real Soon Now.

Anyhow, your comments about this driving the wrong workflow still apply.  
Maybe we want to display them really, really prominently in 
Documentation/merge-strategies.txt.

Ciao,
Dscho