{"thread":{"id":"526","subject":"Broken adding of cache entries","startedAt":"2005-05-07T15:28:50Z","lastAt":"2005-05-08T22:42:28Z","messageCount":13,"participants":["Petr Baudis","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"2747","messageId":"20050507152849.GD9495@pasky.ji.cz","threadId":"526","inReplyTo":"1115431767.32065.182.camel@localhost.localdomain","subject":"Broken adding of cache entries","fromName":"Petr Baudis","fromEmail":"pasky@ucw.cz","sentAt":"2005-05-07T15:28:50Z","receivedAt":"2005-05-07T15:28:50Z","isPatch":false,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Dear diary, on Sat, May 07, 2005 at 04:09:27AM CEST, I got a letter\nwhere Kay Sievers <kay.sievers@vrfy.org> told me that...\n..snip..\n> Look what funny thing you can do:\n>   kay@mam:~/public_html/pub/scm/funny-tree$ git-ls-tree HEAD\n>   100644  blob    b1a17ba136936531b72571844a773fe938b85ad4        entry\n>   040000  tree    eba6ba02f18176500019755ad58c0bdfead16c47        entry\n> \n> Add a file to the cache, replace it with a directory, add that to the\n> cache and then write the tree and you have two entries with the same\n> name. :)\n\nDuh. Well, what could be the reasonwhy cache_name_compare() cares about\nflags at all? Can you _ever_ have two same-named entries? Junio, what do\nyou think about something like this?\n\nIndex: read-cache.c\n===================================================================\n--- e47e2a558a85b33e0652233f78aa1ca8a959685b/read-cache.c  (mode:100644)\n+++ uncommitted/read-cache.c  (mode:100644)\n@@ -68,10 +68,6 @@\n \t\treturn -1;\n \tif (len1 > len2)\n \t\treturn 1;\n-\tif (flags1 < flags2)\n-\t\treturn -1;\n-\tif (flags1 > flags2)\n-\t\treturn 1;\n \treturn 0;\n }\n \n\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\nC++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor\n"},{"id":"2750","messageId":"7vu0leaneg.fsf@assigned-by-dhcp.cox.net","threadId":"526","inReplyTo":"20050507152849.GD9495@pasky.ji.cz","subject":"Re: Broken adding of cache entries","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-07T18:42:15Z","receivedAt":"2005-05-07T18:42:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I'll have a look later but as I recall the code (I'm writing\nthis without looking at the specific code, just your patch\nhunk), the flag comparison there is not about blob vs tree modes\nbut for \"stages\"; it must answer that two entries with the same\nname but in different merge stages are different.\n\n\n"},{"id":"2752","messageId":"7vhdhealjm.fsf@assigned-by-dhcp.cox.net","threadId":"526","inReplyTo":"20050507152849.GD9495@pasky.ji.cz","subject":"Re: Broken adding of cache entries","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-07T19:22:21Z","receivedAt":"2005-05-07T19:22:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I'll keep this in git-jc repository until Linus returns.  Pasky\nand Kay could you give it a try?\n\n-- test case --\n\n$ ls -a\n./  ../\n$ git-init-db\ndefaulting to local storage area\n$ date >path\n$ git-update-cache --add path\n$ rm path\n$ mkdir path\n$ date >path/file\n$ git-update-cache --add path/file\n$ git-ls-files --stage\n100644 1738f2536b1201218c41153941da065cc26174c9 0 path\n100644 620c72f1c1de15f56ff9d63d6d7cdc69e828f1e3 0 path/file\n$ git-ls-tree $(git-write-tree)                     ;# using old one\n100644\tblob\t1738f2536b1201218c41153941da065cc26174c9\tpath\n040000\ttree\tec116937f223e3df95aeac9f076902ae1618ae98\tpath\n$ ../git-write-tree                                 ;# using new one\nYou have both path and path/file\nfatal: write-tree: not able to write tree\n$ exit\n\n----------------------------------------------------------------\nNotice index that has path and path/file and refuse to write such a tree.\n\nKay Sievers noticed that you can have both path and path/file in\nthe cache and write-tree happily creates a tree object from such\na state.  Since a merge can result in such situation and the\nuser should be able to see the situation by looking at the\ncache, rather than forbidding add_cache_entry() to create such\nconflicts, fix it by making write-tree refuse to write such an\nnonsensical tree.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n\nwrite-tree.c |   35 +++++++++++++++++++++++++++++++----\n1 files changed, 31 insertions(+), 4 deletions(-)\n\n--- a/write-tree.c\n+++ b/write-tree.c\n@@ -84,7 +84,7 @@ static int write_tree(struct cache_entry\n \n int main(int argc, char **argv)\n {\n-\tint i, unmerged;\n+\tint i, funny;\n \tint entries = read_cache();\n \tunsigned char sha1[20];\n \n@@ -92,18 +92,45 @@ int main(int argc, char **argv)\n \t\tdie(\"write-tree: no cache contents to write\");\n \n \t/* Verify that the tree is merged */\n-\tunmerged = 0;\n+\tfunny = 0;\n \tfor (i = 0; i < entries; i++) {\n \t\tstruct cache_entry *ce = active_cache[i];\n \t\tif (ntohs(ce->ce_flags) & ~CE_NAMEMASK) {\n-\t\t\tif (++unmerged > 10) {\n+\t\t\tif (10 < ++funny) {\n \t\t\t\tfprintf(stderr, \"...\\n\");\n \t\t\t\tbreak;\n \t\t\t}\n \t\t\tfprintf(stderr, \"%s: unmerged (%s)\\n\", ce->name, sha1_to_hex(ce->sha1));\n \t\t}\n \t}\n-\tif (unmerged)\n+\tif (funny)\n+\t\tdie(\"write-tree: not able to write tree\");\n+\n+\t/* Also verify that the cache does not have path and path/file\n+\t * at the same time.  At this point we know the cache has only\n+\t * stage 0 entries.\n+\t */\n+\tfunny = 0;\n+\tfor (i = 0; i < entries - 1; i++) {\n+\t\t/* path/file always comes after path because of the way\n+\t\t * the cache is sorted.  Also path can appear only once,\n+\t\t * which means conflicting one would immediately follow.\n+\t\t */\n+\t\tconst char *this_name = active_cache[i]->name;\n+\t\tconst char *next_name = active_cache[i+1]->name;\n+\t\tint this_len = strlen(this_name);\n+\t\tif (this_len < strlen(next_name) &&\n+\t\t    strncmp(this_name, next_name, this_len) == 0 &&\n+\t\t    next_name[this_len] == '/') {\n+\t\t\tif (10 < ++funny) {\n+\t\t\t\tfprintf(stderr, \"...\\n\");\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tfprintf(stderr, \"You have both %s and %s\\n\",\n+\t\t\t\tthis_name, next_name);\n+\t\t}\n+\t}\n+\tif (funny)\n \t\tdie(\"write-tree: not able to write tree\");\n \n \t/* Ok, write it out */\n\n"},{"id":"2756","messageId":"20050507224116.GF9495@pasky.ji.cz","threadId":"526","inReplyTo":"7vhdhealjm.fsf@assigned-by-dhcp.cox.net","subject":"Re: Broken adding of cache entries","fromName":"Petr Baudis","fromEmail":"pasky@ucw.cz","sentAt":"2005-05-07T22:41:16Z","receivedAt":"2005-05-07T22:41:16Z","isPatch":false,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Dear diary, on Sat, May 07, 2005 at 09:22:21PM CEST, I got a letter\nwhere Junio C Hamano <junkio@cox.net> told me that...\n> Kay Sievers noticed that you can have both path and path/file in\n> the cache and write-tree happily creates a tree object from such\n> a state.  Since a merge can result in such situation and the\n> user should be able to see the situation by looking at the\n> cache, rather than forbidding add_cache_entry() to create such\n> conflicts, fix it by making write-tree refuse to write such an\n> nonsensical tree.\n\nI'd still prefer add_cache_entry() to just replace the original entry\n(as it does otherwise). Only make it to care about which stage it is\nworking in, to make merges to work. IOW, I think you are solving this at\nthe wrong workflow point. It is too \"late\" to know at that point, and\n(a huge) PITA for the higher levels to deal with it then - that all when\nit shouldn't fail _at all_ in the first place.\n\nWhat about\n\n--- b7ae63ab415e556c2f0f0ad2803f701b4a6d6956/read-cache.c  (mode:100644)\n+++ uncommitted/read-cache.c  (mode:100644)\n@@ -68,9 +68,9 @@\n                return -1;\n        if (len1 > len2)\n                return 1;\n-       if (flags1 < flags2)\n+       if (flags1 & CE_STAGEMASK < flags2 & CE_STAGEMASK)\n                return -1;\n-       if (flags1 > flags2)\n+       if (flags1 & CE_STAGEMASK > flags2 & CE_STAGEMASK)\n                return 1;\n        return 0;\n }\n\nthen? (Completely untested and everything.)\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\nC++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor\n"},{"id":"2759","messageId":"7vll6q8s4o.fsf@assigned-by-dhcp.cox.net","threadId":"526","inReplyTo":"20050507224116.GF9495@pasky.ji.cz","subject":"Re: Broken adding of cache entries","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-08T00:43:03Z","receivedAt":"2005-05-08T00:43:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">>>>> \"PB\" == Petr Baudis <pasky@ucw.cz> writes:\n\nPB> What about\nPB> ... suggested changes removed.\nPB> then? (Completely untested and everything.)\n\nIt is clear that it is untested.  In the sequence Kay had\ntrouble with:\n\n    $ rm -fr path; date >path\n    $ git-update-cache --add path\n    $ rm -f path; mkdir path; date >path/file\n    $ git-update-cache --add path/file\n\nThe cache_name_compare function you are looking at is called\nwith \"path\" and \"path/file\", and compares the name strings (up\nto the length of the shorter one), and when they are the same,\nthen compares the lengths of them.  At that point, the if()\nstatements have already answered that \"path\" comes before\n\"path/file\" and your changes will _not_ change the behavior of\nthat function.\n\nThat said, I understand the behaviour you would want to see, and\nI tend to agree that this should be prevented from happening in\nthe first place when git-update-cache happens.  Let me think a\nbit more about it.\n\nPB> I'd still prefer add_cache_entry() to just replace the original entry\nPB> (as it does otherwise).\n\nI do not think it should just silently replace; instead it\nshould make the user take notice, because this is an unusual\nsituation.  In other words, it should refuse until the user\nmakes a conscious decision to tell it that the user is removing\nit and then placing something completely different.\n\nThis implies (I am thinking aloud here):\n\n    $ find fs -type f -print | xargs git-update-cache --add\n    $ rm -fr fs\n    $ date >fs\n    $ git-update-cache --add fs\n\nwould refuse to add \"fs\" because it notices it has some entry\nwhose \"name\" field starts with \"fs/\".  Likewise, for the\nopposite sequence:\n\n    $ date >fs\n    $ git-update-cache --add fs\n    $ rm -f fs; mkdir fs; date >fs/file    \n    $ find fs -type f -print | xargs git-update-cache --add\n\nTelling git-update-cache about anything that contains a '/' in\nits name would first check if an entry whose name is exactly one\nof the leading path prefix, and refuses the operation if it\nfinds one.\n\nThe above logic should work and it would also work if we decide\nto just replace without refusing as well.  I'll code it up and\nsee which one I like; maybe it will end up as an option.\n\n\n"},{"id":"2762","messageId":"7vbr7m8p05.fsf@assigned-by-dhcp.cox.net","threadId":"526","inReplyTo":"7vll6q8s4o.fsf@assigned-by-dhcp.cox.net","subject":"Re: Broken adding of cache entries","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-08T01:50:34Z","receivedAt":"2005-05-08T01:50:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This patch makes git-update-cache --add refuse to create an\nindex where one entry is a file and another entry needs to have\nthat entry as its leading directory.\n\nI am not quite sure the semantics is quite right, so I am\nholding it off from putting it in git-jc repository for now, but\nplease review it, give it a try and tell me what you think.\n\nHere is what I did.\n\n    $ ls -a\n    ./  ../\n    $ git-init-db\n    defaulting to local storage area\n    $ date >path\n    $ git-update-cache --add path\n    $ rm -f path; mkdir path; date >path/file\n    $ git-update-cache --add path/file\n    fatal: Unable to add path/file to database\n    $ git-update-cache --remove path           ;# *****\n    fatal: Unable to add path to database\n    $ git-update-cache --force-remove path\n    $ git-update-cache --add path/file\n    $ git-ls-files --stage\n    100644 3e3f5e43a8e7baa7f0b1f2d7b99ed25fcbe582d2 0 path/file\n    $ rm -fr path\n    $ git-update-cache --remove path/file\n    $ mkdir path; date >path/file\n    $ git-update-cache --add path/file\n    $ git-ls-files --stage\n    100644 1b4e067c82b90419c2d2ce80e72079f926a49cc3 0 path/file\n    $ rm -fr path; date >path\n    $ git-update-cache --add path\n    fatal: Unable to add path to database\n    $ git-update-cache --remove path/file\n    $ git-update-cache --add path\n    $ git-ls-files --stage\n    100644 b4509b7a4fed200befc45746061eb1d32ad6cbc9 0 path\n\nThe one marked with ***** is what I am debating myself about,\nbut that one would probably be an independent fix.\n\nNot-signed-off-yet-by: Junio C Hamano <junkio@cox.net>\n---\njit-diff 0:7 read-cache.c\n# - HEAD: Notice tree objects with duplicate entries.\n# + 7: Refuse to create file/dir conflict cache entry.\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -123,6 +123,64 @@ int same_name(struct cache_entry *a, str\n \treturn ce_namelen(b) == len && !memcmp(a->name, b->name, len);\n }\n \n+/* We may be in a situation where we already have\n+ * path/file and path is being added, or we already\n+ * have path and path/file is being added.  Either one\n+ * would result in a nonsense tree that has path twice\n+ * when git-write-tree tries to write it out.  Prevent it.\n+ */\n+static int check_file_directory_conflict(const char *path)\n+{\n+\tint pos;\n+\tint namelen = strlen(path);\n+\tchar *pathbuf = xmalloc(namelen + 1);\n+\tchar *cp;\n+\n+\tmemcpy(pathbuf, path, namelen + 1);\n+\n+\tcp = pathbuf;\n+\twhile (1) {\n+\t\tchar *ep = strchr(cp, '/');\n+\t\tif (ep == 0)\n+\t\t\tbreak;\n+\t\t*ep = 0;\n+\t\tpos = cache_name_pos(pathbuf,\n+\t\t\t\t     htons(create_ce_flags(ep-cp, 0)));\n+\t\tif (0 <= pos) {\n+\t\t\t/* Our leading path component is registered as a file,\n+\t\t\t * and we are trying to make it a directory.\n+\t\t\t */\n+\t\t\tfree(pathbuf);\n+\t\t\treturn -1;\n+\t\t}\n+\t\t*ep = '/';\n+\t\tcp = ep + 1;\n+\t}\n+\tfree(pathbuf);\n+\n+\t/* Do we have an entry in the cache that makes our path a prefix\n+\t * of it?  That is, are we creating a file where they expect a\n+\t * directory there?\n+\t */\n+\tpos = cache_name_pos(path,\n+\t\t\t     htons(create_ce_flags(namelen, 0)));\n+\n+\t/* (0 <= pos) cannot happen because add_cache_entry()\n+\t * should have taken care of that case.\n+\t */\n+\tpos = -pos-1;\n+\n+\t/* pos would point at an existing entry that would come immediately\n+\t * after our path.  Does it have our path as a leading path prefix?\n+\t */\n+\tif ((pos < active_nr) &&\n+\t    !strncmp(active_cache[pos]->name, path, namelen) &&\n+\t    active_cache[pos]->name[namelen] == '/')\n+\t\treturn -1;\n+\n+\treturn 0;\n+}\n+\n int add_cache_entry(struct cache_entry *ce, int ok_to_add)\n {\n \tint pos;\n@@ -152,6 +210,9 @@ int add_cache_entry(struct cache_entry *\n \tif (!ok_to_add)\n \t\treturn -1;\n \n+\tif (check_file_directory_conflict(ce->name))\n+\t\treturn -1;\n+\n \t/* Make sure the array is big enough .. */\n \tif (active_nr == active_alloc) {\n \t\tactive_alloc = alloc_nr(active_alloc);\n\n\n"},{"id":"2764","messageId":"7vll6q70mg.fsf@assigned-by-dhcp.cox.net","threadId":"526","inReplyTo":"7vbr7m8p05.fsf@assigned-by-dhcp.cox.net","subject":"Re: Broken adding of cache entries","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-08T05:22:31Z","receivedAt":"2005-05-08T05:22:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">>>>> \"JCH\" == Junio C Hamano <junkio@cox.net> writes:\n\nJCH> I am not quite sure the semantics is quite right, so I am\nJCH> holding it off from putting it in git-jc repository for now, but\nJCH> please review it, give it a try and tell me what you think.\n\nOk, I have two updates pushed out at git-jc archive at\nhttp://members.cox.net/junkio/git-jc.git/\n\nHere is the summary of what the changes do.\n\nThe first commit is to simply prevent Kay's situation from\nhappening from git-update-cache.\n\nBy default, git-update-cache refuses to:\n\n - add \"path\" when \"path/file\" exists.  That is, you cannot\n   replace a directory with a file (or a symlink).\n\n - add \"path/file\" when \"path\" exists.  That is, you cannot\n   replace a file (or a symlink) with a directory.\n\nAnd the second commit introduces --replace option to the\ncommand.  When --replace is in effect, instead of refusing, the\nexisting path that conflicts with whatever is being added is\ndropped with a warning message.  That means you would lose an\nentire subdirectory from the index when you rename it and\nreplace it with a file and then call git-update-cache on it\n(which is probably fine and what the user expects anyway).\n\nNote that this is not enough to prevent nonsensical tree from\nhappening.  Three-way read-tree can be fed with an ancestor tree\nthat does not have anything interesting, one side adding \"path\"\n(as a file) and the other side adding \"path/file\".  Just after\nread-tree finishes reading three trees, you would have:\n\n    100644 1 not-interesting\n    100644 2 not-interesting\n    100644 3 not-interesting\n    100644 2 path\n    100644 3 path/file\n\nAnd all of these paths are candidate for trivial \"collapsing to\nstage 0\" operation.  You would end up with path vs path/file\nconflicts this way, so the previous change to write-tree to\nprevent it from writing such a result is still needed.\n\nIdeally the \"collapsing to stage 0\" logic could also be tweaked\nto detect and prevent this from happening, but I'd rather keep\nit simple for now (the tool should not be too clever).  The user\ncannot write such a tree, and when this kind of conflict\nhappens, essentially both heads being merged needs to be\nexamined and manual resolution is required anyway.\n\n\n"},{"id":"2778","messageId":"20050508165915.GW9495@pasky.ji.cz","threadId":"526","inReplyTo":"7vll6q70mg.fsf@assigned-by-dhcp.cox.net","subject":"Re: Broken adding of cache entries","fromName":"Petr Baudis","fromEmail":"pasky@ucw.cz","sentAt":"2005-05-08T16:59:15Z","receivedAt":"2005-05-08T16:59:15Z","isPatch":false,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Dear diary, on Sun, May 08, 2005 at 07:22:31AM CEST, I got a letter\nwhere Junio C Hamano <junkio@cox.net> told me that...\n> >>>>> \"JCH\" == Junio C Hamano <junkio@cox.net> writes:\n> \n> JCH> I am not quite sure the semantics is quite right, so I am\n> JCH> holding it off from putting it in git-jc repository for now, but\n> JCH> please review it, give it a try and tell me what you think.\n> \n> Ok, I have two updates pushed out at git-jc archive at\n> http://members.cox.net/junkio/git-jc.git/\n\nI wanted to merge with you, but I'd like to make some (mostly minor)\nnitpicks first.\n\nSorry for totally garbled whitespace and everything, this is just gpm.\n\n--- e19665d8d42877246ac7e98a7c671d11adbe8b56/read-cache.c  (mode:100644)\n+++ uncommitted/read-cache.c  (mode:100644)\n+               char *ep = strchr(cp, '/');\n+               if (ep == 0)\n\nPlease use NULL.\n\n+static struct alternate_object_database\n+{\n+       char *base;\n+       char *name;\n+} *alt_odb;\n\nThe commonly accepted style is to have the { bracket on the same line as the\nstruct identifier.\n\nSticking a brief explanation to prepare_alt_odb(), like \"pass 0\nallocates the array and pass 1 fills it\" couldn't hurt, it took me a\nwhile of staring at it to figure out. There's also unused @buf variable.\nBut I personally think all this alt_odb code is quite creepy. ;-)\n\n--- e19665d8d42877246ac7e98a7c671d11adbe8b56/write-tree.c  (mode:100644)\n+++ uncommitted/write-tree.c  (mode:100644)\n-                       if (++unmerged > 10) {\n+                       if (10 < ++funny) {\n\nDo those changes make any sense? The former version is certainly much\neasier to read for me. I can live with it in the new code, but changing\nold code to it...\n\nWith the current update-cache protections, how can you legally achieve a\ncache with duplicate entries, so that you need to check for that in\nwrite-tree?\n\nThanks,\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\nC++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor\n"},{"id":"2823","messageId":"7v3bsx5sxx.fsf@assigned-by-dhcp.cox.net","threadId":"526","inReplyTo":"20050508165915.GW9495@pasky.ji.cz","subject":"Re: Broken adding of cache entries","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-08T21:06:02Z","receivedAt":"2005-05-08T21:06:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">>>>> \"PB\" == Petr Baudis <pasky@ucw.cz> writes:\n\nPB> Dear diary, on Sun, May 08, 2005 at 07:22:31AM CEST, I got a letter\nPB> where Junio C Hamano <junkio@cox.net> told me that...\n\n>> Ok, I have two updates pushed out at git-jc archive at\n>> http://members.cox.net/junkio/git-jc.git/\n\nPB> I wanted to merge with you, but I'd like to make some (mostly minor)\nPB> nitpicks first.\n\nI've addressed the points you raised and pushed them out.\nThanks again for the review.\n\n"},{"id":"2829","messageId":"20050508212202.GM9495@pasky.ji.cz","threadId":"526","inReplyTo":"7v3bsx5sxx.fsf@assigned-by-dhcp.cox.net","subject":"Re: Broken adding of cache entries","fromName":"Petr Baudis","fromEmail":"pasky@ucw.cz","sentAt":"2005-05-08T21:22:02Z","receivedAt":"2005-05-08T21:22:02Z","isPatch":false,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Dear diary, on Sun, May 08, 2005 at 11:06:02PM CEST, I got a letter\nwhere Junio C Hamano <junkio@cox.net> told me that...\n> >>>>> \"PB\" == Petr Baudis <pasky@ucw.cz> writes:\n> \n> PB> Dear diary, on Sun, May 08, 2005 at 07:22:31AM CEST, I got a letter\n> PB> where Junio C Hamano <junkio@cox.net> told me that...\n> \n> >> Ok, I have two updates pushed out at git-jc archive at\n> >> http://members.cox.net/junkio/git-jc.git/\n> \n> PB> I wanted to merge with you, but I'd like to make some (mostly minor)\n> PB> nitpicks first.\n> \n> I've addressed the points you raised and pushed them out.\n> Thanks again for the review.\n\nGreat, thanks.\n\nStill, I'd like to know why the checking in write-tree is necessary.\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\nC++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor\n"},{"id":"2835","messageId":"7v7ji94b1d.fsf@assigned-by-dhcp.cox.net","threadId":"526","inReplyTo":"20050508212202.GM9495@pasky.ji.cz","subject":"Re: Broken adding of cache entries","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-08T22:18:06Z","receivedAt":"2005-05-08T22:18:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">>>>> \"PB\" == Petr Baudis <pasky@ucw.cz> writes:\n\nPB> Still, I'd like to know why the checking in write-tree is necessary.\n\nThe following is just a rephrase of what I wrote in my previous\nmessage <7vll6q70mg.fsf@assigned-by-dhcp.cox.net>.\n\n------------ test.sh ------------\n#!/bin/sh\n\ntest=\"++testdir\"\nrm -fr $test\nmkdir $test\ncd $test || exit\n\nshow () {\n\techo \"# ls ($1)\"\n\tls -F\n\techo \"# git-ls-files --stage ($1)\"\n\tgit-ls-files --stage\n\tcase \"$2\" in\n\t'')\t;;\n\t?*)\n\t\techo \"# git-ls-tree -r ($1)\"\n\t\tgit-ls-tree -r $2\n\t\t;;\n\tesac\n}\n\necho \"# Original\"\ngit-init-db\ndate >not-so-interesting\ngit-update-cache --add not-so-interesting\noriginal=$(echo Original | git-commit-tree $(git-write-tree))\nshow Original $original\nrm -rf path not-so-interesting\n\necho \"# Branch A\"\ngit-read-tree $original\ngit-checkout-cache -f -a\ndate >path\ngit-update-cache --add path\nbranchA=$(echo Branch A Changes |\n          git-commit-tree $(git-write-tree) -p $original)\nshow 'Branch A' $branchA\nrm -rf path not-so-interesting\n\necho \"# Branch B\"\ngit-read-tree $original\ngit-checkout-cache -f -a\nmkdir path\ndate >path/file\ngit-update-cache --add path/file\nbranchB=$(echo Branch B Changes |\n          git-commit-tree $(git-write-tree) -p $original)\nshow 'Branch B' $branchB\nrm -rf path not-so-interesting\n\necho \"# Attempt merge O A B\"\nmb=$(git-merge-base $branchA $branchB)\necho \"Original  $original\"\necho \"Branch-A  $branchA\"\necho \"Branch-B  $branchB\"\necho \"MergeBase $mb\"\n\ngit-read-tree -m $mb $branchA $branchB\nshow \"Merge\"\n------------ test.out ------------\n# Original\ndefaulting to local storage area\nCommitting initial tree 682c2247823f37e4cf64f240009c9ba932b04fe0\n# ls (Original)\nnot-so-interesting\n# git-ls-files --stage (Original)\n100644 9bcc8fabbbb587ba312a45b144dce8f4d9191da5 0 not-so-interesting\n# git-ls-tree -r (Original)\n100644\tblob\t9bcc8fabbbb587ba312a45b144dce8f4d9191da5\tnot-so-interesting\n# Branch A\n# ls (Branch A)\nnot-so-interesting\npath\n# git-ls-files --stage (Branch A)\n100644 9bcc8fabbbb587ba312a45b144dce8f4d9191da5 0 not-so-interesting\n100644 9bcc8fabbbb587ba312a45b144dce8f4d9191da5 0 path\n# git-ls-tree -r (Branch A)\n100644\tblob\t9bcc8fabbbb587ba312a45b144dce8f4d9191da5\tnot-so-interesting\n100644\tblob\t9bcc8fabbbb587ba312a45b144dce8f4d9191da5\tpath\n# Branch B\n# ls (Branch B)\nnot-so-interesting\npath/\n# git-ls-files --stage (Branch B)\n100644 9bcc8fabbbb587ba312a45b144dce8f4d9191da5 0 not-so-interesting\n100644 9bcc8fabbbb587ba312a45b144dce8f4d9191da5 0 path/file\n# git-ls-tree -r (Branch B)\n100644\tblob\t9bcc8fabbbb587ba312a45b144dce8f4d9191da5\tnot-so-interesting\n040000\ttree\tf4e939c9b65aff9a59118f3f3f299ef30744adfa\tpath\n100644\tblob\t9bcc8fabbbb587ba312a45b144dce8f4d9191da5\tpath/file\n# Attempt merge O A B\nOriginal  40c1d8a0e79df9e681667e0074664266b6bd9935\nBranch-A  88b3e7e4ce0ba55c60fe4de6523968ecabb1cc78\nBranch-B  0faa7f6b7f68ce2951fe3f26f937766ee53dc11c\nMergeBase 40c1d8a0e79df9e681667e0074664266b6bd9935\n# ls (Merge)\n# git-ls-files --stage (Merge)\n100644 9bcc8fabbbb587ba312a45b144dce8f4d9191da5 0 not-so-interesting\n100644 9bcc8fabbbb587ba312a45b144dce8f4d9191da5 2 path\n100644 9bcc8fabbbb587ba312a45b144dce8f4d9191da5 3 path/file\n\n"},{"id":"2836","messageId":"7v3bsx4aum.fsf@assigned-by-dhcp.cox.net","threadId":"526","inReplyTo":"7v7ji94b1d.fsf@assigned-by-dhcp.cox.net","subject":"Re: Broken adding of cache entries","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-08T22:22:09Z","receivedAt":"2005-05-08T22:22:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">>>>> \"JCH\" == Junio C Hamano <junkio@cox.net> writes:\n>>>>> \"PB\" == Petr Baudis <pasky@ucw.cz> writes:\nPB> Still, I'd like to know why the checking in write-tree is necessary.\n\nOk, having thought about it a bit more, I think we can yank it\nout.  I'd rather keep ourselves cautious, though; there may be\nsome other ways we haven't thought of to create such nonsense,\nand it would not hurt to be cautious before writing a tree out.\n\n"},{"id":"2837","messageId":"7voebl2vcb.fsf@assigned-by-dhcp.cox.net","threadId":"526","inReplyTo":"7v3bsx4aum.fsf@assigned-by-dhcp.cox.net","subject":"Re: Broken adding of cache entries","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-08T22:42:28Z","receivedAt":"2005-05-08T22:42:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">>>>> \"JCH\" == Junio C Hamano <junkio@cox.net> writes:\n\nJCH> Ok, having thought about it a bit more, I think we can yank it\nJCH> out.  I'd rather keep ourselves cautious, though; there may be\nJCH> some other ways we haven't thought of to create such nonsense,\nJCH> and it would not hurt to be cautious before writing a tree out.\n\nHere is a revert you can apply after merging with the tip of my\ntree if you wanted to.  As you pointed out correctly, three-way\nread-tree would not produce the situation as I feared, and the\nattempt to postprocess the result by git-merge-one-file-script\ngit-merge-cache calls would be caught with the changes we made\nto git-update-cache, so there is no need for the extra check by\nwrite-tree in that workflow example.  Sorry for being ultra\ndense.\n\nThe only reason why I am holding off from pushing this out is\nbecause I still cannot convince myself that we eradicated _all_\nthe possibile paths to create such nonsense in the index, and I\nfeel safer to keep the check this patch is reverting, at least\nfor now.\n\n---\nAuthor: Junio C Hamano <junkio@cox.net>\nDate:   Sun May 8 15:34:17 2005 -0700\n    \nRevert the extra-careful check in write-tree.c\n\nI am not particularly fond of reverting this, but with the\ngit-update-cache changes it is unlikely that we will have file\nvs directory conflicts in the index anymore, so there may be no\npoint in being extra careful when writing the tree out.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n\n--- a/write-tree.c\n+++ b/write-tree.c\n@@ -84,7 +84,7 @@ static int write_tree(struct cache_entry\n \n int main(int argc, char **argv)\n {\n-\tint i, funny;\n+\tint i, unmerged;\n \tint entries = read_cache();\n \tunsigned char sha1[20];\n \n@@ -92,45 +92,18 @@ int main(int argc, char **argv)\n \t\tdie(\"write-tree: no cache contents to write\");\n \n \t/* Verify that the tree is merged */\n-\tfunny = 0;\n+\tunmerged = 0;\n \tfor (i = 0; i < entries; i++) {\n \t\tstruct cache_entry *ce = active_cache[i];\n \t\tif (ntohs(ce->ce_flags) & ~CE_NAMEMASK) {\n-\t\t\tif (10 < ++funny) {\n+\t\t\tif (++unmerged > 10) {\n \t\t\t\tfprintf(stderr, \"...\\n\");\n \t\t\t\tbreak;\n \t\t\t}\n \t\t\tfprintf(stderr, \"%s: unmerged (%s)\\n\", ce->name, sha1_to_hex(ce->sha1));\n \t\t}\n \t}\n-\tif (funny)\n-\t\tdie(\"write-tree: not able to write tree\");\n-\n-\t/* Also verify that the cache does not have path and path/file\n-\t * at the same time.  At this point we know the cache has only\n-\t * stage 0 entries.\n-\t */\n-\tfunny = 0;\n-\tfor (i = 0; i < entries - 1; i++) {\n-\t\t/* path/file always comes after path because of the way\n-\t\t * the cache is sorted.  Also path can appear only once,\n-\t\t * which means conflicting one would immediately follow.\n-\t\t */\n-\t\tconst char *this_name = active_cache[i]->name;\n-\t\tconst char *next_name = active_cache[i+1]->name;\n-\t\tint this_len = strlen(this_name);\n-\t\tif (this_len < strlen(next_name) &&\n-\t\t    strncmp(this_name, next_name, this_len) == 0 &&\n-\t\t    next_name[this_len] == '/') {\n-\t\t\tif (10 < ++funny) {\n-\t\t\t\tfprintf(stderr, \"...\\n\");\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t\tfprintf(stderr, \"You have both %s and %s\\n\",\n-\t\t\t\tthis_name, next_name);\n-\t\t}\n-\t}\n-\tif (funny)\n+\tif (unmerged)\n \t\tdie(\"write-tree: not able to write tree\");\n \n \t/* Ok, write it out */\n\n\n\n"}]}