{"thread":{"id":"44487","subject":"Protecting old temporary objects being reused from concurrent \"git gc\"?","startedAt":"2016-11-15T14:13:24Z","lastAt":"2016-11-17T01:43:15Z","messageCount":13,"participants":["Matt McCutchen","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"305948","messageId":"1479219194.2406.73.camel@mattmccutchen.net","threadId":"44487","inReplyTo":null,"subject":"Protecting old temporary objects being reused from concurrent \"git gc\"?","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2016-11-15T14:13:14Z","receivedAt":"2016-11-15T14:13:24Z","isPatch":false,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"The Braid subproject management tool stores the subproject content in\nthe main tree and is able to switch to a different upstream revision of\na subproject by doing the equivalent of \"git read-tree -m\" on the\nsuperproject tree and the two upstream trees.  The tricky part is\npreparing temporary trees with the upstream content moved to the path\nconfigured for the superproject.  The usual method is \"git read-tree\n--prefix\", but using what index file?  Braid currently uses the user's\nactual worktree, which can leave a mess if it gets interrupted:\n\nhttps://github.com/cristibalan/braid/blob/7d81da6e86e24de62a74f3ab8d880666cb343b04/lib/braid/commands/update.rb#L98\n\nI want to change this to something that won't leave an inconsistent\nstate if interrupted.  I've written code for this kind of thing before\nthat sets GIT_INDEX_FILE and uses a temporary index file and \"git\nwrite-tree\".  But I realized that if \"git gc\" runs concurrently, the\ngenerated tree could be deleted before it is used and the tool would\nfail.  If I had a need to run \"git commit-tree\", it seems like I might\neven end up with a commit object with a broken reference to a tree.\n \"git gc\" normally doesn't delete objects that were created in the last\n2 weeks, but if an identical tree was added to the object database more\nthan 2 weeks ago by another operation and is unreferenced, it could be\nreused without updating its mtime and it could still get deleted.\n\nIs there a recommended way to avoid this kind of problem in add-on\ntools?  (I searched the Git documentation and the web for information\nabout races with \"git gc\" and didn't find anything useful.)  If not, it\nseems to be a significant design flaw in \"git gc\", even if the problem\nis extremely rare in practice.  I wonder if some of the built-in\ncommands may have the same problem, though I haven't tried to test\nthem.  If this is confirmed to be a known problem affecting built-in\ncommands, then at least I won't feel bad about introducing the\nsame problem into add-on tools. :/\n\nThanks,\nMatt\n"},{"id":"305962","messageId":"20161115170634.ichqrqbhmpv2dsiw@sigill.intra.peff.net","threadId":"44487","inReplyTo":"1479219194.2406.73.camel@mattmccutchen.net","subject":"Re: Protecting old temporary objects being reused from concurrent \"git gc\"?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-15T17:06:34Z","receivedAt":"2016-11-15T17:06:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 15, 2016 at 09:13:14AM -0500, Matt McCutchen wrote:\n\n> I want to change this to something that won't leave an inconsistent\n> state if interrupted.  I've written code for this kind of thing before\n> that sets GIT_INDEX_FILE and uses a temporary index file and \"git\n> write-tree\".  But I realized that if \"git gc\" runs concurrently, the\n> generated tree could be deleted before it is used and the tool would\n> fail.  If I had a need to run \"git commit-tree\", it seems like I might\n> even end up with a commit object with a broken reference to a tree.\n>  \"git gc\" normally doesn't delete objects that were created in the last\n> 2 weeks, but if an identical tree was added to the object database more\n> than 2 weeks ago by another operation and is unreferenced, it could be\n> reused without updating its mtime and it could still get deleted.\n\nModern versions of git do two things to help with this:\n\n - any object which is referenced by a \"recent\" object (within the 2\n   weeks) is also considered recent. So if you create a new commit\n   object that points to a tree, even before you reference the commit\n   that tree is protected\n\n - when an object write is optimized out because we already have the\n   object, git will update the mtime on the file (loose object or\n   packfile) to freshen it\n\nThis isn't perfect, though. You can decide to reference an existing\nobject just as it is being deleted. And the pruning process itself is\nnot atomic (and it's tricky to make it so, just because of what we're\npromised by the filesystem).\n\n> Is there a recommended way to avoid this kind of problem in add-on\n> tools?  (I searched the Git documentation and the web for information\n> about races with \"git gc\" and didn't find anything useful.)  If not, it\n> seems to be a significant design flaw in \"git gc\", even if the problem\n> is extremely rare in practice.  I wonder if some of the built-in\n> commands may have the same problem, though I haven't tried to test\n> them.  If this is confirmed to be a known problem affecting built-in\n> commands, then at least I won't feel bad about introducing the\n> same problem into add-on tools. :/\n\nIf you have long-running data (like, a temporary index file that might\nliterally sit around for days or weeks) I think that is a potential\nproblem. And the solution is probably to use refs in some way to point\nto your objects. If you're worried about a short-term operation where\nsomebody happens to run git-gc concurrently, I agree it's a possible\nproblem, but I suspect something you can ignore in practice.\n\nFor the most part, a lot of the client-side git tools assume that one\noperation is happening at a time in the repository. And I think that\nlargely holds for a developer working on a single clone, and things just\nwork in practice.\n\nAuto-gc makes that a little sketchier, but historically does not seem to\nhave really caused problems in practice.\n\nFor a busy multi-user server, I recommend turning off auto-gc entirely,\nand repacking manually with \"-k\" to be on the safe side.\n\n-Peff\n"},{"id":"305964","messageId":"1479231184.2406.88.camel@mattmccutchen.net","threadId":"44487","inReplyTo":"20161115170634.ichqrqbhmpv2dsiw@sigill.intra.peff.net","subject":"Re: Protecting old temporary objects being reused from concurrent \"git gc\"?","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2016-11-15T17:33:04Z","receivedAt":"2016-11-15T17:33:18Z","isPatch":false,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"On Tue, 2016-11-15 at 12:06 -0500, Jeff King wrote:\n>  - when an object write is optimized out because we already have the\n>    object, git will update the mtime on the file (loose object or\n>    packfile) to freshen it\n\nFWIW, I am not seeing this happen when I do \"git read-tree --prefix\"\nfollowed by \"git write-tree\" using the current master (3ab2281).  See\nthe attached test script.\n\n> If you have long-running data (like, a temporary index file that might\n> literally sit around for days or weeks) I think that is a potential\n> problem. And the solution is probably to use refs in some way to point\n> to your objects.\n\nAgreed.  This is not my current scenario.\n\n> If you're worried about a short-term operation where\n> somebody happens to run git-gc concurrently, I agree it's a possible\n> problem, but I suspect something you can ignore in practice.\n> \n> For the most part, a lot of the client-side git tools assume that one\n> operation is happening at a time in the repository. And I think that\n> largely holds for a developer working on a single clone, and things just\n> work in practice.\n> \n> Auto-gc makes that a little sketchier, but historically does not seem to\n> have really caused problems in practice.\n\nOK.  I'll write a patch to add a summary of this information to the\ngit-gc man page.\n\nMatt"},{"id":"305965","messageId":"20161115174028.zvohfcw4jse3jrmm@sigill.intra.peff.net","threadId":"44487","inReplyTo":"1479231184.2406.88.camel@mattmccutchen.net","subject":"Re: Protecting old temporary objects being reused from concurrent \"git gc\"?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-15T17:40:29Z","receivedAt":"2016-11-15T17:40:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 15, 2016 at 12:33:04PM -0500, Matt McCutchen wrote:\n\n> On Tue, 2016-11-15 at 12:06 -0500, Jeff King wrote:\n> >  - when an object write is optimized out because we already have the\n> >    object, git will update the mtime on the file (loose object or\n> >    packfile) to freshen it\n> \n> FWIW, I am not seeing this happen when I do \"git read-tree --prefix\"\n> followed by \"git write-tree\" using the current master (3ab2281).  See\n> the attached test script.\n\nThe optimization I'm thinking about is the one from write_sha1_file(),\nwhich learned to freshen in 33d4221c7 (write_sha1_file: freshen existing\nobjects, 2014-10-15).\n\nI suspect the issue is that read-tree populates the cache-tree index\nextension, and then write-tree omits the object write before it even\ngets to write_sha1_file(). The solution is that it should probably be\ncalling one of the freshen() functions (possibly just replacing\nhas_sha1_file() with check_and_freshen(), but I haven't looked).\n\nI'd definitely welcome patches in this area.\n\n> OK.  I'll write a patch to add a summary of this information to the\n> git-gc man page.\n\nSounds like a good idea. Thanks.\n\n-Peff\n"},{"id":"305976","messageId":"1479237042.2406.89.camel@mattmccutchen.net","threadId":"44487","inReplyTo":"20161115174028.zvohfcw4jse3jrmm@sigill.intra.peff.net","subject":"[PATCH] git-gc.txt: expand discussion of races with other processes","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2016-11-15T19:08:51Z","receivedAt":"2016-11-15T19:10:53Z","isPatch":true,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"In general, \"git gc\" may delete objects that another concurrent process\nis using but hasn't created a reference to.  Git has some mitigations,\nbut they fall short of a complete solution.  Document this in the\ngit-gc(1) man page and add a reference from the documentation of the\ngc.pruneExpire config variable.\n\nBased on a write-up by Jeff King:\n\nhttp://marc.info/?l=git&m=147922960131779&w=2\n\nSigned-off-by: Matt McCutchen <matt@mattmccutchen.net>\n---\n Documentation/config.txt |  4 +++-\n Documentation/git-gc.txt | 34 ++++++++++++++++++++++++++--------\n 2 files changed, 29 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 21fdddf..3f1d931 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1409,7 +1409,9 @@ gc.pruneExpire::\n \tOverride the grace period with this config variable.  The value\n \t\"now\" may be used to disable this grace period and always prune\n \tunreachable objects immediately, or \"never\" may be used to\n-\tsuppress pruning.\n+\tsuppress pruning.  This feature helps prevent corruption when\n+\t'git gc' runs concurrently with another process writing to the\n+\trepository; see the \"NOTES\" section of linkgit:git-gc[1].\n \n gc.worktreePruneExpire::\n \tWhen 'git gc' is run, it calls\ndiff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt\nindex bed60f4..852b72c 100644\n--- a/Documentation/git-gc.txt\n+++ b/Documentation/git-gc.txt\n@@ -63,11 +63,10 @@ automatic consolidation of packs.\n --prune=<date>::\n \tPrune loose objects older than date (default is 2 weeks ago,\n \toverridable by the config variable `gc.pruneExpire`).\n-\t--prune=all prunes loose objects regardless of their age (do\n-\tnot use --prune=all unless you know exactly what you are doing.\n-\tUnless the repository is quiescent, you will lose newly created\n-\tobjects that haven't been anchored with the refs and end up\n-\tcorrupting your repository).  --prune is on by default.\n+\t--prune=all prunes loose objects regardless of their age and\n+\tincreases the risk of corruption if another process is writing to\n+\tthe repository concurrently; see \"NOTES\" below. --prune is on by\n+\tdefault.\n \n --no-prune::\n \tDo not prune any loose objects.\n@@ -138,17 +137,36 @@ default is \"2 weeks ago\".\n Notes\n -----\n \n-'git gc' tries very hard to be safe about the garbage it collects. In\n+'git gc' tries very hard not to delete objects that are referenced\n+anywhere in your repository. In\n particular, it will keep not only objects referenced by your current set\n of branches and tags, but also objects referenced by the index,\n remote-tracking branches, refs saved by 'git filter-branch' in\n refs/original/, or reflogs (which may reference commits in branches\n that were later amended or rewound).\n-\n-If you are expecting some objects to be collected and they aren't, check\n+If you are expecting some objects to be deleted and they aren't, check\n all of those locations and decide whether it makes sense in your case to\n remove those references.\n \n+On the other hand, when 'git gc' runs concurrently with another process,\n+there is a risk of it deleting an object that the other process is using\n+but hasn't created a reference to. This may just cause the other process\n+to fail or may corrupt the repository if the other process later adds a\n+reference to the deleted object. Git has two features that significantly\n+mitigate this problem:\n+\n+. Any object with modification time newer than the `--prune` date is kept,\n+  along with everything reachable from it.\n+\n+. Most operations that add an object to the database update the\n+  modification time of the object if it is already present so that #1\n+  applies.\n+\n+However, these features fall short of a complete solution, so users who\n+run commands concurrently have to live with some risk of corruption (which\n+seems to be low in practice) unless they turn off automatic garbage\n+collection with 'git config gc.auto 0'.\n+\n HOOKS\n -----\n \n-- \n2.7.4\n\n\n"},{"id":"305977","messageId":"1479237172.2406.91.camel@mattmccutchen.net","threadId":"44487","inReplyTo":"20161115174028.zvohfcw4jse3jrmm@sigill.intra.peff.net","subject":"Re: Protecting old temporary objects being reused from concurrent \"git gc\"?","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2016-11-15T19:12:52Z","receivedAt":"2016-11-15T19:13:01Z","isPatch":false,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"On Tue, 2016-11-15 at 12:40 -0500, Jeff King wrote:\n> On Tue, Nov 15, 2016 at 12:33:04PM -0500, Matt McCutchen wrote:\n> \n> > \n> > On Tue, 2016-11-15 at 12:06 -0500, Jeff King wrote:\n> > > \n> > >  - when an object write is optimized out because we already have the\n> > >    object, git will update the mtime on the file (loose object or\n> > >    packfile) to freshen it\n> > \n> > FWIW, I am not seeing this happen when I do \"git read-tree --prefix\"\n> > followed by \"git write-tree\" using the current master (3ab2281).  See\n> > the attached test script.\n> \n> The optimization I'm thinking about is the one from write_sha1_file(),\n> which learned to freshen in 33d4221c7 (write_sha1_file: freshen existing\n> objects, 2014-10-15).\n> \n> I suspect the issue is that read-tree populates the cache-tree index\n> extension, and then write-tree omits the object write before it even\n> gets to write_sha1_file(). The solution is that it should probably be\n> calling one of the freshen() functions (possibly just replacing\n> has_sha1_file() with check_and_freshen(), but I haven't looked).\n> \n> I'd definitely welcome patches in this area.\n\nCool, it's nice to have an idea of what's going on.  I don't think I'm\ngoing to try to fix it myself though.\n\nBy the way, thanks for the fast response to my original question!\n\nMatt\n"},{"id":"305980","messageId":"xmqqk2c4tsv4.fsf@gitster.mtv.corp.google.com","threadId":"44487","inReplyTo":"20161115174028.zvohfcw4jse3jrmm@sigill.intra.peff.net","subject":"Re: Protecting old temporary objects being reused from concurrent \"git gc\"?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-15T20:01:35Z","receivedAt":"2016-11-15T20:01:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I suspect the issue is that read-tree populates the cache-tree index\n> extension, and then write-tree omits the object write before it even\n> gets to write_sha1_file(). The solution is that it should probably be\n> calling one of the freshen() functions (possibly just replacing\n> has_sha1_file() with check_and_freshen(), but I haven't looked).\n\nI think the final writing always happens via write_sha1_file(), but\nan earlier cache-tree update that says \"if we have a tree object\nalready, then use it, otherwise even though we know the object name\nfor this subtree, do not record it in the cache-tree\" codepath may\ndecide to record the subtree's sha1 without refreshing the referent.\n\nA fix may look like this.\n\n cache-tree.c | 2 +-\n cache.h      | 1 +\n sha1_file.c  | 9 +++++++--\n 3 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 345ea35963..3ae6d056b4 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -401,7 +401,7 @@ static int update_one(struct cache_tree *it,\n \tif (repair) {\n \t\tunsigned char sha1[20];\n \t\thash_sha1_file(buffer.buf, buffer.len, tree_type, sha1);\n-\t\tif (has_sha1_file(sha1))\n+\t\tif (freshen_object(sha1))\n \t\t\thashcpy(it->sha1, sha1);\n \t\telse\n \t\t\tto_invalidate = 1;\ndiff --git a/cache.h b/cache.h\nindex 5cdea6833e..1f5694f308 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1126,6 +1126,7 @@ extern int sha1_object_info(const unsigned char *, unsigned long *);\n extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);\n extern int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *return_sha1);\n extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);\n+extern int freshen_object(const unsigned char *);\n extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n extern int force_object_loose(const unsigned char *sha1, time_t mtime);\n extern int git_open_cloexec(const char *name, int flags);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex e030805497..1daeb05dcd 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3275,6 +3275,11 @@ static int freshen_packed_object(const unsigned char *sha1)\n \treturn 1;\n }\n \n+int freshen_object(const unsigned char *sha1)\n+{\n+\treturn freshen_packed_object(sha1) || freshen_loose_object(sha1);\n+}\n+\n int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1)\n {\n \tchar hdr[32];\n@@ -3284,7 +3289,7 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign\n \t * it out into .git/objects/??/?{38} file.\n \t */\n \twrite_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);\n-\tif (freshen_packed_object(sha1) || freshen_loose_object(sha1))\n+\tif (freshen_object(sha1))\n \t\treturn 0;\n \treturn write_loose_object(sha1, hdr, hdrlen, buf, len, 0);\n }\n@@ -3302,7 +3307,7 @@ int hash_sha1_file_literally(const void *buf, unsigned long len, const char *typ\n \n \tif (!(flags & HASH_WRITE_OBJECT))\n \t\tgoto cleanup;\n-\tif (freshen_packed_object(sha1) || freshen_loose_object(sha1))\n+\tif (freshen_object(sha1))\n \t\tgoto cleanup;\n \tstatus = write_loose_object(sha1, header, hdrlen, buf, len, 0);\n \n"},{"id":"306063","messageId":"20161116080753.gkn6v7vhdbifpubn@sigill.intra.peff.net","threadId":"44487","inReplyTo":"xmqqk2c4tsv4.fsf@gitster.mtv.corp.google.com","subject":"Re: Protecting old temporary objects being reused from concurrent \"git gc\"?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-16T08:07:54Z","receivedAt":"2016-11-16T17:18:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 15, 2016 at 12:01:35PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I suspect the issue is that read-tree populates the cache-tree index\n> > extension, and then write-tree omits the object write before it even\n> > gets to write_sha1_file(). The solution is that it should probably be\n> > calling one of the freshen() functions (possibly just replacing\n> > has_sha1_file() with check_and_freshen(), but I haven't looked).\n> \n> I think the final writing always happens via write_sha1_file(), but\n> an earlier cache-tree update that says \"if we have a tree object\n> already, then use it, otherwise even though we know the object name\n> for this subtree, do not record it in the cache-tree\" codepath may\n> decide to record the subtree's sha1 without refreshing the referent.\n> \n> A fix may look like this.\n\nYeah, that's along the lines I was expecting, though I'm not familiar\nenough with cache-tree to say whether it's sufficient. I notice there is\na return very early on in update_one() when has_sha1_file() matches, and\nit seems like that would trigger in some interesting cases, too.\n\n-Peff\n"},{"id":"306067","messageId":"xmqqlgwjqoe2.fsf@gitster.mtv.corp.google.com","threadId":"44487","inReplyTo":"20161116080753.gkn6v7vhdbifpubn@sigill.intra.peff.net","subject":"Re: Protecting old temporary objects being reused from concurrent \"git gc\"?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-16T18:18:45Z","receivedAt":"2016-11-16T18:18:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ... I notice there is\n> a return very early on in update_one() when has_sha1_file() matches, and\n> it seems like that would trigger in some interesting cases, too.\n\nYeah, I missed that.  It says \"we were asked to update one\ncache_tree that corresponds to this subdirectory, found that\nhashes everything below has been rolled up and still valid, and we\nalready have the right tree object in the object store\".\n\nIt can simply become freshen(), which is \"do we have it in the\nobject store?\" with a side effect of touching iff the answer is\n\"yes\".\n"},{"id":"306073","messageId":"xmqq1sybqmjt.fsf@gitster.mtv.corp.google.com","threadId":"44487","inReplyTo":"20161115174028.zvohfcw4jse3jrmm@sigill.intra.peff.net","subject":"Re: Protecting old temporary objects being reused from concurrent \"git gc\"?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-16T18:58:30Z","receivedAt":"2016-11-16T18:58:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I suspect the issue is that read-tree populates the cache-tree index\n> extension, and then write-tree omits the object write before it even\n> gets to write_sha1_file().\n\nWait a minute.  The entries in the index and trees in the cache-tree\nare root of \"still in use\" traversal for the purpose of pruning,\nwhich makes the \"something like this\" patch unnecessary for the real\nindex file.\n\nAnd for temporary index files that is kept for 6 months, touching\ntree objects that cache-tree references is irrelevant---the blobs\nrecorded in the \"list of objects\" part of the index will go stale,\nwhich is a lot more problematic.\n"},{"id":"306093","messageId":"20161117010449.6k3cwo3njvrid4jy@sigill.intra.peff.net","threadId":"44487","inReplyTo":"xmqq1sybqmjt.fsf@gitster.mtv.corp.google.com","subject":"Re: Protecting old temporary objects being reused from concurrent \"git gc\"?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-17T01:04:50Z","receivedAt":"2016-11-17T01:04:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 16, 2016 at 10:58:30AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I suspect the issue is that read-tree populates the cache-tree index\n> > extension, and then write-tree omits the object write before it even\n> > gets to write_sha1_file().\n> \n> Wait a minute.  The entries in the index and trees in the cache-tree\n> are root of \"still in use\" traversal for the purpose of pruning,\n> which makes the \"something like this\" patch unnecessary for the real\n> index file.\n> \n> And for temporary index files that is kept for 6 months, touching\n> tree objects that cache-tree references is irrelevant---the blobs\n> recorded in the \"list of objects\" part of the index will go stale,\n> which is a lot more problematic.\n\nI think the case that is helped here is somebody who runs \"git\nwrite-tree\" and expects that the timestamp on those trees is fresh. So\neven more a briefly used index, like:\n\n  export GIT_INDEX_FILE=/tmp/foo\n  git read-tree ...\n  git write-tree\n  rm -f $GIT_INDEX_FILE\n\nwe'd expect that a \"git gc\" which runs immediately after would see those\ntrees as recent and avoid pruning them (and transitively, any blobs that\nare reachable from the trees). But I don't think that write-tree\nactually freshens them (it sees \"oh, we already have these; there is\nnothing to write\").\n\nI could actually see an argument that the read-tree operation should\nfreshen the blobs themselves (because we know those blobs are now in\nactive use, and probably shouldn't be pruned), but I am not sure I agree\nthere. If only because it is weird that an operation which is otherwise\nread-only with respect to the repository would modify the object\ndatabase.\n\n-Peff\n"},{"id":"306094","messageId":"xmqqvavmopl8.fsf_-_@gitster.mtv.corp.google.com","threadId":"44487","inReplyTo":"20161117010449.6k3cwo3njvrid4jy@sigill.intra.peff.net","subject":"Re* Protecting old temporary objects being reused from concurrent \"git gc\"?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-17T01:35:47Z","receivedAt":"2016-11-17T01:35:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think the case that is helped here is somebody who runs \"git\n> write-tree\" and expects that the timestamp on those trees is fresh. So\n> even more a briefly used index, like:\n>\n>   export GIT_INDEX_FILE=/tmp/foo\n>   git read-tree ...\n>   git write-tree\n>   rm -f $GIT_INDEX_FILE\n>\n> we'd expect that a \"git gc\" which runs immediately after would see those\n> trees as recent and avoid pruning them (and transitively, any blobs that\n> are reachable from the trees). But I don't think that write-tree\n> actually freshens them (it sees \"oh, we already have these; there is\n> nothing to write\").\n\nOK, here is what I have queued.\n\n-- >8 --\nSubject: cache-tree: make sure to \"touch\" tree objects the cache-tree records\n\nThe cache_tree_fully_valid() function is called by callers that want\nto know if they need to call cache_tree_update(), i.e. as an attempt\nto optimize. They all want to have a fully valid cache-tree in the\nend so that they can write a tree object out.\n\nWe used to check if the cached tree object is up-to-date (i.e. the\nindex entires covered by the cache-tree entry hasn't been changed\nsince the roll-up hash was computed for the cache-tree entry) and\nmade sure the tree object is already in the object store.  Since the\ntop-level tree we would soon be asked to write out (and would find\nin the object store) may not be anchored to any refs or commits\nimmediately, freshen the tree object when it happens.\n\nSimilarly, when we actually compute the cache-tree entries in\ncache_tree_update(), we refrained from writing a tree object out\nwhen it already exists in the object store.  We would want to\nfreshen the tree object in that case to protect it from premature\npruning.\n\nStrictly speaking, freshing these tree objects at each and every\nlevel is probably unnecessary, given that anything reachable from a\nyoung object inherits the youth from the referring object to be\nprotected from pruning.  It should be sufficient to freshen only the\nvery top-level tree instead.  Benchmarking and optimization is left\nas an exercise for later days.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n cache-tree.c | 6 +++---\n cache.h      | 1 +\n sha1_file.c  | 9 +++++++--\n 3 files changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 3ebf9c3aa4..c8c74a1e07 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -225,7 +225,7 @@ int cache_tree_fully_valid(struct cache_tree *it)\n \tint i;\n \tif (!it)\n \t\treturn 0;\n-\tif (it->entry_count < 0 || !has_sha1_file(it->sha1))\n+\tif (it->entry_count < 0 || !freshen_object(it->sha1))\n \t\treturn 0;\n \tfor (i = 0; i < it->subtree_nr; i++) {\n \t\tif (!cache_tree_fully_valid(it->down[i]->cache_tree))\n@@ -253,7 +253,7 @@ static int update_one(struct cache_tree *it,\n \n \t*skip_count = 0;\n \n-\tif (0 <= it->entry_count && has_sha1_file(it->sha1))\n+\tif (0 <= it->entry_count && freshen_object(it->sha1))\n \t\treturn it->entry_count;\n \n \t/*\n@@ -393,7 +393,7 @@ static int update_one(struct cache_tree *it,\n \tif (repair) {\n \t\tunsigned char sha1[20];\n \t\thash_sha1_file(buffer.buf, buffer.len, tree_type, sha1);\n-\t\tif (has_sha1_file(sha1))\n+\t\tif (freshen_object(sha1))\n \t\t\thashcpy(it->sha1, sha1);\n \t\telse\n \t\t\tto_invalidate = 1;\ndiff --git a/cache.h b/cache.h\nindex 4ff196c259..72c0b321ac 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1077,6 +1077,7 @@ extern int sha1_object_info(const unsigned char *, unsigned long *);\n extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);\n extern int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *return_sha1);\n extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);\n+extern int freshen_object(const unsigned char *);\n extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n extern int force_object_loose(const unsigned char *sha1, time_t mtime);\n extern int git_open_noatime(const char *name);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex d0f2aa029b..9acce3d3b8 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3151,6 +3151,11 @@ static int freshen_packed_object(const unsigned char *sha1)\n \treturn 1;\n }\n \n+int freshen_object(const unsigned char *sha1)\n+{\n+\treturn freshen_packed_object(sha1) || freshen_loose_object(sha1);\n+}\n+\n int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1)\n {\n \tchar hdr[32];\n@@ -3160,7 +3165,7 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign\n \t * it out into .git/objects/??/?{38} file.\n \t */\n \twrite_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);\n-\tif (freshen_packed_object(sha1) || freshen_loose_object(sha1))\n+\tif (freshen_object(sha1))\n \t\treturn 0;\n \treturn write_loose_object(sha1, hdr, hdrlen, buf, len, 0);\n }\n@@ -3178,7 +3183,7 @@ int hash_sha1_file_literally(const void *buf, unsigned long len, const char *typ\n \n \tif (!(flags & HASH_WRITE_OBJECT))\n \t\tgoto cleanup;\n-\tif (freshen_packed_object(sha1) || freshen_loose_object(sha1))\n+\tif (freshen_object(sha1))\n \t\tgoto cleanup;\n \tstatus = write_loose_object(sha1, header, hdrlen, buf, len, 0);\n \n-- \n2.11.0-rc1-156-g07127df5c1\n\n"},{"id":"306095","messageId":"20161117014306.2ptqd56gur7dlb4c@sigill.intra.peff.net","threadId":"44487","inReplyTo":"xmqqvavmopl8.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: Re* Protecting old temporary objects being reused from concurrent \"git gc\"?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-17T01:43:07Z","receivedAt":"2016-11-17T01:43:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 16, 2016 at 05:35:47PM -0800, Junio C Hamano wrote:\n\n> OK, here is what I have queued.\n> \n> -- >8 --\n> Subject: cache-tree: make sure to \"touch\" tree objects the cache-tree records\n> \n> The cache_tree_fully_valid() function is called by callers that want\n> to know if they need to call cache_tree_update(), i.e. as an attempt\n> to optimize. They all want to have a fully valid cache-tree in the\n> end so that they can write a tree object out.\n\nThat makes sense. I was focusing on cache_tree_update() call, but we do\nnot even get there in the fully-valid case.\n\nSo I think this approach is nice as long as there is not a caller who\nasks \"are we fully valid? I do not need to write, but was just\nwondering\". That should be a read-only operation, but the freshen calls\nmay fail with EPERM, for example.\n\nI do not see any such callers, nor do I really expect any. Just trying\nto think through the possible consequences.\n\n> Strictly speaking, freshing these tree objects at each and every\n> level is probably unnecessary, given that anything reachable from a\n> young object inherits the youth from the referring object to be\n> protected from pruning.  It should be sufficient to freshen only the\n> very top-level tree instead.  Benchmarking and optimization is left\n> as an exercise for later days.\n\nGood observation, and nicely explained all around.\n\n-Peff\n"}]}