[PATCH] fix recursive-merge of empty files with different permissions

[PATCH] fix recursive-merge of empty files with different permissions

[Clemens Buchacher]

If git-merge-recursive attempts to merge two empty new files with
different executable flags, eventually xdl_merge() is called and produces
empty diffs for both files and therefore does not choose either file as
successor. Make xdl_merge() choose one of the files instead.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

Hi,

The change in indentation makes this patch look larger than it is. All I
actually did was remove the "if (xscr1 || xscr2)" condition.

Previously to this patch the included test showed the following output:

	Merging a with b
	Merging:
	82712b3 branch a
	7eacd6f branch b
	found 1 common ancestor(s):
	33b7ba5 initial commit
	Auto-merged a
	fatal: Failed to execute internal merge

I do not understand why, but this does not happen if the file permissions are
the same.

Thanks,
Clemens

 t/t6031-merge-recursive.sh |   23 +++++++++++++++++++++++
 xdiff/xmerge.c             |   30 ++++++++++++++----------------
 2 files changed, 37 insertions(+), 16 deletions(-)
 create mode 100755 t/t6031-merge-recursive.sh

diff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh
new file mode 100755
index 0000000..4e3456b
--- /dev/null
+++ b/t/t6031-merge-recursive.sh
@@ -0,0 +1,23 @@
+#!/bin/sh
+
+test_description='merge-recursive corner cases'
+. ./test-lib.sh
+
+test_expect_success 'merge empty files with different permission flags' '
+	: >dummy &&
+	git add dummy &&
+	git commit -m "initial commit" &&
+	git checkout -b a master &&
+	: >a &&
+	git add a &&
+	git commit -m "branch a" &&
+	git checkout -b b master &&
+	: >a &&
+	chmod +x a &&
+	git add a &&
+	git commit -m "branch b" &&
+	git checkout master &&
+	git merge-recursive master -- a b
+'
+
+test_done
diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index 82b3573..92127e1 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -470,23 +470,21 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, const char *name1,
 		return -1;
 	}
 	status = 0;
-	if (xscr1 || xscr2) {
-		if (!xscr1) {
-			result->ptr = xdl_malloc(mf2->size);
-			memcpy(result->ptr, mf2->ptr, mf2->size);
-			result->size = mf2->size;
-		} else if (!xscr2) {
-			result->ptr = xdl_malloc(mf1->size);
-			memcpy(result->ptr, mf1->ptr, mf1->size);
-			result->size = mf1->size;
-		} else {
-			status = xdl_do_merge(&xe1, xscr1, name1,
-					      &xe2, xscr2, name2,
-					      level, xpp, result);
-		}
-		xdl_free_script(xscr1);
-		xdl_free_script(xscr2);
+	if (!xscr1) {
+		result->ptr = xdl_malloc(mf2->size);
+		memcpy(result->ptr, mf2->ptr, mf2->size);
+		result->size = mf2->size;
+	} else if (!xscr2) {
+		result->ptr = xdl_malloc(mf1->size);
+		memcpy(result->ptr, mf1->ptr, mf1->size);
+		result->size = mf1->size;
+	} else {
+		status = xdl_do_merge(&xe1, xscr1, name1,
+				      &xe2, xscr2, name2,
+				      level, xpp, result);
 	}
+	xdl_free_script(xscr1);
+	xdl_free_script(xscr2);
 	xdl_free_env(&xe1);
 	xdl_free_env(&xe2);
 
-- 
1.5.4.3.468.g36990.dirty

Re: [PATCH] fix recursive-merge of empty files with different permissions

[Johannes Schindelin]

Hi,

On Sat, 8 Mar 2008, Clemens Buchacher wrote:

> I do not understand why, but this does not happen if the file 
> permissions are the same.

I think this is the biggest problem.

>  t/t6031-merge-recursive.sh |   23 +++++++++++++++++++++++
>  xdiff/xmerge.c             |   30 ++++++++++++++----------------

... because xdiff/xmerge.c is definitely the wrong place to "fix" this 
issue.  xdl_merge() does not even _know_ about permissions.

Ciao,
Dscho

Re: [PATCH] fix recursive-merge of empty files with different permissions

[Clemens Buchacher]

Hi,

On Sat, Mar 08, 2008 at 06:17:26PM +0100, Clemens Buchacher wrote:
> If git-merge-recursive attempts to merge two empty new files with
> different executable flags, eventually xdl_merge() is called and produces
> empty diffs for both files and therefore does not choose either file as
> successor. Make xdl_merge() choose one of the files instead.

On Sat, Mar 08, 2008 at 06:51:48PM +0100, Johannes Schindelin wrote:
> On Sat, 8 Mar 2008, Clemens Buchacher wrote:
> > I do not understand why, but this does not happen if the file 
> > permissions are the same.
> 
> I think this is the biggest problem.
> 
> >  t/t6031-merge-recursive.sh |   23 +++++++++++++++++++++++
> >  xdiff/xmerge.c             |   30 ++++++++++++++----------------
> 
> ... because xdiff/xmerge.c is definitely the wrong place to "fix" this 
> issue.  xdl_merge() does not even _know_ about permissions.

After analyzing the problem in greater detail, I have to disagree. It is true,
of course, that xdl_merge() does not and should not know about permissions at
all. However, the bug is still in xdl_merge(). Different permissions are only
the trigger of the problem, because only then will xdl_merge() be called at
all.

What happens is this. Before looking at the file contents directly
merge_trees() attempts to resolve the merge trivially. If both sha1 and mode of
the head and remote entries match, the merge will be resolved as per case #5ALT
(see Documentation/trivial-merge.txt), i.e. head is chosen as the merge result.

If either sha1 _or_ mode differ between the head and remote entries, however,
merge_trees() will use xdl_merge() to merge the file content and the remote
entry's mode will be chosen as result mode.

One could argue that it would be better to mark the mismatching permissions as
a conflict. However, this is how the merge currently silently succeeds _unless_
both files are empty. If they are, xdl_merge() will effectively exit with an
error status and git-merge-recursive will fail with an internal error (as shown
in the testcase).

In any case, I think it is reasonable to expect xdl_merge() to work with empty
files. Whether or not the current "mode merging" behavior is desired is a
different matter.

Regards,
Clemens

Re: [PATCH] fix recursive-merge of empty files with different permissions

[Johannes Schindelin]

Hi,

[Junio, there is a bugfix at the end of this mail.]

On Thu, 13 Mar 2008, Clemens Buchacher wrote:

> Hi,
> 
> On Sat, Mar 08, 2008 at 06:17:26PM +0100, Clemens Buchacher wrote:
> > If git-merge-recursive attempts to merge two empty new files with 
> > different executable flags, eventually xdl_merge() is called and 
> > produces empty diffs for both files and therefore does not choose 
> > either file as successor. Make xdl_merge() choose one of the files 
> > instead.
> 
> On Sat, Mar 08, 2008 at 06:51:48PM +0100, Johannes Schindelin wrote:
> > On Sat, 8 Mar 2008, Clemens Buchacher wrote:
> > > I do not understand why, but this does not happen if the file 
> > > permissions are the same.
> > 
> > I think this is the biggest problem.
> > 
> > >  t/t6031-merge-recursive.sh |   23 +++++++++++++++++++++++
> > >  xdiff/xmerge.c             |   30 ++++++++++++++----------------
> > 
> > ... because xdiff/xmerge.c is definitely the wrong place to "fix" this 
> > issue.  xdl_merge() does not even _know_ about permissions.
> 
> After analyzing the problem in greater detail, I have to disagree. It is 
> true, of course, that xdl_merge() does not and should not know about 
> permissions at all. However, the bug is still in xdl_merge(). Different 
> permissions are only the trigger of the problem, because only then will 
> xdl_merge() be called at all.
> 
> What happens is this. Before looking at the file contents directly 
> merge_trees() attempts to resolve the merge trivially. If both sha1 and 
> mode of the head and remote entries match, the merge will be resolved as 
> per case #5ALT (see Documentation/trivial-merge.txt), i.e. head is 
> chosen as the merge result.
> 
> If either sha1 _or_ mode differ between the head and remote entries, 
> however, merge_trees() will use xdl_merge() to merge the file content 
> and the remote entry's mode will be chosen as result mode.
> 
> One could argue that it would be better to mark the mismatching 
> permissions as a conflict.

Right you are.  Your whole "it still is xdl_merge()s fault" point was just 
contradicted by your own analysis.  Calling xdl_merge() when the sha1 does 
_not_ differ is _a mistake_.  _That_ is the bug.  Not xdl_merge() 
returning 0 (because there were 0 conflicts).

> However, this is how the merge currently silently succeeds _unless_ both 
> files are empty. If they are, xdl_merge() will effectively exit with an 
> error status and git-merge-recursive will fail with an internal error 
> (as shown in the testcase).
> 
> In any case, I think it is reasonable to expect xdl_merge() to work with 
> empty files. Whether or not the current "mode merging" behavior is 
> desired is a different matter.

I just tested.  xdl_merge() is _just fine_ with empty files.  However, 
git-merge-file was not.  Fixed by this:

-- snipsnap --
[PATCH] merge-file: handle empty files gracefully

Earlier, it would error out while trying to read and/or writing them.
Now, calling merge-file with empty files is neither interesting nor
useful, but it is a bug that needed fixing.

Noticed by Clemens Buchacher.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin-merge-file.c |    3 ++-
 xdiff-interface.c    |    4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin-merge-file.c b/builtin-merge-file.c
index adce6d4..cde4b2c 100644
--- a/builtin-merge-file.c
+++ b/builtin-merge-file.c
@@ -57,7 +57,8 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 
 		if (!f)
 			ret = error("Could not open %s for writing", filename);
-		else if (fwrite(result.ptr, result.size, 1, f) != 1)
+		else if (result.size &&
+				fwrite(result.ptr, result.size, 1, f) != 1)
 			ret = error("Could not write to %s", filename);
 		else if (fclose(f))
 			ret = error("Could not close %s", filename);
diff --git a/xdiff-interface.c b/xdiff-interface.c
index bba2364..61dc5c5 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -152,8 +152,8 @@ int read_mmfile(mmfile_t *ptr, const char *filename)
 	if ((f = fopen(filename, "rb")) == NULL)
 		return error("Could not open %s", filename);
 	sz = xsize_t(st.st_size);
-	ptr->ptr = xmalloc(sz);
-	if (fread(ptr->ptr, sz, 1, f) != 1)
+	ptr->ptr = xmalloc(sz ? sz : 1);
+	if (sz && fread(ptr->ptr, sz, 1, f) != 1)
 		return error("Could not read %s", filename);
 	fclose(f);
 	ptr->size = sz;
-- 
1.5.4.4.706.ga822

Re: [PATCH] fix recursive-merge of empty files with different permissions

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> diff --git a/builtin-merge-file.c b/builtin-merge-file.c
> index adce6d4..cde4b2c 100644
> --- a/builtin-merge-file.c
> +++ b/builtin-merge-file.c
> @@ -57,7 +57,8 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
>  
>  		if (!f)
>  			ret = error("Could not open %s for writing", filename);
> -		else if (fwrite(result.ptr, result.size, 1, f) != 1)
> +		else if (result.size &&
> +				fwrite(result.ptr, result.size, 1, f) != 1)
>  			ret = error("Could not write to %s", filename);
>  		else if (fclose(f))
>  			ret = error("Could not close %s", filename);

Lol.  We are dealing with N-byte quantity so we send one record of length
N and make sure we processed one record, and it does not work when N is
zero.

We could instead send N records of size 1 and make sure we processed N
records to lose the conditional instead, but the patch avoids unnecessary
call to fread/fwrite so that is good.  Thanks.

It felt funny because my current bedtime reading happens to be "Zero: The
Biography of a Dangerous Idea (ISBN 0140296476)".

> diff --git a/xdiff-interface.c b/xdiff-interface.c
> index bba2364..61dc5c5 100644
> --- a/xdiff-interface.c
> +++ b/xdiff-interface.c
> @@ -152,8 +152,8 @@ int read_mmfile(mmfile_t *ptr, const char *filename)
>  	if ((f = fopen(filename, "rb")) == NULL)
>  		return error("Could not open %s", filename);
>  	sz = xsize_t(st.st_size);
> -	ptr->ptr = xmalloc(sz);
> -	if (fread(ptr->ptr, sz, 1, f) != 1)
> +	ptr->ptr = xmalloc(sz ? sz : 1);
> +	if (sz && fread(ptr->ptr, sz, 1, f) != 1)
>  		return error("Could not read %s", filename);
>  	fclose(f);
>  	ptr->size = sz;

Do you need to actually allocate ptr->ptr when sz is zero, instead of
setting it to NULL, like:

	sz = xsize_t(st.st_size);
	ptr->size = sz;
        if (!sz)
        	ptr->ptr = NULL;
	else {
        	ptr->ptr = xmalloc(sz);
		if (fread(ptr->ptr, 1, sz, f) != sz)
			return error("Could not read %s", filename);
	}
	fclose(f);

[PATCH] merge-recursive: cause a conflict if file mode does not match

[Clemens Buchacher]

Previously, mismatching file modes would be auto-merged by picking the
mode in the remote tree.

This also fixes a bug which caused merge-recursive to fail if the merged
files were empty.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

Hi Dscho,

Your patch certainly fixes a bug in git-merge-file. It does not fix the bug in
git-merge-recursive, however. The test script also fails with your patch.

On Thu, Mar 13, 2008 at 04:19:35PM +0100, Johannes Schindelin wrote:
> On Sat, Mar 08, 2008 at 06:17:26PM +0100, Clemens Buchacher wrote:
> > One could argue that it would be better to mark the mismatching 
> > permissions as a conflict.
> 
> Right you are.  Your whole "it still is xdl_merge()s fault" point was just 
> contradicted by your own analysis.  Calling xdl_merge() when the sha1 does 
> _not_ differ is _a mistake_.  _That_ is the bug.

Alright, fixed in the appended patch.

Regards,
Clemens

 merge-recursive.c          |    9 +++++++--
 t/t6031-merge-recursive.sh |   23 +++++++++++++++++++++++
 2 files changed, 30 insertions(+), 2 deletions(-)
 create mode 100755 t/t6031-merge-recursive.sh

diff --git a/merge-recursive.c b/merge-recursive.c
index 34e3167..01918a7 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1028,9 +1028,14 @@ static struct merge_file_info merge_file(struct diff_filespec *o,
 		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 (!o->mode) {
+			if (a->mode != b->mode)
+				result.clean = 0;
+			result.mode = b->mode;
+		} else
+			result.mode = a->mode == o->mode ? b->mode: a->mode;
 
-		if (sha_eq(a->sha1, o->sha1))
+		if (sha_eq(a->sha1, b->sha1) || sha_eq(a->sha1, o->sha1))
 			hashcpy(result.sha, b->sha1);
 		else if (sha_eq(b->sha1, o->sha1))
 			hashcpy(result.sha, a->sha1);
diff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh
new file mode 100755
index 0000000..7ea371e
--- /dev/null
+++ b/t/t6031-merge-recursive.sh
@@ -0,0 +1,23 @@
+#!/bin/sh
+
+test_description='merge-recursive corner cases'
+. ./test-lib.sh
+
+test_expect_success 'merge empty files with different permission flags' '
+	: >dummy &&
+	git add dummy &&
+	git commit -m "initial commit" &&
+	git checkout -b a master &&
+	: >a &&
+	git add a &&
+	git commit -m "branch a" &&
+	git checkout -b b master &&
+	: >a &&
+	chmod +x a &&
+	git add a &&
+	git commit -m "branch b" &&
+	git checkout master &&
+	! (git merge-recursive master -- a b || test $? -ne 1)
+'
+
+test_done
-- 
1.5.4.4.2.gd2fe

Re: [PATCH] merge-recursive: cause a conflict if file mode does not match

[Johannes Schindelin]

Hi,

On Thu, 13 Mar 2008, Clemens Buchacher wrote:

> Previously, mismatching file modes would be auto-merged by picking the 
> mode in the remote tree.
> 
> This also fixes a bug which caused merge-recursive to fail if the merged 
> files were empty.
> 
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> ---
> 
> Hi Dscho,
> 
> Your patch certainly fixes a bug in git-merge-file. It does not fix the 
> bug in git-merge-recursive, however. The test script also fails with 
> your patch.

Now, _that_ is funny.  I tested before sending, and my test suit runs just 
fine.

> On Thu, Mar 13, 2008 at 04:19:35PM +0100, Johannes Schindelin wrote:
> > On Sat, Mar 08, 2008 at 06:17:26PM +0100, Clemens Buchacher wrote:
> > > One could argue that it would be better to mark the mismatching 
> > > permissions as a conflict.
> > 
> > Right you are.  Your whole "it still is xdl_merge()s fault" point was 
> > just contradicted by your own analysis.  Calling xdl_merge() when the 
> > sha1 does _not_ differ is _a mistake_.  _That_ is the bug.
> 
> Alright, fixed in the appended patch.

I have to admit that I wanted you to fix that patch, instead of me, 
because you were already researching the issue.

>  merge-recursive.c          |    9 +++++++--
>  t/t6031-merge-recursive.sh |   23 +++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 2 deletions(-)
>  create mode 100755 t/t6031-merge-recursive.sh

Looks much better.

> diff --git a/merge-recursive.c b/merge-recursive.c
> index 34e3167..01918a7 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1028,9 +1028,14 @@ static struct merge_file_info merge_file(struct diff_filespec *o,
>  		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 (!o->mode) {
> +			if (a->mode != b->mode)
> +				result.clean = 0;
> +			result.mode = b->mode;
> +		} else
> +			result.mode = a->mode == o->mode ? b->mode: a->mode;

So you only set clean = 0 if o->mode == 0, i.e. the file did not exist?  
That was not what I had in mind.  I would have expected that "if (a->mode 
!= b->mode)" to come _before_ the assignment to result.mode, which should 
have been left alone.

The rationale would have been this:

If the modes are different, the merge is not clean.

If the SHA-1s differ, the merge is not clean, and xld_merge() should be 
called.

> -		if (sha_eq(a->sha1, o->sha1))
> +		if (sha_eq(a->sha1, b->sha1) || sha_eq(a->sha1, o->sha1))

Why do you still compare to o->sha1?

/me goes looking in the original source, since the issue and the fix does 
not become apparent from your patch, including the commit message.

Oh, okay.

You are reusing a _different_ case, which just happens to have the same 
outcome.

In a perfect world, this would have a one-line comment above to explain 
issues.

> diff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh
> new file mode 100755
> index 0000000..7ea371e
> --- /dev/null
> +++ b/t/t6031-merge-recursive.sh
> @@ -0,0 +1,23 @@
> +#!/bin/sh
> +
> +test_description='merge-recursive corner cases'
> +. ./test-lib.sh
> +
> +test_expect_success 'merge empty files with different permission flags' '

The point is not that they are empty.  Maybe you want to fix that message.

> +	: >dummy &&
> +	git add dummy &&
> +	git commit -m "initial commit" &&
> +	git checkout -b a master &&
> +	: >a &&
> +	git add a &&
> +	git commit -m "branch a" &&
> +	git checkout -b b master &&
> +	: >a &&
> +	chmod +x a &&
> +	git add a &&
> +	git commit -m "branch b" &&
> +	git checkout master &&
> +	! (git merge-recursive master -- a b || test $? -ne 1)
> +'
> +
> +test_done
> -- 
> 1.5.4.4.2.gd2fe
> 

Thanks,
Dscho

Re: [PATCH] fix recursive-merge of empty files with different permissions

[Johannes Schindelin]

Hi,

On Thu, 13 Mar 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > diff --git a/builtin-merge-file.c b/builtin-merge-file.c
> > index adce6d4..cde4b2c 100644
> > --- a/builtin-merge-file.c
> > +++ b/builtin-merge-file.c
> > @@ -57,7 +57,8 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
> >  
> >  		if (!f)
> >  			ret = error("Could not open %s for writing", filename);
> > -		else if (fwrite(result.ptr, result.size, 1, f) != 1)
> > +		else if (result.size &&
> > +				fwrite(result.ptr, result.size, 1, f) != 1)
> >  			ret = error("Could not write to %s", filename);
> >  		else if (fclose(f))
> >  			ret = error("Could not close %s", filename);
> 
> Lol.  We are dealing with N-byte quantity so we send one record of length
> N and make sure we processed one record, and it does not work when N is
> zero.
> 
> We could instead send N records of size 1 and make sure we processed N
> records to lose the conditional instead, but the patch avoids unnecessary
> call to fread/fwrite so that is good.  Thanks.
> 
> It felt funny because my current bedtime reading happens to be "Zero: The
> Biography of a Dangerous Idea (ISBN 0140296476)".

Heh.

> > diff --git a/xdiff-interface.c b/xdiff-interface.c
> > index bba2364..61dc5c5 100644
> > --- a/xdiff-interface.c
> > +++ b/xdiff-interface.c
> > @@ -152,8 +152,8 @@ int read_mmfile(mmfile_t *ptr, const char *filename)
> >  	if ((f = fopen(filename, "rb")) == NULL)
> >  		return error("Could not open %s", filename);
> >  	sz = xsize_t(st.st_size);
> > -	ptr->ptr = xmalloc(sz);
> > -	if (fread(ptr->ptr, sz, 1, f) != 1)
> > +	ptr->ptr = xmalloc(sz ? sz : 1);
> > +	if (sz && fread(ptr->ptr, sz, 1, f) != 1)
> >  		return error("Could not read %s", filename);
> >  	fclose(f);
> >  	ptr->size = sz;
> 
> Do you need to actually allocate ptr->ptr when sz is zero, instead of
> setting it to NULL, like:
> 
> 	sz = xsize_t(st.st_size);
> 	ptr->size = sz;
>         if (!sz)
>         	ptr->ptr = NULL;
> 	else {
>         	ptr->ptr = xmalloc(sz);
> 		if (fread(ptr->ptr, 1, sz, f) != sz)
> 			return error("Could not read %s", filename);
> 	}
> 	fclose(f);

I was going for the safe option.  In theory, you are right, but I cannot 
really be sure that e.g. a memcpy() with size 0 will not access the source 
pointer at all.

Ciao,
Dscho

[PATCH] merge-recursive: handle file mode changes

[Clemens Buchacher]

Handle file mode changes similarly to changes of content. If the file mode
changed in only one branch, keep the changed version. If the file mode
changed in both branches, prompt the user to choose one by reporting a
conflict.

This also fixes a bug which caused the merge to fail if the merged files were
empty.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

Hi,

On Thu, Mar 13, 2008 at 10:17:07PM +0100, Johannes Schindelin wrote:
> If the modes are different, the merge is not clean.

If the mode has only changed in either the head or the remote tree, I believe
we should keep the changed version without conflict - just like we do for
non-overlapping changes of content.

If, however, both files changed in a different way, we report a conflict and
keep the remote version by default. I based this decision on the assumption
that the user is more likely to have acknowledged the head branch, while he may
want to think about whether or not the change in the remote version is ok.

I cleaned up the code a bit and added a comment, so I hope the behavior is
clearer now.

> The point is not that they are empty.  Maybe you want to fix that message.

Indeed.

I am not exactly sure how I should set the result.merge flag. In this context
it seems to have the exact opposite meaning of result.clean. Is that correct?

Regards,
Clemens

 merge-recursive.c          |   21 ++++++++++++++++-----
 t/t6031-merge-recursive.sh |   39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 5 deletions(-)
 create mode 100755 t/t6031-merge-recursive.sh

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.
+		 */
+		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;
 
-		if (sha_eq(a->sha1, o->sha1))
+		if (sha_eq(a->sha1, b->sha1))
+			hashcpy(result.sha, b->sha1);
+		else if (sha_eq(a->sha1, o->sha1))
 			hashcpy(result.sha, b->sha1);
 		else if (sha_eq(b->sha1, o->sha1))
 			hashcpy(result.sha, a->sha1);
@@ -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;
 	}
 
 	return result;
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
+
+test_description='merge-recursive: handle file mode'
+. ./test-lib.sh
+
+test_expect_success 'mode change in one branch: keep changed version' '
+	: >file1 &&
+	git add file1 &&
+	git commit -m initial &&
+	git checkout -b a1 master &&
+	: >dummy &&
+	git add dummy &&
+	git commit -m a &&
+	git checkout -b b1 master &&
+	chmod +x file1 &&
+	git add file1 &&
+	git commit -m b1 &&
+	git checkout a1 &&
+	git merge-recursive master -- a1 b1 &&
+	test -x file1
+'
+
+test_expect_success 'mode change in both branches: expect conflict' '
+	git reset --hard HEAD &&
+	git checkout -b a2 master &&
+	: >file2 &&
+	chmod +x file2 &&
+	git add file2 &&
+	git commit -m a2 &&
+	git checkout -b b2 master &&
+	: >file2 &&
+	git add file2 &&
+	git commit -m b2 &&
+	git checkout a2 &&
+	! (git merge-recursive master -- a2 b2 || test $? -ne 1) &&
+	! test -x file2
+'
+
+test_done
-- 
1.5.4.4

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

[Johannes Schindelin]

Hi,

On Thu, 13 Mar 2008, Clemens Buchacher wrote:

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

That is new.  Doesn't this overwrite what has been set in

        } else {
                if (!sha_eq(a->sha1, o->sha1) && !sha_eq(b->sha1, o->sha1))
                        result.merge = 1;

?

Ciao,
Dscho

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

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> I am not exactly sure how I should set the result.merge flag. In this context
> it seems to have the exact opposite meaning of result.clean. Is that correct?

My reading of the code is that result.merge is "if a content level merge
has happened", and result.clean is "given that a content level merge has
been attempted, was it done cleanly, or are there conflicts for the user
to fix in the result".  If result.clean is not true, we obviously cannot
store the result in stage #0.

The result.merge flag is used only for reporting purposes; I am not sure
why the non-rename codepath does not pay attention to the result.merge,
though.

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

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> I am not exactly sure how I should set the result.merge flag. In this context
> it seems to have the exact opposite meaning of result.clean. Is that correct?

This is from the e-mail header of your message:

  Mail-Followup-To: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
   git@vger.kernel.org, gitster@pobox.com

If you are asking people question, do not do this.  You want answers
addressed to you.  Johannes and I may not be interested in learning what
may want to find out, and redirecting answers for you to us is simply
rude.

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

[Jakub Narebski]

Clemens Buchacher wrote:

> Handle file mode changes similarly to changes of content. If the file mode
> changed in only one branch, keep the changed version. If the file mode
> changed in both branches, prompt the user to choose one by reporting a
> conflict.

Shouldn't it print "CONFLICT(mode/mode)" then?

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

[PATCH] merge-recursive: handle file mode and links similarly to file content

[Clemens Buchacher]

If the file mode or link changed in only one branch, keep the changed
version. If the file mode or link changed differently in both branches,
report a conflict. If this happens, the user is more likely to be aware of
the change in the head branch. Choose the remote version by default, in
order to make the user think about the change.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

On Fri, Mar 14, 2008 at 12:40:05AM +0100, Johannes Schindelin wrote:
> On Thu, 13 Mar 2008, Clemens Buchacher wrote:
> > +		result.merge = !result.clean;
> 
> That is new.  Doesn't this overwrite what has been set in
> 
>         } else {
>                 if (!sha_eq(a->sha1, o->sha1) && !sha_eq(b->sha1, o->sha1))
>                         result.merge = 1;

Yeah, that's no good. I think I understand the meaning of result.merge and
result.clean now.

result.merge indicates that there are changes in both branches, whereas
result.clean indicates that the merge was trivial.

I amended the patch to reflect this.

I also noticed that in case of LINKs or GITLINKs which changed in both
branches, the head version is kept by default. By the same rationale I gave for
the file modes, I think the remote version should be kept instead, in order to
make the user aware of this change.

Regards,
Clemens

 merge-recursive.c          |   34 +++++++++++++++++++++++-----------
 t/t6031-merge-recursive.sh |   39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 11 deletions(-)
 create mode 100755 t/t6031-merge-recursive.sh

diff --git a/merge-recursive.c b/merge-recursive.c
index 34e3167..36f78a2 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1025,16 +1025,31 @@ 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;
+		/*
+		 * If the file changed in only one branch, keep the changed
+		 * version. If the file changed in both, try to merge
+		 * automatically. If the merge is not trivial, report a
+		 * conflict. In case of conflicting file modes or links, choose
+		 * remote version by default. They can only be merged trivially
+		 * if they are equal.
+		 */
 
-		result.mode = a->mode == o->mode ? b->mode: a->mode;
+		if (a->mode != o->mode && b->mode != o->mode) {
+			result.mode = b->mode;
+			if (a->mode != b->mode)
+				result.clean = 0;
+			result.merge = 1;
+		} else
+			result.mode = o->mode == a->mode ? b->mode : a->mode;
 
 		if (sha_eq(a->sha1, o->sha1))
 			hashcpy(result.sha, b->sha1);
 		else if (sha_eq(b->sha1, o->sha1))
 			hashcpy(result.sha, a->sha1);
-		else if (S_ISREG(a->mode)) {
+		else if (sha_eq(a->sha1, b->sha1)) {
+			hashcpy(result.sha, a->sha1);
+			result.merge = 1;
+		} else if (S_ISREG(a->mode)) {
 			mmbuffer_t result_buf;
 			int merge_status;
 
@@ -1051,14 +1066,11 @@ static struct merge_file_info merge_file(struct diff_filespec *o,
 
 			free(result_buf.ptr);
 			result.clean = (merge_status == 0);
-		} else if (S_ISGITLINK(a->mode)) {
+			result.merge = 1;
+		} else if (S_ISGITLINK(a->mode) || S_ISLNK(a->mode)) {
+			hashcpy(result.sha, b->sha1);
 			result.clean = 0;
-			hashcpy(result.sha, a->sha1);
-		} else if (S_ISLNK(a->mode)) {
-			hashcpy(result.sha, a->sha1);
-
-			if (!sha_eq(a->sha1, b->sha1))
-				result.clean = 0;
+			result.merge = 1;
 		} else {
 			die("unsupported object type in the tree");
 		}
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
+
+test_description='merge-recursive: handle file mode'
+. ./test-lib.sh
+
+test_expect_success 'mode change in one branch: keep changed version' '
+	: >file1 &&
+	git add file1 &&
+	git commit -m initial &&
+	git checkout -b a1 master &&
+	: >dummy &&
+	git add dummy &&
+	git commit -m a &&
+	git checkout -b b1 master &&
+	chmod +x file1 &&
+	git add file1 &&
+	git commit -m b1 &&
+	git checkout a1 &&
+	git merge-recursive master -- a1 b1 &&
+	test -x file1
+'
+
+test_expect_success 'mode change in both branches: expect conflict' '
+	git reset --hard HEAD &&
+	git checkout -b a2 master &&
+	: >file2 &&
+	chmod +x file2 &&
+	git add file2 &&
+	git commit -m a2 &&
+	git checkout -b b2 master &&
+	: >file2 &&
+	git add file2 &&
+	git commit -m b2 &&
+	git checkout a2 &&
+	! (git merge-recursive master -- a2 b2 || test $? -ne 1) &&
+	! test -x file2
+'
+
+test_done
-- 
1.5.4.4

Re: [PATCH] merge-recursive: cause a conflict if file mode does not match

[Clemens Buchacher]

Hi,

On Thu, Mar 13, 2008 at 10:17:07PM +0100, Johannes Schindelin wrote:
> > Your patch certainly fixes a bug in git-merge-file. It does not fix the 
> > bug in git-merge-recursive, however. The test script also fails with 
> > your patch.
> 
> Now, _that_ is funny.  I tested before sending, and my test suit runs just 
> fine.

I didn't mean the existing test suite, which is fine with your bugfix, of
course. I was talking about the test which I submitted along with my bugfix.

Regards,
Clemens

Re: [PATCH] merge-recursive: handle file mode and links similarly to file content

[Clemens Buchacher]

On Fri, Mar 14, 2008 at 10:21:05AM +0100, Clemens Buchacher wrote:
> @@ -1051,14 +1066,11 @@ static struct merge_file_info merge_file(struct diff_filespec *o,
>  
>  			free(result_buf.ptr);
>  			result.clean = (merge_status == 0);
> -		} else if (S_ISGITLINK(a->mode)) {
> +			result.merge = 1;
> +		} else if (S_ISGITLINK(a->mode) || S_ISLNK(a->mode)) {
> +			hashcpy(result.sha, b->sha1);
>  			result.clean = 0;
> -			hashcpy(result.sha, a->sha1);
> -		} else if (S_ISLNK(a->mode)) {
> -			hashcpy(result.sha, a->sha1);
> -
> -			if (!sha_eq(a->sha1, b->sha1))
> -				result.clean = 0;
> +			result.merge = 1;

To clarify: The above two cases don't need to be handled separately any
more, because equal sha1s are already handled above.

Clemens

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

[Junio C Hamano]

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.

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

[Clemens Buchacher]

On Fri, Mar 14, 2008 at 03:15:26AM -0700, Junio C Hamano wrote:
> > 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.

Indeed. I think "b" should be favored over "a", however.

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

Yes, but if "b" and "a" both changed in the same way, we should still set
result.merge = 1, because that's more akin to an automatic merge than a
fast-forward merge, IMO.

Clemens

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

[Clemens Buchacher]

On Fri, Mar 14, 2008 at 01:17:10AM +0100, Jakub Narebski wrote:
> > Handle file mode changes similarly to changes of content. If the file mode
> > changed in only one branch, keep the changed version. If the file mode
> > changed in both branches, prompt the user to choose one by reporting a
> > conflict.
> 
> Shouldn't it print "CONFLICT(mode/mode)" then?

I think currently the only way to get a mode conflict is when a file is added
in both branches with different permissions, since we only track the executable
bit.

Clemens

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

[Clemens Buchacher]

On Thu, Mar 13, 2008 at 05:12:39PM -0700, Junio C Hamano wrote:
> This is from the e-mail header of your message:
> 
>   Mail-Followup-To: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
>    git@vger.kernel.org, gitster@pobox.com
> 
> If you are asking people question, do not do this.  You want answers
> addressed to you.  Johannes and I may not be interested in learning what
> may want to find out, and redirecting answers for you to us is simply
> rude.

My apologies. I wasn't aware that was doing that.

Clemens

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

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

>> Reading the rest of the function, I notice that it consistently favor "a"
>> over "b", when a conflict cannot be reconciled.
>
> Indeed. I think "b" should be favored over "a", however.

Why?

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

[Clemens Buchacher]

On Fri, Mar 14, 2008 at 09:01:06AM -0700, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
> >> Reading the rest of the function, I notice that it consistently favor "a"
> >> over "b", when a conflict cannot be reconciled.
> >
> > Indeed. I think "b" should be favored over "a", however.
> 
> Why?

>From the commit message to the latest version of my patch:
http://marc.info/?l=git&m=120548648727308&w=2

On Fri, Mar 14, 2008 at 10:21:05AM +0100, Clemens Buchacher wrote:
> If the file mode or link changed in only one branch, keep the changed
> version. If the file mode or link changed differently in both branches,
> report a conflict. If this happens, the user is more likely to be aware of
> the change in the head branch. Choose the remote version by default, in
> order to make the user think about the change.

In principle, both decisions are equally right or wrong. However, suggesting
the remote version (i.e., "b") by default gives more incentive to think about
it because the file now changed with respect to the head version (i.e., "a"),
which the user started out with.

Clemens

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

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> On Fri, Mar 14, 2008 at 10:21:05AM +0100, Clemens Buchacher wrote:
>> If the file mode or link changed in only one branch, keep the changed
>> version. If the file mode or link changed differently in both branches,
>> report a conflict. If this happens, the user is more likely to be aware of
>> the change in the head branch. Choose the remote version by default, in
>> order to make the user think about the change.
>
> In principle, both decisions are equally right or wrong. However, suggesting
> the remote version (i.e., "b") by default gives more incentive to think about
> it because the file now changed with respect to the head version (i.e., "a"),
> which the user started out with.

This matters only when the conflict is irreconcilable in the work tree,
and as long as we get the most important part right, i.e. marking the path
to be conflicting, that is good enough to make sure that the user does not
commit without thinking.

Besides the change breaks the traditional behaviour of leaving the "a"
side in the work tree.  As you already know, what you have done is as
equally valid as the other side did.  Be assertive and more confident in
what you have done ;-).