{"thread":{"id":"12576","subject":"[PATCH] fix recursive-merge of empty files with different permissions","startedAt":"2008-03-08T17:17:26Z","lastAt":"2008-03-14T17:49:44Z","messageCount":23,"participants":["Clemens Buchacher","Johannes Schindelin","Junio C Hamano","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"71423","messageId":"20080308171726.GA16129@localhost","threadId":"12576","inReplyTo":null,"subject":"[PATCH] fix recursive-merge of empty files with different permissions","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-03-08T17:17:26Z","receivedAt":"2008-03-08T17:17:26Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"If git-merge-recursive attempts to merge two empty new files with\ndifferent executable flags, eventually xdl_merge() is called and produces\nempty diffs for both files and therefore does not choose either file as\nsuccessor. Make xdl_merge() choose one of the files instead.\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n\nHi,\n\nThe change in indentation makes this patch look larger than it is. All I\nactually did was remove the \"if (xscr1 || xscr2)\" condition.\n\nPreviously to this patch the included test showed the following output:\n\n\tMerging a with b\n\tMerging:\n\t82712b3 branch a\n\t7eacd6f branch b\n\tfound 1 common ancestor(s):\n\t33b7ba5 initial commit\n\tAuto-merged a\n\tfatal: Failed to execute internal merge\n\nI do not understand why, but this does not happen if the file permissions are\nthe same.\n\nThanks,\nClemens\n\n t/t6031-merge-recursive.sh |   23 +++++++++++++++++++++++\n xdiff/xmerge.c             |   30 ++++++++++++++----------------\n 2 files changed, 37 insertions(+), 16 deletions(-)\n create mode 100755 t/t6031-merge-recursive.sh\n\ndiff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh\nnew file mode 100755\nindex 0000000..4e3456b\n--- /dev/null\n+++ b/t/t6031-merge-recursive.sh\n@@ -0,0 +1,23 @@\n+#!/bin/sh\n+\n+test_description='merge-recursive corner cases'\n+. ./test-lib.sh\n+\n+test_expect_success 'merge empty files with different permission flags' '\n+\t: >dummy &&\n+\tgit add dummy &&\n+\tgit commit -m \"initial commit\" &&\n+\tgit checkout -b a master &&\n+\t: >a &&\n+\tgit add a &&\n+\tgit commit -m \"branch a\" &&\n+\tgit checkout -b b master &&\n+\t: >a &&\n+\tchmod +x a &&\n+\tgit add a &&\n+\tgit commit -m \"branch b\" &&\n+\tgit checkout master &&\n+\tgit merge-recursive master -- a b\n+'\n+\n+test_done\ndiff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex 82b3573..92127e1 100644\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -470,23 +470,21 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, const char *name1,\n \t\treturn -1;\n \t}\n \tstatus = 0;\n-\tif (xscr1 || xscr2) {\n-\t\tif (!xscr1) {\n-\t\t\tresult->ptr = xdl_malloc(mf2->size);\n-\t\t\tmemcpy(result->ptr, mf2->ptr, mf2->size);\n-\t\t\tresult->size = mf2->size;\n-\t\t} else if (!xscr2) {\n-\t\t\tresult->ptr = xdl_malloc(mf1->size);\n-\t\t\tmemcpy(result->ptr, mf1->ptr, mf1->size);\n-\t\t\tresult->size = mf1->size;\n-\t\t} else {\n-\t\t\tstatus = xdl_do_merge(&xe1, xscr1, name1,\n-\t\t\t\t\t      &xe2, xscr2, name2,\n-\t\t\t\t\t      level, xpp, result);\n-\t\t}\n-\t\txdl_free_script(xscr1);\n-\t\txdl_free_script(xscr2);\n+\tif (!xscr1) {\n+\t\tresult->ptr = xdl_malloc(mf2->size);\n+\t\tmemcpy(result->ptr, mf2->ptr, mf2->size);\n+\t\tresult->size = mf2->size;\n+\t} else if (!xscr2) {\n+\t\tresult->ptr = xdl_malloc(mf1->size);\n+\t\tmemcpy(result->ptr, mf1->ptr, mf1->size);\n+\t\tresult->size = mf1->size;\n+\t} else {\n+\t\tstatus = xdl_do_merge(&xe1, xscr1, name1,\n+\t\t\t\t      &xe2, xscr2, name2,\n+\t\t\t\t      level, xpp, result);\n \t}\n+\txdl_free_script(xscr1);\n+\txdl_free_script(xscr2);\n \txdl_free_env(&xe1);\n \txdl_free_env(&xe2);\n \n-- \n1.5.4.3.468.g36990.dirty\n\n"},{"id":"71425","messageId":"alpine.LSU.1.00.0803081850470.3975@racer.site","threadId":"12576","inReplyTo":"20080308171726.GA16129@localhost","subject":"Re: [PATCH] fix recursive-merge of empty files with different permissions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-03-08T17:51:48Z","receivedAt":"2008-03-08T17:51:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 8 Mar 2008, Clemens Buchacher wrote:\n\n> I do not understand why, but this does not happen if the file \n> permissions are the same.\n\nI think this is the biggest problem.\n\n>  t/t6031-merge-recursive.sh |   23 +++++++++++++++++++++++\n>  xdiff/xmerge.c             |   30 ++++++++++++++----------------\n\n... because xdiff/xmerge.c is definitely the wrong place to \"fix\" this \nissue.  xdl_merge() does not even _know_ about permissions.\n\nCiao,\nDscho\n"},{"id":"71927","messageId":"20080313125229.GA24758@localhost","threadId":"12576","inReplyTo":"alpine.LSU.1.00.0803081850470.3975@racer.site","subject":"Re: [PATCH] fix recursive-merge of empty files with different permissions","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-03-13T12:52:29Z","receivedAt":"2008-03-13T12:52:29Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Hi,\n\nOn Sat, Mar 08, 2008 at 06:17:26PM +0100, Clemens Buchacher wrote:\n> If git-merge-recursive attempts to merge two empty new files with\n> different executable flags, eventually xdl_merge() is called and produces\n> empty diffs for both files and therefore does not choose either file as\n> successor. Make xdl_merge() choose one of the files instead.\n\nOn Sat, Mar 08, 2008 at 06:51:48PM +0100, Johannes Schindelin wrote:\n> On Sat, 8 Mar 2008, Clemens Buchacher wrote:\n> > I do not understand why, but this does not happen if the file \n> > permissions are the same.\n> \n> I think this is the biggest problem.\n> \n> >  t/t6031-merge-recursive.sh |   23 +++++++++++++++++++++++\n> >  xdiff/xmerge.c             |   30 ++++++++++++++----------------\n> \n> ... because xdiff/xmerge.c is definitely the wrong place to \"fix\" this \n> issue.  xdl_merge() does not even _know_ about permissions.\n\nAfter analyzing the problem in greater detail, I have to disagree. It is true,\nof course, that xdl_merge() does not and should not know about permissions at\nall. However, the bug is still in xdl_merge(). Different permissions are only\nthe trigger of the problem, because only then will xdl_merge() be called at\nall.\n\nWhat happens is this. Before looking at the file contents directly\nmerge_trees() attempts to resolve the merge trivially. If both sha1 and mode of\nthe head and remote entries match, the merge will be resolved as per case #5ALT\n(see Documentation/trivial-merge.txt), i.e. head is chosen as the merge result.\n\nIf either sha1 _or_ mode differ between the head and remote entries, however,\nmerge_trees() will use xdl_merge() to merge the file content and the remote\nentry's mode will be chosen as result mode.\n\nOne could argue that it would be better to mark the mismatching permissions as\na conflict. However, this is how the merge currently silently succeeds _unless_\nboth files are empty. If they are, xdl_merge() will effectively exit with an\nerror status and git-merge-recursive will fail with an internal error (as shown\nin the testcase).\n\nIn any case, I think it is reasonable to expect xdl_merge() to work with empty\nfiles. Whether or not the current \"mode merging\" behavior is desired is a\ndifferent matter.\n\nRegards,\nClemens\n"},{"id":"71951","messageId":"alpine.LSU.1.00.0803131607030.1656@racer.site","threadId":"12576","inReplyTo":"20080313125229.GA24758@localhost","subject":"Re: [PATCH] fix recursive-merge of empty files with different permissions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-03-13T15:19:35Z","receivedAt":"2008-03-13T15:19:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\n[Junio, there is a bugfix at the end of this mail.]\n\nOn Thu, 13 Mar 2008, Clemens Buchacher wrote:\n\n> Hi,\n> \n> On Sat, Mar 08, 2008 at 06:17:26PM +0100, Clemens Buchacher wrote:\n> > If git-merge-recursive attempts to merge two empty new files with \n> > different executable flags, eventually xdl_merge() is called and \n> > produces empty diffs for both files and therefore does not choose \n> > either file as successor. Make xdl_merge() choose one of the files \n> > instead.\n> \n> On Sat, Mar 08, 2008 at 06:51:48PM +0100, Johannes Schindelin wrote:\n> > On Sat, 8 Mar 2008, Clemens Buchacher wrote:\n> > > I do not understand why, but this does not happen if the file \n> > > permissions are the same.\n> > \n> > I think this is the biggest problem.\n> > \n> > >  t/t6031-merge-recursive.sh |   23 +++++++++++++++++++++++\n> > >  xdiff/xmerge.c             |   30 ++++++++++++++----------------\n> > \n> > ... because xdiff/xmerge.c is definitely the wrong place to \"fix\" this \n> > issue.  xdl_merge() does not even _know_ about permissions.\n> \n> After analyzing the problem in greater detail, I have to disagree. It is \n> true, of course, that xdl_merge() does not and should not know about \n> permissions at all. However, the bug is still in xdl_merge(). Different \n> permissions are only the trigger of the problem, because only then will \n> xdl_merge() be called at all.\n> \n> What happens is this. Before looking at the file contents directly \n> merge_trees() attempts to resolve the merge trivially. If both sha1 and \n> mode of the head and remote entries match, the merge will be resolved as \n> per case #5ALT (see Documentation/trivial-merge.txt), i.e. head is \n> chosen as the merge result.\n> \n> If either sha1 _or_ mode differ between the head and remote entries, \n> however, merge_trees() will use xdl_merge() to merge the file content \n> and the remote entry's mode will be chosen as result mode.\n> \n> One could argue that it would be better to mark the mismatching \n> permissions as a conflict.\n\nRight you are.  Your whole \"it still is xdl_merge()s fault\" point was just \ncontradicted by your own analysis.  Calling xdl_merge() when the sha1 does \n_not_ differ is _a mistake_.  _That_ is the bug.  Not xdl_merge() \nreturning 0 (because there were 0 conflicts).\n\n> However, this is how the merge currently silently succeeds _unless_ both \n> files are empty. If they are, xdl_merge() will effectively exit with an \n> error status and git-merge-recursive will fail with an internal error \n> (as shown in the testcase).\n> \n> In any case, I think it is reasonable to expect xdl_merge() to work with \n> empty files. Whether or not the current \"mode merging\" behavior is \n> desired is a different matter.\n\nI just tested.  xdl_merge() is _just fine_ with empty files.  However, \ngit-merge-file was not.  Fixed by this:\n\n-- snipsnap --\n[PATCH] merge-file: handle empty files gracefully\n\nEarlier, it would error out while trying to read and/or writing them.\nNow, calling merge-file with empty files is neither interesting nor\nuseful, but it is a bug that needed fixing.\n\nNoticed by Clemens Buchacher.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin-merge-file.c |    3 ++-\n xdiff-interface.c    |    4 ++--\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-merge-file.c b/builtin-merge-file.c\nindex adce6d4..cde4b2c 100644\n--- a/builtin-merge-file.c\n+++ b/builtin-merge-file.c\n@@ -57,7 +57,8 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n \n \t\tif (!f)\n \t\t\tret = error(\"Could not open %s for writing\", filename);\n-\t\telse if (fwrite(result.ptr, result.size, 1, f) != 1)\n+\t\telse if (result.size &&\n+\t\t\t\tfwrite(result.ptr, result.size, 1, f) != 1)\n \t\t\tret = error(\"Could not write to %s\", filename);\n \t\telse if (fclose(f))\n \t\t\tret = error(\"Could not close %s\", filename);\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex bba2364..61dc5c5 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -152,8 +152,8 @@ int read_mmfile(mmfile_t *ptr, const char *filename)\n \tif ((f = fopen(filename, \"rb\")) == NULL)\n \t\treturn error(\"Could not open %s\", filename);\n \tsz = xsize_t(st.st_size);\n-\tptr->ptr = xmalloc(sz);\n-\tif (fread(ptr->ptr, sz, 1, f) != 1)\n+\tptr->ptr = xmalloc(sz ? sz : 1);\n+\tif (sz && fread(ptr->ptr, sz, 1, f) != 1)\n \t\treturn error(\"Could not read %s\", filename);\n \tfclose(f);\n \tptr->size = sz;\n-- \n1.5.4.4.706.ga822\n"},{"id":"71983","messageId":"7vabl2xxun.fsf@gitster.siamese.dyndns.org","threadId":"12576","inReplyTo":"alpine.LSU.1.00.0803131607030.1656@racer.site","subject":"Re: [PATCH] fix recursive-merge of empty files with different permissions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-13T18:50:08Z","receivedAt":"2008-03-13T18:50:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> diff --git a/builtin-merge-file.c b/builtin-merge-file.c\n> index adce6d4..cde4b2c 100644\n> --- a/builtin-merge-file.c\n> +++ b/builtin-merge-file.c\n> @@ -57,7 +57,8 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n>  \n>  \t\tif (!f)\n>  \t\t\tret = error(\"Could not open %s for writing\", filename);\n> -\t\telse if (fwrite(result.ptr, result.size, 1, f) != 1)\n> +\t\telse if (result.size &&\n> +\t\t\t\tfwrite(result.ptr, result.size, 1, f) != 1)\n>  \t\t\tret = error(\"Could not write to %s\", filename);\n>  \t\telse if (fclose(f))\n>  \t\t\tret = error(\"Could not close %s\", filename);\n\nLol.  We are dealing with N-byte quantity so we send one record of length\nN and make sure we processed one record, and it does not work when N is\nzero.\n\nWe could instead send N records of size 1 and make sure we processed N\nrecords to lose the conditional instead, but the patch avoids unnecessary\ncall to fread/fwrite so that is good.  Thanks.\n\nIt felt funny because my current bedtime reading happens to be \"Zero: The\nBiography of a Dangerous Idea (ISBN 0140296476)\".\n\n> diff --git a/xdiff-interface.c b/xdiff-interface.c\n> index bba2364..61dc5c5 100644\n> --- a/xdiff-interface.c\n> +++ b/xdiff-interface.c\n> @@ -152,8 +152,8 @@ int read_mmfile(mmfile_t *ptr, const char *filename)\n>  \tif ((f = fopen(filename, \"rb\")) == NULL)\n>  \t\treturn error(\"Could not open %s\", filename);\n>  \tsz = xsize_t(st.st_size);\n> -\tptr->ptr = xmalloc(sz);\n> -\tif (fread(ptr->ptr, sz, 1, f) != 1)\n> +\tptr->ptr = xmalloc(sz ? sz : 1);\n> +\tif (sz && fread(ptr->ptr, sz, 1, f) != 1)\n>  \t\treturn error(\"Could not read %s\", filename);\n>  \tfclose(f);\n>  \tptr->size = sz;\n\nDo you need to actually allocate ptr->ptr when sz is zero, instead of\nsetting it to NULL, like:\n\n\tsz = xsize_t(st.st_size);\n\tptr->size = sz;\n        if (!sz)\n        \tptr->ptr = NULL;\n\telse {\n        \tptr->ptr = xmalloc(sz);\n\t\tif (fread(ptr->ptr, 1, sz, f) != sz)\n\t\t\treturn error(\"Could not read %s\", filename);\n\t}\n\tfclose(f);\n"},{"id":"71988","messageId":"20080313192246.GA30361@localhost","threadId":"12576","inReplyTo":"alpine.LSU.1.00.0803131607030.1656@racer.site","subject":"[PATCH] merge-recursive: cause a conflict if file mode does not match","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-03-13T19:22:46Z","receivedAt":"2008-03-13T19:22:46Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Previously, mismatching file modes would be auto-merged by picking the\nmode in the remote tree.\n\nThis also fixes a bug which caused merge-recursive to fail if the merged\nfiles were empty.\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n\nHi Dscho,\n\nYour patch certainly fixes a bug in git-merge-file. It does not fix the bug in\ngit-merge-recursive, however. The test script also fails with your patch.\n\nOn Thu, Mar 13, 2008 at 04:19:35PM +0100, Johannes Schindelin wrote:\n> On Sat, Mar 08, 2008 at 06:17:26PM +0100, Clemens Buchacher wrote:\n> > One could argue that it would be better to mark the mismatching \n> > permissions as a conflict.\n> \n> Right you are.  Your whole \"it still is xdl_merge()s fault\" point was just \n> contradicted by your own analysis.  Calling xdl_merge() when the sha1 does \n> _not_ differ is _a mistake_.  _That_ is the bug.\n\nAlright, fixed in the appended patch.\n\nRegards,\nClemens\n\n merge-recursive.c          |    9 +++++++--\n t/t6031-merge-recursive.sh |   23 +++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 2 deletions(-)\n create mode 100755 t/t6031-merge-recursive.sh\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 34e3167..01918a7 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1028,9 +1028,14 @@ static struct merge_file_info merge_file(struct diff_filespec *o,\n \t\tif (!sha_eq(a->sha1, o->sha1) && !sha_eq(b->sha1, o->sha1))\n \t\t\tresult.merge = 1;\n \n-\t\tresult.mode = a->mode == o->mode ? b->mode: a->mode;\n+\t\tif (!o->mode) {\n+\t\t\tif (a->mode != b->mode)\n+\t\t\t\tresult.clean = 0;\n+\t\t\tresult.mode = b->mode;\n+\t\t} else\n+\t\t\tresult.mode = a->mode == o->mode ? b->mode: a->mode;\n \n-\t\tif (sha_eq(a->sha1, o->sha1))\n+\t\tif (sha_eq(a->sha1, b->sha1) || sha_eq(a->sha1, o->sha1))\n \t\t\thashcpy(result.sha, b->sha1);\n \t\telse if (sha_eq(b->sha1, o->sha1))\n \t\t\thashcpy(result.sha, a->sha1);\ndiff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh\nnew file mode 100755\nindex 0000000..7ea371e\n--- /dev/null\n+++ b/t/t6031-merge-recursive.sh\n@@ -0,0 +1,23 @@\n+#!/bin/sh\n+\n+test_description='merge-recursive corner cases'\n+. ./test-lib.sh\n+\n+test_expect_success 'merge empty files with different permission flags' '\n+\t: >dummy &&\n+\tgit add dummy &&\n+\tgit commit -m \"initial commit\" &&\n+\tgit checkout -b a master &&\n+\t: >a &&\n+\tgit add a &&\n+\tgit commit -m \"branch a\" &&\n+\tgit checkout -b b master &&\n+\t: >a &&\n+\tchmod +x a &&\n+\tgit add a &&\n+\tgit commit -m \"branch b\" &&\n+\tgit checkout master &&\n+\t! (git merge-recursive master -- a b || test $? -ne 1)\n+'\n+\n+test_done\n-- \n1.5.4.4.2.gd2fe\n"},{"id":"71995","messageId":"alpine.LSU.1.00.0803132216580.4174@racer.site","threadId":"12576","inReplyTo":"20080313192246.GA30361@localhost","subject":"Re: [PATCH] merge-recursive: cause a conflict if file mode does not match","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-03-13T21:17:07Z","receivedAt":"2008-03-13T21:17:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 13 Mar 2008, Clemens Buchacher wrote:\n\n> Previously, mismatching file modes would be auto-merged by picking the \n> mode in the remote tree.\n> \n> This also fixes a bug which caused merge-recursive to fail if the merged \n> files were empty.\n> \n> Signed-off-by: Clemens Buchacher <drizzd@aon.at>\n> ---\n> \n> Hi Dscho,\n> \n> Your patch certainly fixes a bug in git-merge-file. It does not fix the \n> bug in git-merge-recursive, however. The test script also fails with \n> your patch.\n\nNow, _that_ is funny.  I tested before sending, and my test suit runs just \nfine.\n\n> On Thu, Mar 13, 2008 at 04:19:35PM +0100, Johannes Schindelin wrote:\n> > On Sat, Mar 08, 2008 at 06:17:26PM +0100, Clemens Buchacher wrote:\n> > > One could argue that it would be better to mark the mismatching \n> > > permissions as a conflict.\n> > \n> > Right you are.  Your whole \"it still is xdl_merge()s fault\" point was \n> > just contradicted by your own analysis.  Calling xdl_merge() when the \n> > sha1 does _not_ differ is _a mistake_.  _That_ is the bug.\n> \n> Alright, fixed in the appended patch.\n\nI have to admit that I wanted you to fix that patch, instead of me, \nbecause you were already researching the issue.\n\n>  merge-recursive.c          |    9 +++++++--\n>  t/t6031-merge-recursive.sh |   23 +++++++++++++++++++++++\n>  2 files changed, 30 insertions(+), 2 deletions(-)\n>  create mode 100755 t/t6031-merge-recursive.sh\n\nLooks much better.\n\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 34e3167..01918a7 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -1028,9 +1028,14 @@ static struct merge_file_info merge_file(struct diff_filespec *o,\n>  \t\tif (!sha_eq(a->sha1, o->sha1) && !sha_eq(b->sha1, o->sha1))\n>  \t\t\tresult.merge = 1;\n>  \n> -\t\tresult.mode = a->mode == o->mode ? b->mode: a->mode;\n> +\t\tif (!o->mode) {\n> +\t\t\tif (a->mode != b->mode)\n> +\t\t\t\tresult.clean = 0;\n> +\t\t\tresult.mode = b->mode;\n> +\t\t} else\n> +\t\t\tresult.mode = a->mode == o->mode ? b->mode: a->mode;\n\nSo you only set clean = 0 if o->mode == 0, i.e. the file did not exist?  \nThat was not what I had in mind.  I would have expected that \"if (a->mode \n!= b->mode)\" to come _before_ the assignment to result.mode, which should \nhave been left alone.\n\nThe rationale would have been this:\n\nIf the modes are different, the merge is not clean.\n\nIf the SHA-1s differ, the merge is not clean, and xld_merge() should be \ncalled.\n\n> -\t\tif (sha_eq(a->sha1, o->sha1))\n> +\t\tif (sha_eq(a->sha1, b->sha1) || sha_eq(a->sha1, o->sha1))\n\nWhy do you still compare to o->sha1?\n\n/me goes looking in the original source, since the issue and the fix does \nnot become apparent from your patch, including the commit message.\n\nOh, okay.\n\nYou are reusing a _different_ case, which just happens to have the same \noutcome.\n\nIn a perfect world, this would have a one-line comment above to explain \nissues.\n\n> diff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh\n> new file mode 100755\n> index 0000000..7ea371e\n> --- /dev/null\n> +++ b/t/t6031-merge-recursive.sh\n> @@ -0,0 +1,23 @@\n> +#!/bin/sh\n> +\n> +test_description='merge-recursive corner cases'\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'merge empty files with different permission flags' '\n\nThe point is not that they are empty.  Maybe you want to fix that message.\n\n> +\t: >dummy &&\n> +\tgit add dummy &&\n> +\tgit commit -m \"initial commit\" &&\n> +\tgit checkout -b a master &&\n> +\t: >a &&\n> +\tgit add a &&\n> +\tgit commit -m \"branch a\" &&\n> +\tgit checkout -b b master &&\n> +\t: >a &&\n> +\tchmod +x a &&\n> +\tgit add a &&\n> +\tgit commit -m \"branch b\" &&\n> +\tgit checkout master &&\n> +\t! (git merge-recursive master -- a b || test $? -ne 1)\n> +'\n> +\n> +test_done\n> -- \n> 1.5.4.4.2.gd2fe\n> \n\nThanks,\nDscho\n"},{"id":"71996","messageId":"alpine.LSU.1.00.0803132227510.4174@racer.site","threadId":"12576","inReplyTo":"7vabl2xxun.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] fix recursive-merge of empty files with different permissions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-03-13T21:28:45Z","receivedAt":"2008-03-13T21:28:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 13 Mar 2008, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > diff --git a/builtin-merge-file.c b/builtin-merge-file.c\n> > index adce6d4..cde4b2c 100644\n> > --- a/builtin-merge-file.c\n> > +++ b/builtin-merge-file.c\n> > @@ -57,7 +57,8 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n> >  \n> >  \t\tif (!f)\n> >  \t\t\tret = error(\"Could not open %s for writing\", filename);\n> > -\t\telse if (fwrite(result.ptr, result.size, 1, f) != 1)\n> > +\t\telse if (result.size &&\n> > +\t\t\t\tfwrite(result.ptr, result.size, 1, f) != 1)\n> >  \t\t\tret = error(\"Could not write to %s\", filename);\n> >  \t\telse if (fclose(f))\n> >  \t\t\tret = error(\"Could not close %s\", filename);\n> \n> Lol.  We are dealing with N-byte quantity so we send one record of length\n> N and make sure we processed one record, and it does not work when N is\n> zero.\n> \n> We could instead send N records of size 1 and make sure we processed N\n> records to lose the conditional instead, but the patch avoids unnecessary\n> call to fread/fwrite so that is good.  Thanks.\n> \n> It felt funny because my current bedtime reading happens to be \"Zero: The\n> Biography of a Dangerous Idea (ISBN 0140296476)\".\n\nHeh.\n\n> > diff --git a/xdiff-interface.c b/xdiff-interface.c\n> > index bba2364..61dc5c5 100644\n> > --- a/xdiff-interface.c\n> > +++ b/xdiff-interface.c\n> > @@ -152,8 +152,8 @@ int read_mmfile(mmfile_t *ptr, const char *filename)\n> >  \tif ((f = fopen(filename, \"rb\")) == NULL)\n> >  \t\treturn error(\"Could not open %s\", filename);\n> >  \tsz = xsize_t(st.st_size);\n> > -\tptr->ptr = xmalloc(sz);\n> > -\tif (fread(ptr->ptr, sz, 1, f) != 1)\n> > +\tptr->ptr = xmalloc(sz ? sz : 1);\n> > +\tif (sz && fread(ptr->ptr, sz, 1, f) != 1)\n> >  \t\treturn error(\"Could not read %s\", filename);\n> >  \tfclose(f);\n> >  \tptr->size = sz;\n> \n> Do you need to actually allocate ptr->ptr when sz is zero, instead of\n> setting it to NULL, like:\n> \n> \tsz = xsize_t(st.st_size);\n> \tptr->size = sz;\n>         if (!sz)\n>         \tptr->ptr = NULL;\n> \telse {\n>         \tptr->ptr = xmalloc(sz);\n> \t\tif (fread(ptr->ptr, 1, sz, f) != sz)\n> \t\t\treturn error(\"Could not read %s\", filename);\n> \t}\n> \tfclose(f);\n\nI was going for the safe option.  In theory, you are right, but I cannot \nreally be sure that e.g. a memcpy() with size 0 will not access the source \npointer at all.\n\nCiao,\nDscho\n"},{"id":"71999","messageId":"20080313224741.GA5000@localhost","threadId":"12576","inReplyTo":"alpine.LSU.1.00.0803132216580.4174@racer.site","subject":"[PATCH] merge-recursive: handle file mode changes","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-03-13T22:47:41Z","receivedAt":"2008-03-13T22:47:41Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Handle file mode changes similarly to changes of content. If the file mode\nchanged in only one branch, keep the changed version. If the file mode\nchanged in both branches, prompt the user to choose one by reporting a\nconflict.\n\nThis also fixes a bug which caused the merge to fail if the merged files were\nempty.\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n\nHi,\n\nOn Thu, Mar 13, 2008 at 10:17:07PM +0100, Johannes Schindelin wrote:\n> If the modes are different, the merge is not clean.\n\nIf the mode has only changed in either the head or the remote tree, I believe\nwe should keep the changed version without conflict - just like we do for\nnon-overlapping changes of content.\n\nIf, however, both files changed in a different way, we report a conflict and\nkeep the remote version by default. I based this decision on the assumption\nthat the user is more likely to have acknowledged the head branch, while he may\nwant to think about whether or not the change in the remote version is ok.\n\nI cleaned up the code a bit and added a comment, so I hope the behavior is\nclearer now.\n\n> The point is not that they are empty.  Maybe you want to fix that message.\n\nIndeed.\n\nI am not exactly sure how I should set the result.merge flag. In this context\nit seems to have the exact opposite meaning of result.clean. Is that correct?\n\nRegards,\nClemens\n\n merge-recursive.c          |   21 ++++++++++++++++-----\n t/t6031-merge-recursive.sh |   39 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 55 insertions(+), 5 deletions(-)\n create mode 100755 t/t6031-merge-recursive.sh\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 34e3167..d8938cc 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1025,12 +1025,21 @@ static struct merge_file_info merge_file(struct diff_filespec *o,\n \t\t\thashcpy(result.sha, b->sha1);\n \t\t}\n \t} else {\n-\t\tif (!sha_eq(a->sha1, o->sha1) && !sha_eq(b->sha1, o->sha1))\n-\t\t\tresult.merge = 1;\n-\n-\t\tresult.mode = a->mode == o->mode ? b->mode: a->mode;\n+\t\t/*\n+\t\t * If mode changed in only one branch, keep the changed\n+\t\t * version. Otherwise, keep remote version and create a\n+\t\t * conflict.\n+\t\t */\n+\t\tif (a->mode != o->mode && b->mode != o->mode &&\n+\t\t\t\ta->mode != b->mode) {\n+\t\t\tresult.clean = 0;\n+\t\t\tresult.mode = b->mode;\n+\t\t} else\n+\t\t\tresult.mode = o->mode == a->mode ? b->mode : a->mode;\n \n-\t\tif (sha_eq(a->sha1, o->sha1))\n+\t\tif (sha_eq(a->sha1, b->sha1))\n+\t\t\thashcpy(result.sha, b->sha1);\n+\t\telse if (sha_eq(a->sha1, o->sha1))\n \t\t\thashcpy(result.sha, b->sha1);\n \t\telse if (sha_eq(b->sha1, o->sha1))\n \t\t\thashcpy(result.sha, a->sha1);\n@@ -1062,6 +1071,8 @@ static struct merge_file_info merge_file(struct diff_filespec *o,\n \t\t} else {\n \t\t\tdie(\"unsupported object type in the tree\");\n \t\t}\n+\n+\t\tresult.merge = !result.clean;\n \t}\n \n \treturn result;\ndiff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh\nnew file mode 100755\nindex 0000000..36cd664\n--- /dev/null\n+++ b/t/t6031-merge-recursive.sh\n@@ -0,0 +1,39 @@\n+#!/bin/sh\n+\n+test_description='merge-recursive: handle file mode'\n+. ./test-lib.sh\n+\n+test_expect_success 'mode change in one branch: keep changed version' '\n+\t: >file1 &&\n+\tgit add file1 &&\n+\tgit commit -m initial &&\n+\tgit checkout -b a1 master &&\n+\t: >dummy &&\n+\tgit add dummy &&\n+\tgit commit -m a &&\n+\tgit checkout -b b1 master &&\n+\tchmod +x file1 &&\n+\tgit add file1 &&\n+\tgit commit -m b1 &&\n+\tgit checkout a1 &&\n+\tgit merge-recursive master -- a1 b1 &&\n+\ttest -x file1\n+'\n+\n+test_expect_success 'mode change in both branches: expect conflict' '\n+\tgit reset --hard HEAD &&\n+\tgit checkout -b a2 master &&\n+\t: >file2 &&\n+\tchmod +x file2 &&\n+\tgit add file2 &&\n+\tgit commit -m a2 &&\n+\tgit checkout -b b2 master &&\n+\t: >file2 &&\n+\tgit add file2 &&\n+\tgit commit -m b2 &&\n+\tgit checkout a2 &&\n+\t! (git merge-recursive master -- a2 b2 || test $? -ne 1) &&\n+\t! test -x file2\n+'\n+\n+test_done\n-- \n1.5.4.4\n"},{"id":"72006","messageId":"alpine.LSU.1.00.0803140039250.4174@racer.site","threadId":"12576","inReplyTo":"20080313224741.GA5000@localhost","subject":"Re: [PATCH] merge-recursive: handle file mode changes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-03-13T23:40:05Z","receivedAt":"2008-03-13T23:40:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 13 Mar 2008, Clemens Buchacher wrote:\n\n> @@ -1062,6 +1071,8 @@ static struct merge_file_info merge_file(struct diff_filespec *o,\n>  \t\t} else {\n>  \t\t\tdie(\"unsupported object type in the tree\");\n>  \t\t}\n> +\n> +\t\tresult.merge = !result.clean;\n\nThat is new.  Doesn't this overwrite what has been set in\n\n        } else {\n                if (!sha_eq(a->sha1, o->sha1) && !sha_eq(b->sha1, o->sha1))\n                        result.merge = 1;\n\n?\n\nCiao,\nDscho\n"},{"id":"72009","messageId":"7vejaew4jp.fsf@gitster.siamese.dyndns.org","threadId":"12576","inReplyTo":"20080313224741.GA5000@localhost","subject":"Re: [PATCH] merge-recursive: handle file mode changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-14T00:08:26Z","receivedAt":"2008-03-14T00:08:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher <drizzd@aon.at> writes:\n\n> I am not exactly sure how I should set the result.merge flag. In this context\n> it seems to have the exact opposite meaning of result.clean. Is that correct?\n\nMy reading of the code is that result.merge is \"if a content level merge\nhas happened\", and result.clean is \"given that a content level merge has\nbeen attempted, was it done cleanly, or are there conflicts for the user\nto fix in the result\".  If result.clean is not true, we obviously cannot\nstore the result in stage #0.\n\nThe result.merge flag is used only for reporting purposes; I am not sure\nwhy the non-rename codepath does not pay attention to the result.merge,\nthough.\n"},{"id":"72010","messageId":"7vabl2w4co.fsf@gitster.siamese.dyndns.org","threadId":"12576","inReplyTo":"20080313224741.GA5000@localhost","subject":"Re: [PATCH] merge-recursive: handle file mode changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-14T00:12:39Z","receivedAt":"2008-03-14T00:12:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher <drizzd@aon.at> writes:\n\n> I am not exactly sure how I should set the result.merge flag. In this context\n> it seems to have the exact opposite meaning of result.clean. Is that correct?\n\nThis is from the e-mail header of your message:\n\n  Mail-Followup-To: Johannes Schindelin <Johannes.Schindelin@gmx.de>,\n   git@vger.kernel.org, gitster@pobox.com\n\nIf you are asking people question, do not do this.  You want answers\naddressed to you.  Johannes and I may not be interested in learning what\nmay want to find out, and redirecting answers for you to us is simply\nrude.\n"},{"id":"72011","messageId":"frcg66$2nb$1@ger.gmane.org","threadId":"12576","inReplyTo":"20080313224741.GA5000@localhost","subject":"Re: [PATCH] merge-recursive: handle file mode changes","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-03-14T00:17:10Z","receivedAt":"2008-03-14T00:17:10Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Clemens Buchacher wrote:\n\n> Handle file mode changes similarly to changes of content. If the file mode\n> changed in only one branch, keep the changed version. If the file mode\n> changed in both branches, prompt the user to choose one by reporting a\n> conflict.\n\nShouldn't it print \"CONFLICT(mode/mode)\" then?\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"72051","messageId":"20080314092105.GA18683@localhost","threadId":"12576","inReplyTo":"alpine.LSU.1.00.0803140039250.4174@racer.site","subject":"[PATCH] merge-recursive: handle file mode and links similarly to file content","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-03-14T09:21:05Z","receivedAt":"2008-03-14T09:21:05Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"If the file mode or link changed in only one branch, keep the changed\nversion. If the file mode or link changed differently in both branches,\nreport a conflict. If this happens, the user is more likely to be aware of\nthe change in the head branch. Choose the remote version by default, in\norder to make the user think about the change.\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n\nOn Fri, Mar 14, 2008 at 12:40:05AM +0100, Johannes Schindelin wrote:\n> On Thu, 13 Mar 2008, Clemens Buchacher wrote:\n> > +\t\tresult.merge = !result.clean;\n> \n> That is new.  Doesn't this overwrite what has been set in\n> \n>         } else {\n>                 if (!sha_eq(a->sha1, o->sha1) && !sha_eq(b->sha1, o->sha1))\n>                         result.merge = 1;\n\nYeah, that's no good. I think I understand the meaning of result.merge and\nresult.clean now.\n\nresult.merge indicates that there are changes in both branches, whereas\nresult.clean indicates that the merge was trivial.\n\nI amended the patch to reflect this.\n\nI also noticed that in case of LINKs or GITLINKs which changed in both\nbranches, the head version is kept by default. By the same rationale I gave for\nthe file modes, I think the remote version should be kept instead, in order to\nmake the user aware of this change.\n\nRegards,\nClemens\n\n merge-recursive.c          |   34 +++++++++++++++++++++++-----------\n t/t6031-merge-recursive.sh |   39 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 62 insertions(+), 11 deletions(-)\n create mode 100755 t/t6031-merge-recursive.sh\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 34e3167..36f78a2 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1025,16 +1025,31 @@ static struct merge_file_info merge_file(struct diff_filespec *o,\n \t\t\thashcpy(result.sha, b->sha1);\n \t\t}\n \t} else {\n-\t\tif (!sha_eq(a->sha1, o->sha1) && !sha_eq(b->sha1, o->sha1))\n-\t\t\tresult.merge = 1;\n+\t\t/*\n+\t\t * If the file changed in only one branch, keep the changed\n+\t\t * version. If the file changed in both, try to merge\n+\t\t * automatically. If the merge is not trivial, report a\n+\t\t * conflict. In case of conflicting file modes or links, choose\n+\t\t * remote version by default. They can only be merged trivially\n+\t\t * if they are equal.\n+\t\t */\n \n-\t\tresult.mode = a->mode == o->mode ? b->mode: a->mode;\n+\t\tif (a->mode != o->mode && b->mode != o->mode) {\n+\t\t\tresult.mode = b->mode;\n+\t\t\tif (a->mode != b->mode)\n+\t\t\t\tresult.clean = 0;\n+\t\t\tresult.merge = 1;\n+\t\t} else\n+\t\t\tresult.mode = o->mode == a->mode ? b->mode : a->mode;\n \n \t\tif (sha_eq(a->sha1, o->sha1))\n \t\t\thashcpy(result.sha, b->sha1);\n \t\telse if (sha_eq(b->sha1, o->sha1))\n \t\t\thashcpy(result.sha, a->sha1);\n-\t\telse if (S_ISREG(a->mode)) {\n+\t\telse if (sha_eq(a->sha1, b->sha1)) {\n+\t\t\thashcpy(result.sha, a->sha1);\n+\t\t\tresult.merge = 1;\n+\t\t} else if (S_ISREG(a->mode)) {\n \t\t\tmmbuffer_t result_buf;\n \t\t\tint merge_status;\n \n@@ -1051,14 +1066,11 @@ static struct merge_file_info merge_file(struct diff_filespec *o,\n \n \t\t\tfree(result_buf.ptr);\n \t\t\tresult.clean = (merge_status == 0);\n-\t\t} else if (S_ISGITLINK(a->mode)) {\n+\t\t\tresult.merge = 1;\n+\t\t} else if (S_ISGITLINK(a->mode) || S_ISLNK(a->mode)) {\n+\t\t\thashcpy(result.sha, b->sha1);\n \t\t\tresult.clean = 0;\n-\t\t\thashcpy(result.sha, a->sha1);\n-\t\t} else if (S_ISLNK(a->mode)) {\n-\t\t\thashcpy(result.sha, a->sha1);\n-\n-\t\t\tif (!sha_eq(a->sha1, b->sha1))\n-\t\t\t\tresult.clean = 0;\n+\t\t\tresult.merge = 1;\n \t\t} else {\n \t\t\tdie(\"unsupported object type in the tree\");\n \t\t}\ndiff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh\nnew file mode 100755\nindex 0000000..36cd664\n--- /dev/null\n+++ b/t/t6031-merge-recursive.sh\n@@ -0,0 +1,39 @@\n+#!/bin/sh\n+\n+test_description='merge-recursive: handle file mode'\n+. ./test-lib.sh\n+\n+test_expect_success 'mode change in one branch: keep changed version' '\n+\t: >file1 &&\n+\tgit add file1 &&\n+\tgit commit -m initial &&\n+\tgit checkout -b a1 master &&\n+\t: >dummy &&\n+\tgit add dummy &&\n+\tgit commit -m a &&\n+\tgit checkout -b b1 master &&\n+\tchmod +x file1 &&\n+\tgit add file1 &&\n+\tgit commit -m b1 &&\n+\tgit checkout a1 &&\n+\tgit merge-recursive master -- a1 b1 &&\n+\ttest -x file1\n+'\n+\n+test_expect_success 'mode change in both branches: expect conflict' '\n+\tgit reset --hard HEAD &&\n+\tgit checkout -b a2 master &&\n+\t: >file2 &&\n+\tchmod +x file2 &&\n+\tgit add file2 &&\n+\tgit commit -m a2 &&\n+\tgit checkout -b b2 master &&\n+\t: >file2 &&\n+\tgit add file2 &&\n+\tgit commit -m b2 &&\n+\tgit checkout a2 &&\n+\t! (git merge-recursive master -- a2 b2 || test $? -ne 1) &&\n+\t! test -x file2\n+'\n+\n+test_done\n-- \n1.5.4.4\n"},{"id":"72054","messageId":"20080314100747.GA23145@localhost","threadId":"12576","inReplyTo":"alpine.LSU.1.00.0803132216580.4174@racer.site","subject":"Re: [PATCH] merge-recursive: cause a conflict if file mode does not match","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-03-14T10:07:47Z","receivedAt":"2008-03-14T10:07:47Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Hi,\n\nOn Thu, Mar 13, 2008 at 10:17:07PM +0100, Johannes Schindelin wrote:\n> > Your patch certainly fixes a bug in git-merge-file. It does not fix the \n> > bug in git-merge-recursive, however. The test script also fails with \n> > your patch.\n> \n> Now, _that_ is funny.  I tested before sending, and my test suit runs just \n> fine.\n\nI didn't mean the existing test suite, which is fine with your bugfix, of\ncourse. I was talking about the test which I submitted along with my bugfix.\n\nRegards,\nClemens\n"},{"id":"72055","messageId":"20080314101349.GB23145@localhost","threadId":"12576","inReplyTo":"20080314092105.GA18683@localhost","subject":"Re: [PATCH] merge-recursive: handle file mode and links similarly to file content","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-03-14T10:13:49Z","receivedAt":"2008-03-14T10:13:49Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Fri, Mar 14, 2008 at 10:21:05AM +0100, Clemens Buchacher wrote:\n> @@ -1051,14 +1066,11 @@ static struct merge_file_info merge_file(struct diff_filespec *o,\n>  \n>  \t\t\tfree(result_buf.ptr);\n>  \t\t\tresult.clean = (merge_status == 0);\n> -\t\t} else if (S_ISGITLINK(a->mode)) {\n> +\t\t\tresult.merge = 1;\n> +\t\t} else if (S_ISGITLINK(a->mode) || S_ISLNK(a->mode)) {\n> +\t\t\thashcpy(result.sha, b->sha1);\n>  \t\t\tresult.clean = 0;\n> -\t\t\thashcpy(result.sha, a->sha1);\n> -\t\t} else if (S_ISLNK(a->mode)) {\n> -\t\t\thashcpy(result.sha, a->sha1);\n> -\n> -\t\t\tif (!sha_eq(a->sha1, b->sha1))\n> -\t\t\t\tresult.clean = 0;\n> +\t\t\tresult.merge = 1;\n\nTo clarify: The above two cases don't need to be handled separately any\nmore, because equal sha1s are already handled above.\n\nClemens\n"},{"id":"72057","messageId":"7vhcf9r4qp.fsf@gitster.siamese.dyndns.org","threadId":"12576","inReplyTo":"20080313224741.GA5000@localhost","subject":"Re: [PATCH] merge-recursive: handle file mode changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-14T10:15:26Z","receivedAt":"2008-03-14T10:15:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher <drizzd@aon.at> writes:\n\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 34e3167..d8938cc 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -1025,12 +1025,21 @@ static struct merge_file_info merge_file(struct diff_filespec *o,\n>  \t\t\thashcpy(result.sha, b->sha1);\n>  \t\t}\n>  \t} else {\n> -\t\tif (!sha_eq(a->sha1, o->sha1) && !sha_eq(b->sha1, o->sha1))\n> -\t\t\tresult.merge = 1;\n> -\n> -\t\tresult.mode = a->mode == o->mode ? b->mode: a->mode;\n> +\t\t/*\n> +\t\t * If mode changed in only one branch, keep the changed\n> +\t\t * version. Otherwise, keep remote version and create a\n> +\t\t * conflict.\n> +\t\t */\n\nReading the rest of the function, I notice that it consistently favor \"a\"\nover \"b\", when a conflict cannot be reconciled.\n\n> +\t\tif (a->mode != o->mode && b->mode != o->mode &&\n> +\t\t\t\ta->mode != b->mode) {\n> +\t\t\tresult.clean = 0;\n> +\t\t\tresult.mode = b->mode;\n> +\t\t} else\n> +\t\t\tresult.mode = o->mode == a->mode ? b->mode : a->mode;\n\nI think this is much easier to read:\n\n\t\tif (a->mode == b->mode || a->mode == o->mode)\n\t\t\tresult.mode = b->mode;\n\t\telse {\n\t\t\tresult.mode = a->mode;\n\t\t\tif (b->mode != o->mode) {\n\t\t\t\tresult.clean = 0;\n\t\t\t\tresult.merge = 1;\n\t\t\t}\n\t\t}\n\nWe keep \"b\" only if \"a\" hasn't touched the mode, or it happens to be the\nsame as \"a\".  Otherwise we take \"a\" anyway, but taking \"a\" when \"b\" also\ntouched means we detected an unreconcilable conflict.\n\n> @@ -1062,6 +1071,8 @@ static struct merge_file_info merge_file(struct diff_filespec *o,\n>  \t\t} else {\n>  \t\t\tdie(\"unsupported object type in the tree\");\n>  \t\t}\n> +\n> +\t\tresult.merge = !result.clean;\n\nAs pointed out by Dscho, this is too much.  We could mimick the one that\nis done for the contents, which is why I have \"result.merge = 1\" where it\nis in the above.\n\n> diff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh\n> new file mode 100755\n> index 0000000..36cd664\n> --- /dev/null\n> +++ b/t/t6031-merge-recursive.sh\n> @@ -0,0 +1,39 @@\n> +#!/bin/sh\n> ...\n> +\t! (git merge-recursive master -- a2 b2 || test $? -ne 1) &&\n> +\t! test -x file2\n> +'\n\nAs we would favor our side in unreconsilable conflicts, the last test\nneeds to become \"test -x file2\".\n\nAlso we should test ls-files -u output to make sure we have correct stages\nin the index.\n\nI've done minor fixups and committed the result.\n"},{"id":"72070","messageId":"20080314121752.GB3315@localhost","threadId":"12576","inReplyTo":"7vhcf9r4qp.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] merge-recursive: handle file mode changes","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-03-14T12:17:52Z","receivedAt":"2008-03-14T12:17:52Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Fri, Mar 14, 2008 at 03:15:26AM -0700, Junio C Hamano wrote:\n> > diff --git a/merge-recursive.c b/merge-recursive.c\n> > index 34e3167..d8938cc 100644\n> > --- a/merge-recursive.c\n> > +++ b/merge-recursive.c\n> > @@ -1025,12 +1025,21 @@ static struct merge_file_info merge_file(struct diff_filespec *o,\n> >  \t\t\thashcpy(result.sha, b->sha1);\n> >  \t\t}\n> >  \t} else {\n> > -\t\tif (!sha_eq(a->sha1, o->sha1) && !sha_eq(b->sha1, o->sha1))\n> > -\t\t\tresult.merge = 1;\n> > -\n> > -\t\tresult.mode = a->mode == o->mode ? b->mode: a->mode;\n> > +\t\t/*\n> > +\t\t * If mode changed in only one branch, keep the changed\n> > +\t\t * version. Otherwise, keep remote version and create a\n> > +\t\t * conflict.\n> > +\t\t */\n> \n> Reading the rest of the function, I notice that it consistently favor \"a\"\n> over \"b\", when a conflict cannot be reconciled.\n\nIndeed. I think \"b\" should be favored over \"a\", however.\n\n> > +\t\tif (a->mode != o->mode && b->mode != o->mode &&\n> > +\t\t\t\ta->mode != b->mode) {\n> > +\t\t\tresult.clean = 0;\n> > +\t\t\tresult.mode = b->mode;\n> > +\t\t} else\n> > +\t\t\tresult.mode = o->mode == a->mode ? b->mode : a->mode;\n> \n> I think this is much easier to read:\n> \n> \t\tif (a->mode == b->mode || a->mode == o->mode)\n> \t\t\tresult.mode = b->mode;\n> \t\telse {\n> \t\t\tresult.mode = a->mode;\n> \t\t\tif (b->mode != o->mode) {\n> \t\t\t\tresult.clean = 0;\n> \t\t\t\tresult.merge = 1;\n> \t\t\t}\n> \t\t}\n> \n> We keep \"b\" only if \"a\" hasn't touched the mode, or it happens to be the\n> same as \"a\".  Otherwise we take \"a\" anyway, but taking \"a\" when \"b\" also\n> touched means we detected an unreconcilable conflict.\n\nYes, but if \"b\" and \"a\" both changed in the same way, we should still set\nresult.merge = 1, because that's more akin to an automatic merge than a\nfast-forward merge, IMO.\n\nClemens\n"},{"id":"72072","messageId":"20080314125612.GA6157@localhost","threadId":"12576","inReplyTo":"frcg66$2nb$1@ger.gmane.org","subject":"Re: [PATCH] merge-recursive: handle file mode changes","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-03-14T12:56:12Z","receivedAt":"2008-03-14T12:56:12Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Fri, Mar 14, 2008 at 01:17:10AM +0100, Jakub Narebski wrote:\n> > Handle file mode changes similarly to changes of content. If the file mode\n> > changed in only one branch, keep the changed version. If the file mode\n> > changed in both branches, prompt the user to choose one by reporting a\n> > conflict.\n> \n> Shouldn't it print \"CONFLICT(mode/mode)\" then?\n\nI think currently the only way to get a mode conflict is when a file is added\nin both branches with different permissions, since we only track the executable\nbit.\n\nClemens\n"},{"id":"72073","messageId":"20080314130206.GB6157@localhost","threadId":"12576","inReplyTo":"7vabl2w4co.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] merge-recursive: handle file mode changes","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-03-14T13:02:06Z","receivedAt":"2008-03-14T13:02:06Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Thu, Mar 13, 2008 at 05:12:39PM -0700, Junio C Hamano wrote:\n> This is from the e-mail header of your message:\n> \n>   Mail-Followup-To: Johannes Schindelin <Johannes.Schindelin@gmx.de>,\n>    git@vger.kernel.org, gitster@pobox.com\n> \n> If you are asking people question, do not do this.  You want answers\n> addressed to you.  Johannes and I may not be interested in learning what\n> may want to find out, and redirecting answers for you to us is simply\n> rude.\n\nMy apologies. I wasn't aware that was doing that.\n\nClemens\n"},{"id":"72105","messageId":"7vd4pxqoql.fsf@gitster.siamese.dyndns.org","threadId":"12576","inReplyTo":"20080314121752.GB3315@localhost","subject":"Re: [PATCH] merge-recursive: handle file mode changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-14T16:01:06Z","receivedAt":"2008-03-14T16:01:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher <drizzd@aon.at> writes:\n\n>> Reading the rest of the function, I notice that it consistently favor \"a\"\n>> over \"b\", when a conflict cannot be reconciled.\n>\n> Indeed. I think \"b\" should be favored over \"a\", however.\n\nWhy?\n"},{"id":"72112","messageId":"20080314172848.GA5080@localhost","threadId":"12576","inReplyTo":"7vd4pxqoql.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] merge-recursive: handle file mode changes","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-03-14T17:28:48Z","receivedAt":"2008-03-14T17:28:48Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Fri, Mar 14, 2008 at 09:01:06AM -0700, Junio C Hamano wrote:\n> Clemens Buchacher <drizzd@aon.at> writes:\n> >> Reading the rest of the function, I notice that it consistently favor \"a\"\n> >> over \"b\", when a conflict cannot be reconciled.\n> >\n> > Indeed. I think \"b\" should be favored over \"a\", however.\n> \n> Why?\n\n>From the commit message to the latest version of my patch:\nhttp://marc.info/?l=git&m=120548648727308&w=2\n\nOn Fri, Mar 14, 2008 at 10:21:05AM +0100, Clemens Buchacher wrote:\n> If the file mode or link changed in only one branch, keep the changed\n> version. If the file mode or link changed differently in both branches,\n> report a conflict. If this happens, the user is more likely to be aware of\n> the change in the head branch. Choose the remote version by default, in\n> order to make the user think about the change.\n\nIn principle, both decisions are equally right or wrong. However, suggesting\nthe remote version (i.e., \"b\") by default gives more incentive to think about\nit because the file now changed with respect to the head version (i.e., \"a\"),\nwhich the user started out with.\n\nClemens\n"},{"id":"72122","messageId":"7vlk4lp553.fsf@gitster.siamese.dyndns.org","threadId":"12576","inReplyTo":"20080314172848.GA5080@localhost","subject":"Re: [PATCH] merge-recursive: handle file mode changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-14T17:49:44Z","receivedAt":"2008-03-14T17:49:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher <drizzd@aon.at> writes:\n\n> On Fri, Mar 14, 2008 at 10:21:05AM +0100, Clemens Buchacher wrote:\n>> If the file mode or link changed in only one branch, keep the changed\n>> version. If the file mode or link changed differently in both branches,\n>> report a conflict. If this happens, the user is more likely to be aware of\n>> the change in the head branch. Choose the remote version by default, in\n>> order to make the user think about the change.\n>\n> In principle, both decisions are equally right or wrong. However, suggesting\n> the remote version (i.e., \"b\") by default gives more incentive to think about\n> it because the file now changed with respect to the head version (i.e., \"a\"),\n> which the user started out with.\n\nThis matters only when the conflict is irreconcilable in the work tree,\nand as long as we get the most important part right, i.e. marking the path\nto be conflicting, that is good enough to make sure that the user does not\ncommit without thinking.\n\nBesides the change breaks the traditional behaviour of leaving the \"a\"\nside in the work tree.  As you already know, what you have done is as\nequally valid as the other side did.  Be assertive and more confident in\nwhat you have done ;-).\n"}]}