Re: [PATCH] merge-recursive: handle file mode changes

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

Clemens Buchacher <drizzd@aon.at> writes:

> diff --git a/merge-recursive.c b/merge-recursive.c
> index 34e3167..d8938cc 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1025,12 +1025,21 @@ static struct merge_file_info merge_file(struct diff_filespec *o,
>  			hashcpy(result.sha, b->sha1);
>  		}
>  	} else {
> -		if (!sha_eq(a->sha1, o->sha1) && !sha_eq(b->sha1, o->sha1))
> -			result.merge = 1;
> -
> -		result.mode = a->mode == o->mode ? b->mode: a->mode;
> +		/*
> +		 * If mode changed in only one branch, keep the changed
> +		 * version. Otherwise, keep remote version and create a
> +		 * conflict.
> +		 */

Reading the rest of the function, I notice that it consistently favor "a"
over "b", when a conflict cannot be reconciled.

> +		if (a->mode != o->mode && b->mode != o->mode &&
> +				a->mode != b->mode) {
> +			result.clean = 0;
> +			result.mode = b->mode;
> +		} else
> +			result.mode = o->mode == a->mode ? b->mode : a->mode;

I think this is much easier to read:

		if (a->mode == b->mode || a->mode == o->mode)
			result.mode = b->mode;
		else {
			result.mode = a->mode;
			if (b->mode != o->mode) {
				result.clean = 0;
				result.merge = 1;
			}
		}

We keep "b" only if "a" hasn't touched the mode, or it happens to be the
same as "a".  Otherwise we take "a" anyway, but taking "a" when "b" also
touched means we detected an unreconcilable conflict.

> @@ -1062,6 +1071,8 @@ static struct merge_file_info merge_file(struct diff_filespec *o,
>  		} else {
>  			die("unsupported object type in the tree");
>  		}
> +
> +		result.merge = !result.clean;

As pointed out by Dscho, this is too much.  We could mimick the one that
is done for the contents, which is why I have "result.merge = 1" where it
is in the above.

> diff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh
> new file mode 100755
> index 0000000..36cd664
> --- /dev/null
> +++ b/t/t6031-merge-recursive.sh
> @@ -0,0 +1,39 @@
> +#!/bin/sh
> ...
> +	! (git merge-recursive master -- a2 b2 || test $? -ne 1) &&
> +	! test -x file2
> +'

As we would favor our side in unreconsilable conflicts, the last test
needs to become "test -x file2".

Also we should test ls-files -u output to make sure we have correct stages
in the index.

I've done minor fixups and committed the result.