{"thread":{"id":"46402","subject":"[PATCH/RFC] diff/read-cache: don't assume empty files will filter to empty","startedAt":"2017-07-17T00:55:33Z","lastAt":"2017-07-17T19:07:20Z","messageCount":2,"participants":["Andrei Purdea","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"324636","messageId":"CA+mMEgf1Pg0bbGesCFr6sY60PxR=ewKPqnkG1M3fO2qurqR=1g@mail.gmail.com","threadId":"46402","inReplyTo":null,"subject":"[PATCH/RFC] diff/read-cache: don't assume empty files will filter to empty","fromName":"Andrei Purdea","fromEmail":"andrei@purdea.ro","sentAt":"2017-07-17T00:55:07Z","receivedAt":"2017-07-17T00:55:33Z","isPatch":true,"sender":{"key":"andrei@purdea.ro","avatar":null},"body":"Hey guys, please see the below patch. (please read commit message\nfirst and then the following:)\nIn particular I'd like to hear comments about the changes I made to\nhow smudging works.\nI am not confident on whether it is a good way to permanently fix the problem.\nI'm wondering if there's a better way, and I am also wondering if this\nwill have an effect\nwhen a user upgrades git on their system, and they already had a\nrepository checked out.\n\nI know there were other approaches to smudging cache entries, for\nexample JGIT had\nthis slightly funny method:\nhttps://github.com/eclipse/jgit/commit/c98d97731b87417b196341fa63a50fffea4e123c#diff-2c773508260c1ab8eb01b5444c3395c8L320\nbut since then they have reverted to the c-git way of doing things.\n\nAny comments?\n\nAndrei\n\nFrom 296a83046e77c1d49db8d324f298280265dfb6dc Mon Sep 17 00:00:00 2001\nFrom: Purdea Andrei <andrei.purdea@movidius.com>\nDate: Tue, 11 Jul 2017 23:00:15 +0300\nSubject: [PATCH] diff/read-cache: don't assume empty files will filter to\n empty\n\nThere are some projects that are based on git smudge/clean filters that won't\nnecessarily produce empty files when clean-ing empty files. An example of such\na project is git-fat, which stores binary files elsewhere, but uses git to\nstore only a reference to that binary file. The reference to the file consists\nof a hash and the file size. So an empty will will produce a non-empty file\nafter passing it through the git-fat clean filter.\n\nCurrent GIT, when it sees an empty file on git diff, it interprets it specially,\nand it won't pass it through the clean filter. But in other conditions it will.\nFor example you can create a commit containing an empty git-fat managed file,\nand in that case the file will be passed through the clean filter, and the git\nrepository will contain the expanded hash of an empty file. If you try to run\ngit diff on such a repository, it will always show this file as changed, because\ngit diff will compare the expanded hash that is already in the repository, to\nthe empty file that has not been passed through the clean filter. This will\nresult in files that will always show up as modified. The \"changes\" cannot be\ncommitted, or checked-out to the working tree, and git will not allow us to\nperform more complex operations such as rebase / merge. The only action is to\ndelete the file.\n\nThe patch fixes the issue in two places:\n\ndiff.c: send the file through git-clean filter even if it is empty.\n\nread-cache.c: read-cache uses a magic number of zero-sized files to smudge\ncache entries that have the same timestamp as other file modifications.\n(Note: the word smudge here is unrelated to the \"smudge filter\" concept)\nSmudged entries will have a zero file size, but the stored hash will not equal\nthe unique, known hash of an empty file. In the absence of clean filters that\ncan produce non-zero files from zero-sized files, this is a good way to\ndetectably corrupt a cache entry. However in the presence of git clean filters\nthat do this, the file will be considered smudged, and it will keep showing up\nas touched by the user. That is because the stored hash is actually the hash\nof the file after it has passed to the clean filter, and it's not empty anymore.\nThis change fixes the condition by using a different magic number of smudged\nentries. It uses -1 which is the biggest possible file size stored.\n\nSigned-off-by: Purdea Andrei <andrei@purdea.ro>\n---\n diff.c       | 18 ++++++++++--------\n read-cache.c |  7 +++----\n 2 files changed, 13 insertions(+), 12 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 00b4c8669..76bb536ce 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2858,8 +2858,6 @@ int diff_populate_filespec(struct diff_filespec\n*s, unsigned int flags)\n  }\n  }\n  s->size = xsize_t(st.st_size);\n- if (!s->size)\n- goto empty;\n  if (S_ISLNK(st.st_mode)) {\n  struct strbuf sb = STRBUF_INIT;\n\n@@ -2894,12 +2892,16 @@ int diff_populate_filespec(struct\ndiff_filespec *s, unsigned int flags)\n  s->is_binary = 1;\n  return 0;\n  }\n- fd = open(s->path, O_RDONLY);\n- if (fd < 0)\n- goto err_empty;\n- s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);\n- close(fd);\n- s->should_munmap = 1;\n+ if (!s->size) {\n+ s->data = \"\";\n+ } else {\n+ fd = open(s->path, O_RDONLY);\n+ if (fd < 0)\n+ goto err_empty;\n+ s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);\n+ close(fd);\n+ s->should_munmap = 1;\n+ }\n\n  /*\n  * Convert from working tree format to canonical git format\ndiff --git a/read-cache.c b/read-cache.c\nindex 2121b6e7b..ca306993c 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -262,9 +262,8 @@ static int ce_match_stat_basic(const struct\ncache_entry *ce, struct stat *st)\n  changed |= match_stat_data(&ce->ce_stat_data, st);\n\n  /* Racily smudged entry? */\n- if (!ce->ce_stat_data.sd_size) {\n- if (!is_empty_blob_sha1(ce->oid.hash))\n- changed |= DATA_CHANGED;\n+ if (ce->ce_stat_data.sd_size == (unsigned int)-1) {\n+ changed |= DATA_CHANGED;\n  }\n\n  return changed;\n@@ -2028,7 +2027,7 @@ static void ce_smudge_racily_clean_entry(struct\ncache_entry *ce)\n  * file, and never calls us, so the cached size information\n  * for \"frotz\" stays 6 which does not match the filesystem.\n  */\n- ce->ce_stat_data.sd_size = 0;\n+ ce->ce_stat_data.sd_size = (unsigned int)-1;\n  }\n }\n\n-- \n2.11.0\n\n\nFrom 296a83046e77c1d49db8d324f298280265dfb6dc Mon Sep 17 00:00:00 2001\nFrom: Purdea Andrei <andrei.purdea@movidius.com>\nDate: Tue, 11 Jul 2017 23:00:15 +0300\nSubject: [PATCH] diff/read-cache: don't assume empty files will filter to\n empty\n\nThere are some projects that are based on git smudge/clean filters that won't\nnecessarily produce empty files when clean-ing empty files. An example of such\na project is git-fat, which stores binary files elsewhere, but uses git to\nstore only a reference to that binary file. The reference to the file consists\nof a hash and the file size. So an empty will will produce a non-empty file\nafter passing it through the git-fat clean filter.\n\nCurrent GIT, when it sees an empty file on git diff, it interprets it specially,\nand it won't pass it through the clean filter. But in other conditions it will.\nFor example you can create a commit containing an empty git-fat managed file,\nand in that case the file will be passed through the clean filter, and the git\nrepository will contain the expanded hash of an empty file. If you try to run\ngit diff on such a repository, it will always show this file as changed, because\ngit diff will compare the expanded hash that is already in the repository, to\nthe empty file that has not been passed through the clean filter. This will\nresult in files that will always show up as modified. The \"changes\" cannot be\ncommitted, or checked-out to the working tree, and git will not allow us to\nperform more complex operations such as rebase / merge. The only action is to\ndelete the file.\n\nThe patch fixes the issue in two places:\n\ndiff.c: send the file through git-clean filter even if it is empty.\n\nread-cache.c: read-cache uses a magic number of zero-sized files to smudge\ncache entries that have the same timestamp as other file modifications.\n(Note: the word smudge here is unrelated to the \"smudge filter\" concept)\nSmudged entries will have a zero file size, but the stored hash will not equal\nthe unique, known hash of an empty file. In the absence of clean filters that\ncan produce non-zero files from zero-sized files, this is a good way to\ndetectably corrupt a cache entry. However in the presence of git clean filters\nthat do this, the file will be considered smudged, and it will keep showing up\nas touched by the user. That is because the stored hash is actually the hash\nof the file after it has passed to the clean filter, and it's not empty anymore.\nThis change fixes the condition by using a different magic number of smudged\nentries. It uses -1 which is the biggest possible file size stored.\n\nSigned-off-by: Purdea Andrei <andrei@purdea.ro>\n---\n diff.c       | 18 ++++++++++--------\n read-cache.c |  7 +++----\n 2 files changed, 13 insertions(+), 12 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 00b4c8669..76bb536ce 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2858,8 +2858,6 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n \t\t\t}\n \t\t}\n \t\ts->size = xsize_t(st.st_size);\n-\t\tif (!s->size)\n-\t\t\tgoto empty;\n \t\tif (S_ISLNK(st.st_mode)) {\n \t\t\tstruct strbuf sb = STRBUF_INIT;\n \n@@ -2894,12 +2892,16 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n \t\t\ts->is_binary = 1;\n \t\t\treturn 0;\n \t\t}\n-\t\tfd = open(s->path, O_RDONLY);\n-\t\tif (fd < 0)\n-\t\t\tgoto err_empty;\n-\t\ts->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\t\tclose(fd);\n-\t\ts->should_munmap = 1;\n+\t\tif (!s->size) {\n+\t\t\ts->data = \"\";\n+\t\t} else {\n+\t\t\tfd = open(s->path, O_RDONLY);\n+\t\t\tif (fd < 0)\n+\t\t\t\tgoto err_empty;\n+\t\t\ts->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\t\t\tclose(fd);\n+\t\t\ts->should_munmap = 1;\n+\t\t}\n \n \t\t/*\n \t\t * Convert from working tree format to canonical git format\ndiff --git a/read-cache.c b/read-cache.c\nindex 2121b6e7b..ca306993c 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -262,9 +262,8 @@ static int ce_match_stat_basic(const struct cache_entry *ce, struct stat *st)\n \tchanged |= match_stat_data(&ce->ce_stat_data, st);\n \n \t/* Racily smudged entry? */\n-\tif (!ce->ce_stat_data.sd_size) {\n-\t\tif (!is_empty_blob_sha1(ce->oid.hash))\n-\t\t\tchanged |= DATA_CHANGED;\n+\tif (ce->ce_stat_data.sd_size == (unsigned int)-1) {\n+\t\tchanged |= DATA_CHANGED;\n \t}\n \n \treturn changed;\n@@ -2028,7 +2027,7 @@ static void ce_smudge_racily_clean_entry(struct cache_entry *ce)\n \t\t * file, and never calls us, so the cached size information\n \t\t * for \"frotz\" stays 6 which does not match the filesystem.\n \t\t */\n-\t\tce->ce_stat_data.sd_size = 0;\n+\t\tce->ce_stat_data.sd_size = (unsigned int)-1;\n \t}\n }\n \n-- \n2.11.0\n\n"},{"id":"324662","messageId":"xmqqpocyhogz.fsf@gitster.mtv.corp.google.com","threadId":"46402","inReplyTo":"CA+mMEgf1Pg0bbGesCFr6sY60PxR=ewKPqnkG1M3fO2qurqR=1g@mail.gmail.com","subject":"Re: [PATCH/RFC] diff/read-cache: don't assume empty files will filter to empty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-17T19:07:08Z","receivedAt":"2017-07-17T19:07:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrei Purdea <andrei@purdea.ro> writes:\n\n> diff --git a/read-cache.c b/read-cache.c\n> index 2121b6e7b..ca306993c 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -262,9 +262,8 @@ static int ce_match_stat_basic(const struct\n> cache_entry *ce, struct stat *st)\n>   changed |= match_stat_data(&ce->ce_stat_data, st);\n>\n>   /* Racily smudged entry? */\n> - if (!ce->ce_stat_data.sd_size) {\n> - if (!is_empty_blob_sha1(ce->oid.hash))\n> - changed |= DATA_CHANGED;\n> + if (ce->ce_stat_data.sd_size == (unsigned int)-1) {\n> + changed |= DATA_CHANGED;\n>   }\n>\n>   return changed;\n> @@ -2028,7 +2027,7 @@ static void ce_smudge_racily_clean_entry(struct\n> cache_entry *ce)\n>   * file, and never calls us, so the cached size information\n>   * for \"frotz\" stays 6 which does not match the filesystem.\n>   */\n> - ce->ce_stat_data.sd_size = 0;\n> + ce->ce_stat_data.sd_size = (unsigned int)-1;\n>   }\n>  }\n\nThis is not a good idea.  A change like this will break existing\ninstallations where a zeroed size is understood as \"not verified as\nclean\".\n\nIs it infeasible to fix your clean-smudge filter pair to clean\nand/or smudge zero-sized payload to empty?  After all, it does not\nhelp offloading the storing of blob that is known to be empty and\nread another emptiness from the network or whatever external service\nwhen the end result is known to be zero bytes anyway, no?\n\n"}]}