{"thread":{"id":"15990","subject":"[PATCH 2/2] rm: loosen safety valve for empty files","startedAt":"2008-10-21T00:37:12Z","lastAt":"2008-10-21T13:54:19Z","messageCount":3,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"93568","messageId":"20081021003712.GB32569@coredump.intra.peff.net","threadId":"15990","inReplyTo":null,"subject":"[PATCH 2/2] rm: loosen safety valve for empty files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-21T00:37:12Z","receivedAt":"2008-10-21T00:37:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If a file is different between the working tree copy, the\nindex, and the HEAD, then we do not allow it to be deleted\nwithout --force.\n\nHowever, this is overly tight in the face of \"git add\n--intent-to-add\":\n\n  $ git add --intent-to-add file\n  $ : oops, I don't actually want to stage that yet\n  $ git rm --cached file\n  error: 'file' has staged content different from both the\n  file and the HEAD (use -f to force removal)\n  $ git rm -f --cached file\n\nThis patch loosens the safety valve to allow the deletion\nonly if we are deleting the cached entry and the cached\ncontent is empty.\n\nThis covers the intent-to-add situation, and presumably\nthere is little harm in not protecting users who have\nlegitimately added an empty file. In many cases, the file\nwill still be empty, in which case the safety valve does not\ntrigger anyway (since the content remains untouched in the\nworking tree). Otherwise, we do remove the fact that no\ncontent was staged, but given that the content is by\ndefinition empty, it is not terribly difficult for a user to\nrecreate it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin-rm.c  |    3 ++-\n cache.h       |    1 +\n read-cache.c  |    2 +-\n t/t3600-rm.sh |    7 +++++++\n 4 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-rm.c b/builtin-rm.c\nindex e06640c..b7126e3 100644\n--- a/builtin-rm.c\n+++ b/builtin-rm.c\n@@ -79,7 +79,8 @@ static int check_local_mod(unsigned char *head, int index_only)\n \t\t     || hashcmp(ce->sha1, sha1))\n \t\t\tstaged_changes = 1;\n \n-\t\tif (local_changes && staged_changes)\n+\t\tif (local_changes && staged_changes &&\n+\t\t    !(index_only && is_empty_blob_sha1(ce->sha1)))\n \t\t\terrs = error(\"'%s' has staged content different \"\n \t\t\t\t     \"from both the file and the HEAD\\n\"\n \t\t\t\t     \"(use -f to force removal)\", name);\ndiff --git a/cache.h b/cache.h\nindex ae6647e..7141705 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -517,6 +517,7 @@ static inline void hashclr(unsigned char *hash)\n {\n \tmemset(hash, 0, 20);\n }\n+extern int is_empty_blob_sha1(const unsigned char *sha1);\n \n int git_mkstemp(char *path, size_t n, const char *template);\n \ndiff --git a/read-cache.c b/read-cache.c\nindex 780f2c7..d624cb3 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -160,7 +160,7 @@ static int ce_modified_check_fs(struct cache_entry *ce, struct stat *st)\n \treturn 0;\n }\n \n-static int is_empty_blob_sha1(const unsigned char *sha1)\n+int is_empty_blob_sha1(const unsigned char *sha1)\n {\n \tstatic const unsigned char empty_blob_sha1[20] = {\n \t\t0xe6,0x9d,0xe2,0x9b,0xb2,0xd1,0xd6,0x43,0x4b,0x8b,\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 66aca99..88ae672 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -187,6 +187,13 @@ test_expect_success 'but with -f it should work.' '\n \ttest_must_fail git ls-files --error-unmatch baz\n '\n \n+test_expect_success 'ok to remove cached empty file' '\n+\ttouch empty &&\n+\tgit add empty &&\n+\techo content >empty &&\n+\tgit rm --cached empty\n+'\n+\n test_expect_success 'Recursive test setup' '\n \tmkdir -p frotz &&\n \techo qfwfq >frotz/nitfol &&\n-- \n1.6.0.2.770.gb4241.dirty\n"},{"id":"93572","messageId":"7v4p361x1f.fsf@gitster.siamese.dyndns.org","threadId":"15990","inReplyTo":"20081021003712.GB32569@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] rm: loosen safety valve for empty files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-21T02:50:36Z","receivedAt":"2008-10-21T02:50:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This covers the intent-to-add situation, and presumably\n> there is little harm in not protecting users who have\n> legitimately added an empty file. In many cases, the file\n> will still be empty, in which case the safety valve does not\n> trigger anyway (since the content remains untouched in the\n> working tree). Otherwise, we do remove the fact that no\n> content was staged, but given that the content is by\n> definition empty, it is not terribly difficult for a user to\n> recreate it.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> ...\n> +test_expect_success 'ok to remove cached empty file' '\n> +\ttouch empty &&\n> +\tgit add empty &&\n> +\techo content >empty &&\n> +\tgit rm --cached empty\n> +'\n\nI am actually of two minds about this patch.\n\nWith one of the commits in the nd/narrow series, we can easily add more\nflag bits to the index entries, and it is conceivable that we would want\nto change the \"add -N\" implementation to set an \"intent to add\" bit (which\nwe don't), in addition to registering an empty blob at the path (which we\ncurrently do).  I envision that such a change would allow us to:\n\n - let \"git diff\" continue to diff with an emptiness and keep showing what\n   people would expect;\n\n - teach \"git write-tree\" (and various \"commit\" building commands) to\n   either (1) ignore a staged empty blob when the \"intent to add\" bit is\n   set, or (2) warn and abort, saying \"you told me you will tell me what\n   the actual contents will be later, but you haven't done so -- I'll\n   refuse to operate until you make up your mind\";\n\nin addition to what you are trying to fix here with \"git rm\".  With such a\nchange, your \"git rm empty\" code can also distinguish between an empty\nblob the user wanted to add _as the final contents_, and a path that has\nbeen marked with \"add -N\", and behave differently (the former would not\nrequire -f while the latter would).\n\nAs an interim measure, I suspect your patch is an improvement from the\ncurrent state of affairs, but the above test will then break when an\nimprovement to \"git add -N\" implementation outlined above materializes.\n\nSo how about changing the test to explicitly check that a path that was\nadded by \"git add -N\" can be removed, and either (1) not check about an\nempty blob that was explicitly added by the user, or (2) check that an\nempty blob that was explicitly added by the user cannot be \"git rm\"'ed\nwithout -f, with expect_failure?\n"},{"id":"93604","messageId":"20081021135419.GA17999@coredump.intra.peff.net","threadId":"15990","inReplyTo":"7v4p361x1f.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] rm: loosen safety valve for empty files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-21T13:54:19Z","receivedAt":"2008-10-21T13:54:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 20, 2008 at 07:50:36PM -0700, Junio C Hamano wrote:\n\n> in addition to what you are trying to fix here with \"git rm\".  With such a\n> change, your \"git rm empty\" code can also distinguish between an empty\n> blob the user wanted to add _as the final contents_, and a path that has\n> been marked with \"add -N\", and behave differently (the former would not\n> require -f while the latter would).\n\nYour reasoning makes sense.\n\n> As an interim measure, I suspect your patch is an improvement from the\n> current state of affairs, but the above test will then break when an\n> improvement to \"git add -N\" implementation outlined above materializes.\n\nYeah, I actually considered writing it as \"git add -N\", but I thought it\nbetter that the person who changes the behavior breaks the test, reads\nthe commit message, and decides then what the best behavior is.\n\nBut if you are comfortable saying now \"this is what the behavior\n_should_ be\", then I think the test modifications you proposed make\nsense (and after reading your discussion above, I think the behavior you\npropose is reasonable).\n\n> So how about changing the test to explicitly check that a path that was\n> added by \"git add -N\" can be removed, and either (1) not check about an\n> empty blob that was explicitly added by the user, or (2) check that an\n> empty blob that was explicitly added by the user cannot be \"git rm\"'ed\n> without -f, with expect_failure?\n\nPatch below. I tried to expand the commit message to explain the\nsituation to the future developer who extends intent-to-add, but please\nmark up if anything seems unclear.\n\n-- >8 --\nrm: loosen safety valve for empty files\n\nIf a file is different between the working tree copy, the\nindex, and the HEAD, then we do not allow it to be deleted\nwithout --force.\n\nHowever, this is overly tight in the face of \"git add\n--intent-to-add\":\n\n  $ git add --intent-to-add file\n  $ : oops, I don't actually want to stage that yet\n  $ git rm --cached file\n  error: 'empty' has staged content different from both the\n  file and the HEAD (use -f to force removal)\n  $ git rm -f --cached file\n\nUnfortunately, there is currently no way to distinguish\nbetween an empty file that has been added and an \"intent to\nadd\" file. The ideal behavior would be to disallow the\nformer while allowing the latter.\n\nThis patch loosens the safety valve to allow the deletion\nonly if we are deleting the cached entry and the cached\ncontent is empty.  This covers the intent-to-add situation,\nand assumes there is little harm in not protecting users who\nhave legitimately added an empty file.  In many cases,\nthe file will still be empty, in which case the safety valve\ndoes not trigger anyway (since the content remains untouched\nin the working tree). Otherwise, we do remove the fact that\nno content was staged, but given that the content is by\ndefinition empty, it is not terribly difficult for a user to\nrecreate it.\n\nHowever, we still document the desired behavior in the form\nof two tests. One checks the correct removal of an\nintent-to-add file. The other checks that we still disallow\nremoval of empty files, but is marked as expect_failure to\nindicate this compromise. If the intent-to-add feature is\never extended to differentiate between normal empty files\nand intent-to-add files, then the safety valve can be\nre-tightened.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin-rm.c  |    3 ++-\n cache.h       |    1 +\n read-cache.c  |    2 +-\n t/t3600-rm.sh |   13 +++++++++++++\n 4 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-rm.c b/builtin-rm.c\nindex e06640c..b7126e3 100644\n--- a/builtin-rm.c\n+++ b/builtin-rm.c\n@@ -79,7 +79,8 @@ static int check_local_mod(unsigned char *head, int index_only)\n \t\t     || hashcmp(ce->sha1, sha1))\n \t\t\tstaged_changes = 1;\n \n-\t\tif (local_changes && staged_changes)\n+\t\tif (local_changes && staged_changes &&\n+\t\t    !(index_only && is_empty_blob_sha1(ce->sha1)))\n \t\t\terrs = error(\"'%s' has staged content different \"\n \t\t\t\t     \"from both the file and the HEAD\\n\"\n \t\t\t\t     \"(use -f to force removal)\", name);\ndiff --git a/cache.h b/cache.h\nindex cdbeb48..b0edbf9 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -519,6 +519,7 @@ static inline void hashclr(unsigned char *hash)\n {\n \tmemset(hash, 0, 20);\n }\n+extern int is_empty_blob_sha1(const unsigned char *sha1);\n \n int git_mkstemp(char *path, size_t n, const char *template);\n \ndiff --git a/read-cache.c b/read-cache.c\nindex fdb41b8..2c45086 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -160,7 +160,7 @@ static int ce_modified_check_fs(struct cache_entry *ce, struct stat *st)\n \treturn 0;\n }\n \n-static int is_empty_blob_sha1(const unsigned char *sha1)\n+int is_empty_blob_sha1(const unsigned char *sha1)\n {\n \tstatic const unsigned char empty_blob_sha1[20] = {\n \t\t0xe6,0x9d,0xe2,0x9b,0xb2,0xd1,0xd6,0x43,0x4b,0x8b,\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 66aca99..5b4d6f7 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -187,6 +187,19 @@ test_expect_success 'but with -f it should work.' '\n \ttest_must_fail git ls-files --error-unmatch baz\n '\n \n+test_expect_failure 'refuse to remove cached empty file with modifications' '\n+\ttouch empty &&\n+\tgit add empty &&\n+\techo content >empty &&\n+\ttest_must_fail git rm --cached empty\n+'\n+\n+test_expect_success 'remove intent-to-add file without --force' '\n+\techo content >intent-to-add &&\n+\tgit add -N intent-to-add\n+\tgit rm --cached intent-to-add\n+'\n+\n test_expect_success 'Recursive test setup' '\n \tmkdir -p frotz &&\n \techo qfwfq >frotz/nitfol &&\n-- \n1.6.0.2.808.gf6cf2\n"}]}