fix recursive-merge of empty files with different permissions
PATCH 1/1 fix recursive-merge of empty files with different permissions
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.
---
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
t/t6031-merge-recursive.sh new file +23 -0
@@ -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
xdiff/xmerge.c +14 -16
@@ -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);
Signature · 1 line
1.5.4.3.468.g36990.dirty
Hi,
Quoted 2 lines · Clemens Buchacher
I do not understand why, but this does not happen if the file permissions are the same.
I think this is the biggest problem.
Quoted 2 lines
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
Hi,
Quoted 4 lines · 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. Makexdl_merge()choose one of the files instead.
Quoted 11 lines · Johannes Schindelin
Quoted 2 lines · Clemens Buchacher
I do not understand why, but this does not happen if the file permissions are the same.
I think this is the biggest problem.
Quoted 2 lines
t/t6031-merge-recursive.sh | 23 +++++++++++++++++++++++ xdiff/xmerge.c | 30 ++++++++++++++----------------... because
xdiff/xmerge.cis 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
Hi,
[Junio, there is a bugfix at the end of this mail.]
Quoted 34 lines · Clemens Buchacher
Hi,
Quoted 5 lines · 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. Makexdl_merge()choose one of the files instead.Quoted 11 lines · Johannes Schindelin
Quoted 2 lines · Clemens Buchacher
I do not understand why, but this does not happen if the file permissions are the same.
I think this is the biggest problem.
Quoted 2 lines
t/t6031-merge-recursive.sh | 23 +++++++++++++++++++++++ xdiff/xmerge.c | 30 ++++++++++++++----------------... because
xdiff/xmerge.cis 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 inxdl_merge(). Different permissions are only the trigger of the problem, because only then willxdl_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 (seeDocumentation/trivial-merge.txt), i.e. head is chosen as the merge result.
Quoted 6 lines
If either sha1 _or_ mode differ between the head and remote entries, however,
merge_trees()will usexdl_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).
Quoted 8 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.
--- builtin-merge-file.c | 3 ++- xdiff-interface.c | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-)
builtin-merge-file.c +2 -1
@@ -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);
xdiff-interface.c +2 -2
@@ -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;
Signature · 1 line
1.5.4.4.706.ga822
PATCH 1/1 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.
---
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.
Quoted 7 lines · Johannes Schindelin
Quoted 2 lines · Clemens Buchacher
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 _amistake_. _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
merge-recursive.c +7 -2
@@ -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);
t/t6031-merge-recursive.sh new file +23 -0
@@ -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
Signature · 1 line
1.5.4.4.2.gd2fe
Re: [PATCH] merge-recursive: cause a conflict if file mode does not match
Hi,
Quoted 6 lines · 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.
Quoted 8 lines
Signed-off-by Clemens Buchacher <[email protected]>---
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.
Quoted 10 lines · Johannes Schindelin
Quoted 7 lines · Clemens Buchacher
Quoted 2 lines
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 _amistake_. _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.
Quoted 4 lines
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.
Quoted 5 lines
merge-recursive.c +0
-0
merge-recursive.c static struct merge_file_info merge_file(struct diff_filespec *o, · line 1036
Quoted 10 lines
merge-recursive.c +6
-1@@ -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.
merge-recursive.c static struct merge_file_info merge_file(struct diff_filespec *o, · line 1038
Quoted 2 lines
merge-recursive.c +1
-1-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.
t/t6031-merge-recursive.sh line 6
Quoted 12 lines
t/t6031-merge-recursive.sh new file +6
-0@@ -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.
Quoted 20 lines
t/t6031-merge-recursive.sh new file +17
-0+ : >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_doneSignature · 1 line
1.5.4.4.2.gd2fe
Thanks,
Dscho
PATCH 1/1 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.
---
Hi,
Quoted 1 line · Johannes Schindelin
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.
Quoted 1 line
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
merge-recursive.c +16 -5
@@ -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;
t/t6031-merge-recursive.sh new file +39 -0
@@ -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
Signature · 1 line
1.5.4.4
Re: [PATCH] merge-recursive: handle file mode changes
Quoted 2 lines · Clemens Buchacher
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 <[email protected]>,
[email protected], [email protected]
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
Quoted 9 lines · Junio C Hamano
This is from the e-mail header of your message:
Mail-Followup-To: Johannes Schindelin <[email protected]>,
[email protected], [email protected]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
In reply to Clemens BuchacherQuoted 2 lines · Clemens Buchacher
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
In reply to Clemens BuchacherQuoted 7 lines · Clemens Buchacher
merge-recursive.c +0
-0@@ -1025,12 +1025,21 @@ static struct merge_file_info merge_file(struct diff_filespec *o,hashcpy (result.sha, b->sha1); }
merge-recursive.c static struct merge_file_info merge_file(struct diff_filespec *o, · line 1032
Quoted 10 lines
merge-recursive.c +5
-4}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.
merge-recursive.c static struct merge_file_info merge_file(struct diff_filespec *o, · line 1038
Quoted 6 lines
merge-recursive.c +6
-0+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.
merge-recursive.c static struct merge_file_info merge_file(struct diff_filespec *o, · line 1075
Quoted 6 lines
merge-recursive.c +2
-0@@ -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.
t/t6031-merge-recursive.sh line 37
Quoted 11 lines
t/t6031-merge-recursive.sh new file +4
-0@@ -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
Quoted 12 lines · Junio C Hamano
Quoted 12 lines
merge-recursive.c +0
-4@@ -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;
Quoted 8 lines
merge-recursive.c static struct merge_file_info merge_file(struct diff_filespec *o, · line 1032
Quoted 5 lines
merge-recursive.c +5
-0+/* +* 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.
Quoted 19 lines
merge-recursive.c static struct merge_file_info merge_file(struct diff_filespec *o, · line 1038
Quoted 6 lines
merge-recursive.c +6
-0+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; } }
Quoted 3 lines
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
Quoted 4 lines · Clemens Buchacher
Quoted 2 lines
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
Quoted 9 lines · Junio C Hamano
Quoted 4 lines · Clemens Buchacher
Quoted 2 lines
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
Quoted 5 lines · 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.
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
Quoted 11 lines · Clemens Buchacher
Quoted 5 lines · 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.
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 ;-).
Re: [PATCH] merge-recursive: handle file mode changes
In reply to Clemens BuchacherHi,
merge-recursive.c static struct merge_file_info merge_file(struct diff_filespec *o, · line 1075
Quoted 6 lines · Clemens Buchacher
merge-recursive.c +2
-0@@ -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
PATCH 1/1 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.
---
Quoted 8 lines · Johannes Schindelin
Review
Quoted 1 line · Clemens Buchacher
(quoted diff) +1
-0+ 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
merge-recursive.c +23 -11
@@ -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" ); }
t/t6031-merge-recursive.sh new file +39 -0
@@ -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
Signature · 1 line
1.5.4.4
Re: [PATCH] merge-recursive: handle file mode and links similarly to file content
Quoted 6 lines · Clemens Buchacher
merge-recursive.c +1
-1@@ -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 ;
merge-recursive.c static struct merge_file_info merge_file(struct diff_filespec *o, · line 1073
Quoted 10 lines
merge-recursive.c +3
-6+ }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
In reply to Clemens BuchacherQuoted 4 lines · 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.
Shouldn't it print "CONFLICT(mode/mode)" then?
Signature · 3 lines
Jakub Narebski Warsaw, Poland ShadeHawk on #git
Re: [PATCH] merge-recursive: handle file mode changes
Quoted 6 lines · Jakub Narebski
Quoted 4 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
Re: [PATCH] merge-recursive: cause a conflict if file mode does not match
In reply to Johannes SchindelinHi,
Quoted 6 lines · Johannes Schindelin
Quoted 3 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
Quoted 5 lines · Johannes Schindelin
builtin-merge-file.c +0
-0
builtin-merge-file.c int cmd_merge_file(int argc, const char **argv, const char *prefix) · line 64
Quoted 9 lines
builtin-merge-file.c +2
-1@@ -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)".
Quoted 5 lines
xdiff-interface.c +0
-0
xdiff-interface.c int read_mmfile(mmfile_t *ptr, const char *filename) · line 159
Quoted 10 lines
xdiff-interface.c +2
-2@@ -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);Hi,
Quoted 21 lines · Junio C Hamano
Quoted 5 lines · Johannes Schindelin
builtin-merge-file.c +0
-0builtin-merge-file.c int cmd_merge_file(int argc, const char **argv, const char *prefix) · line 64
Quoted 9 lines
builtin-merge-file.c +2
-1@@ -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.
Quoted 6 lines
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.
Quoted 19 lines
Quoted 5 lines
xdiff-interface.c +0
-0xdiff-interface.c int read_mmfile(mmfile_t *ptr, const char *filename) · line 159
Quoted 10 lines
xdiff-interface.c +2
-2@@ -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->ptrwhen sz is zero, instead of setting it to NULL, like:
Quoted 10 lines
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