{"thread":{"id":"7264","subject":"bug in read-tree -m on A -> A/A","startedAt":"2007-03-16T04:19:32Z","lastAt":"2007-03-16T06:25:22Z","messageCount":3,"participants":["Shawn O. Pearce","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"37199","messageId":"20070316041932.GA31606@spearce.org","threadId":"7264","inReplyTo":null,"subject":"bug in read-tree -m on A -> A/A","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-03-16T04:19:32Z","receivedAt":"2007-03-16T04:19:32Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"dancor on #git just noticed a bug in read-tree -m that prevents\nhim from switching branches when the type of a path changes\nbetween a directory and a file.  The following test appears to\ntrigger the same failure, but I likely won't have time to fix\nit myself tonight.  So if someone else gets a chance...\n\ndiff --git a/t/t9999-typechange.sh b/t/t9999-typechange.sh\nnew file mode 100755\nindex 0000000..dc92094\n--- /dev/null\n+++ b/t/t9999-typechange.sh\n@@ -0,0 +1,25 @@\n+#!/bin/sh\n+\n+test_description='switch branches over path typechange'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\t: >A &&\n+\tgit-add A &&\n+\ttree1=$(git-write-tree) &&\n+\tcomm1=$(echo A | git-commit-tree $tree1) &&\n+\tgit-update-ref refs/heads/comm1 $comm1 &&\n+\trm .git/index &&\n+\trm -f A &&\n+\tmkdir A &&\n+\techo : >A/A &&\n+\tgit-add A/A &&\n+\ttree2=$(git-write-tree) &&\n+\tcomm2=$(echo A/A | git-commit-tree $tree2) &&\n+\tgit-update-ref HEAD $comm2\n+'\n+\n+test_expect_success checkout 'git-checkout $comm1'\n+\n+test_done\n\n-- \nShawn.\n"},{"id":"37204","messageId":"7v4polzsrn.fsf@assigned-by-dhcp.cox.net","threadId":"7264","inReplyTo":"20070316041932.GA31606@spearce.org","subject":"Re: bug in read-tree -m on A -> A/A","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-16T05:01:16Z","receivedAt":"2007-03-16T05:01:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n> ....  The following test appears to\n> trigger the same failure,...\n\nYou have file \"A\" on one branch, and file \"A/A\" on another\nbranch.  You are on the latter branch and switching to the\nformer one.\n\nThe following patch illustrates where you need to implement an\nalternate, loosened check, but should not be applied to your\ntree as-is.  If you have local modification to path \"A/A\", this\nwill lose it.\n\n-- >8 --\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 2e2232c..345b2ee 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -479,9 +479,27 @@ static void verify_absent(const char *path, const char *action,\n \n \tif (o->index_only || o->reset || !o->update)\n \t\treturn;\n-\tif (!lstat(path, &st) && !(o->dir && excluded(o->dir, path)))\n+\n+\tif (!lstat(path, &st)) {\n+\t\tif (o->dir && excluded(o->dir, path))\n+\t\t\t/*\n+\t\t\t * path is explicitly excluded, so it is Ok to\n+\t\t\t * overwrite it.\n+\t\t\t */\n+\t\t\treturn;\n+\t\tif (S_ISDIR(st.st_mode))\n+\t\t\t/*\n+\t\t\t * We are checking out path \"foo\" and\n+\t\t\t * found \"foo/.\" in the working tree.\n+\t\t\t * This is tricky -- if we have modified\n+\t\t\t * files that are in \"foo/\" we would lose\n+\t\t\t * it if we just uncoditinally return here.\n+\t\t\t */\n+\t\t\treturn;\n+\n \t\tdie(\"Untracked working tree file '%s' \"\n \t\t    \"would be %s by merge.\", path, action);\n+\t}\n }\n \n static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n\n\n-- 8< --\n\nTo solve this sanely I think you would need to merge bottom-up,\nwhich is quite a large change to read-tree.  Currently the code\nasks \"is it Ok to extract A from the new tree?\" and if we say\nyes here it would remove all existing cache entries \"A/*\" and\nreplaces it with \"A\".  After that happens, you would not even\nhave a chance to see if A/A has local modifications, or if you\nhave a local file that is not even known to git, say A/C, that\nwill also be lost by this tree switching.\n"},{"id":"37208","messageId":"7v6491yab1.fsf@assigned-by-dhcp.cox.net","threadId":"7264","inReplyTo":"7v4polzsrn.fsf@assigned-by-dhcp.cox.net","subject":"Re: bug in read-tree -m on A -> A/A","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-16T06:25:22Z","receivedAt":"2007-03-16T06:25:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <junkio@cox.net> writes:\n\n> \"Shawn O. Pearce\" <spearce@spearce.org> writes:\n>\n>> ....  The following test appears to\n>> trigger the same failure,...\n>\n> You have file \"A\" on one branch, and file \"A/A\" on another\n> branch.  You are on the latter branch and switching to the\n> former one.\n>\n> The following patch illustrates where you need to implement an\n> alternate, loosened check, but should not be applied to your\n> tree as-is.  If you have local modification to path \"A/A\", this\n> will lose it.\n\nThis does not do the bottom-up merge, but tries to catch the\nlossy case within the limit of the current framework.\n\nOnly lightly tested, and I won't be applying it as-is yet as I\nam not thinking very clearly tonight (no, I am not drunk, just\nunder the weather a bit).\n\nTesting and improvements are very much appreciated.\n\n---\n unpack-trees.c |   75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 74 insertions(+), 1 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 2e2232c..2288762 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -468,6 +468,60 @@ static void invalidate_ce_path(struct cache_entry *ce)\n \t\tcache_tree_invalidate_path(active_cache_tree, ce->name);\n }\n \n+static void verify_clean_subdirectory(const char *path, const char *action,\n+\t\t\t\t      struct unpack_trees_options *o)\n+{\n+\t/*\n+\t * we are about to extract \"path\"; we would not want to lose\n+\t * anything in the existing directory there.\n+\t */\n+\tint namelen;\n+\tint pos, i;\n+\tstruct dir_struct d;\n+\tchar *pathbuf;\n+\n+\t/*\n+\t * First let's make sure we do not have a local modification\n+\t * in that directory.\n+\t */\n+\tnamelen = strlen(path);\n+\tpos = cache_name_pos(path, namelen);\n+\tif (0 <= pos)\n+\t\treturn; /* we have it as nondirectory */\n+\tpos = -pos - 1;\n+\tfor (i = pos; i < active_nr; i++) {\n+\t\tstruct cache_entry *ce = active_cache[i];\n+\t\tint len = ce_namelen(ce);\n+\t\tif (len < namelen ||\n+\t\t    strncmp(path, ce->name, namelen) ||\n+\t\t    ce->name[namelen] != '/')\n+\t\t\tbreak;\n+\t\t/*\n+\t\t * ce->name is an entry in the subdirectory.\n+\t\t */\n+\t\tverify_uptodate(ce, o);\n+\t}\n+\n+\t/*\n+\t * Then we need to make sure that we do not lose a locally\n+\t * present file that is not ignored.\n+\t */\n+\tif (!o->dir)\n+\t\treturn;\n+\n+\tpathbuf = xmalloc(namelen + 2);\n+\tmemcpy(pathbuf, path, namelen);\n+\tstrcpy(pathbuf+namelen, \"/\");\n+\n+\tmemset(&d, 0, sizeof(d));\n+\td.exclude_per_dir = o->dir->exclude_per_dir;\n+\ti = read_directory(&d, path, pathbuf, namelen+1);\n+\tif (i)\n+\t\tdie(\"Updating '%s' would lose untracked files in it\",\n+\t\t    path);\n+\tfree(pathbuf);\n+}\n+\n /*\n  * We do not want to remove or overwrite a working tree file that\n  * is not tracked, unless it is ignored.\n@@ -479,9 +533,28 @@ static void verify_absent(const char *path, const char *action,\n \n \tif (o->index_only || o->reset || !o->update)\n \t\treturn;\n-\tif (!lstat(path, &st) && !(o->dir && excluded(o->dir, path)))\n+\n+\tif (!lstat(path, &st)) {\n+\t\tif (o->dir && excluded(o->dir, path))\n+\t\t\t/*\n+\t\t\t * path is explicitly excluded, so it is Ok to\n+\t\t\t * overwrite it.\n+\t\t\t */\n+\t\t\treturn;\n+\t\tif (S_ISDIR(st.st_mode))\n+\t\t\t/*\n+\t\t\t * We are checking out path \"foo\" and\n+\t\t\t * found \"foo/.\" in the working tree.\n+\t\t\t * This is tricky -- if we have modified\n+\t\t\t * files that are in \"foo/\" we would lose\n+\t\t\t * it.\n+\t\t\t */\n+\t\t\tverify_clean_subdirectory(path, action, o);\n+\t\t\treturn;\n+\n \t\tdie(\"Untracked working tree file '%s' \"\n \t\t    \"would be %s by merge.\", path, action);\n+\t}\n }\n \n static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n"}]}