{"thread":{"id":"37336","subject":"[PATCH] read-cache.c: Ensure unmerged entries are removed","startedAt":"2014-08-12T15:31:30Z","lastAt":"2014-08-29T17:06:13Z","messageCount":51,"participants":["Jaime Soriano Pastor","Junio C Hamano","Duy Nguyen","Johannes Sixt","Jeff King","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"247607","messageId":"1407857491-16633-1-git-send-email-jsorianopastor@gmail.com","threadId":"37336","inReplyTo":null,"subject":"[PATCH] read-cache.c: Ensure unmerged entries are removed","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-12T15:31:30Z","receivedAt":"2014-08-12T15:31:30Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"A file in the index can be left as merged and unmerged at the same time\nby some tools as libgit2, this causes some undesiderable behaviours in git.\n\nI have seen, at least, these behaviours:\n- git reset --hard consuming 100% CPU and never ending\n- git reset --hard consuming all memory in git < 2.0\n- git add/git mergetool not resolving a conflict, even if they finish\n succesfully\n\nThe state is something like this:\n\n$ git ls-files -s\n100644 257cc5642cb1a054f08cc83f2d943e56fd3ebe99 0 conflict\n100644 257cc5642cb1a054f08cc83f2d943e56fd3ebe99 1 conflict\n100644 5716ca5987cbf97d6bb54920bea6adde242d87e6 2 conflict\n100644 f2e41136eac73c39554dede1fd7e67b12502d577 3 conflict\n\nThis can be caused e.g. by libgit2 doing this:\n1. Merge with conflicts, without solving them\n2. Force checkout\n\nI see that this is not caused by git (I haven't been able to reproduce it\nonly using git) but I think that git should be able to detect this situation\nand even handle it, specially to avoid the never-ending git resets. \n\nThe proposed patch serves as protection and autoremediation for this\nkind of cases.\nAnother option would be to detect the issue and tell the user to clean the\nindex with git read-tree --empty, but I think this would be more intrussive\nthan the patch.\n\nRegards,\nJaime Soriano.\n\nJaime Soriano Pastor (1):\n read-cache.c: Ensure unmerged entries are removed\n\n read-cache.c | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\n-- \n2.0.4.1.g8a38f21.dirty\n"},{"id":"247608","messageId":"1407857491-16633-2-git-send-email-jsorianopastor@gmail.com","threadId":"37336","inReplyTo":"1407857491-16633-1-git-send-email-jsorianopastor@gmail.com","subject":"[PATCH] read-cache.c: Ensure unmerged entries are removed","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-12T15:31:31Z","receivedAt":"2014-08-12T15:31:31Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"Wrong implementations of tools that modify the index can left\nsome files as merged and unmerged at the same time. Avoid undesiderable\nbehaviours by handling this situation.\n\nSigned-off-by: Jaime Soriano Pastor \n---\n read-cache.c | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 7f5645e..23e46e1 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -935,6 +935,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n \tint ok_to_replace = option & ADD_CACHE_OK_TO_REPLACE;\n \tint skip_df_check = option & ADD_CACHE_SKIP_DFCHECK;\n \tint new_only = option & ADD_CACHE_NEW_ONLY;\n+\tint replaced = 0;\n \n \tcache_tree_invalidate_path(istate->cache_tree, ce->name);\n \tpos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce));\n@@ -943,9 +944,10 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n \tif (pos >= 0) {\n \t\tif (!new_only)\n \t\t\treplace_index_entry(istate, pos, ce);\n-\t\treturn 0;\n-\t}\n-\tpos = -pos-1;\n+\t\tpos++;\n+\t\treplaced = 1;\n+\t} else\n+\t\tpos = -pos-1;\n \n \t/*\n \t * Inserting a merged entry (\"stage 0\") into the index\n@@ -959,6 +961,8 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n \t\t}\n \t}\n \n+\tif (replaced)\n+\t\treturn 0;\n \tif (!ok_to_add)\n \t\treturn -1;\n \tif (!verify_path(ce->name))\n-- \n2.0.4.1.g8a38f21.dirty\n"},{"id":"247617","messageId":"xmqqd2c55zl6.fsf@gitster.dls.corp.google.com","threadId":"37336","inReplyTo":"1407857491-16633-1-git-send-email-jsorianopastor@gmail.com","subject":"Re: [PATCH] read-cache.c: Ensure unmerged entries are removed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-12T18:31:49Z","receivedAt":"2014-08-12T18:31:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jaime Soriano Pastor writes:\n\n> A file in the index can be left as merged and unmerged at the same time\n> by some tools as libgit2, this causes some undesiderable behaviours in git.\n\nWell, doesn't it mean that libgit2 is broken? Have you filed a bug\nover there yet?\n\nHaving said that, protecting ourselves from insanity left by other\npeople is always a good idea, provided that it can be done without\nbending overly backwards.\n"},{"id":"247618","messageId":"xmqq8umt5z8i.fsf@gitster.dls.corp.google.com","threadId":"37336","inReplyTo":"1407857491-16633-2-git-send-email-jsorianopastor@gmail.com","subject":"Re: [PATCH] read-cache.c: Ensure unmerged entries are removed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-12T18:39:25Z","receivedAt":"2014-08-12T18:39:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jaime Soriano Pastor writes:\n\n> Wrong implementations of tools that modify the index can left\n> some files as merged and unmerged at the same time. Avoid undesiderable\n> behaviours by handling this situation.\n\nIt is understandable that the way _you_ decided to \"handle the\nsituation\" is so obvious to be spelled out to _you_, but that is the\nmost important design decision that needs to be described here. Do\nyou silently remove higher-stage entries when an entry at stage 0\nexists? Do you silently remove stage 0 entry when higher-stage\nentries exist? Do you error out without doing neither?\n\nSilently removing these at runtime may not be something we would\nwant to do; after all, we do not know if the broken tool actually\nwanted to have the higher stage entries, or the merged entry.\n\nIdeally, I think we should error out and let the users figure out\nhow to proceed (we may of course need to make sure they have the\nnecessary tools to do so, e.g. \"git cat-file blob 0:$path\" to\nresurrect the contents and \"git update-index --cacheinfo\" to stuff\nthe contents into the stages).\n\nThanks.\n"},{"id":"247711","messageId":"CAPuZ2NGB710TUW4VJB5kGWjoXV+eiebv_ZYvooG=E-J-MLXywQ@mail.gmail.com","threadId":"37336","inReplyTo":"xmqqd2c55zl6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] read-cache.c: Ensure unmerged entries are removed","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-13T22:10:03Z","receivedAt":"2014-08-13T22:10:03Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"On Tue, Aug 12, 2014 at 8:31 PM, Junio C Hamano wrote:\n>\n> Jaime Soriano Pastor writes:\n>\n> > A file in the index can be left as merged and unmerged at the same time\n> > by some tools as libgit2, this causes some undesiderable behaviours in git.\n>\n> Well, doesn't it mean that libgit2 is broken? Have you filed a bug\n> over there yet?\n>\n\nYes, exactly, I think libgit2 is broken but I wanted to double-check\nthat it was still happening in their master branch, and it is. I have\nreported the bug after checking it.\nhttps://github.com/libgit2/libgit2/issues/2515\n\n>\n> Having said that, protecting ourselves from insanity left by other\n> people is always a good idea, provided that it can be done without\n> bending overly backwards.\n\n\nYes, I think the most important thing in this case is to protect git\nagainst this kind of inconsistencies.\n"},{"id":"247712","messageId":"CAPuZ2NFcJ5FFeJP=0-oG59fs=3ztenoPOGWOZKfZDdHpLGOYeA@mail.gmail.com","threadId":"37336","inReplyTo":"xmqq8umt5z8i.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] read-cache.c: Ensure unmerged entries are removed","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-13T22:10:21Z","receivedAt":"2014-08-13T22:10:21Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"On Tue, Aug 12, 2014 at 8:39 PM, Junio C Hamano wrote:\n>\n> Jaime Soriano Pastor writes:\n>\n> > Wrong implementations of tools that modify the index can left\n> > some files as merged and unmerged at the same time. Avoid undesiderable\n> > behaviours by handling this situation.\n>\n> It is understandable that the way _you_ decided to \"handle the\n> situation\" is so obvious to be spelled out to _you_, but that is the\n> most important design decision that needs to be described here. Do\n> you silently remove higher-stage entries when an entry at stage 0\n> exists? Do you silently remove stage 0 entry when higher-stage\n> entries exist? Do you error out without doing neither?\n>\n\nSorry, I didn't explain my decission enough, and my knowledge of git\ninternals is not so good.\nThe idea of my proposal is to remove higher stage entries when, after\nreplacing an existing entry at stage 0, there are still entries in\nhigher stages.\n\nIn the problematic cases I've seen (specially git add and git reset\n--hard) the final state of both, merged and unmerged files, is that\nonly an entry in stage 0 exists.\nAlso, the current implementation of git checkout -f silently removes\nhigher stage entries in this case.\n\n>\n> Silently removing these at runtime may not be something we would\n> want to do; after all, we do not know if the broken tool actually\n> wanted to have the higher stage entries, or the merged entry.\n>\n\nYes, I have to agree on that, the user should have the final decission\nabout what stage entry to use, although I'm not sure if in the\npreviously commented cases there could be such an additional loss as\nthe operations that can be modified are already intended to silently\nremove stage entries.\n\n> Ideally, I think we should error out and let the users figure out\n> how to proceed (we may of course need to make sure they have the\n> necessary tools to do so, e.g. \"git cat-file blob 0:$path\" to\n> resurrect the contents and \"git update-index --cacheinfo\" to stuff\n> the contents into the stages).\n>\n\nI have also tried a couple of implementations of this patch with die()\nand warning().\nThe implementation with die() would have a message like \"There are\nother staged versions for merged file\", and maybe some recomendation\nabout how to see the blobs.\nThe warning implementation could return -1, what would prevent git add\nto remove the higher-stage entries, but would still make git reset\n--hard to clean the index as it seems that it does it anyway if it\nmanages to finish the call to read_index_unmerged.\nAnother option would be to print the deleted entries as a warning but\ndeleting them anyway.\n\nWhich option would be better? And what could be a good message?\n\nBTW, I didn't know \"git cat-file blob 0:$path\", but I only manage to\nget \"Not a valid object name\" fatals. How is it supposed to be used?\n\nThanks.\n"},{"id":"247713","messageId":"xmqqiolw2dqc.fsf@gitster.dls.corp.google.com","threadId":"37336","inReplyTo":"CAPuZ2NFcJ5FFeJP=0-oG59fs=3ztenoPOGWOZKfZDdHpLGOYeA@mail.gmail.com","subject":"Re: [PATCH] read-cache.c: Ensure unmerged entries are removed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-13T23:04:27Z","receivedAt":"2014-08-13T23:04:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jaime Soriano Pastor writes:\n\n> In the problematic cases I've seen (specially git add and git reset\n> --hard) the final state of both, merged and unmerged files, is that\n> only an entry in stage 0 exists.\n> Also, the current implementation of git checkout -f silently removes\n> higher stage entries in this case.\n>>\n>> Silently removing these at runtime may not be something we would\n>> want to do; after all, we do not know if the broken tool actually\n>> wanted to have the higher stage entries, or the merged entry.\n>>\n>\n> Yes, I have to agree on that, the user should have the final decission\n> about what stage entry to use, although I'm not sure if in the\n> previously commented cases there could be such an additional loss as\n> the operations that can be modified are already intended to silently\n> remove stage entries.\n> ...\n> Which option would be better? And what could be a good message?\n\nBeing a conservative, I'd rather avoid doing any magic during\nread_cache() time. \"ls-files -s\" for example should show the four\nstages so that the \"broken\" state can be inspected.\n\nInstead, I suspect that the code paths with problematic iterations\nover the index entries that assume that having stage #0 entry for a\npath guarantees that there will not be any higher stage entry first\nneed to be identified (you already said \"add\" and \"reset\" may be\nproblematic, there may be others, or they may be the only ones, I\ndunno), and then the most sensible one, which would be different\nfrom case to case, among various possibilities need to be chosen as\na fix to each of them:\n\n (1) the loop may be fixed to ignore/skip unmerged entries;\n (2) the loop may be fixed to ignore/skip the merged entry;\n (3) the loop may be fixed not to spin indefinitely on a path with\n mixed entries; or\n (4) the command should error out.\n\nYes, it would be more work, but I'd feel safer if the following\nworked:\n\n\t$ git ls-files -s\n 100644 3cc58df83752123644fef39faab2393af643b1d2 0 conflict\n 100644 f70f10e4db19068f79bc43844b49f3eece45c4e8 1 conflict\n 100644 3cc58df83752123644fef39faab2393af643b1d2 2 conflict\n 100644 223b7836fb19fdf64ba2d3cd6173c6a283141f78 3 conflict\n\t$ >empty\n $ git add empty\n 100644 3cc58df83752123644fef39faab2393af643b1d2 0 conflict\n 100644 f70f10e4db19068f79bc43844b49f3eece45c4e8 1 conflict\n 100644 3cc58df83752123644fef39faab2393af643b1d2 2 conflict\n 100644 223b7836fb19fdf64ba2d3cd6173c6a283141f78 3 conflict\n\t100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 empty\n\t$ git cat-file blob :empty >output\n $ cmp empty output && echo OK\n OK\n\nwhich would be impossible to do if we nuked the \"problematic\" stages\nwhenever we read the index, I am afraid.\n\n> BTW, I didn't know \"git cat-file blob 0:$path\", but I only manage to\n> get \"Not a valid object name\" fatals. How is it supposed to be used?\n\nThat was a typo of \":$n:$path\" (where 0 <= $n <= 3).\n"},{"id":"247754","messageId":"CAPuZ2NFJiUt1p_OfefswS8O8_BA6mQy=PStmYyNw=ABZCOQprQ@mail.gmail.com","threadId":"37336","inReplyTo":"xmqqiolw2dqc.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] read-cache.c: Ensure unmerged entries are removed","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-15T19:50:11Z","receivedAt":"2014-08-15T19:50:11Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"On Thu, Aug 14, 2014 at 1:04 AM, Junio C Hamano wrote:\n> Being a conservative, I'd rather avoid doing any magic during\n> read_cache() time. \"ls-files -s\" for example should show the four\n> stages so that the \"broken\" state can be inspected.\n>\nWell, only read_cache_unmerged() is modified in the sent patch, so no\nmagic is done in read_cache(), I'd also avoid changes there. Indeed\nwith the patch, \"ls-files -s\" can be used to inspect the problem\nwithout further problems.\n\n> Instead, I suspect that the code paths with problematic iterations\n> over the index entries that assume that having stage #0 entry for a\n> path guarantees that there will not be any higher stage entry first\n> need to be identified (you already said \"add\" and \"reset\" may be\n> problematic, there may be others, or they may be the only ones, I\n> dunno), and then the most sensible one, which would be different\n> from case to case, among various possibilities need to be chosen as\n> a fix to each of them:\n>\n> (1) the loop may be fixed to ignore/skip unmerged entries;\n> (2) the loop may be fixed to ignore/skip the merged entry;\n> (3) the loop may be fixed not to spin indefinitely on a path with\n> mixed entries; or\n> (4) the command should error out.\n>\ngit reset will clean the index anyway if the loop finishes, would it\nbe ok? I think that it'd be acceptable for git reset --hard to clean\nthe index as git checkout -f already does it even in this case.\n\ngit merge is also affected by the loop in read_cache_unmerged(), but\nany of the solutions would be enough for it as only by finishing the\nloop with unmerged entries it will die without commiting the cache to\nthe index file.\n\nFor git add probably the best option is to error out and ask the user\nto check \"git ls-files -s\" to investigate the problem and decide what\nto do.\n\nThe error message given by \"git commit -a\" is a bit confusing in this\ncase, I can take a look to this too.\n\nI'll try to prepare a patch with these cases, and rethinking the loop\nto avoid future problems there, I think that is a bit dangerous to\nlook for the position of a path entry (with index_name_pos) for the\nnext iteration.\n\n> Yes, it would be more work, but I'd feel safer if the following\n> worked:\n>\n> $ git ls-files -s\n> 100644 3cc58df83752123644fef39faab2393af643b1d2 0 conflict\n> 100644 f70f10e4db19068f79bc43844b49f3eece45c4e8 1 conflict\n> 100644 3cc58df83752123644fef39faab2393af643b1d2 2 conflict\n> 100644 223b7836fb19fdf64ba2d3cd6173c6a283141f78 3 conflict\n> $ >empty\n> $ git add empty\n> 100644 3cc58df83752123644fef39faab2393af643b1d2 0 conflict\n> 100644 f70f10e4db19068f79bc43844b49f3eece45c4e8 1 conflict\n> 100644 3cc58df83752123644fef39faab2393af643b1d2 2 conflict\n> 100644 223b7836fb19fdf64ba2d3cd6173c6a283141f78 3 conflict\n> 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 empty\n> $ git cat-file blob :empty >output\n> $ cmp empty output && echo OK\n> OK\n>\n> which would be impossible to do if we nuked the \"problematic\" stages\n> whenever we read the index, I am afraid.\n>\nThis works with the first patch as read_cache() is not modified, and\ngit add would only clean the entries for the paths passed as\narguments.\n\n>> BTW, I didn't know \"git cat-file blob 0:$path\", but I only manage to\n>> get \"Not a valid object name\" fatals. How is it supposed to be used?\n>\n> That was a typo of \":$n:$path\" (where 0 <= $n <= 3).\nGreat, thanks!\n"},{"id":"247758","messageId":"xmqqmwb5zatx.fsf@gitster.dls.corp.google.com","threadId":"37336","inReplyTo":"CAPuZ2NFJiUt1p_OfefswS8O8_BA6mQy=PStmYyNw=ABZCOQprQ@mail.gmail.com","subject":"Re: [PATCH] read-cache.c: Ensure unmerged entries are removed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-15T21:45:14Z","receivedAt":"2014-08-15T21:45:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jaime Soriano Pastor writes:\n\n> On Thu, Aug 14, 2014 at 1:04 AM, Junio C Hamano wrote:\n>> Being a conservative, I'd rather avoid doing any magic during\n>> read_cache() time. \"ls-files -s\" for example should show the four\n>> stages so that the \"broken\" state can be inspected.\n>>\n> Well, only read_cache_unmerged() is modified in the sent patch, so no\n> magic is done in read_cache(), I'd also avoid changes there.\n\nAhh, I must have overlooked that; changes being only in _unmerged()\nvariant makes me feel much better, and it probably would make much\nof ...\n\n>> Yes, it would be more work,...\n\n... moot, hopefully ;-)\n\n> git reset will clean the index anyway if the loop finishes, would it\n> be ok?\n\nSurely.\n\n> git merge is also affected by the loop in read_cache_unmerged(), but\n> any of the solutions would be enough for it as only by finishing the\n> loop with unmerged entries it will die without commiting the cache to\n> the index file.\n\nAgain, true. The mergy operations want to start from a clean slate\nand they call _unmerged() variant primarily to learn that there were\nunmerged entries in the index, only to abort the operation in that\ncase, so a change to _unmerged() variant should be safe for them.\n\nI'll take another look at your patch later, but not before the 2.1\nfinal, and by that time you may already have sent a reroll ;-)\n\nThanks.\n"},{"id":"247775","messageId":"CAPuZ2NEE0d_5Vj3MSGEJCtFBFOe75Rb+q9fk7q1YC-WcvAMb4A@mail.gmail.com","threadId":"37336","inReplyTo":"xmqqmwb5zatx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] read-cache.c: Ensure unmerged entries are removed","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-16T14:51:41Z","receivedAt":"2014-08-16T14:51:41Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"I'd like to add some tests too for this, but I don't know how to\nreproduce this state with git commands only, is there any way to add\nentries to the index without checkings?\nOr maybe it could be done by creating a \"test-\" command that adds the\nentries to an index?\n\nThanks.\n\nOn Fri, Aug 15, 2014 at 11:45 PM, Junio C Hamano wrote:\n> Jaime Soriano Pastor writes:\n>\n>> On Thu, Aug 14, 2014 at 1:04 AM, Junio C Hamano wrote:\n>>> Being a conservative, I'd rather avoid doing any magic during\n>>> read_cache() time. \"ls-files -s\" for example should show the four\n>>> stages so that the \"broken\" state can be inspected.\n>>>\n>> Well, only read_cache_unmerged() is modified in the sent patch, so no\n>> magic is done in read_cache(), I'd also avoid changes there.\n>\n> Ahh, I must have overlooked that; changes being only in _unmerged()\n> variant makes me feel much better, and it probably would make much\n> of ...\n>\n>>> Yes, it would be more work,...\n>\n> ... moot, hopefully ;-)\n>\n>> git reset will clean the index anyway if the loop finishes, would it\n>> be ok?\n>\n> Surely.\n>\n>> git merge is also affected by the loop in read_cache_unmerged(), but\n>> any of the solutions would be enough for it as only by finishing the\n>> loop with unmerged entries it will die without commiting the cache to\n>> the index file.\n>\n> Again, true. The mergy operations want to start from a clean slate\n> and they call _unmerged() variant primarily to learn that there were\n> unmerged entries in the index, only to abort the operation in that\n> case, so a change to _unmerged() variant should be safe for them.\n>\n> I'll take another look at your patch later, but not before the 2.1\n> final, and by that time you may already have sent a reroll ;-)\n>\n> Thanks.\n\n\n\n-- \nJaime Soriano Pastor - Software Developer\n"},{"id":"247845","messageId":"xmqqppfxycxj.fsf@gitster.dls.corp.google.com","threadId":"37336","inReplyTo":"CAPuZ2NEE0d_5Vj3MSGEJCtFBFOe75Rb+q9fk7q1YC-WcvAMb4A@mail.gmail.com","subject":"Re: [PATCH] read-cache.c: Ensure unmerged entries are removed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-18T16:34:16Z","receivedAt":"2014-08-18T16:34:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jaime Soriano Pastor writes:\n\n> I'd like to add some tests too for this, but I don't know how to\n> reproduce this state with git commands only, is there any way to add\n> entries to the index without checkings?\n\nPerhaps feeding \"update-index --index-info\" four input lines would work?\n"},{"id":"247853","messageId":"CAPuZ2NFqR67LA=eeDQVJsm_vGAHHGBy2hVNugrovzCS_kzXtMg@mail.gmail.com","threadId":"37336","inReplyTo":"xmqqppfxycxj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] read-cache.c: Ensure unmerged entries are removed","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-18T17:23:29Z","receivedAt":"2014-08-18T17:23:29Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"Yes, --index-info worked for this purpouse, thanks!\n\nhttps://github.com/jsoriano/git/blob/remove-unmerged-index-entry/t/t9904-unmerged-file-with-merged-entry.sh#L25\n\nI'll try to send the patches to the mailing lists later today or tomorrow.\n\nOn Mon, Aug 18, 2014 at 6:34 PM, Junio C Hamano wrote:\n> Jaime Soriano Pastor writes:\n>\n>> I'd like to add some tests too for this, but I don't know how to\n>> reproduce this state with git commands only, is there any way to add\n>> entries to the index without checkings?\n>\n> Perhaps feeding \"update-index --index-info\" four input lines would work?\n"},{"id":"248015","messageId":"cover.1408533065.git.jsorianopastor@gmail.com","threadId":"37336","inReplyTo":"CAPuZ2NFqR67LA=eeDQVJsm_vGAHHGBy2hVNugrovzCS_kzXtMg@mail.gmail.com","subject":"[PATCH 0/4] Handling unmerged files with merged entries","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-20T11:25:59Z","receivedAt":"2014-08-20T11:25:59Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"New approach for the case of finding unmerged files with merged entries\nin the index.\nAfter some discussion the solution tries to:\n- Avoid the problems with infinite loops in this case.\n- Provide better information to the user in the commands affected.\n- Make sure there are ways to clean the index.\n- Provide also a way to specifically recover each one of the files with\n this problem.\n\nWith these patches the behaviour of these commands (for this case) change:\n- git reset is able to finish, cleaning the index, but warning out the\n information about the removed stages.\n- git merge is able to finish, reporting that there is a merge in progress as\n usual, it also warns about the unmerged files with merged entries.\n- git add fails when this case happens, telling the user to check the state\n with 'git ls-files -s', before, it did nothing. The same with git commit -a.\n- git update-index --cacheinfo can be used to select an specific staged\n version to resolve the conflict, without the need of reseting the working\n copy. It did nothing before.\n\nTests added for these cases. Rest of the tests remain unchanged and pass too.\n\nJaime Soriano Pastor (4):\n read_index_unmerged doesn't loop forever if merged stage exists for\n unmerged file\n Error out when adding a file with merged and unmerged entries\n Added tests for the case of merged and unmerged entries for the same\n file\n git update-index --cacheinfo can be used to select a stage when there\n are merged and unmerged entries\n\n builtin/update-index.c | 1 +\n cache.h | 1 +\n read-cache.c | 36 ++++++++++--\n t/t9904-unmerged-file-with-merged-entry.sh | 94 ++++++++++++++++++++++++++++++\n 4 files changed, 127 insertions(+), 5 deletions(-)\n create mode 100755 t/t9904-unmerged-file-with-merged-entry.sh\n\n-- \n2.0.4.4.gaf54b2b\n"},{"id":"248013","messageId":"3c1dfe8a1cc03c083e93ce834aac574e4e321f6e.1408533065.git.jsorianopastor@gmail.com","threadId":"37336","inReplyTo":"cover.1408533065.git.jsorianopastor@gmail.com","subject":"[PATCH 1/4] read_index_unmerged doesn't loop forever if merged stage exists for unmerged file","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-20T11:26:00Z","receivedAt":"2014-08-20T11:26:00Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"Signed-off-by: Jaime Soriano Pastor \n---\n read-cache.c | 21 +++++++++++++++++++--\n 1 file changed, 19 insertions(+), 2 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 7f5645e..c932b83 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1933,6 +1933,7 @@ int read_index_unmerged(struct index_state *istate)\n {\n \tint i;\n \tint unmerged = 0;\n+\tstruct cache_entry *merged_ce = NULL;\n \n \tread_index(istate);\n \tfor (i = 0; i < istate->cache_nr; i++) {\n@@ -1940,9 +1941,26 @@ int read_index_unmerged(struct index_state *istate)\n \t\tstruct cache_entry *new_ce;\n \t\tint size, len;\n \n-\t\tif (!ce_stage(ce))\n+\t\tif (!ce_stage(ce)) {\n+\t\t\tmerged_ce = ce;\n \t\t\tcontinue;\n+\t\t}\n \t\tunmerged = 1;\n+\t\tif (merged_ce && ce_same_name(merged_ce, ce)) {\n+\t\t\twarning(\"Unexpected stages for merged file '%s':\",\n+\t\t\t\tmerged_ce->name);\n+\t\t\ti--;\n+\t\t\twhile (i < istate->cache_nr &&\n+\t\t\t\t ce_same_name(merged_ce, istate->cache[i])) {\n+\t\t\t\tce = istate->cache[i++];\n+\t\t\t\twarning(\"%06o %s %d\",\n+\t\t\t\t ce->ce_mode, sha1_to_hex(ce->sha1), ce_stage(ce));\n+\t\t\t}\n+\t\t\ti--;\n+\t\t\tmerged_ce->ce_flags = create_ce_flags(0) | CE_CONFLICTED;\n+\t\t\tmerged_ce = NULL;\n+\t\t\tcontinue;\n+\t\t}\n \t\tlen = ce_namelen(ce);\n \t\tsize = cache_entry_size(len);\n \t\tnew_ce = xcalloc(1, size);\n@@ -1953,7 +1971,6 @@ int read_index_unmerged(struct index_state *istate)\n \t\tif (add_index_entry(istate, new_ce, 0))\n \t\t\treturn error(\"%s: cannot drop to stage #0\",\n \t\t\t\t new_ce->name);\n-\t\ti = index_name_pos(istate, new_ce->name, len);\n \t}\n \treturn unmerged;\n }\n-- \n2.0.4.4.gaf54b2b\n"},{"id":"248014","messageId":"e3abc57b58e7cfae11a1221c9a06e74a34148295.1408533065.git.jsorianopastor@gmail.com","threadId":"37336","inReplyTo":"cover.1408533065.git.jsorianopastor@gmail.com","subject":"[PATCH 2/4] Error out when adding a file with merged and unmerged entries","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-20T11:26:01Z","receivedAt":"2014-08-20T11:26:01Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"Signed-off-by: Jaime Soriano Pastor \n---\n read-cache.c | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex c932b83..d549d0b 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -935,6 +935,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n \tint ok_to_replace = option & ADD_CACHE_OK_TO_REPLACE;\n \tint skip_df_check = option & ADD_CACHE_SKIP_DFCHECK;\n \tint new_only = option & ADD_CACHE_NEW_ONLY;\n+\tint replaced = 0;\n \n \tcache_tree_invalidate_path(istate->cache_tree, ce->name);\n \tpos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce));\n@@ -943,9 +944,10 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n \tif (pos >= 0) {\n \t\tif (!new_only)\n \t\t\treplace_index_entry(istate, pos, ce);\n-\t\treturn 0;\n-\t}\n-\tpos = -pos-1;\n+\t\treplaced = 1;\n+\t\tpos++;\n+\t} else\n+\t\tpos = -pos-1;\n \n \t/*\n \t * Inserting a merged entry (\"stage 0\") into the index\n@@ -953,12 +955,18 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n \t */\n \tif (pos < istate->cache_nr && ce_stage(ce) == 0) {\n \t\twhile (ce_same_name(istate->cache[pos], ce)) {\n+\t\t\tif (replaced)\n+\t\t\t\tdie(\"Merged and unmerged entries found for \"\n+\t\t\t\t \"'%s', check 'git ls-files -s \\\"%s\\\"'\",\n+\t\t\t\t ce->name, ce->name);\n \t\t\tok_to_add = 1;\n \t\t\tif (!remove_index_entry_at(istate, pos))\n \t\t\t\tbreak;\n \t\t}\n \t}\n \n+\tif (replaced)\n+\t\treturn 0;\n \tif (!ok_to_add)\n \t\treturn -1;\n \tif (!verify_path(ce->name))\n-- \n2.0.4.4.gaf54b2b\n"},{"id":"248012","messageId":"7782c581021d529947a1385939231eeb145d150e.1408533065.git.jsorianopastor@gmail.com","threadId":"37336","inReplyTo":"cover.1408533065.git.jsorianopastor@gmail.com","subject":"[PATCH 3/4] Added tests for the case of merged and unmerged entries for the same file","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-20T11:26:02Z","receivedAt":"2014-08-20T11:26:02Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"Signed-off-by: Jaime Soriano Pastor \n---\n t/t9904-unmerged-file-with-merged-entry.sh | 86 ++++++++++++++++++++++++++++++\n 1 file changed, 86 insertions(+)\n create mode 100755 t/t9904-unmerged-file-with-merged-entry.sh\n\ndiff --git a/t/t9904-unmerged-file-with-merged-entry.sh b/t/t9904-unmerged-file-with-merged-entry.sh\nnew file mode 100755\nindex 0000000..945bc1c\n--- /dev/null\n+++ b/t/t9904-unmerged-file-with-merged-entry.sh\n@@ -0,0 +1,86 @@\n+#!/bin/sh\n+\n+test_description='Operations with unmerged files with merged entries'\n+\n+. ./test-lib.sh\n+\n+setup_repository() {\n+\ttest_commit A conflict A\n+\ttest_commit A conflict2 A2 branchbase\n+\ttest_commit B conflict B\n+\ttest_commit B conflict2 B2\n+\tgit checkout branchbase -b branch1\n+\ttest_commit C conflict C\n+\ttest_commit C conflict2 C2\n+\ttest_commit something otherfile otherfile\n+}\n+\n+setup_stage_state() {\n+\tgit checkout -f HEAD\n+\t{\n+\t\tgit ls-files -s conflict conflict2\n+\t\tgit merge master > /dev/null\n+\t\tgit ls-files -s conflict conflict2\n+\t} > index\n+\tcat index | git update-index --index-info\n+\trm index\n+}\n+\n+test_expect_success 'setup - two branches with conflicting files' '\n+\tsetup_repository &&\n+\tsetup_stage_state &&\n+\tgit ls-files -s conflict > output &&\n+\ttest_line_count = 4 output &&\n+\tgit ls-files -s conflict2 > output &&\n+\ttest_line_count = 4 output &&\n+\trm output\n+'\n+\n+test_expect_success 'git commit -a' '\n+\tsetup_stage_state &&\n+\ttest_must_fail git commit -a\n+'\n+\n+test_expect_success 'git add conflict' '\n+\tsetup_stage_state &&\n+\ttest_must_fail git add conflict\n+'\n+\n+test_expect_success 'git rm conflict' '\n+\tsetup_stage_state &&\n+\ttest_must_fail git rm conflict\n+'\n+\n+test_expect_success 'git add otherfile' '\n+\tsetup_stage_state &&\n+\t>otherfile &&\n+\tgit add otherfile\n+'\n+\n+test_expect_success 'git rm otherfile' '\n+\tsetup_stage_state &&\n+\tgit rm otherfile\n+'\n+\n+test_expect_success 'git add newfile' '\n+\tsetup_stage_state &&\n+\t>newfile &&\n+\tgit add newfile\n+'\n+\n+test_expect_success 'git merge branch' '\n+\tsetup_stage_state &&\n+\ttest_must_fail git merge master\n+'\n+\n+test_expect_success 'git reset --hard' '\n+\tsetup_stage_state &&\n+\tgit reset --hard &&\n+\tgit show HEAD:conflict > expected &&\n+\tcat conflict > current &&\n+\tgit show HEAD:conflict2 > expected &&\n+\tcat conflict2 > current &&\n+\ttest_cmp expected current\n+'\n+\n+test_done\n-- \n2.0.4.4.gaf54b2b\n"},{"id":"248011","messageId":"af54b2b3e80a6ff76b07ea129ead43079ef06a7a.1408533065.git.jsorianopastor@gmail.com","threadId":"37336","inReplyTo":"cover.1408533065.git.jsorianopastor@gmail.com","subject":"[PATCH 4/4] git update-index --cacheinfo can be used to select a stage when there are merged and unmerged entries","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-20T11:26:03Z","receivedAt":"2014-08-20T11:26:03Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"Signed-off-by: Jaime Soriano Pastor \n---\n builtin/update-index.c | 1 +\n cache.h | 1 +\n read-cache.c | 3 ++-\n t/t9904-unmerged-file-with-merged-entry.sh | 14 +++++++++++---\n 4 files changed, 15 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex ebea285..509fae7 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -239,6 +239,7 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,\n \t\tce->ce_flags |= CE_VALID;\n \toption = allow_add ? ADD_CACHE_OK_TO_ADD : 0;\n \toption |= allow_replace ? ADD_CACHE_OK_TO_REPLACE : 0;\n+\toption |= ADD_CACHE_OK_TO_REPLACE_MERGED;\n \tif (add_cache_entry(ce, option))\n \t\treturn error(\"%s: cannot add to the index - missing --add option?\",\n \t\t\t path);\ndiff --git a/cache.h b/cache.h\nindex c708062..ecac41c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -474,6 +474,7 @@ extern int index_name_pos(const struct index_state *, const char *name, int name\n #define ADD_CACHE_SKIP_DFCHECK 4\t/* Ok to skip DF conflict checks */\n #define ADD_CACHE_JUST_APPEND 8\t\t/* Append only; tree.c::read_tree() */\n #define ADD_CACHE_NEW_ONLY 16\t\t/* Do not replace existing ones */\n+#define ADD_CACHE_OK_TO_REPLACE_MERGED 32 /* Ok to replace even if a merged entry exists */\n extern int add_index_entry(struct index_state *, struct cache_entry *ce, int option);\n extern void rename_index_entry_at(struct index_state *, int pos, const char *new_name);\n extern int remove_index_entry_at(struct index_state *, int pos);\ndiff --git a/read-cache.c b/read-cache.c\nindex d549d0b..c4ddefe 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -933,6 +933,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n \tint pos;\n \tint ok_to_add = option & ADD_CACHE_OK_TO_ADD;\n \tint ok_to_replace = option & ADD_CACHE_OK_TO_REPLACE;\n+\tint ok_to_replace_merged = option & ADD_CACHE_OK_TO_REPLACE_MERGED;\n \tint skip_df_check = option & ADD_CACHE_SKIP_DFCHECK;\n \tint new_only = option & ADD_CACHE_NEW_ONLY;\n \tint replaced = 0;\n@@ -955,7 +956,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n \t */\n \tif (pos < istate->cache_nr && ce_stage(ce) == 0) {\n \t\twhile (ce_same_name(istate->cache[pos], ce)) {\n-\t\t\tif (replaced)\n+\t\t\tif (replaced && !ok_to_replace_merged)\n \t\t\t\tdie(\"Merged and unmerged entries found for \"\n \t\t\t\t \"'%s', check 'git ls-files -s \\\"%s\\\"'\",\n \t\t\t\t ce->name, ce->name);\ndiff --git a/t/t9904-unmerged-file-with-merged-entry.sh b/t/t9904-unmerged-file-with-merged-entry.sh\nindex 945bc1c..9138821 100755\n--- a/t/t9904-unmerged-file-with-merged-entry.sh\n+++ b/t/t9904-unmerged-file-with-merged-entry.sh\n@@ -28,11 +28,11 @@ setup_stage_state() {\n \n test_expect_success 'setup - two branches with conflicting files' '\n \tsetup_repository &&\n-\tsetup_stage_state &&\n+\tgit merge master\n \tgit ls-files -s conflict > output &&\n-\ttest_line_count = 4 output &&\n+\ttest_line_count = 3 output &&\n \tgit ls-files -s conflict2 > output &&\n-\ttest_line_count = 4 output &&\n+\ttest_line_count = 3 output &&\n \trm output\n '\n \n@@ -83,4 +83,12 @@ test_expect_success 'git reset --hard' '\n \ttest_cmp expected current\n '\n \n+test_expect_success 'git update-index --cacheinfo to select a stage to use' '\n+\tsetup_stage_state &&\n+\tgit cat-file blob :1:conflict > conflict &&\n+\tgit update-index --cacheinfo 100644,`git hash-object conflict`,conflict\n+\tgit ls-files -s conflict > output &&\n+\ttest_line_count = 1 output\n+'\n+\n test_done\n-- \n2.0.4.4.gaf54b2b\n"},{"id":"248032","messageId":"xmqq8umiub9x.fsf@gitster.dls.corp.google.com","threadId":"37336","inReplyTo":"7782c581021d529947a1385939231eeb145d150e.1408533065.git.jsorianopastor@gmail.com","subject":"Re: [PATCH 3/4] Added tests for the case of merged and unmerged entries for the same file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-20T21:00:26Z","receivedAt":"2014-08-20T21:00:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jaime Soriano Pastor writes:\n\n> Signed-off-by: Jaime Soriano Pastor \n> ---\n> t/t9904-unmerged-file-with-merged-entry.sh | 86 ++++++++++++++++++++++++++++++\n\nIsn't this number already used for another test? A test on the\nindex probably belongs to t2XXX or t3XXX family.\n\n> 1 file changed, 86 insertions(+)\n> create mode 100755 t/t9904-unmerged-file-with-merged-entry.sh\n>\n> diff --git a/t/t9904-unmerged-file-with-merged-entry.sh b/t/t9904-unmerged-file-with-merged-entry.sh\n> new file mode 100755\n> index 0000000..945bc1c\n> --- /dev/null\n> +++ b/t/t9904-unmerged-file-with-merged-entry.sh\n> @@ -0,0 +1,86 @@\n> +#!/bin/sh\n> +\n> +test_description='Operations with unmerged files with merged entries'\n> +\n> +. ./test-lib.sh\n> +\n> +setup_repository() {\n> +\ttest_commit A conflict A\n> +\ttest_commit A conflict2 A2 branchbase\n> +\ttest_commit B conflict B\n> +\ttest_commit B conflict2 B2\n> +\tgit checkout branchbase -b branch1\n> +\ttest_commit C conflict C\n> +\ttest_commit C conflict2 C2\n> +\ttest_commit something otherfile otherfile\n> +}\n\nNo error is checked here?\n\n> +setup_stage_state() {\n> +\tgit checkout -f HEAD\n> +\t{\n> +\t\tgit ls-files -s conflict conflict2\n> +\t\tgit merge master > /dev/null\n> +\t\tgit ls-files -s conflict conflict2\n> +\t} > index\n\nNo error is checked here?\n\nStyle: no SP between redirection operator and its target, i.e.\n\n\tgit merge master >/dev/null\n\t{ ... } >index\n\n> +\tcat index | git update-index --index-info\n\nDo not cat a single file into a pipeline, i.e.\n\n\tgit update-index --index-info writes:\n\n> Subject: Re: [PATCH 4/4] git update-index --cacheinfo can be used to select\n> a stage when there are merged and unmerged entries\n\nHmph, what does it even mean? Shared with your [1/4] is that it is\nunclear if you are stating an existing problem to be fixed or\ndescribing the desired end result.\n\nAlso \"update-index --cacheinfo\" is not about \"selecting\" but is\nabout stuffing an entry to the index, so \"can be used to select\"\nis doubly puzzling...\n\n> ...\n> +test_expect_success 'git update-index --cacheinfo to select a stage to use' '\n> +\tsetup_stage_state &&\n> +\tgit cat-file blob :1:conflict > conflict &&\n\nStyle: no SP between redirection and its target.\n\n> +\tgit update-index --cacheinfo 100644,`git hash-object conflict`,conflict\n\nStyle: we prefer $() over ``\n\n> +\tgit ls-files -s conflict > output &&\n> +\ttest_line_count = 1 output\n\nIs \"we have only one line\" the only thing we care about? Don't we\nwant to check which stage the entry is at?\n\n> +'\n> +\n> test_done\n"},{"id":"248037","messageId":"xmqqr40ast2g.fsf@gitster.dls.corp.google.com","threadId":"37336","inReplyTo":"cover.1408533065.git.jsorianopastor@gmail.com","subject":"Re: [PATCH 0/4] Handling unmerged files with merged entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-20T22:19:03Z","receivedAt":"2014-08-20T22:19:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jaime Soriano Pastor writes:\n\n> New approach for the case of finding unmerged files with merged entries\n> in the index.\n> After some discussion the solution tries to:\n> - Avoid the problems with infinite loops in this case.\n> - Provide better information to the user in the commands affected.\n> - Make sure there are ways to clean the index.\n> - Provide also a way to specifically recover each one of the files with\n> this problem.\n>\n> With these patches the behaviour of these commands (for this case) change:\n> - git reset is able to finish, cleaning the index, but warning out the\n> information about the removed stages.\n> - git merge is able to finish, reporting that there is a merge in progress as\n> usual, it also warns about the unmerged files with merged entries.\n> - git add fails when this case happens, telling the user to check the state\n> with 'git ls-files -s', before, it did nothing. The same with git commit -a.\n> - git update-index --cacheinfo can be used to select an specific staged\n> version to resolve the conflict, without the need of reseting the working\n> copy. It did nothing before.\n>\n> Tests added for these cases. Rest of the tests remain unchanged and pass too.\n\nThanks.\n\nAfter looking at what you did in 1/4, I started to wonder if we can\nsolve this in add_index_entry_with_check() in a less intrusive way.\nWhen we call the function with a stage #0 entry, we are telling the\nindex that any entry in higher stage for the same path must\ndisappear. Since the current implementation of the function assumes\nthat the index is not corrupt in this particular way to have both\nmerged and unmerged entries for the same path, it fails to remove\nthe higher stage entries. If we fix the function, wouldn't it make\nyour 1/4 unnecessary? Read-only operations such as \"ls-files -s\"\nwould not call add_index_entry() so diagnostic tools would not be\naffected even with such a fix.\n\n... which may look something like the one attached at the end.\n\nBut then it made me wonder even more.\n\nThere are other ways a piece of software can leave a corrupt index\nfor us to read from. Your fix, or the simpler one I suggested for\nthat matter, would still assume that the index entries are in the\nsorted order, and a corrupt index that does not sort its entries\ncorrectly will cause us to behave in an undefined way. At some\npoint we should draw a line and say \"Your index is hopelessly\ncorrupt.\", send it back to whatever broken software that originally\nwrote such a mess and have the user use that software to fix the\ncorrupt index up before talking to us.\n\nFor that, we need to catch an index whose entries are not sorted and\nerror out, perhaps when read_index_from() iterates over the mmapped\nindex entries. We can even draw that \"hopelessly corrupt\" line\nabove the breakage you are addressing and add a check to make sure\nno path has both merged and unmerged entries to the same check to\nmake it error out.\n\nI suspect that such a \"detect and error out\" may be sufficient and\nalso may be more robust than the approach that assumes that a\nbreakage is only to have both merged and unmerged entries for the\nsame path, the entries are still correctly sorted.\n\n\n read-cache.c | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 7f5645e..56006a3 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -943,9 +943,16 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n \tif (pos >= 0) {\n \t\tif (!new_only)\n \t\t\treplace_index_entry(istate, pos, ce);\n-\t\treturn 0;\n+\t\t/*\n+\t\t * ... but protect ourselves from a corrupt index\n+\t\t * that has an unmerged entry for the same path.\n+\t\t */\n+\t\tif (istate->cache_nr <= pos + 1 ||\n+\t\t !ce_same_name(ce, istate->cache[pos + 1]))\n+\t\t\treturn 0;\n+\t} else {\n+\t\tpos = -pos-1;\n \t}\n-\tpos = -pos-1;\n \n \t/*\n \t * Inserting a merged entry (\"stage 0\") into the index\n\n\n\n\n\n[Footnote]\n"},{"id":"248055","messageId":"CAPuZ2NEksZzt29XpAaszK8oc4iXH4OTGzRSsHUrtwgrv6bU_0Q@mail.gmail.com","threadId":"37336","inReplyTo":"xmqqr40ast2g.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/4] Handling unmerged files with merged entries","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-21T13:42:46Z","receivedAt":"2014-08-21T13:42:46Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"Good points.\n\nOn Thu, Aug 21, 2014 at 12:19 AM, Junio C Hamano wrote:\n> After looking at what you did in 1/4, I started to wonder if we can\n> solve this in add_index_entry_with_check() in a less intrusive way.\n> When we call the function with a stage #0 entry, we are telling the\n> index that any entry in higher stage for the same path must\n> disappear. Since the current implementation of the function assumes\n> that the index is not corrupt in this particular way to have both\n> merged and unmerged entries for the same path, it fails to remove\n> the higher stage entries. If we fix the function, wouldn't it make\n> your 1/4 unnecessary? Read-only operations such as \"ls-files -s\"\n> would not call add_index_entry() so diagnostic tools would not be\n> affected even with such a fix.\n>\nAnother thing that is done in 1/4 is to get rid of the call to\nindex_name_pos, that can lead to infinite loops depending on what the\nprevious add_index_entry call does as we have seen, and I wonder why\nis it really needed, specially if we guarantee the order in the index.\n\n> ... which may look something like the one attached at the end.\n>\nAnd it would be more in the line of my first patch.\n\n> But then it made me wonder even more.\n>\n> There are other ways a piece of software can leave a corrupt index\n> for us to read from. Your fix, or the simpler one I suggested for\n> that matter, would still assume that the index entries are in the\n> sorted order, and a corrupt index that does not sort its entries\n> correctly will cause us to behave in an undefined way. At some\n> point we should draw a line and say \"Your index is hopelessly\n> corrupt.\", send it back to whatever broken software that originally\n> wrote such a mess and have the user use that software to fix the\n> corrupt index up before talking to us.\n>\nTrue.\n\n> For that, we need to catch an index whose entries are not sorted and\n> error out, perhaps when read_index_from() iterates over the mmapped\n> index entries. We can even draw that \"hopelessly corrupt\" line\n> above the breakage you are addressing and add a check to make sure\n> no path has both merged and unmerged entries to the same check to\n> make it error out.\n>\n> I suspect that such a \"detect and error out\" may be sufficient and\n> also may be more robust than the approach that assumes that a\n> breakage is only to have both merged and unmerged entries for the\n> same path, the entries are still correctly sorted.\n>\nAgree. I have prepared an initial patch for this to discuss, but\nadding checks in read_index_from() can add a small(?) penalization to\nall git operations, specially with big indexes.\nAnd it wouldn't probably allow the user to fix the repository using\ngit commands (unless we only warn instead of die depending on the\nthing that is broken).\n"},{"id":"248056","messageId":"1408628606-12975-1-git-send-email-jsorianopastor@gmail.com","threadId":"37336","inReplyTo":"CAPuZ2NEksZzt29XpAaszK8oc4iXH4OTGzRSsHUrtwgrv6bU_0Q@mail.gmail.com","subject":"[PATCH] Check order when reading index","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-21T13:43:26Z","receivedAt":"2014-08-21T13:43:26Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"Signed-off-by: Jaime Soriano Pastor \n---\n read-cache.c | 18 ++++++++++++++++++\n 1 file changed, 18 insertions(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 7f5645e..e117d3a 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1438,6 +1438,21 @@ static struct cache_entry *create_from_disk(struct ondisk_cache_entry *ondisk,\n \treturn ce;\n }\n \n+void check_next_ce(struct cache_entry *ce, struct cache_entry *next_ce) {\n+\tif (!ce || !next_ce)\n+\t\treturn;\n+\tif (cache_name_compare(ce->name, ce_namelen(ce),\n+\t\t\t\t\t\t next_ce->name, ce_namelen(next_ce)) > 1)\n+\t\tdie(\"Unordered stage entries in index\");\n+\tif (ce_same_name(ce, next_ce)) {\n+\t\tif (!ce_stage(ce))\n+\t\t\tdie(\"Multiple stage entries for merged file '%s'\",\n+\t\t\t\tce->name);\n+\t\tif (ce_stage(ce) >= ce_stage(next_ce))\n+\t\t\tdie(\"Unordered stage entries for '%s'\", ce->name);\n+\t}\n+}\n+\n /* remember to discard_cache() before reading a different cache! */\n int read_index_from(struct index_state *istate, const char *path)\n {\n@@ -1499,6 +1514,9 @@ int read_index_from(struct index_state *istate, const char *path)\n \t\tce = create_from_disk(disk_ce, &consumed, previous_name);\n \t\tset_index_entry(istate, i, ce);\n \n+\t\tif (i > 0)\n+\t\t\tcheck_next_ce(istate->cache[i-1], ce);\n+\n \t\tsrc_offset += consumed;\n \t}\n \tstrbuf_release(&previous_name_buf);\n-- \n2.0.4.dirty\n"},{"id":"248057","messageId":"CAPuZ2NGfKtc3+JyywNeuEk5Yx_mTAM6QLs1S5p49TWxkUe6_SQ@mail.gmail.com","threadId":"37336","inReplyTo":"1408628606-12975-1-git-send-email-jsorianopastor@gmail.com","subject":"Re: [PATCH] Check order when reading index","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-21T13:49:02Z","receivedAt":"2014-08-21T13:49:02Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"On Thu, Aug 21, 2014 at 3:43 PM, Jaime Soriano Pastor\n wrote:\n> + if (!ce_stage(ce))\n> + die(\"Multiple stage entries for merged file '%s'\",\n> + ce->name);\n\nThis case can be provoked by \"git update-index --index-info\" as shown\nin the patch with the added test, maybe it should be only a warning.\nAnd add too some variation of the patches in this thread to make the\nsame command able to fix the situation.\n"},{"id":"248058","messageId":"CAPuZ2NGUTw8NLTyJSgphh=XBgxy4eTFQLF5yxGD3GgpKbc-Vog@mail.gmail.com","threadId":"37336","inReplyTo":"xmqq8umiub9x.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/4] Added tests for the case of merged and unmerged entries for the same file","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-21T13:51:42Z","receivedAt":"2014-08-21T13:51:42Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"On Wed, Aug 20, 2014 at 11:00 PM, Junio C Hamano wrote:\n> Jaime Soriano Pastor writes:\n>\n>> Signed-off-by: Jaime Soriano Pastor \n>> ---\n>> t/t9904-unmerged-file-with-merged-entry.sh | 86 ++++++++++++++++++++++++++++++\n>\n> Isn't this number already used for another test? A test on the\n> index probably belongs to t2XXX or t3XXX family.\n>\nUmm, I though this test number was free, I just added it to the last+1\nposition, if I finally add a test I'll take this into account. Thanks.\n\n>> 1 file changed, 86 insertions(+)\n>> create mode 100755 t/t9904-unmerged-file-with-merged-entry.sh\n>>\n>> diff --git a/t/t9904-unmerged-file-with-merged-entry.sh b/t/t9904-unmerged-file-with-merged-entry.sh\n>> new file mode 100755\n>> index 0000000..945bc1c\n>> --- /dev/null\n>> +++ b/t/t9904-unmerged-file-with-merged-entry.sh\n>> @@ -0,0 +1,86 @@\n>> +#!/bin/sh\n>> +\n>> +test_description='Operations with unmerged files with merged entries'\n>> +\n>> +. ./test-lib.sh\n>> +\n>> +setup_repository() {\n>> + test_commit A conflict A\n>> + test_commit A conflict2 A2 branchbase\n>> + test_commit B conflict B\n>> + test_commit B conflict2 B2\n>> + git checkout branchbase -b branch1\n>> + test_commit C conflict C\n>> + test_commit C conflict2 C2\n>> + test_commit something otherfile otherfile\n>> +}\n>\n> No error is checked here?\n>\nThis is only a helper function for setup, not a test itself.\n\n>> +setup_stage_state() {\n>> + git checkout -f HEAD\n>> + {\n>> + git ls-files -s conflict conflict2\n>> + git merge master > /dev/null\n>> + git ls-files -s conflict conflict2\n>> + } > index\n>\n> No error is checked here?\n>\nSame here.\n\n> Style: no SP between redirection operator and its target, i.e.\n>\n> git merge master >/dev/null\n> { ... } >index\n>\n>> + cat index | git update-index --index-info\n>\n> Do not cat a single file into a pipeline, i.e.\n>\n> git update-index --index-info \nTrue :) Thanks.\n"},{"id":"248059","messageId":"CAPuZ2NHMjo996rqNPhpt6MJ5bCbT_zUCfjQUVmDip4dmqiP+yQ@mail.gmail.com","threadId":"37336","inReplyTo":"xmqq38cquavu.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 4/4] git update-index --cacheinfo can be used to select a stage when there are merged and unmerged entries","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-21T13:57:01Z","receivedAt":"2014-08-21T13:57:01Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"On Wed, Aug 20, 2014 at 11:08 PM, Junio C Hamano wrote:\n> Jaime Soriano Pastor writes:\n>\n>> Subject: Re: [PATCH 4/4] git update-index --cacheinfo can be used to select\n>> a stage when there are merged and unmerged entries\n>\n> Hmph, what does it even mean? Shared with your [1/4] is that it is\n> unclear if you are stating an existing problem to be fixed or\n> describing the desired end result.\n>\n> Also \"update-index --cacheinfo\" is not about \"selecting\" but is\n> about stuffing an entry to the index, so \"can be used to select\"\n> is doubly puzzling...\n>\nWell, somehow I understand \"update-index --cacheinfo\" as a low level\nversion of add. I was trying to explain the desired end result, yes.\n\n>> ...\n>> +test_expect_success 'git update-index --cacheinfo to select a stage to use' '\n>> + setup_stage_state &&\n>> + git cat-file blob :1:conflict > conflict &&\n>\n> Style: no SP between redirection and its target.\n>\nOk.\n\n>> + git update-index --cacheinfo 100644,`git hash-object conflict`,conflict\n>\n> Style: we prefer $() over ``\n>\nOk.\n\n>> + git ls-files -s conflict > output &&\n>> + test_line_count = 1 output\n>\n> Is \"we have only one line\" the only thing we care about? Don't we\n> want to check which stage the entry is at?\n>\nYes, it'd be better.\n\nThanks.\n"},{"id":"248060","messageId":"CACsJy8A-aMmU0oYgo=eR21CZT6X__-Mnrg6O74sCQ9Ta4QPWPQ@mail.gmail.com","threadId":"37336","inReplyTo":"1408628606-12975-1-git-send-email-jsorianopastor@gmail.com","subject":"Re: [PATCH] Check order when reading index","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-08-21T13:59:45Z","receivedAt":"2014-08-21T13:59:45Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Aug 21, 2014 at 8:43 PM, Jaime Soriano Pastor\n wrote:\n> @@ -1499,6 +1514,9 @@ int read_index_from(struct index_state *istate, const char *path)\n> ce = create_from_disk(disk_ce, &consumed, previous_name);\n> set_index_entry(istate, i, ce);\n>\n> + if (i > 0)\n> + check_next_ce(istate->cache[i-1], ce);\n> +\n> src_offset += consumed;\n> }\n> strbuf_release(&previous_name_buf);\n\nIt may be nice to save the \"good index\" stamp as an index extension so\nwe don't have to check this over and over. I'm thinking about big\nindexes where compare cost might matter (I'm not so sure yet, will do\nsome testing when I have time).\n-- \nDuy\n"},{"id":"248074","messageId":"53F63D2F.9060704@kdbg.org","threadId":"37336","inReplyTo":"xmqqr40ast2g.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/4] Handling unmerged files with merged entries","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-08-21T18:40:47Z","receivedAt":"2014-08-21T18:40:47Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 21.08.2014 00:19, schrieb Junio C Hamano:\n> For that, we need to catch an index whose entries are not sorted and\n> error out, perhaps when read_index_from() iterates over the mmapped\n> index entries. We can even draw that \"hopelessly corrupt\" line\n> above the breakage you are addressing and add a check to make sure\n> no path has both merged and unmerged entries to the same check to\n> make it error out.\n\nExcept that we can't declare an index with both merged and unmerged\nentries as \"hopelessly corrupt, return to sender\" when it's dead easy to\ngenerate with the git tool set:\n\n >x\n name=$(git hash-object -w x)\n for i in 0 1 2 3; do printf '100644 %s %d\\tx\\n' $name $i; done |\n git update-index --index-info\n\n-- Hannes\n"},{"id":"248075","messageId":"xmqq38cpsmli.fsf@gitster.dls.corp.google.com","threadId":"37336","inReplyTo":"1408628606-12975-1-git-send-email-jsorianopastor@gmail.com","subject":"Re: [PATCH] Check order when reading index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-21T18:51:05Z","receivedAt":"2014-08-21T18:51:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jaime Soriano Pastor writes:\n\n> Signed-off-by: Jaime Soriano Pastor \n> ---\n> read-cache.c | 18 ++++++++++++++++++\n> 1 file changed, 18 insertions(+)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index 7f5645e..e117d3a 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1438,6 +1438,21 @@ static struct cache_entry *create_from_disk(struct ondisk_cache_entry *ondisk,\n> \treturn ce;\n> }\n> \n> +void check_next_ce(struct cache_entry *ce, struct cache_entry *next_ce) {\n\nHave opening brace for the function on its own line, i.e.\n\n\tvoid check_next_ce(struct cache_entry *ce, struct cache_entry *next_ce)\n\t{\n\nThe function might be misnamed (see below), though.\n\n> +\tif (!ce || !next_ce)\n> +\t\treturn;\n\nHmph, would it be either a programming error or a corrupt index\ninput to see a NULL in either of these variables?\n\n> +\tif (cache_name_compare(ce->name, ce_namelen(ce),\n> +\t\t\t\t\t\t next_ce->name, ce_namelen(next_ce)) > 1)\n\nAn odd indentation that is overly deep to make it hard to read.\n\n\tif (cache_name_compare(ce->name, ce_namelen(ce),\n\t\t\t next_ce->name, ce_namelen(next_ce)) > 1)\n\nshould be sufficient (replacing 7-SP before next_ce with a HT is OK\nif the existing code nearby does so).\n\nWhat is the significance of the return value from cache_name_compare()\nthat is strictly greater than 1 (as opposed to merely \"is it positive?\")?\n\nPerhaps you want something that is modeled after ce_same_name() that\nignores the stage, i.e.\n\n\tint ce_name_compare(const struct cache_entry *a, const struct cache_entry *b)\n\t{\n\t\treturn strcmp(a->ce_name, b->ce_name);\n\t}\n\nwithout reimplementing the cache-name-compare() as\n\n\tint bad_ce_same_name(const struct cache_entry *a, const struct cache_entry *b)\n\t{\n \treturn !ce_same_name(a, b);\n\t}\n\nto keep the \"two names with different length could never be the\nsame\" optimization.\n\n\t- if (0 <= ce_name_compare(ce, next)) then the names are not sorted\n\n - if (!stage(ce) && !name_compare(ce, next)) then the merged\n entry 'ce' is not the only entry for the path\n\n\n\n> +\t\tdie(\"Unordered stage entries in index\");\n> +\tif (ce_same_name(ce, next_ce)) {\n> +\t\tif (!ce_stage(ce))\n> +\t\t\tdie(\"Multiple stage entries for merged file '%s'\",\n> +\t\t\t\tce->name);\n\n> +\t\tif (ce_stage(ce) >= ce_stage(next_ce))\n> +\t\t\tdie(\"Unordered stage entries for '%s'\", ce->name);\n> +\t}\n> +}\n> +\n> /* remember to discard_cache() before reading a different cache! */\n> int read_index_from(struct index_state *istate, const char *path)\n> {\n> @@ -1499,6 +1514,9 @@ int read_index_from(struct index_state *istate, const char *path)\n> \t\tce = create_from_disk(disk_ce, &consumed, previous_name);\n> \t\tset_index_entry(istate, i, ce);\n> \n> +\t\tif (i > 0)\n> +\t\t\tcheck_next_ce(istate->cache[i-1], ce);\n\nHave a SP each on both sides of binary operator \"-\".\n\nJudging from the way this helper function is used, it looks like\ncheck_with_previous_ce() is a more appropriate name. After all, you\nare not checking the next ce which you haven't even created yet ;-)\n\n\nThanks.\n"},{"id":"248084","messageId":"xmqqbnrdqyex.fsf@gitster.dls.corp.google.com","threadId":"37336","inReplyTo":"53F63D2F.9060704@kdbg.org","subject":"Re: [PATCH 0/4] Handling unmerged files with merged entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-21T22:18:46Z","receivedAt":"2014-08-21T22:18:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 21.08.2014 00:19, schrieb Junio C Hamano:\n>> For that, we need to catch an index whose entries are not sorted and\n>> error out, perhaps when read_index_from() iterates over the mmapped\n>> index entries. We can even draw that \"hopelessly corrupt\" line\n>> above the breakage you are addressing and add a check to make sure\n>> no path has both merged and unmerged entries to the same check to\n>> make it error out.\n>\n> Except that we can't declare an index with both merged and unmerged\n> entries as \"hopelessly corrupt, return to sender\" when it's dead easy to\n> generate with the git tool set:\n>\n> >x\n> name=$(git hash-object -w x)\n> for i in 0 1 2 3; do printf '100644 %s %d\\tx\\n' $name $i; done |\n> git update-index --index-info\n\nBecause hash-object and update-index deliberately have these holes\nto allow us (read: me ;-) to easily experiment new and/or unallowed\nformats, I wouldn't take that as a serious objection. It is dead\neasy to corrupt your repository or lose your data by /bin/rm, too\n;-)\n"},{"id":"248085","messageId":"xmqq7g21qyac.fsf@gitster.dls.corp.google.com","threadId":"37336","inReplyTo":"CAPuZ2NGUTw8NLTyJSgphh=XBgxy4eTFQLF5yxGD3GgpKbc-Vog@mail.gmail.com","subject":"Re: [PATCH 3/4] Added tests for the case of merged and unmerged entries for the same file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-21T22:21:31Z","receivedAt":"2014-08-21T22:21:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jaime Soriano Pastor writes:\n\n> On Wed, Aug 20, 2014 at 11:00 PM, Junio C Hamano wrote:\n>> Jaime Soriano Pastor writes:\n>>\n>>> Signed-off-by: Jaime Soriano Pastor \n>>> ---\n>>> t/t9904-unmerged-file-with-merged-entry.sh | 86 ++++++++++++++++++++++++++++++\n>>\n>> Isn't this number already used for another test? A test on the\n>> index probably belongs to t2XXX or t3XXX family.\n>>\n> Umm, I though this test number was free, I just added it to the last+1\n> position, if I finally add a test I'll take this into account. Thanks.\n\nPlease check t/README for classes of features and appropriate first\ndigit; also do not forget that there are topics by other people in\nflight and you may need to at least check with the tip of the 'pu'\nbranch.\n\nThanks.\n\n>>> 1 file changed, 86 insertions(+)\n>>> create mode 100755 t/t9904-unmerged-file-with-merged-entry.sh\n>>>\n>>> diff --git a/t/t9904-unmerged-file-with-merged-entry.sh b/t/t9904-unmerged-file-with-merged-entry.sh\n>>> new file mode 100755\n>>> index 0000000..945bc1c\n>>> --- /dev/null\n>>> +++ b/t/t9904-unmerged-file-with-merged-entry.sh\n>>> @@ -0,0 +1,86 @@\n>>> +#!/bin/sh\n>>> +\n>>> +test_description='Operations with unmerged files with merged entries'\n>>> +\n>>> +. ./test-lib.sh\n>>> +\n>>> +setup_repository() {\n>>> +...\n>>> +}\n>>\n>> No error is checked here?\n>>\n> This is only a helper function for setup, not a test itself.\n\nSo what? If the set-up fails, we would want\n\n $ sh tXXXX-my-test.sh -i\n\nto immediately stop without going further.\n"},{"id":"248212","messageId":"1408903047-8302-1-git-send-email-jsorianopastor@gmail.com","threadId":"37336","inReplyTo":"xmqq38cpsmli.fsf@gitster.dls.corp.google.com","subject":"[PATCH 1/2] Check order when reading index","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-24T17:57:26Z","receivedAt":"2014-08-24T17:57:26Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"Signed-off-by: Jaime Soriano Pastor \n---\n read-cache.c | 18 ++++++++++++++++++\n 1 file changed, 18 insertions(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 7f5645e..c1a9619 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1438,6 +1438,21 @@ static struct cache_entry *create_from_disk(struct ondisk_cache_entry *ondisk,\n \treturn ce;\n }\n \n+void check_ce_order(struct cache_entry *ce, struct cache_entry *next_ce)\n+{\n+\tint name_compare = strcmp(ce->name, next_ce->name);\n+\tif (0 < name_compare)\n+\t\tdie(\"Unordered stage entries in index\");\n+\tif (!name_compare) {\n+\t\tif (!ce_stage(ce))\n+\t\t\tdie(\"Multiple stage entries for merged file '%s'\",\n+\t\t\t\tce->name);\n+\t\tif (ce_stage(ce) >= ce_stage(next_ce))\n+\t\t\tdie(\"Unordered stage entries for '%s'\",\n+\t\t\t\tce->name);\n+\t}\n+}\n+\n /* remember to discard_cache() before reading a different cache! */\n int read_index_from(struct index_state *istate, const char *path)\n {\n@@ -1499,6 +1514,9 @@ int read_index_from(struct index_state *istate, const char *path)\n \t\tce = create_from_disk(disk_ce, &consumed, previous_name);\n \t\tset_index_entry(istate, i, ce);\n \n+\t\tif (i > 0)\n+\t\t\tcheck_ce_order(istate->cache[i - 1], ce);\n+\n \t\tsrc_offset += consumed;\n \t}\n \tstrbuf_release(&previous_name_buf);\n-- \n2.0.4.1.g0b8a4f9.dirty\n"},{"id":"248213","messageId":"1408903047-8302-2-git-send-email-jsorianopastor@gmail.com","threadId":"37336","inReplyTo":"1408903047-8302-1-git-send-email-jsorianopastor@gmail.com","subject":"[PATCH 2/2] Loop index increases monotonically when reading unmerged entries","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-24T17:57:27Z","receivedAt":"2014-08-24T17:57:27Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"Signed-off-by: Jaime Soriano Pastor \n---\n read-cache.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex c1a9619..3d70386 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1971,7 +1971,6 @@ int read_index_unmerged(struct index_state *istate)\n \t\tif (add_index_entry(istate, new_ce, 0))\n \t\t\treturn error(\"%s: cannot drop to stage #0\",\n \t\t\t\t new_ce->name);\n-\t\ti = index_name_pos(istate, new_ce->name, len);\n \t}\n \treturn unmerged;\n }\n-- \n2.0.4.1.g0b8a4f9.dirty\n"},{"id":"248214","messageId":"CAPuZ2NEPcRm4S8m76kJ+8L7zR2RiVwu3Z6bNbZgfzAcVTYgOKA@mail.gmail.com","threadId":"37336","inReplyTo":"1408903047-8302-2-git-send-email-jsorianopastor@gmail.com","subject":"Re: [PATCH 2/2] Loop index increases monotonically when reading unmerged entries","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-24T18:04:22Z","receivedAt":"2014-08-24T18:04:22Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"I think this line is dangerous, if add_cache_entry is not able to\nremove higher-stages it will be looping forever, as happens in the\ncase of this thread.\nI cannot see why it's even needed, and removing it doesn't break any test.\n\nOn Sun, Aug 24, 2014 at 7:57 PM, Jaime Soriano Pastor\n wrote:\n> Signed-off-by: Jaime Soriano Pastor \n> ---\n> read-cache.c | 1 -\n> 1 file changed, 1 deletion(-)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index c1a9619..3d70386 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1971,7 +1971,6 @@ int read_index_unmerged(struct index_state *istate)\n> if (add_index_entry(istate, new_ce, 0))\n> return error(\"%s: cannot drop to stage #0\",\n> new_ce->name);\n> - i = index_name_pos(istate, new_ce->name, len);\n> }\n> return unmerged;\n> }\n> --\n> 2.0.4.1.g0b8a4f9.dirty\n>\n"},{"id":"248238","messageId":"xmqq1ts4o5qw.fsf@gitster.dls.corp.google.com","threadId":"37336","inReplyTo":"CAPuZ2NEPcRm4S8m76kJ+8L7zR2RiVwu3Z6bNbZgfzAcVTYgOKA@mail.gmail.com","subject":"Re: [PATCH 2/2] Loop index increases monotonically when reading unmerged entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-25T17:09:59Z","receivedAt":"2014-08-25T17:09:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jaime Soriano Pastor writes:\n\n> I think this line is dangerous, if add_cache_entry is not able to\n> remove higher-stages it will be looping forever, as happens in the\n> case of this thread.\n> I cannot see why it's even needed, and removing it doesn't break any test.\n>\n> On Sun, Aug 24, 2014 at 7:57 PM, Jaime Soriano Pastor\n> wrote:\n>> Signed-off-by: Jaime Soriano Pastor \n>> ---\n>> read-cache.c | 1 -\n>> 1 file changed, 1 deletion(-)\n>>\n>> diff --git a/read-cache.c b/read-cache.c\n>> index c1a9619..3d70386 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -1971,7 +1971,6 @@ int read_index_unmerged(struct index_state *istate)\n>> if (add_index_entry(istate, new_ce, 0))\n>> return error(\"%s: cannot drop to stage #0\",\n>> new_ce->name);\n>> - i = index_name_pos(istate, new_ce->name, len);\n\nI think the original idea was that regardless of how many entries\nwith the same name were removed because of the replacement (or\naddition) of \"new_ce\", by making \"i\" point at the newly added\n\"new_ce\", we would make sure that the loop will continue from the\nnext entry. The if/return expected that add_index_entry() will get\nrid of all the other entries with the same name as \"new_ce\" has or it\nwill return an error.\n\nWithout the \"bug\" in add_index_entry(), because \"new_ce\" always has\nthe same name as \"ce\", the entry we found at \"i\" by the loop, we know\nthat index_name_pos() will give the same \"i\" we already have, so\nremoving this line should be a no-op.\n\nNow, add_index_entry() in your case did not notice that it failed to\nremove all other entries with the same name as \"new_ce\", resulting\nin your \"looping forever\". Among the \"merged and unmerged entries\nwith the same name exists in the index file\" class of index file\ncorruption, we could treat the \"merged and unmerged entries with the\nsame name not just exists but next to each other, unmerged ones\ncoming immediately after merged one\" case specially (i.e. declaring\nthat it is more likely for a broken software to leave both next to\neach other than otherwise) and try to accomodate it as your original\npatch did. I am not absolutely sure if such a special case is worth\nit, and with your updated \"[1/2] read_index_from(): check order of\nentries when reading index\" we will not be doing so, which is good.\n\nWith that safety in place, the \"bug\" in add_index_entry() will\ndisappear; it is safe not to adjust \"i\" by calling index_name_pos()\nand this patch, \"[2/2] read_index_unmerged(): remove unnecessary\nloop index adjustment\", will be a good thing to do.\n\nThanks.\n\n>> }\n>> return unmerged;\n>> }\n>> --\n>> 2.0.4.1.g0b8a4f9.dirty\n>>\n"},{"id":"248241","messageId":"xmqqvbpgmqmh.fsf@gitster.dls.corp.google.com","threadId":"37336","inReplyTo":"1408903047-8302-1-git-send-email-jsorianopastor@gmail.com","subject":"Re: [PATCH 1/2] Check order when reading index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-25T17:21:58Z","receivedAt":"2014-08-25T17:21:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jaime Soriano Pastor writes:\n\n> Subject: Re: [PATCH 1/2] Check order when reading index\n\nPlease be careful when crafting the commit title. This single line\nwill be the only one that readers will have to identify the change\namong hundreds of entries in \"git shortlog\" output when trying to\nsee what kind of change went into the project during the given\nperiod. Something like:\n\n read_index_from(): catch out of order entries while reading an index file\n\nperhaps?\n\n> Signed-off-by: Jaime Soriano Pastor \n> ---\n> read-cache.c | 18 ++++++++++++++++++\n> 1 file changed, 18 insertions(+)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index 7f5645e..c1a9619 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1438,6 +1438,21 @@ static struct cache_entry *create_from_disk(struct ondisk_cache_entry *ondisk,\n> \treturn ce;\n> }\n> \n> +void check_ce_order(struct cache_entry *ce, struct cache_entry *next_ce)\n\nDoes this have to be global, i.e. not \"static void ...\"?\n\n> +{\n> +\tint name_compare = strcmp(ce->name, next_ce->name);\n> +\tif (0 < name_compare)\n> +\t\tdie(\"Unordered stage entries in index\");\n> +\tif (!name_compare) {\n> +\t\tif (!ce_stage(ce))\n> +\t\t\tdie(\"Multiple stage entries for merged file '%s'\",\n> +\t\t\t\tce->name);\n\nOK. If ce is at stage #0, no other entry can have the same name\nregardless of the stage, and next_ce having the same name violates\nthat rule.\n\n> +\t\tif (ce_stage(ce) >= ce_stage(next_ce))\n> +\t\t\tdie(\"Unordered stage entries for '%s'\",\n> +\t\t\t\tce->name);\n\nNot quite. We do allow multiple higher stage entries; having two or\nmore stage #1 entries is perfectly fine during a merge resolution,\nand both ce and next_ce may be pointing at the stage #1 entries of\nthe same path. Replacing the comparison with \">\" is sufficient, I\nthink.\n\nThanks.\n\n> +\t}\n> +}\n> +\n> /* remember to discard_cache() before reading a different cache! */\n> int read_index_from(struct index_state *istate, const char *path)\n> {\n> @@ -1499,6 +1514,9 @@ int read_index_from(struct index_state *istate, const char *path)\n> \t\tce = create_from_disk(disk_ce, &consumed, previous_name);\n> \t\tset_index_entry(istate, i, ce);\n> \n> +\t\tif (i > 0)\n> +\t\t\tcheck_ce_order(istate->cache[i - 1], ce);\n> +\n> \t\tsrc_offset += consumed;\n> \t}\n> \tstrbuf_release(&previous_name_buf);\n"},{"id":"248260","messageId":"20140825194430.GI30953@peff.net","threadId":"37336","inReplyTo":"xmqqvbpgmqmh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] Check order when reading index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-25T19:44:30Z","receivedAt":"2014-08-25T19:44:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 25, 2014 at 10:21:58AM -0700, Junio C Hamano wrote:\n\n> > +\t\tif (ce_stage(ce) >= ce_stage(next_ce))\n> > +\t\t\tdie(\"Unordered stage entries for '%s'\",\n> > +\t\t\t\tce->name);\n> \n> Not quite. We do allow multiple higher stage entries; having two or\n> more stage #1 entries is perfectly fine during a merge resolution,\n> and both ce and next_ce may be pointing at the stage #1 entries of\n> the same path. Replacing the comparison with \">\" is sufficient, I\n> think.\n\nFor my own curiosity, how do you get into this situation, and what does\nit mean to have multiple stage#1 entries for the same path? What would\n\"git cat-file :1:path\" output?\n\n-Peff\n"},{"id":"248265","messageId":"CAPuZ2NGy7VSW0ibfXwqAndJbDcnByDMJzNZrrLDYt8vDfYPYTg@mail.gmail.com","threadId":"37336","inReplyTo":"xmqqvbpgmqmh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] Check order when reading index","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-25T20:26:49Z","receivedAt":"2014-08-25T20:26:49Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"On Mon, Aug 25, 2014 at 7:21 PM, Junio C Hamano wrote:\n> Jaime Soriano Pastor writes:\n>\n>> Subject: Re: [PATCH 1/2] Check order when reading index\n>\n> Please be careful when crafting the commit title. This single line\n> will be the only one that readers will have to identify the change\n> among hundreds of entries in \"git shortlog\" output when trying to\n> see what kind of change went into the project during the given\n> period. Something like:\n>\n> read_index_from(): catch out of order entries while reading an index file\n>\n> perhaps?\n>\nOk, reprashing it.\n\n>> +void check_ce_order(struct cache_entry *ce, struct cache_entry *next_ce)\n>\n> Does this have to be global, i.e. not \"static void ...\"?\n>\nNot really, changing it to static.\n\n>> + if (ce_stage(ce) >= ce_stage(next_ce))\n>> + die(\"Unordered stage entries for '%s'\",\n>> + ce->name);\n>\n> Not quite. We do allow multiple higher stage entries; having two or\n> more stage #1 entries is perfectly fine during a merge resolution,\n> and both ce and next_ce may be pointing at the stage #1 entries of\n> the same path. Replacing the comparison with \">\" is sufficient, I\n> think.\n>\nOk, but like Jeff, I'm also curious about how to have multiple stage\n#1 entries for the same path.\n\nThanks.\n"},{"id":"248268","messageId":"CAPc5daW-ZckFfhyueNLnPaBeriAmCUVJjFc1cw0O5iRi8F+Kng@mail.gmail.com","threadId":"37336","inReplyTo":"20140825194430.GI30953@peff.net","subject":"Re: [PATCH 1/2] Check order when reading index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-25T20:52:17Z","receivedAt":"2014-08-25T20:52:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Mon, Aug 25, 2014 at 12:44 PM, Jeff King wrote:\n> For my own curiosity, how do you get into this situation, and what does\n> it mean to have multiple stage#1 entries for the same path? What would\n> \"git cat-file :1:path\" output?\n\nThat is how we natively (read: not with the funky \"virtual\" stuff\nmerge-recursive does) express a merge with multiple merge bases.\nYou also should be able to read this in the way how \"git merge\" invokes\nmerge strategies (one or more bases, double-dash and then current\nHEAD and the other branches). I think there are some tests in 3way\nmerge tests that checks what should happen when our HEAD matches\none of the stage #1 while their branch matches a different one of the\nstage #1, too.\n"},{"id":"248293","messageId":"CAPuZ2NHafXQthtuq-RnTvpjVfNPaXHEy8SejuhPEnG+MwCK=sg@mail.gmail.com","threadId":"37336","inReplyTo":"CAPc5daW-ZckFfhyueNLnPaBeriAmCUVJjFc1cw0O5iRi8F+Kng@mail.gmail.com","subject":"Re: [PATCH 1/2] Check order when reading index","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-26T12:08:35Z","receivedAt":"2014-08-26T12:08:35Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"On Mon, Aug 25, 2014 at 10:52 PM, Junio C Hamano wrote:\n> On Mon, Aug 25, 2014 at 12:44 PM, Jeff King wrote:\n>> For my own curiosity, how do you get into this situation, and what does\n>> it mean to have multiple stage#1 entries for the same path? What would\n>> \"git cat-file :1:path\" output?\n>\n> That is how we natively (read: not with the funky \"virtual\" stuff\n> merge-recursive does) express a merge with multiple merge bases.\n> You also should be able to read this in the way how \"git merge\" invokes\n> merge strategies (one or more bases, double-dash and then current\n> HEAD and the other branches). I think there are some tests in 3way\n> merge tests that checks what should happen when our HEAD matches\n> one of the stage #1 while their branch matches a different one of the\n> stage #1, too.\n\nI'm a bit lost with this, conceptually it doesn't seem to be any\nproblem with having multiple merge bases, but I don't manage to\nreproduce it.\nWith \"natively\" do you mean some internal state that is never written\ninto the index? If this were the case then there wouldn't be any\nproblem with the restriction when reading the index file.\n\nI have also tried to reproduce it by directly calling\ngit-merge-recursive and the most I have got is what it seemed to be\nlike a conflict in the stage #1:\n\n$ git ls-files -s\n100644 1036ba5101378f06aa10c5fa249b67e14f83166b 1 conflict\n100644 2638c45f8e7bc5b56f70e92390153572811782c3 2 conflict\n100644 178481050188cf00d7d9cd5a11e43ab8fab9294f 3 conflict\n\n$ git cat-file blob 1036ba5101378f06aa10c5fa249b67e14f83166b\n<<<<<<< Temporary merge branch 1\nG\n=======\nE\nF\n>>>>>>> Temporary merge branch 2\n\nAnd after thinking a bit more about it, I don't see a way to have two\nstage #1 entries for the same path with git commands only. It seems\nthat all entries are added through the add_index_entry_with_check()\nfunction (except maybe the added to the cached tree extension), and\nthis function replaces existing entries if they have the same name and\nstage.\nAlso, all tests pass with the patch, without allowing two entries for\nthe same stage.\n\nSo I'm afraid that I don't fully understand this case :/\n"},{"id":"248296","messageId":"20140826122008.GC29180@peff.net","threadId":"37336","inReplyTo":"CAPuZ2NHafXQthtuq-RnTvpjVfNPaXHEy8SejuhPEnG+MwCK=sg@mail.gmail.com","subject":"Re: [PATCH 1/2] Check order when reading index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-26T12:20:09Z","receivedAt":"2014-08-26T12:20:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 26, 2014 at 02:08:35PM +0200, Jaime Soriano Pastor wrote:\n\n> > That is how we natively (read: not with the funky \"virtual\" stuff\n> > merge-recursive does) express a merge with multiple merge bases.\n> > You also should be able to read this in the way how \"git merge\" invokes\n> > merge strategies (one or more bases, double-dash and then current\n> > HEAD and the other branches). I think there are some tests in 3way\n> > merge tests that checks what should happen when our HEAD matches\n> > one of the stage #1 while their branch matches a different one of the\n> > stage #1, too.\n> \n> I'm a bit lost with this, conceptually it doesn't seem to be any\n> problem with having multiple merge bases, but I don't manage to\n> reproduce it.\n> With \"natively\" do you mean some internal state that is never written\n> into the index? If this were the case then there wouldn't be any\n> problem with the restriction when reading the index file.\n\nFWIW, that was my question on reading Junio's response, too.\n\n> I have also tried to reproduce it by directly calling\n> git-merge-recursive and the most I have got is what it seemed to be\n> like a conflict in the stage #1:\n> \n> $ git ls-files -s\n> 100644 1036ba5101378f06aa10c5fa249b67e14f83166b 1 conflict\n> 100644 2638c45f8e7bc5b56f70e92390153572811782c3 2 conflict\n> 100644 178481050188cf00d7d9cd5a11e43ab8fab9294f 3 conflict\n> \n> $ git cat-file blob 1036ba5101378f06aa10c5fa249b67e14f83166b\n> <<<<<<< Temporary merge branch 1\n> G\n> =======\n> E\n> F\n> >>>>>>> Temporary merge branch 2\n\nYes, I think merge-recursive resolves the earlier merges and then feeds\nthe result into the main merge. And that's how you end up with the\n\"temporary merge branch\" name instead of something useful.\n\nIt might work to have a recursive merge that causes a conflict on path\nX, and then we further need to resolve that conflict. I'm not sure if we\ntry to represent that in the index somehow, or if merge-recursive just\nbails in this case (I didn't try it).\n\n-Peff\n"},{"id":"248319","messageId":"xmqqmwarjiq7.fsf@gitster.dls.corp.google.com","threadId":"37336","inReplyTo":"20140826122008.GC29180@peff.net","subject":"Re: [PATCH 1/2] Check order when reading index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-26T16:53:04Z","receivedAt":"2014-08-26T16:53:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> With \"natively\" do you mean some internal state that is never written\n>> into the index? If this were the case then there wouldn't be any\n>> problem with the restriction when reading the index file.\n>\n> FWIW, that was my question on reading Junio's response, too.\n\nThe current code may not put two stage #1 entries for the same path\nbut allowing such entries was a part of the design from very early\ndays; iow it is a valid index file, unlike the one that has both\nstage #0 and stage #1 for the same path. It is a no-no to reject\nsuch an index as long as we are discussing to add a new code to\ntighten the sanity check on the file content.\n\n>> I have also tried to reproduce it by directly calling\n>> git-merge-recursive and the most I have got is what it seemed to be\n>> like a conflict in the stage #1:\n>> \n>> $ git ls-files -s\n>> 100644 1036ba5101378f06aa10c5fa249b67e14f83166b 1 conflict\n>> 100644 2638c45f8e7bc5b56f70e92390153572811782c3 2 conflict\n>> 100644 178481050188cf00d7d9cd5a11e43ab8fab9294f 3 conflict\n>> \n>> $ git cat-file blob 1036ba5101378f06aa10c5fa249b67e14f83166b\n>> <<<<<<< Temporary merge branch 1\n>> G\n>> =======\n>> E\n>> F\n>> >>>>>>> Temporary merge branch 2\n>\n> Yes, I think merge-recursive resolves the earlier merges and then feeds\n> the result into the main merge. And that's how you end up with the\n> \"temporary merge branch\" name instead of something useful.\n\nYes---that is what I meant by the \"virtual stuff\". Unlike resolve\nwork by Daniel (around Sep 2005 $gmane/8088) that tried to use\nmultiple ancestors directly in a single merge, recursive limits\nitself to repeated use of pairwise merges.\n"},{"id":"248323","messageId":"CAPuZ2NGTQoKnSfeN2zte5=fqswN5PcfAULdFy9WnGWPtc2Zskg@mail.gmail.com","threadId":"37336","inReplyTo":"xmqqmwarjiq7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] Check order when reading index","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-26T17:22:30Z","receivedAt":"2014-08-26T17:22:30Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"On Tue, Aug 26, 2014 at 6:53 PM, Junio C Hamano wrote:\n>\n> Yes---that is what I meant by the \"virtual stuff\". Unlike resolve\n> work by Daniel (around Sep 2005 $gmane/8088) that tried to use\n> multiple ancestors directly in a single merge, recursive limits\n> itself to repeated use of pairwise merges.\n\nOk, I see now. Then what about checking also in check_ce_order() that\nonly stage #1 can be repeated?\n"},{"id":"248330","messageId":"xmqqy4ubi1ty.fsf@gitster.dls.corp.google.com","threadId":"37336","inReplyTo":"CAPuZ2NGTQoKnSfeN2zte5=fqswN5PcfAULdFy9WnGWPtc2Zskg@mail.gmail.com","subject":"Re: [PATCH 1/2] Check order when reading index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-26T17:43:21Z","receivedAt":"2014-08-26T17:43:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jaime Soriano Pastor writes:\n\n> On Tue, Aug 26, 2014 at 6:53 PM, Junio C Hamano wrote:\n>>\n>> Yes---that is what I meant by the \"virtual stuff\". Unlike resolve\n>> work by Daniel (around Sep 2005 $gmane/8088) that tried to use\n>> multiple ancestors directly in a single merge, recursive limits\n>> itself to repeated use of pairwise merges.\n>\n> Ok, I see now. Then what about checking also in check_ce_order() that\n> only stage #1 can be repeated?\n\nWe could use multiple stage #3 entries to natively represent an\noctopus merge in progress if we wanted to. I do not think we want\nto close the door for doing so, unless there is some compelling\nreason.\n\nDoes the current codebase choke with such entries in the index file,\nlike you saw in your index file with both stage #0 and stage #1\nentries?\n"},{"id":"248436","messageId":"1409168892-11345-1-git-send-email-jsorianopastor@gmail.com","threadId":"37336","inReplyTo":"xmqqy4ubi1ty.fsf@gitster.dls.corp.google.com","subject":"[PATCH 1/2] read_index_from(): catch out of order entries when reading an index file","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-27T19:48:11Z","receivedAt":"2014-08-27T19:48:11Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"Signed-off-by: Jaime Soriano Pastor \n---\n read-cache.c | 18 ++++++++++++++++++\n 1 file changed, 18 insertions(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 7f5645e..1cdb762 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1438,6 +1438,21 @@ static struct cache_entry *create_from_disk(struct ondisk_cache_entry *ondisk,\n \treturn ce;\n }\n \n+static void check_ce_order(struct cache_entry *ce, struct cache_entry *next_ce)\n+{\n+\tint name_compare = strcmp(ce->name, next_ce->name);\n+\tif (0 < name_compare)\n+\t\tdie(\"Unordered stage entries in index\");\n+\tif (!name_compare) {\n+\t\tif (!ce_stage(ce))\n+\t\t\tdie(\"Multiple stage entries for merged file '%s'\",\n+\t\t\t\tce->name);\n+\t\tif (ce_stage(ce) > ce_stage(next_ce))\n+\t\t\tdie(\"Unordered stage entries for '%s'\",\n+\t\t\t\tce->name);\n+\t}\n+}\n+\n /* remember to discard_cache() before reading a different cache! */\n int read_index_from(struct index_state *istate, const char *path)\n {\n@@ -1499,6 +1514,9 @@ int read_index_from(struct index_state *istate, const char *path)\n \t\tce = create_from_disk(disk_ce, &consumed, previous_name);\n \t\tset_index_entry(istate, i, ce);\n \n+\t\tif (i > 0)\n+\t\t\tcheck_ce_order(istate->cache[i - 1], ce);\n+\n \t\tsrc_offset += consumed;\n \t}\n \tstrbuf_release(&previous_name_buf);\n-- \n2.0.4.2.g7bc378e.dirty\n"},{"id":"248437","messageId":"1409168892-11345-2-git-send-email-jsorianopastor@gmail.com","threadId":"37336","inReplyTo":"1409168892-11345-1-git-send-email-jsorianopastor@gmail.com","subject":"[PATCH 2/2] read_index_unmerged(): remove unnecessary loop index adjustment","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-27T19:48:12Z","receivedAt":"2014-08-27T19:48:12Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"Signed-off-by: Jaime Soriano Pastor \n---\n read-cache.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 1cdb762..39fca8c 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1971,7 +1971,6 @@ int read_index_unmerged(struct index_state *istate)\n \t\tif (add_index_entry(istate, new_ce, 0))\n \t\t\treturn error(\"%s: cannot drop to stage #0\",\n \t\t\t\t new_ce->name);\n-\t\ti = index_name_pos(istate, new_ce->name, len);\n \t}\n \treturn unmerged;\n }\n-- \n2.0.4.2.g7bc378e.dirty\n"},{"id":"248438","messageId":"CAPuZ2NFzHjUSfi1H0RFvOuWdRptwxv-gsUOAJv0Uh5BFLWmnRA@mail.gmail.com","threadId":"37336","inReplyTo":"xmqqy4ubi1ty.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] Check order when reading index","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-27T19:52:45Z","receivedAt":"2014-08-27T19:52:45Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"On Tue, Aug 26, 2014 at 7:43 PM, Junio C Hamano wrote:\n> Does the current codebase choke with such entries in the index file,\n> like you saw in your index file with both stage #0 and stage #1\n> entries?\n\nNot sure, I couldn't reproduce an scenario with an index with multiple\nentries in the same stage for the same path.\n"},{"id":"248445","messageId":"xmqq61hdd4e9.fsf@gitster.dls.corp.google.com","threadId":"37336","inReplyTo":"CAPuZ2NFzHjUSfi1H0RFvOuWdRptwxv-gsUOAJv0Uh5BFLWmnRA@mail.gmail.com","subject":"Re: [PATCH 1/2] Check order when reading index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-27T21:11:26Z","receivedAt":"2014-08-27T21:11:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jaime Soriano Pastor writes:\n\n> On Tue, Aug 26, 2014 at 7:43 PM, Junio C Hamano wrote:\n>> Does the current codebase choke with such entries in the index file,\n>> like you saw in your index file with both stage #0 and stage #1\n>> entries?\n>\n> Not sure, I couldn't reproduce an scenario with an index with multiple\n> entries in the same stage for the same path.\n\nI think we have been discussing how to protect broken index file\nleft by tools other people wrote, so I wouldn't be so surprised if\nour current toolset does not let you recreate certain breakages ;-)\n\nI was asking for an answer more from what you know about the code.\nFor example, would read_index_unmerged() choke if the index has two\nor more stage #1 (or stage #3) entries for the same path (provided\nthat the index is otherwise normal, i.e. no stage #0 entry for that\npath, entries are sorted by pathname order and stages are in an\norder that does not decrease)?\n"},{"id":"248453","messageId":"CAPuZ2NHTz2Qu3oorRVTHSb9E-gC_d1kNniVmtUPtpAqPVrUDtA@mail.gmail.com","threadId":"37336","inReplyTo":"xmqq61hdd4e9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] Check order when reading index","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-27T22:13:52Z","receivedAt":"2014-08-27T22:13:52Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"On Wed, Aug 27, 2014 at 11:11 PM, Junio C Hamano wrote:\n> I was asking for an answer more from what you know about the code.\n> For example, would read_index_unmerged() choke if the index has two\n> or more stage #1 (or stage #3) entries for the same path (provided\n> that the index is otherwise normal, i.e. no stage #0 entry for that\n> path, entries are sorted by pathname order and stages are in an\n> order that does not decrease)?\n\nOh, ok :) Re-reading the code a bit I think that there can be a\npotential problem in the add_index_entry_with_check() function. It's\ncurrently implemented to allow an only entry for each stage and each\npath, if an entry for a path and a stage is being added, and another\none existed before, the old one is replaced, but just the first one,\nso adding an entry to stage #1 in an index with multiple entries at\nstage #1 would replace the first occurence, but not the rest, what\ncould not be expected. The user could maybe expect that all entries\nare replaced, or only an specific one.\nIf an stage #0 entry is added and there are multiple entries for any\nof the higher-stage entries there wouldn't be any problem as this\nfunction removes all the higher-stage entries for the same path\nwithout checking the stage. This last case is the one in\nread_index_unmerged().\n"},{"id":"248534","messageId":"CAPig+cS__Sw1gNj3Pz20OC51QBsuxBEXTzMStAerwfMuT2afQg@mail.gmail.com","threadId":"37336","inReplyTo":"1409168892-11345-1-git-send-email-jsorianopastor@gmail.com","subject":"Re: [PATCH 1/2] read_index_from(): catch out of order entries when reading an index file","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-08-29T02:16:39Z","receivedAt":"2014-08-29T02:16:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 27, 2014 at 3:48 PM, Jaime Soriano Pastor\n wrote:\n> Signed-off-by: Jaime Soriano Pastor \n> ---\n> read-cache.c | 18 ++++++++++++++++++\n> 1 file changed, 18 insertions(+)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index 7f5645e..1cdb762 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1438,6 +1438,21 @@ static struct cache_entry *create_from_disk(struct ondisk_cache_entry *ondisk,\n> return ce;\n> }\n>\n> +static void check_ce_order(struct cache_entry *ce, struct cache_entry *next_ce)\n> +{\n> + int name_compare = strcmp(ce->name, next_ce->name);\n> + if (0 < name_compare)\n> + die(\"Unordered stage entries in index\");\n> + if (!name_compare) {\n> + if (!ce_stage(ce))\n> + die(\"Multiple stage entries for merged file '%s'\",\n> + ce->name);\n> + if (ce_stage(ce) > ce_stage(next_ce))\n> + die(\"Unordered stage entries for '%s'\",\n> + ce->name);\n\nPerhaps consider dropping capitalization from error messages [1]\n(despite existing code in read-cache.c having a mix of the two\nstyles). See \"Error Messages\" in Documentation/CodingGuidelines.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/251715/focus=253209\n\n> + }\n> +}\n> +\n> /* remember to discard_cache() before reading a different cache! */\n> int read_index_from(struct index_state *istate, const char *path)\n> {\n> @@ -1499,6 +1514,9 @@ int read_index_from(struct index_state *istate, const char *path)\n> ce = create_from_disk(disk_ce, &consumed, previous_name);\n> set_index_entry(istate, i, ce);\n>\n> + if (i > 0)\n> + check_ce_order(istate->cache[i - 1], ce);\n> +\n> src_offset += consumed;\n> }\n> strbuf_release(&previous_name_buf);\n> --\n> 2.0.4.2.g7bc378e.dirty\n"},{"id":"248537","messageId":"1409302481-4914-1-git-send-email-jsorianopastor@gmail.com","threadId":"37336","inReplyTo":"CAPig+cS__Sw1gNj3Pz20OC51QBsuxBEXTzMStAerwfMuT2afQg@mail.gmail.com","subject":"[PATCH] read_index_from(): catch out of order entries when reading an index file","fromName":"Jaime Soriano Pastor","fromEmail":"jsorianopastor@gmail.com","sentAt":"2014-08-29T08:54:41Z","receivedAt":"2014-08-29T08:54:41Z","isPatch":true,"sender":{"key":"jsorianopastor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15763?v=4"},"body":"Signed-off-by: Jaime Soriano Pastor \n---\n read-cache.c | 18 ++++++++++++++++++\n 1 file changed, 18 insertions(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 5d3c8bd..023d6d7 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1465,6 +1465,21 @@ static struct cache_entry *create_from_disk(struct ondisk_cache_entry *ondisk,\n \treturn ce;\n }\n \n+static void check_ce_order(struct cache_entry *ce, struct cache_entry *next_ce)\n+{\n+\tint name_compare = strcmp(ce->name, next_ce->name);\n+\tif (0 < name_compare)\n+\t\tdie(\"unordered stage entries in index\");\n+\tif (!name_compare) {\n+\t\tif (!ce_stage(ce))\n+\t\t\tdie(\"multiple stage entries for merged file '%s'\",\n+\t\t\t\tce->name);\n+\t\tif (ce_stage(ce) > ce_stage(next_ce))\n+\t\t\tdie(\"unordered stage entries for '%s'\",\n+\t\t\t\tce->name);\n+\t}\n+}\n+\n /* remember to discard_cache() before reading a different cache! */\n int do_read_index(struct index_state *istate, const char *path, int must_exist)\n {\n@@ -1526,6 +1541,9 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)\n \t\tce = create_from_disk(disk_ce, &consumed, previous_name);\n \t\tset_index_entry(istate, i, ce);\n \n+\t\tif (i > 0)\n+\t\t\tcheck_ce_order(istate->cache[i - 1], ce);\n+\n \t\tsrc_offset += consumed;\n \t}\n \tstrbuf_release(&previous_name_buf);\n-- \n2.0.4.1.gca370f9.dirty\n"},{"id":"248562","messageId":"xmqqsikf6xa2.fsf@gitster.dls.corp.google.com","threadId":"37336","inReplyTo":"1409302481-4914-1-git-send-email-jsorianopastor@gmail.com","subject":"Re: [PATCH] read_index_from(): catch out of order entries when reading an index file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-29T17:06:13Z","receivedAt":"2014-08-29T17:06:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks, both.\n"}]}