{"thread":{"id":"2884","subject":"[PATCH] Racy GIT","startedAt":"2005-12-20T08:38:47Z","lastAt":"2005-12-20T19:28:17Z","messageCount":2,"participants":["Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"13835","messageId":"7vwti0go2w.fsf@assigned-by-dhcp.cox.net","threadId":"2884","inReplyTo":null,"subject":"[PATCH] Racy GIT","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-12-20T08:38:47Z","receivedAt":"2005-12-20T08:38:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This fixes the longstanding \"Racy GIT\" problem, which was pretty\nmuch there from the beginning of time, but was first\ndemonstrated by Pasky in this message on October 24, 2005:\n\n    http://marc.theaimsgroup.com/?l=git&m=113014629716878\n\nIf you run the following sequence of commands:\n\n\techo frotz >infocom\n        git update-index --add infocom\n        echo xyzzy >infocom\n\nso that the second update to file \"infocom\" does not change\nst_mtime, what is recorded as the stat information for the cache\nentry \"infocom\" exactly matches what is on the filesystem\n(owner, group, inum, mtime, ctime, mode, length).  After this\nsequence, we incorrectly think \"infocom\" file still has string\n\"frotz\" in it, and get really confused.  E.g. git-diff-files\nwould say there is no change, git-update-index --refresh would\nnot even look at the filesystem to correct the situation.\n\nSome ways of working around this issue were already suggested by\nLinus in the same thread on the same day, including waiting\nuntil the next second before returning from update-index if a\ncache entry written out has the current timestamp, but that\nmeans we can make at most one commit per second, and given that\nthe e-mail patch workflow used by Linus needs to process at\nleast 5 commits per second, it is not an acceptable solution.\nLinus notes that git-apply is primarily used to update the index\nwhile processing e-mailed patches, which is true, and\ngit-apply's up-to-date check is fooled by the same problem but\nluckily in the other direction, so it is not really a big issue,\nbut still it is disturbing.\n\nThe function ce_match_stat() is called to bypass the comparison\nagainst filesystem data when the stat data recorded in the cache\nentry matches what stat() returns from the filesystem.  This\npatch tackles the problem by changing it to actually go to the\nfilesystem data for cache entries that have the same mtime as\nthe index file itself.  This works as long as the index file and\nworking tree files are on the filesystems that share the same\nmonotonic clock.  Files on network mounted filesystems sometimes\nget skewed timestamps compared to \"date\" output, but as long as\nworking tree files' timestamps are skewed the same way as the\nindex file's, this approach still works.  The only problematic\nfiles are the ones that have the same timestamp as the index\nfile's, because two file updates that sandwitch the index file\nupdate must happen within the same second to trigger the\nproblem.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n\n---\n\n read-cache.c        |  140 +++++++++++++++++++++++++++++++--------------------\n t/t0010-racy-git.sh |   24 +++++++++\n 2 files changed, 110 insertions(+), 54 deletions(-)\n create mode 100755 t/t0010-racy-git.sh\n\n29e4d3635709778bcc808dbad0477efad82f8d7e\ndiff --git a/read-cache.c b/read-cache.c\nindex 6932736..780601f 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -6,6 +6,7 @@\n #include \"cache.h\"\n \n struct cache_entry **active_cache = NULL;\n+static time_t index_file_timestamp;\n unsigned int active_nr = 0, active_alloc = 0, active_cache_changed = 0;\n \n /*\n@@ -28,6 +29,64 @@ void fill_stat_cache_info(struct cache_e\n \tce->ce_size = htonl(st->st_size);\n }\n \n+static int ce_compare_data(struct cache_entry *ce, struct stat *st)\n+{\n+\tint match = -1;\n+\tint fd = open(ce->name, O_RDONLY);\n+\n+\tif (fd >= 0) {\n+\t\tunsigned char sha1[20];\n+\t\tif (!index_fd(sha1, fd, st, 0, NULL))\n+\t\t\tmatch = memcmp(sha1, ce->sha1, 20);\n+\t\tclose(fd);\n+\t}\n+\treturn match;\n+}\n+\n+static int ce_compare_link(struct cache_entry *ce, unsigned long expected_size)\n+{\n+\tint match = -1;\n+\tchar *target;\n+\tvoid *buffer;\n+\tunsigned long size;\n+\tchar type[10];\n+\tint len;\n+\n+\ttarget = xmalloc(expected_size);\n+\tlen = readlink(ce->name, target, expected_size);\n+\tif (len != expected_size) {\n+\t\tfree(target);\n+\t\treturn -1;\n+\t}\n+\tbuffer = read_sha1_file(ce->sha1, type, &size);\n+\tif (!buffer) {\n+\t\tfree(target);\n+\t\treturn -1;\n+\t}\n+\tif (size == expected_size)\n+\t\tmatch = memcmp(buffer, target, size);\n+\tfree(buffer);\n+\tfree(target);\n+\treturn match;\n+}\n+\n+static int ce_modified_check_fs(struct cache_entry *ce, struct stat *st)\n+{\n+\tswitch (st->st_mode & S_IFMT) {\n+\tcase S_IFREG:\n+\t\tif (ce_compare_data(ce, st))\n+\t\t\treturn DATA_CHANGED;\n+\t\tbreak;\n+\tcase S_IFLNK:\n+\t\tif (ce_compare_link(ce, st->st_size))\n+\t\t\treturn DATA_CHANGED;\n+\t\tbreak;\n+\tdefault:\n+\t\treturn TYPE_CHANGED;\n+\t}\n+\treturn 0;\n+}\n+\n int ce_match_stat(struct cache_entry *ce, struct stat *st)\n {\n \tunsigned int changed = 0;\n@@ -83,57 +142,37 @@ int ce_match_stat(struct cache_entry *ce\n \n \tif (ce->ce_size != htonl(st->st_size))\n \t\tchanged |= DATA_CHANGED;\n-\treturn changed;\n-}\n-\n-static int ce_compare_data(struct cache_entry *ce, struct stat *st)\n-{\n-\tint match = -1;\n-\tint fd = open(ce->name, O_RDONLY);\n \n-\tif (fd >= 0) {\n-\t\tunsigned char sha1[20];\n-\t\tif (!index_fd(sha1, fd, st, 0, NULL))\n-\t\t\tmatch = memcmp(sha1, ce->sha1, 20);\n-\t\tclose(fd);\n-\t}\n-\treturn match;\n-}\n-\n-static int ce_compare_link(struct cache_entry *ce, unsigned long expected_size)\n-{\n-\tint match = -1;\n-\tchar *target;\n-\tvoid *buffer;\n-\tunsigned long size;\n-\tchar type[10];\n-\tint len;\n+\t/*\n+\t * Within 1 second of this sequence:\n+\t * \techo xyzzy >file && git-update-index --add file\n+\t * running this command:\n+\t * \techo frotz >file\n+\t * would give a falsely clean cache entry.  The mtime and\n+\t * length match the cache, and other stat fields do not change.\n+\t *\n+\t * We could detect this at update-index time (the cache entry\n+\t * being registered/updated records the same time as \"now\")\n+\t * and delay the return from git-update-index, but that would\n+\t * effectively mean we can make at most one commit per second,\n+\t * which is not acceptable.  Instead, we check cache entries\n+\t * whose mtime are the same as the index file timestamp more\n+\t * careful than others.\n+\t */\n+\tif (!changed &&\n+\t    index_file_timestamp &&\n+\t    index_file_timestamp <= ntohl(ce->ce_mtime.sec))\n+\t\tchanged |= ce_modified_check_fs(ce, st);\n \n-\ttarget = xmalloc(expected_size);\n-\tlen = readlink(ce->name, target, expected_size);\n-\tif (len != expected_size) {\n-\t\tfree(target);\n-\t\treturn -1;\n-\t}\n-\tbuffer = read_sha1_file(ce->sha1, type, &size);\n-\tif (!buffer) {\n-\t\tfree(target);\n-\t\treturn -1;\n-\t}\n-\tif (size == expected_size)\n-\t\tmatch = memcmp(buffer, target, size);\n-\tfree(buffer);\n-\tfree(target);\n-\treturn match;\n+\treturn changed;\n }\n \n int ce_modified(struct cache_entry *ce, struct stat *st)\n {\n-\tint changed;\n+\tint changed, changed_fs;\n \tchanged = ce_match_stat(ce, st);\n \tif (!changed)\n \t\treturn 0;\n-\n \t/*\n \t * If the mode or type has changed, there's no point in trying\n \t * to refresh the entry - it's not going to match\n@@ -148,18 +187,9 @@ int ce_modified(struct cache_entry *ce, \n \tif ((changed & DATA_CHANGED) && ce->ce_size != htonl(0))\n \t\treturn changed;\n \n-\tswitch (st->st_mode & S_IFMT) {\n-\tcase S_IFREG:\n-\t\tif (ce_compare_data(ce, st))\n-\t\t\treturn changed | DATA_CHANGED;\n-\t\tbreak;\n-\tcase S_IFLNK:\n-\t\tif (ce_compare_link(ce, st->st_size))\n-\t\t\treturn changed | DATA_CHANGED;\n-\t\tbreak;\n-\tdefault:\n-\t\treturn changed | TYPE_CHANGED;\n-\t}\n+\tchanged_fs = ce_modified_check_fs(ce, st);\n+\tif (changed_fs)\n+\t\treturn changed | changed_fs;\n \treturn 0;\n }\n \n@@ -471,6 +501,7 @@ int read_cache(void)\n \t\treturn active_nr;\n \n \terrno = ENOENT;\n+\tindex_file_timestamp = 0;\n \tfd = open(get_index_file(), O_RDONLY);\n \tif (fd < 0) {\n \t\tif (errno == ENOENT)\n@@ -504,6 +535,7 @@ int read_cache(void)\n \t\toffset = offset + ce_size(ce);\n \t\tactive_cache[i] = ce;\n \t}\n+\tindex_file_timestamp = st.st_mtime;\n \treturn active_nr;\n \n unmap:\ndiff --git a/t/t0010-racy-git.sh b/t/t0010-racy-git.sh\nnew file mode 100755\nindex 0000000..eb175b7\n--- /dev/null\n+++ b/t/t0010-racy-git.sh\n@@ -0,0 +1,24 @@\n+#!/bin/sh\n+\n+test_description='racy GIT'\n+\n+. ./test-lib.sh\n+\n+# This test can give false success if your machine is sufficiently\n+# slow or your trial happened to happen on second boundary.\n+\n+for trial in 0 1 2 3 4 5 6 7 8 9\n+do\n+\trm -f .git/index\n+\techo frotz >infocom\n+\techo xyzzy >activision\n+\tgit update-index --add infocom activision\n+\techo xyzzy >infocom\n+\n+\tfiles=`git diff-files -p`\n+\ttest_expect_success \\\n+\t\"Racy GIT trial #$trial\" \\\n+\t'test \"\" != \"$files\"'\n+done\n+\n+test_done\n-- \n0.99.9.GIT\n"},{"id":"13841","messageId":"7vr787h8ku.fsf@assigned-by-dhcp.cox.net","threadId":"2884","inReplyTo":"7vwti0go2w.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Racy GIT","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-12-20T19:28:17Z","receivedAt":"2005-12-20T19:28:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <junkio@cox.net> writes:\n\n> This fixes the longstanding \"Racy GIT\" problem,...\n\nSorry, it does not really.\n\n> If you run the following sequence of commands:\n>\n>         echo frotz >infocom\n>         git update-index --add infocom\n>         echo xyzzy >infocom\n\nThe patch deals with the above sequence correctly, but it breaks\ndown with:\n\n\techo frotz >infocom\n        git update-index --add infocom\n        echo xyzzy >infocom\n        sleep 3 ;# a little while later\n        echo nitfol >activision\n        git update-index --add activision\n        git diff-files infocom\n\nWe need to detect the false clean entries just before we write\nout index in the second update-index and mark them out of date.\n"}]}