threads / patch / 12576

patchfix recursive-merge of empty files with different permissions

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

## tl;dr

23 messages between Mar 8, 2008 and Mar 14, 2008. Diffs are folded; open one to read it.

replies: 22people: 4as markdown or json

Clemens Buchacher· Mar 8, 2008, 17:17 UTC · lore

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
Show changes to 2 files +37 −16

t/t6031-merge-recursive.sh, xdiff/xmerge.c

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
Johannes Schindelin· Mar 8, 2008, 17:51 UTC · re: Clemens Buchacher · lore

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

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

Clemens Buchacher· Mar 13, 2008, 12:52 UTC · re: Johannes Schindelin · lore

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

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:
Show 11 quoted lines
> 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

Johannes Schindelin· Mar 13, 2008, 15:19 UTC · re: Clemens Buchacher · lore

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

Hi,
[Junio, there is a bugfix at the end of this mail.]
On Thu, 13 Mar 2008, Clemens Buchacher wrote:
Show 40 quoted lines
> 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).

Show 8 quoted lines
> 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(-)
Show changes to 2 files +4 −3

builtin-merge-file.c, xdiff-interface.c

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
Junio C Hamano· Mar 13, 2008, 18:50 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 14 quoted lines
> 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)".

Show 15 quoted lines
> 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);
Johannes Schindelin· Mar 13, 2008, 21:28 UTC · re: Junio C Hamano · lore

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

Hi,
On Thu, 13 Mar 2008, Junio C Hamano wrote:
Show 27 quoted lines
> 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.
Show 29 quoted lines
> > 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

Clemens Buchacher· Mar 13, 2008, 19:22 UTC · re: Johannes Schindelin · lore

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

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:
Show 7 quoted lines
> 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
Show changes to 2 files +30 −2

merge-recursive.c, 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
Johannes Schindelin· Mar 13, 2008, 21:17 UTC · re: Clemens Buchacher · lore

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

Hi,
On Thu, 13 Mar 2008, Clemens Buchacher wrote:
Show 14 quoted lines
> 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.

Show 10 quoted lines
> 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.
Show 15 quoted lines
> 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.

Show 12 quoted lines
> 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.
Show 20 quoted lines
> +	: >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

Clemens Buchacher· Mar 13, 2008, 22:47 UTC · re: Johannes Schindelin · lore

[PATCH] merge-recursive: handle file mode changes

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
Show changes to 2 files +55 −5

merge-recursive.c, 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
Johannes Schindelin· Mar 13, 2008, 23:40 UTC · re: Clemens Buchacher · lore

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

Hi,
On Thu, 13 Mar 2008, Clemens Buchacher wrote:
Show 6 quoted lines
> @@ -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

Clemens Buchacher· Mar 14, 2008, 09:21 UTC · re: Johannes Schindelin · lore

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

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:
Show 8 quoted lines
> 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
Show changes to 2 files +62 −11

merge-recursive.c, 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
Clemens Buchacher· Mar 14, 2008, 10:13 UTC · re: Clemens Buchacher · lore

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

On Fri, Mar 14, 2008 at 10:21:05AM +0100, Clemens Buchacher wrote:
Show 16 quoted lines
> @@ -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
Junio C Hamano· Mar 14, 2008, 00:08 UTC · re: Clemens Buchacher · lore

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

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.

Junio C Hamano· Mar 14, 2008, 00:12 UTC · re: Clemens Buchacher · lore

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

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.

Clemens Buchacher· Mar 14, 2008, 13:02 UTC · re: Junio C Hamano · lore

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

On Thu, Mar 13, 2008 at 05:12:39PM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> 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
Jakub Narebski· Mar 14, 2008, 00:17 UTC · re: Clemens Buchacher · lore

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

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
Clemens Buchacher· Mar 14, 2008, 12:56 UTC · re: Jakub Narebski · lore

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

On Fri, Mar 14, 2008 at 01:17:10AM +0100, Jakub Narebski wrote:
Show 6 quoted lines
> > 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
Junio C Hamano· Mar 14, 2008, 10:15 UTC · re: Clemens Buchacher · lore

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

Clemens Buchacher <drizzd@aon.at> writes:
Show 17 quoted lines
> 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.

Show 6 quoted lines
> +		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.

Show 6 quoted lines
> @@ -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.

Show 11 quoted lines
> 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.
Clemens Buchacher· Mar 14, 2008, 12:17 UTC · re: Junio C Hamano · lore

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

On Fri, Mar 14, 2008 at 03:15:26AM -0700, Junio C Hamano wrote:
Show 20 quoted lines
> > 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.
Show 22 quoted lines
> > +		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
Junio C Hamano· Mar 14, 2008, 16:01 UTC · re: Clemens Buchacher · lore

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

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?
Clemens Buchacher· Mar 14, 2008, 17:28 UTC · re: Junio C Hamano · lore

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

On Fri, Mar 14, 2008 at 09:01:06AM -0700, Junio C Hamano wrote:
Show 7 quoted lines
> 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:
Show 5 quoted lines
> 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
Junio C Hamano· Mar 14, 2008, 17:49 UTC · re: Clemens Buchacher · lore

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

Clemens Buchacher <drizzd@aon.at> writes:
Show 11 quoted lines
> 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 ;-).

Clemens Buchacher· Mar 14, 2008, 10:07 UTC · re: Johannes Schindelin · lore

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

Hi,
On Thu, Mar 13, 2008 at 10:17:07PM +0100, Johannes Schindelin wrote:
Show 6 quoted lines
> > 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

← back to recent threads