{"thread":{"id":"19110","subject":"different git-merge behavior with regard to submodules in 1.6.2.4 vs. 1.6.2.1","startedAt":"2009-04-28T17:36:59Z","lastAt":"2009-04-30T06:07:31Z","messageCount":15,"participants":["Tim Olsen","Junio C Hamano","Finn Arne Gangstad","Clemens Buchacher","Alex Riesen","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"112545","messageId":"gt7err$3m4$1@ger.gmane.org","threadId":"19110","inReplyTo":null,"subject":"different git-merge behavior with regard to submodules in 1.6.2.4 vs. 1.6.2.1","fromName":"Tim Olsen","fromEmail":"tim@brooklynpenguin.com","sentAt":"2009-04-28T17:36:59Z","receivedAt":"2009-04-28T17:36:59Z","isPatch":false,"sender":{"key":"tim@brooklynpenguin.com","avatar":"https://gravatar.com/avatar/a468fd97b6433c4d22252ca75017d30a55046f5715e67b83e62b00a0833deb22?d=mp&s=160"},"body":"Hello,\n\nI noticed some weirdness lately when trying to merge branches in a\nrepository containing submodules. I get the following behavior with git\n1.6.2.4:\n\n$ git merge origin/deployed\nfatal: cannot read object 83055ffdddde60d41d9811aae77e78be50b329f8\n'rubydav': It is a submodule!\n\nNothing in my history suggests that rubydav was at one point not a\nsubmodule.\n\nI looked at the 1.6.2.4 release notes and noticed the following:\n\n * \"git-merge-recursive\" was broken when a submodule entry was involved\nin a criss-cross merge situation.\n\nSo then I downgraded to the last debian package of git which is 1.6.2.1.\n Now I get a result which is more approachable:\n\n$ git merge origin/deployed\nAuto-merging rubydav\nCONFLICT (submodule): Merge conflict in rubydav - needs\n167a344227c4745031d50a210869e6fb59a5ac03\nAuto-merging server\nCONFLICT (submodule): Merge conflict in server - needs\n82a74ae791c8563ca65f29187d2fe5ebfbc167ea\nAutomatic merge failed; fix conflicts and then commit the result.\n\nBoth merges are from freshly checked out clones.\n\nIs this a bug in 1.6.2.4? Please let me know what other information I\ncan provide to help debug the problem.\n\nThanks,\nTim\n"},{"id":"112547","messageId":"7v4ow8my1u.fsf@gitster.siamese.dyndns.org","threadId":"19110","inReplyTo":"gt7err$3m4$1@ger.gmane.org","subject":"Re: different git-merge behavior with regard to submodules in 1.6.2.4 vs. 1.6.2.1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-28T18:29:49Z","receivedAt":"2009-04-28T18:29:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Olsen writes:\n\n> $ git merge origin/deployed\n> fatal: cannot read object 83055ffdddde60d41d9811aae77e78be50b329f8\n> 'rubydav': It is a submodule!\n>\n> Nothing in my history suggests that rubydav was at one point not a\n> submodule.\n>\n> I looked at the 1.6.2.4 release notes and noticed the following:\n>\n> * \"git-merge-recursive\" was broken when a submodule entry was involved\n> in a criss-cross merge situation.\n>\n> So then I downgraded to the last debian package of git which is 1.6.2.1.\n> Now I get a result which is more approachable:\n>\n> $ git merge origin/deployed\n> Auto-merging rubydav\n> CONFLICT (submodule): Merge conflict in rubydav - needs\n> 167a344227c4745031d50a210869e6fb59a5ac03\n> Auto-merging server\n> CONFLICT (submodule): Merge conflict in server - needs\n> 82a74ae791c8563ca65f29187d2fe5ebfbc167ea\n> Automatic merge failed; fix conflicts and then commit the result.\n>\n> Both merges are from freshly checked out clones.\n>\n> Is this a bug in 1.6.2.4? Please let me know what other information I\n> can provide to help debug the problem.\n\nThanks for a report. I think the following commits are involved.\n\n 39d8e27 simplify output of conflicting merge\n 0eb6574 update cache for conflicting submodule entries\n f37ae35 add tests for merging with submodules\n\nClemens, these seem to be yours. Thoughts?\n"},{"id":"112565","messageId":"20090428211257.GA31191@pvv.org","threadId":"19110","inReplyTo":"7v4ow8my1u.fsf@gitster.siamese.dyndns.org","subject":"Re: different git-merge behavior with regard to submodules in 1.6.2.4 vs. 1.6.2.1","fromName":"Finn Arne Gangstad","fromEmail":"finnag@pvv.org","sentAt":"2009-04-28T21:12:57Z","receivedAt":"2009-04-28T21:12:57Z","isPatch":false,"sender":{"key":"finnag@pvv.org","avatar":"https://gravatar.com/avatar/b421ddd58c3f0f93aa473e17b98bb8d53c221fef741746bc8cb59fae4ec6d95e?d=mp&s=160"},"body":"On Tue, Apr 28, 2009 at 11:29:49AM -0700, Junio C Hamano wrote:\n> Tim Olsen writes:\n> \n> > $ git merge origin/deployed\n> > fatal: cannot read object 83055ffdddde60d41d9811aae77e78be50b329f8\n> > 'rubydav': It is a submodule!\n> >\n> > Nothing in my history suggests that rubydav was at one point not a\n> > submodule.\n[...]\n> > So then I downgraded to the last debian package of git which is 1.6.2.1.\n> > Now I get a result which is more approachable:\n> >\n> > $ git merge origin/deployed\n> > Auto-merging rubydav\n> > CONFLICT (submodule): Merge conflict in rubydav - needs\n> > 167a344227c4745031d50a210869e6fb59a5ac03\n> > Auto-merging server\n> > CONFLICT (submodule): Merge conflict in server - needs\n> > 82a74ae791c8563ca65f29187d2fe5ebfbc167ea\n> > Automatic merge failed; fix conflicts and then commit the result.\n> >\n> > Both merges are from freshly checked out clones.\n> >\n> > Is this a bug in 1.6.2.4? Please let me know what other information I\n> > can provide to help debug the problem.\n> \n> Thanks for a report. I think the following commits are involved.\n> \n> 39d8e27 simplify output of conflicting merge\n> 0eb6574 update cache for conflicting submodule entries\n> f37ae35 add tests for merging with submodules\n> \n> Clemens, these seem to be yours. Thoughts?\n\nThe current error message is not an improvement I think, it should say\nthat merge does not support merging submodules, not complain about\nbeing unable to read some object because it is a submodule.\n\nI added the \"CONFLICT (submodule) Merge conflict .. needs \"\nmessages when I tried to work with submodules a while (1-2 years?)\nago. The intention was that you could enter the submodule(s), write\n\"git merge \", and resolve the conflict that way.\n\ngit is unfortunately not capable of merging submodules at all, so I\nadded these error messages to give me a hint about what I needed to do\nin conflicting submodules to get something useful. I have used git a\nlot more now, so maybe it is time to pick this up again and implement\nproper recursive sub-module merging.\n\n- Finn Arne\n"},{"id":"112614","messageId":"20090429084209.GA24064@localhost","threadId":"19110","inReplyTo":"20090428211257.GA31191@pvv.org","subject":"Re: different git-merge behavior with regard to submodules in 1.6.2.4 vs. 1.6.2.1","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-04-29T08:42:09Z","receivedAt":"2009-04-29T08:42:09Z","isPatch":false,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"From: Clemens Buchacher \nDate: Tue, 28 Apr 2009 21:16:02 +0200\nSubject: [PATCH] fix recursive merge with submodule add/add conflict\n\nIn case of a submodule we should not attempt to update the working\ncopy, but we do have to update the index.\n\nSigned-off-by: Clemens Buchacher \n---\n\nOn Tue, Apr 28, 2009 at 11:12:57PM +0200, Finn Arne Gangstad wrote:\n> On Tue, Apr 28, 2009 at 11:29:49AM -0700, Junio C Hamano wrote:\n> > Tim Olsen writes:\n> > \n> > > $ git merge origin/deployed\n> > > fatal: cannot read object 83055ffdddde60d41d9811aae77e78be50b329f8\n> > > 'rubydav': It is a submodule!\n\n> The current error message is not an improvement I think, it should say\n> that merge does not support merging submodules, not complain about\n> being unable to read some object because it is a submodule.\n\nThe fatal error is indeed caused by 0eb6574 (update cache for conflicting\nsubmodule entries). The problem is also documented by t7405. The test\nexposes a problem even previous to this commit, wherein \"git diff\" aborts\nwith\n\n\tfatal: read error 'sub'\n\nwhich is why I thought this was broken anyways. Only I see now that I have\nmade things worse.\n\nThe appended patch should fix both problems for now. I'm still not satisfied\nwith the result, because the conflicting submodule path is not marked as\nunmerged. I modified t7405 to reflect this failure. I think we should be\nable to handle this in the same way we handle symlinks.\n\n> I added the \"CONFLICT (submodule) Merge conflict .. needs \"\n> messages when I tried to work with submodules a while (1-2 years?)\n> ago. The intention was that you could enter the submodule(s), write\n> \"git merge \", and resolve the conflict that way.\n\nI think you should get that information from git diff instead.\n\n> git is unfortunately not capable of merging submodules at all, so I\n> added these error messages to give me a hint about what I needed to do\n> in conflicting submodules to get something useful. I have used git a\n> lot more now, so maybe it is time to pick this up again and implement\n> proper recursive sub-module merging.\n\nAre you sure it's always the right thing to merge conflicting submodule\nversions? The user could easily commit two versions, which you would never\nwant merge -- due to changed history, for example. On the other hand, if a\nfast-forward merge is possible I suppose this could be considered a\nnon-conflicting change.\n\nClemens\n---\n merge-recursive.c | 11 ++++++-----\n t/t7405-submodule-merge.sh | 3 ++-\n 2 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex d6f0582..b14b3fe 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1116,13 +1116,14 @@ static int process_entry(struct merge_options *o,\n \t\t\t\t o->branch1, o->branch2);\n \n \t\tclean_merge = mfi.clean;\n-\t\tif (!mfi.clean) {\n-\t\t\tif (S_ISGITLINK(mfi.mode))\n-\t\t\t\treason = \"submodule\";\n+\t\tif (S_ISGITLINK(mfi.mode)) {\n+\t\t\treason = \"submodule\";\n+\t\t\tadd_cacheinfo(mfi.mode, mfi.sha, path, 0, 0, ADD_CACHE_OK_TO_ADD);\n+\t\t} else\n+\t\t\tupdate_file(o, mfi.clean, mfi.sha, mfi.mode, path);\n+\t\tif (!mfi.clean)\n \t\t\toutput(o, 1, \"CONFLICT (%s): Merge conflict in %s\",\n \t\t\t\t\treason, path);\n-\t\t}\n-\t\tupdate_file(o, mfi.clean, mfi.sha, mfi.mode, path);\n \t} else if (!o_sha && !a_sha && !b_sha) {\n \t\t/*\n \t\t * this entry was deleted altogether. a_mode == 0 means\ndiff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh\nindex aa6c44c..b881370 100755\n--- a/t/t7405-submodule-merge.sh\n+++ b/t/t7405-submodule-merge.sh\n@@ -59,7 +59,8 @@ test_expect_failure 'merging with modify/modify conflict' '\n \tgit checkout -b test1 a &&\n \ttest_must_fail git merge b &&\n \ttest -f .git/MERGE_MSG &&\n-\tgit diff\n+\tgit diff &&\n+\ttest -n \"`git ls-files -u`\"\n \n '\n \n-- \n1.6.3.rc2\n"},{"id":"112622","messageId":"20090429121506.GA1266@pvv.org","threadId":"19110","inReplyTo":"20090429084209.GA24064@localhost","subject":"Re: different git-merge behavior with regard to submodules in 1.6.2.4 vs. 1.6.2.1","fromName":"Finn Arne Gangstad","fromEmail":"finnag@pvv.org","sentAt":"2009-04-29T12:15:06Z","receivedAt":"2009-04-29T12:15:06Z","isPatch":false,"sender":{"key":"finnag@pvv.org","avatar":"https://gravatar.com/avatar/b421ddd58c3f0f93aa473e17b98bb8d53c221fef741746bc8cb59fae4ec6d95e?d=mp&s=160"},"body":"On Wed, Apr 29, 2009 at 10:42:09AM +0200, Clemens Buchacher wrote:\n[...]\n> > git is unfortunately not capable of merging submodules at all, so I\n> > added these error messages to give me a hint about what I needed to do\n> > in conflicting submodules to get something useful. I have used git a\n> > lot more now, so maybe it is time to pick this up again and implement\n> > proper recursive sub-module merging.\n> \n> Are you sure it's always the right thing to merge conflicting submodule\n> versions? The user could easily commit two versions, which you would never\n> want merge -- due to changed history, for example. On the other hand, if a\n> fast-forward merge is possible I suppose this could be considered a\n> non-conflicting change.\n\nI would _like_ git to be able to merge submodules. However, if we\nwant to keep limiting sub-modules to only track external independent\n3rdparty modules, it isn't so useful I guess. But I also think that\nseriously reduces the usefulness of sub-modules.\n\nMaybe a per-submodule flag that indicates whether or not it wants to\nbe automatically merged from the supermodule? Closely coupled vs\nloosely coupled sub-modules.\n\n- Finn Arne\n"},{"id":"112657","messageId":"7v8wljcmvk.fsf_-_@gitster.siamese.dyndns.org","threadId":"19110","inReplyTo":"20090429084209.GA24064@localhost","subject":"[PATCH] Teach gitlinks to combine-diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-29T18:53:35Z","receivedAt":"2009-04-29T18:53:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The combine diff logic knew only about blobs (and their checked-out form\nin the work tree, either regular files or symlinks), and barfed when fed\nsubmodules. This \"externalizes\" gitlinks in the same way as the normal\npatch generation codepath does (i.e. \"Subproject commit Xxx\\n\") to fix the\nissue.\n\nSigned-off-by: Junio C Hamano \n---\n\n I think we should share the implementation betweeen grab_blob() and\n diff.c::diff_populate_gitlink() to keep their submodule representation\n the same, but I am doing this as a \"fix\" patch and I am lazy...\n\n combine-diff.c | 31 +++++++++++++++++++++++--------\n 1 files changed, 23 insertions(+), 8 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 066ce84..0f192e1 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -6,6 +6,7 @@\n #include \"quote.h\"\n #include \"xdiff-interface.h\"\n #include \"log-tree.h\"\n+#include \"refs.h\"\n \n static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr, int n, int num_parent)\n {\n@@ -90,7 +91,7 @@ struct sline {\n \tunsigned long *p_lno;\n };\n \n-static char *grab_blob(const unsigned char *sha1, unsigned long *size)\n+static char *grab_blob(const unsigned char *sha1, unsigned int mode, unsigned long *size)\n {\n \tchar *blob;\n \tenum object_type type;\n@@ -98,10 +99,15 @@ static char *grab_blob(const unsigned char *sha1, unsigned long *size)\n \t\t/* deleted blob */\n \t\t*size = 0;\n \t\treturn xcalloc(1, 1);\n+\t} else if (S_ISGITLINK(mode)) {\n+\t\tblob = xmalloc(100);\n+\t\t*size = snprintf(blob, 100,\n+\t\t\t\t \"Subproject commit %s\\n\", sha1_to_hex(sha1));\n+\t} else {\n+\t\tblob = read_sha1_file(sha1, &type, size);\n+\t\tif (type != OBJ_BLOB)\n+\t\t\tdie(\"object '%s' is not a blob!\", sha1_to_hex(sha1));\n \t}\n-\tblob = read_sha1_file(sha1, &type, size);\n-\tif (type != OBJ_BLOB)\n-\t\tdie(\"object '%s' is not a blob!\", sha1_to_hex(sha1));\n \treturn blob;\n }\n \n@@ -195,7 +201,8 @@ static void consume_line(void *state_, char *line, unsigned long len)\n \t}\n }\n \n-static void combine_diff(const unsigned char *parent, mmfile_t *result_file,\n+static void combine_diff(const unsigned char *parent, unsigned int mode,\n+\t\t\t mmfile_t *result_file,\n \t\t\t struct sline *sline, unsigned int cnt, int n,\n \t\t\t int num_parent)\n {\n@@ -211,7 +218,7 @@ static void combine_diff(const unsigned char *parent, mmfile_t *result_file,\n \tif (!cnt)\n \t\treturn; /* result deleted */\n \n-\tparent_file.ptr = grab_blob(parent, &sz);\n+\tparent_file.ptr = grab_blob(parent, mode, &sz);\n \tparent_file.size = sz;\n \tmemset(&xpp, 0, sizeof(xpp));\n \txpp.flags = XDF_NEED_MINIMAL;\n@@ -692,7 +699,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \n \t/* Read the result of merge first */\n \tif (!working_tree_file)\n-\t\tresult = grab_blob(elem->sha1, &result_size);\n+\t\tresult = grab_blob(elem->sha1, elem->mode, &result_size);\n \telse {\n \t\t/* Used by diff-tree to read from the working tree */\n \t\tstruct stat st;\n@@ -712,6 +719,12 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\tresult_size = buf.len;\n \t\t\tresult = strbuf_detach(&buf, NULL);\n \t\t\telem->mode = canon_mode(st.st_mode);\n+\t\t} else if (S_ISDIR(st.st_mode)) {\n+\t\t\tunsigned char sha1[20];\n+\t\t\tif (resolve_gitlink_ref(elem->path, \"HEAD\", sha1) < 0)\n+\t\t\t\tresult = grab_blob(elem->sha1, elem->mode, &result_size);\n+\t\t\telse\n+\t\t\t\tresult = grab_blob(sha1, elem->mode, &result_size);\n \t\t} else if (0 <= (fd = open(elem->path, O_RDONLY))) {\n \t\t\tsize_t len = xsize_t(st.st_size);\n \t\t\tssize_t done;\n@@ -804,7 +817,9 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\t}\n \t\t}\n \t\tif (i <= j)\n-\t\t\tcombine_diff(elem->parent[i].sha1, &result_file, sline,\n+\t\t\tcombine_diff(elem->parent[i].sha1,\n+\t\t\t\t elem->parent[i].mode,\n+\t\t\t\t &result_file, sline,\n \t\t\t\t cnt, i, num_parent);\n \t\tif (elem->parent[i].mode != elem->mode)\n \t\t\tmode_differs = 1;\n"},{"id":"112663","messageId":"7vskjrb896.fsf@gitster.siamese.dyndns.org","threadId":"19110","inReplyTo":"20090429084209.GA24064@localhost","subject":"Re: different git-merge behavior with regard to submodules in 1.6.2.4 vs. 1.6.2.1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-29T18:54:45Z","receivedAt":"2009-04-29T18:54:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> The fatal error is indeed caused by 0eb6574 (update cache for conflicting\n> submodule entries). The problem is also documented by t7405. The test\n> exposes a problem even previous to this commit, wherein \"git diff\" aborts\n> with\n>\n> \tfatal: read error 'sub'\n>\n> which is why I thought this was broken anyways. Only I see now that I have\n> made things worse.\n\nYour \"git diff\" calls into the combine-diff logic, and it was not updated\nwhen the \"gitlink\" was added. With your change you are merely hiding the\nissue under the rug. I've sent a fix as a separate patch.\n\nHow about doing this to fix the merge-recursive part? Looks a lot simpler;\nI haven't tested it very much, though.\n\n-- >8 --\nFrom: Junio C Hamano \nDate: Wed, 29 Apr 2009 11:08:18 -0700\nSubject: [PATCH] merge-recursive: do not die on a conflicting submodule\n\nWe cannot represent the 3-way conflicted state in the work tree\nfor these entries, but it is normal not to have commit objects\nfor them in our repository. Just update the index and the life\nwill be good.\n\nSigned-off-by: Junio C Hamano \n---\n merge-recursive.c | 8 ++++++--\n 1 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex d6f0582..a3721ef 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -520,8 +520,12 @@ static void update_file_flags(struct merge_options *o,\n \t\tunsigned long size;\n \n \t\tif (S_ISGITLINK(mode))\n-\t\t\tdie(\"cannot read object %s '%s': It is a submodule!\",\n-\t\t\t sha1_to_hex(sha), path);\n+\t\t\t/*\n+\t\t\t * We may later decide to recursively descend into\n+\t\t\t * the submodule directory and update its index\n+\t\t\t * and/or work tree, but we do not do that now.\n+\t\t\t */\n+\t\t\tgoto update_index;\n \n \t\tbuf = read_sha1_file(sha, &type, &size);\n \t\tif (!buf)\n-- \n1.6.3.rc3.16.gb37759\n"},{"id":"112687","messageId":"7vy6tj8aur.fsf_-_@gitster.siamese.dyndns.org","threadId":"19110","inReplyTo":"7v8wljcmvk.fsf_-_@gitster.siamese.dyndns.org","subject":"[PATCH v2] diff -c -p: do not die on submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-29T20:26:52Z","receivedAt":"2009-04-29T20:26:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The combine diff logic knew only about blobs (and their checked-out form\nin the work tree, either regular files or symlinks), and barfed when fed\nsubmodules. This \"externalizes\" gitlinks in the same way as the normal\npatch generation codepath does (i.e. \"Subproject commit Xxx\\n\") to fix the\nissue.\n\nSigned-off-by: Junio C Hamano \n---\n\n This is a re-roll of the earlier \"[PATCH] Teach gitlinks to combine-diff\",\n meant to apply to maint-1.6.0 and later.\n\n combine-diff.c | 38 ++++++++++++++++++++++++++------------\n t/t4027-diff-submodule.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 65 insertions(+), 12 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex aa9d79e..f617e9d 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -6,6 +6,7 @@\n #include \"quote.h\"\n #include \"xdiff-interface.h\"\n #include \"log-tree.h\"\n+#include \"refs.h\"\n \n static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr, int n, int num_parent)\n {\n@@ -90,18 +91,24 @@ struct sline {\n \tunsigned long *p_lno;\n };\n \n-static char *grab_blob(const unsigned char *sha1, unsigned long *size)\n+static char *grab_blob(const unsigned char *sha1, unsigned int mode, unsigned long *size)\n {\n \tchar *blob;\n \tenum object_type type;\n-\tif (is_null_sha1(sha1)) {\n+\n+\tif (S_ISGITLINK(mode)) {\n+\t\tblob = xmalloc(100);\n+\t\t*size = snprintf(blob, 100,\n+\t\t\t\t \"Subproject commit %s\\n\", sha1_to_hex(sha1));\n+\t} else if (is_null_sha1(sha1)) {\n \t\t/* deleted blob */\n \t\t*size = 0;\n \t\treturn xcalloc(1, 1);\n+\t} else {\n+\t\tblob = read_sha1_file(sha1, &type, size);\n+\t\tif (type != OBJ_BLOB)\n+\t\t\tdie(\"object '%s' is not a blob!\", sha1_to_hex(sha1));\n \t}\n-\tblob = read_sha1_file(sha1, &type, size);\n-\tif (type != OBJ_BLOB)\n-\t\tdie(\"object '%s' is not a blob!\", sha1_to_hex(sha1));\n \treturn blob;\n }\n \n@@ -197,7 +204,8 @@ static void consume_line(void *state_, char *line, unsigned long len)\n \t}\n }\n \n-static void combine_diff(const unsigned char *parent, mmfile_t *result_file,\n+static void combine_diff(const unsigned char *parent, unsigned int mode,\n+\t\t\t mmfile_t *result_file,\n \t\t\t struct sline *sline, unsigned int cnt, int n,\n \t\t\t int num_parent)\n {\n@@ -213,7 +221,7 @@ static void combine_diff(const unsigned char *parent, mmfile_t *result_file,\n \tif (!cnt)\n \t\treturn; /* result deleted */\n \n-\tparent_file.ptr = grab_blob(parent, &sz);\n+\tparent_file.ptr = grab_blob(parent, mode, &sz);\n \tparent_file.size = sz;\n \txpp.flags = XDF_NEED_MINIMAL;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n@@ -692,7 +700,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \tcontext = opt->context;\n \t/* Read the result of merge first */\n \tif (!working_tree_file)\n-\t\tresult = grab_blob(elem->sha1, &result_size);\n+\t\tresult = grab_blob(elem->sha1, elem->mode, &result_size);\n \telse {\n \t\t/* Used by diff-tree to read from the working tree */\n \t\tstruct stat st;\n@@ -712,9 +720,13 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\t}\n \t\t\tresult[len] = 0;\n \t\t\telem->mode = canon_mode(st.st_mode);\n-\t\t}\n-\t\telse if (0 <= (fd = open(elem->path, O_RDONLY)) &&\n-\t\t\t !fstat(fd, &st)) {\n+\t\t} else if (S_ISDIR(st.st_mode)) {\n+\t\t\tunsigned char sha1[20];\n+\t\t\tif (resolve_gitlink_ref(elem->path, \"HEAD\", sha1) < 0)\n+\t\t\t\tresult = grab_blob(elem->sha1, elem->mode, &result_size);\n+\t\t\telse\n+\t\t\t\tresult = grab_blob(sha1, elem->mode, &result_size);\n+\t\t} else if (0 <= (fd = open(elem->path, O_RDONLY))) {\n \t\t\tsize_t len = xsize_t(st.st_size);\n \t\t\tssize_t done;\n \t\t\tint is_file, i;\n@@ -807,7 +819,9 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\t}\n \t\t}\n \t\tif (i <= j)\n-\t\t\tcombine_diff(elem->parent[i].sha1, &result_file, sline,\n+\t\t\tcombine_diff(elem->parent[i].sha1,\n+\t\t\t\t elem->parent[i].mode,\n+\t\t\t\t &result_file, sline,\n \t\t\t\t cnt, i, num_parent);\n \t\tif (elem->parent[i].mode != elem->mode)\n \t\t\tmode_differs = 1;\ndiff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh\nindex ba6679c..718efe8 100755\n--- a/t/t4027-diff-submodule.sh\n+++ b/t/t4027-diff-submodule.sh\n@@ -57,4 +57,43 @@ test_expect_success 'git diff (empty submodule dir)' '\n \ttest_cmp empty actual.empty\n '\n \n+test_expect_success 'conflicted submodule setup' '\n+\n+\t# 39 efs\n+\tc=fffffffffffffffffffffffffffffffffffffff\n+\t(\n+\t\techo \"000000 $_z40 0\tsub\"\n+\t\techo \"160000 1$c 1\tsub\"\n+\t\techo \"160000 2$c 2\tsub\"\n+\t\techo \"160000 3$c 3\tsub\"\n+\t) | git update-index --index-info &&\n+\techo >expect.nosub '\\''diff --cc sub\n+index 2ffffff,3ffffff..0000000\n+--- a/sub\n++++ b/sub\n+@@@ -1,1 -1,1 +1,1 @@@\n+- Subproject commit 2fffffffffffffffffffffffffffffffffffffff\n+ -Subproject commit 3fffffffffffffffffffffffffffffffffffffff\n+++Subproject commit 0000000000000000000000000000000000000000'\\'' &&\n+\n+\thh=$(git rev-parse HEAD) &&\n+\tsed -e \"s/$_z40/$hh/\" expect.nosub >expect.withsub\n+\n+'\n+\n+test_expect_success 'combined (empty submodule)' '\n+\trm -fr sub && mkdir sub &&\n+\tgit diff >actual &&\n+\ttest_cmp expect.nosub actual\n+'\n+\n+test_expect_success 'combined (with submodule)' '\n+\trm -fr sub &&\n+\tgit clone --no-checkout . sub &&\n+\tgit diff >actual &&\n+\ttest_cmp expect.withsub actual\n+'\n+\n+\n+\n test_done\n-- \n1.6.3.rc3.16.gb37759\n"},{"id":"112701","messageId":"81b0412b0904291450w3d292ed5i3b2ab5164c0ae0f4@mail.gmail.com","threadId":"19110","inReplyTo":"7vy6tj8aur.fsf_-_@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] diff -c -p: do not die on submodules","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-04-29T21:50:20Z","receivedAt":"2009-04-29T21:50:20Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/4/29 Junio C Hamano :\n> +\n> +       if (S_ISGITLINK(mode)) {\n> +               blob = xmalloc(100);\n> +               *size = snprintf(blob, 100,\n> +                                \"Subproject commit %s\\n\", sha1_to_hex(sha1));\n\nsnprintf returns a signed value. It also has a bad record of returning\nnegative values for obscure reasons (on obscure platforms, admittedly).\n\nFor this particular case,\n\n strcpy(blob, \"Subproject commit \");\n strcat(blob, sha1_to_hex(sha1));\n strcat(blob, \"\\n\");\n *size = strlen(blob); /* that's a constant */\n\ncould be considered.\n"},{"id":"112709","messageId":"alpine.DEB.1.00.0904300011140.10279@pacific.mpi-cbg.de","threadId":"19110","inReplyTo":"81b0412b0904291450w3d292ed5i3b2ab5164c0ae0f4@mail.gmail.com","subject":"Re: [PATCH v2] diff -c -p: do not die on submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-29T22:13:00Z","receivedAt":"2009-04-29T22:13:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 29 Apr 2009, Alex Riesen wrote:\n\n> 2009/4/29 Junio C Hamano :\n> > +\n> > +       if (S_ISGITLINK(mode)) {\n> > +               blob = xmalloc(100);\n> > +               *size = snprintf(blob, 100,\n> > +                                \"Subproject commit %s\\n\", sha1_to_hex(sha1));\n> \n> snprintf returns a signed value. It also has a bad record of returning\n> negative values for obscure reasons (on obscure platforms, admittedly).\n> \n> For this particular case,\n> \n> strcpy(blob, \"Subproject commit \");\n> strcat(blob, sha1_to_hex(sha1));\n> strcat(blob, \"\\n\");\n> *size = strlen(blob); /* that's a constant */\n> \n> could be considered.\n\nActually, we know _exactly_ the size of the thing. It is 18+40+1. But I \nthink that *size wants to have the size, not the length. So add 1.\n\nIn any case, I don't think that we have to jump through hoops here: \nsnprintf() is _most_ unlikely to return something negative here. So I'd \nsay that readability trumps paranoia here.\n\nCiao,\nDscho\n"},{"id":"112710","messageId":"81b0412b0904291519m41035200n408b6ab409eed127@mail.gmail.com","threadId":"19110","inReplyTo":"alpine.DEB.1.00.0904300011140.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH v2] diff -c -p: do not die on submodules","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-04-29T22:19:58Z","receivedAt":"2009-04-29T22:19:58Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/4/30 Johannes Schindelin :\n> Hi,\n>\n> On Wed, 29 Apr 2009, Alex Riesen wrote:\n>\n>> 2009/4/29 Junio C Hamano :\n>> > +\n>> > +       if (S_ISGITLINK(mode)) {\n>> > +               blob = xmalloc(100);\n>> > +               *size = snprintf(blob, 100,\n>> > +                                \"Subproject commit %s\\n\", sha1_to_hex(sha1));\n>>\n>> snprintf returns a signed value. It also has a bad record of returning\n>> negative values for obscure reasons (on obscure platforms, admittedly).\n>>\n>> For this particular case,\n>>\n>>   strcpy(blob, \"Subproject commit \");\n>>   strcat(blob, sha1_to_hex(sha1));\n>>   strcat(blob, \"\\n\");\n>>   *size = strlen(blob); /* that's a constant */\n>>\n>> could be considered.\n>\n> Actually, we know _exactly_ the size of the thing.  It is 18+40+1.  But I\n> think that *size wants to have the size, not the length.  So add 1.\n>\n> In any case, I don't think that we have to jump through hoops here:\n> snprintf() is _most_ unlikely to return something negative here.  So I'd\n> say that readability trumps paranoia here.\n>\n\nhttp://www.google.com/search?q=snprintf+negative+return+value\n\nFirst link: http://bytes.com/groups/c/590845-snprintf-return-value\n\nLook for \"(Windows, mingw)\"\n"},{"id":"112715","messageId":"alpine.DEB.1.00.0904300037300.10279@pacific.mpi-cbg.de","threadId":"19110","inReplyTo":"81b0412b0904291519m41035200n408b6ab409eed127@mail.gmail.com","subject":"Re: [PATCH v2] diff -c -p: do not die on submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-29T22:39:21Z","receivedAt":"2009-04-29T22:39:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 30 Apr 2009, Alex Riesen wrote:\n\n> 2009/4/30 Johannes Schindelin :\n> > Hi,\n> >\n> > On Wed, 29 Apr 2009, Alex Riesen wrote:\n> >\n> >> 2009/4/29 Junio C Hamano :\n> >> > +\n> >> > +       if (S_ISGITLINK(mode)) {\n> >> > +               blob = xmalloc(100);\n> >> > +               *size = snprintf(blob, 100,\n> >> > +                                \"Subproject commit %s\\n\", sha1_to_hex(sha1));\n> >>\n> >> snprintf returns a signed value. It also has a bad record of returning\n> >> negative values for obscure reasons (on obscure platforms, admittedly).\n> >>\n> >> For this particular case,\n> >>\n> >>   strcpy(blob, \"Subproject commit \");\n> >>   strcat(blob, sha1_to_hex(sha1));\n> >>   strcat(blob, \"\\n\");\n> >>   *size = strlen(blob); /* that's a constant */\n> >>\n> >> could be considered.\n> >\n> > Actually, we know _exactly_ the size of the thing.  It is 18+40+1.  But I\n> > think that *size wants to have the size, not the length.  So add 1.\n> >\n> > In any case, I don't think that we have to jump through hoops here:\n> > snprintf() is _most_ unlikely to return something negative here.  So I'd\n> > say that readability trumps paranoia here.\n> >\n> \n> http://www.google.com/search?q=snprintf+negative+return+value\n> \n> First link: http://bytes.com/groups/c/590845-snprintf-return-value\n> \n> Look for \"(Windows, mingw)\"\n\nNo, I will not.\n\nThis is a _lousy_ time balance you propose: yourself saving a few minutes, \neverybody spending more time, trying to figure out what your point is, \nonly so that you do not have to summarize in a concise manner why you \nthing that I am wrong.\n\nI fully disagree with that, and refuse to obey your command,\nDscho\n"},{"id":"112720","messageId":"7vr5zb5a6v.fsf@gitster.siamese.dyndns.org","threadId":"19110","inReplyTo":"81b0412b0904291450w3d292ed5i3b2ab5164c0ae0f4@mail.gmail.com","subject":"Re: [PATCH v2] diff -c -p: do not die on submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-29T23:09:28Z","receivedAt":"2009-04-29T23:09:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> 2009/4/29 Junio C Hamano :\n>> +\n>> +       if (S_ISGITLINK(mode)) {\n>> +               blob = xmalloc(100);\n>> +               *size = snprintf(blob, 100,\n>> +                                \"Subproject commit %s\\n\", sha1_to_hex(sha1));\n>\n> snprintf returns a signed value. It also has a bad record of returning\n> negative values for obscure reasons (on obscure platforms, admittedly).\n\nThe arena is sufficiently large that there is no way any broken snprintf\ncan return negative here.\n\nThis is a copy from Linus's diff_populate_gitlink(), that dates back to\n0478675 (Expose subprojects as special files to \"git diff\" machinery,\n2007-04-15), and you have never seen any breakage, which should tell you\nsomething.\n\nAs I mentioned in the original patch, the codepath that reads one side of\ndiff (either from a blob or from a work tree entity) in show_patch_diff()\nand grab_blob() in combine-diff.c should do the same thing as what\ndiff_populate_filespec() in diff.c does, and these three functions need\nsome refactoring to share more code. The patch however is about fixing\nthe existing breakage without invasive refactoring.\n"},{"id":"112736","messageId":"81b0412b0904292247tdcf515bs67f809b559ac2504@mail.gmail.com","threadId":"19110","inReplyTo":"alpine.DEB.1.00.0904300037300.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH v2] diff -c -p: do not die on submodules","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-04-30T05:47:57Z","receivedAt":"2009-04-30T05:47:57Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/4/30 Johannes Schindelin :\n> On Thu, 30 Apr 2009, Alex Riesen wrote:\n>> http://www.google.com/search?q=snprintf+negative+return+value\n>>\n>> First link: http://bytes.com/groups/c/590845-snprintf-return-value\n>>\n>> Look for \"(Windows, mingw)\"\n>\n> No, I will not.\n>\n\n\"The following are the results from different systems:\ngcc 3.3 (Mac OS X): 10\ngcc 3.4.5 (Windows, mingw): -1\nBC++ 5.5.1 (Windows): 10\nMS VC++ 6.0 and 2005 (Windows, _snprintf() used): -1\nDigitalMars 8.42n (Windows): -1\"\n"},{"id":"112737","messageId":"20090430060731.GA4106@pvv.org","threadId":"19110","inReplyTo":"81b0412b0904292247tdcf515bs67f809b559ac2504@mail.gmail.com","subject":"Re: [PATCH v2] diff -c -p: do not die on submodules","fromName":"Finn Arne Gangstad","fromEmail":"finnag@pvv.org","sentAt":"2009-04-30T06:07:31Z","receivedAt":"2009-04-30T06:07:31Z","isPatch":true,"sender":{"key":"finnag@pvv.org","avatar":"https://gravatar.com/avatar/b421ddd58c3f0f93aa473e17b98bb8d53c221fef741746bc8cb59fae4ec6d95e?d=mp&s=160"},"body":"On Thu, Apr 30, 2009 at 07:47:57AM +0200, Alex Riesen wrote:\n> 2009/4/30 Johannes Schindelin :\n> > On Thu, 30 Apr 2009, Alex Riesen wrote:\n> >> http://www.google.com/search?q=snprintf+negative+return+value\n> >>\n> >> First link: http://bytes.com/groups/c/590845-snprintf-return-value\n> >>\n> >> Look for \"(Windows, mingw)\"\n> >\n> > No, I will not.\n> >\n> \n> \"The following are the results from different systems:\n> gcc 3.3 (Mac OS X): 10\n> gcc 3.4.5 (Windows, mingw): -1\n> BC++ 5.5.1 (Windows): 10\n> MS VC++ 6.0 and 2005 (Windows, _snprintf() used): -1\n> DigitalMars 8.42n (Windows): -1\"\n\nsnprintf always (on all these arechitectures) returns the number of\ncharacters written if the buffer is large enough. In the code we are\ndiscussing here the buffer is always large enough, so this is not a\nproblem.\n\n- Finn Arne\n"}]}