{"thread":{"id":"42492","subject":"git gc and worktrees","startedAt":"2016-05-31T07:07:22Z","lastAt":"2016-06-03T16:45:01Z","messageCount":25,"participants":["Johannes Sixt","Duy Nguyen","Jeff King","Michael Haggerty","Nguyễn Thái Ngọc Duy","Junio C Hamano","Eric Sunshine","David Turner"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"287882","messageId":"574D382A.8030809@kdbg.org","threadId":"42492","inReplyTo":null,"subject":"git gc and worktrees","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-05-31T07:07:22Z","receivedAt":"2016-05-31T07:07:22Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Earlier this year I had a largish merge going on in a separate worktree. \nWith a mix of staged resolutions and unmerged paths in the index, I ran \n'git gc' in the main worktree. This removed a lot of objects that were \nrecorded in that separate worktree index. (I was able to recover them \nbecause the content was still on disk.)\n\nNow I wanted to derive a test case that shows that breakage, but I am \nunable to. The objects recorded in a separate worktree index, but not \ncommitted, yet, are not pruned anymore.\n\nHave there been any fixes in this regard recently? Or does this look \nlike something else going on?\n\n-- Hannes\n"},{"id":"287891","messageId":"CACsJy8BHU0YtgvjuefRPuMPLhvoOPLVMhR4YzH8=wVFeOie+Xw@mail.gmail.com","threadId":"42492","inReplyTo":"574D382A.8030809@kdbg.org","subject":"Re: git gc and worktrees","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-05-31T12:02:15Z","receivedAt":"2016-05-31T12:02:15Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, May 31, 2016 at 2:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n> Earlier this year I had a largish merge going on in a separate worktree.\n> With a mix of staged resolutions and unmerged paths in the index, I ran 'git\n> gc' in the main worktree. This removed a lot of objects that were recorded\n> in that separate worktree index. (I was able to recover them because the\n> content was still on disk.)\n>\n> Now I wanted to derive a test case that shows that breakage, but I am unable\n> to. The objects recorded in a separate worktree index, but not committed,\n> yet, are not pruned anymore.\n>\n> Have there been any fixes in this regard recently? Or does this look like\n> something else going on?\n\nNot sure. I vaguely recall Jeff touched this pruning issue once,\nsomething about recent objects will not be pruned based on mtime. But\nmaybe some of those objects in the index are not so young, and because\nI think we never check indexes in git-gc/git-prune, they have a chance\nto be deleted.\n\nNo I'm wrong. mark_reachable_objects() which is used by git-prune,\ndoes add objects from index and HEAD, which only covers those from\n_current_ worktree. This should be fixed even if it is not the root\ncause of your problem. I'll look into it.\n-- \nDuy\n"},{"id":"287941","messageId":"20160531221415.GA3824@sigill.intra.peff.net","threadId":"42492","inReplyTo":"CACsJy8BHU0YtgvjuefRPuMPLhvoOPLVMhR4YzH8=wVFeOie+Xw@mail.gmail.com","subject":"Re: git gc and worktrees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-31T22:14:15Z","receivedAt":"2016-05-31T22:14:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 31, 2016 at 07:02:15PM +0700, Duy Nguyen wrote:\n\n> On Tue, May 31, 2016 at 2:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n> > Earlier this year I had a largish merge going on in a separate worktree.\n> > With a mix of staged resolutions and unmerged paths in the index, I ran 'git\n> > gc' in the main worktree. This removed a lot of objects that were recorded\n> > in that separate worktree index. (I was able to recover them because the\n> > content was still on disk.)\n> >\n> > Now I wanted to derive a test case that shows that breakage, but I am unable\n> > to. The objects recorded in a separate worktree index, but not committed,\n> > yet, are not pruned anymore.\n> >\n> > Have there been any fixes in this regard recently? Or does this look like\n> > something else going on?\n> \n> Not sure. I vaguely recall Jeff touched this pruning issue once,\n> something about recent objects will not be pruned based on mtime. But\n> maybe some of those objects in the index are not so young, and because\n> I think we never check indexes in git-gc/git-prune, they have a chance\n> to be deleted.\n> \n> No I'm wrong. mark_reachable_objects() which is used by git-prune,\n> does add objects from index and HEAD, which only covers those from\n> _current_ worktree. This should be fixed even if it is not the root\n> cause of your problem. I'll look into it.\n\nRight, we use the index for reachability checks (both in \"prune\", but\nalso during a \"repack -a\", which uses the revision parser's\n'--indexed-objects\" option). That obviously should handle per-worktree\nindex files, but I don't know whether it currently does.\n\nMichael (cc'd) noted to me off-list recently that there may be some\nspecial cases there regarding reflogs in other worktrees (i.e., that we\ndon't always include them for our reachability checks). I don't know the\ndetails, though.\n\nThe \"recent object\" stuff you're thinking of is that we will keep a\nnon-recent object X if it is referenced (directly or indirectly) by\nanother object Y that _is_ recent, but not otherwise reachable. That\nshould work independent of worktrees, because it finds the recent\nobjects directly in the object database, and then walks their graphs\n(but it could indeed make writing tests a little trickier, as it may be\nharder to convince some objects to get pruned in the first place).\n\n-Peff\n"},{"id":"287979","messageId":"574E8821.3070502@kdbg.org","threadId":"42492","inReplyTo":"20160531221415.GA3824@sigill.intra.peff.net","subject":"Re: git gc and worktrees","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-06-01T07:00:49Z","receivedAt":"2016-06-01T07:00:49Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 01.06.2016 um 00:14 schrieb Jeff King:\n> Right, we use the index for reachability checks (both in \"prune\", but\n> also during a \"repack -a\", which uses the revision parser's\n> '--indexed-objects\" option). That obviously should handle per-worktree\n> index files, but I don't know whether it currently does.\n\nThanks. Here's a test case to make sure that --indexed-objects looks at\nthe indexes of separate worktrees. I'm not submitting a proper patch\nbecause I don't feel like being able to fix the problem and I'm not\nsure what the expected order of the listed objects is.\n\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex 3e752ce..dbd3679 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -96,6 +96,34 @@ test_expect_success 'rev-list can show index objects' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--indexed-objects in the index of a separate worktrees' '\n+\t# see also the comments in the previous case\n+\tcat >expect <<-\\EOF &&\n+\t8e4020bb5a8d8c873b25de15933e75cc0fc275df one\n+\td9d3a7417b9605cfd88ee6306b28dadc29e6ab08 only-in-index\n+\t2043d83f5a374f119437856d2f1773c936938876 only-in-wt-index\n+\t9200b628cf9dc883a85a7abc8d6e6730baee589c two\n+\tEOF\n+\techo only-in-index >only-in-index &&\n+\tgit add only-in-index &&\n+\n+\tgit worktree add -b side wt &&\n+\ttest_when_finished \"rm -r wt; git worktree prune\" &&\n+\t(\n+\t\tcd wt &&\n+\t\techo only-in-wt-index >only-in-wt-index &&\n+\t\tgit add only-in-wt-index &&\n+\n+\t\t# this must also include the objects of the main worktree\n+\t\tgit rev-list --objects --indexed-objects >../actual\n+\t) &&\n+\ttest_cmp expect actual &&\n+\n+\t# same result when invoked from the main worktree\n+\tgit rev-list --objects --indexed-objects >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '--bisect and --first-parent can not be combined' '\n \ttest_must_fail git rev-list --bisect --first-parent HEAD\n '\n"},{"id":"287989","messageId":"574EA364.60408@alum.mit.edu","threadId":"42492","inReplyTo":"20160531221415.GA3824@sigill.intra.peff.net","subject":"Re: git gc and worktrees","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-01T08:57:08Z","receivedAt":"2016-06-01T08:57:08Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/01/2016 12:14 AM, Jeff King wrote:\n> [...]\n> Michael (cc'd) noted to me off-list recently that there may be some\n> special cases there regarding reflogs in other worktrees (i.e., that we\n> don't always include them for our reachability checks). I don't know the\n> details, though.\n\nThat's correct. `for_each_reflog()` does its work by walking the\ndirectory tree under `$GIT_DIR/logs`. So when run in the main\nrepository, it never looks at the per-worktree reflogs. Therefore,\nobjects that are only reachable from per-worktree reflogs can end up\ngetting pruned.\n\nThis is closely tangled up with my ref-iterators patch series (which is\nwhy I noticed it), especially with\n\n    [PATCH 13/13] for_each_reflog(): reimplement using iterators\n\n[1].\n\nI think reference stores are going to need two distinct types of\nreference iteration: one to iterate over the *logical* reference space\nof a single repo or worktree, and one to find all *local* references\nand/or reachability roots (e.g., when run within a linked repo). The\ncurrent approach, where subtrees of the reference namespace are pasted\nad-hoc into each other partly by manhandling the ref_cache and partly\nvia the hack in `git_path()` is not, I think, a very good long-term design.\n\nI'd prefer the elementary operation on a low-level reference store to be\niterating over all of the references that are stored locally, and to use\n`merge_ref_iterator` for pasting together the parts of low-level\nreference stores that are needed to form a logical view of the\nreferences in a linked repo.\n\nMichael\n\n[1]\nhttp://thread.gmane.org/gmane.comp.version-control.git/295860/focus=295866\n"},{"id":"287997","messageId":"20160601104519.16563-1-pclouds@gmail.com","threadId":"42492","inReplyTo":"574D382A.8030809@kdbg.org","subject":"[PATCH 0/4] Fix prune/gc problem with multiple worktrees","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-01T10:45:15Z","receivedAt":"2016-06-01T10:45:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This series makes sure that objects referenced by all worktrees are\nmarked reachable so that we don't accidentally delete objects that are\nbeing used. Previously per-worktree references in index, detached HEAD\nor per-worktree reflogs come from current worktree only, not all\nworktrees.\n\nThe series deals with git-prune and git-gc specifically. I left out\n\"git rev-list\". It shares the same problem because it will only\nconsider current worktree's HEAD, index and per-worktree reflogs. The\nproblem is I am not sure if we simply just change, say\n--indexed-objects, to cover all indexes, or should we only do that\nwith \"--all-worktrees --indexed-objects\". I guess this is up for\ndiscussion.\n\nNguyễn Thái Ngọc Duy (4):\n  revision.c: move read_cache() out of add_index_objects_to_pending()\n  reachable.c: mark reachable objects in index from all worktrees\n  reachable.c: mark reachable detached HEAD from all worktrees\n  reachable.c: make reachable reflogs for all per-worktree reflogs\n\n reachable.c      | 47 +++++++++++++++++++++++++++++++++++++++++------\n revision.c       | 34 +++++++++++++++++++++++++++-------\n revision.h       |  7 ++++++-\n t/t5304-prune.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 114 insertions(+), 14 deletions(-)\n\n-- \n2.8.2.524.g6ff3d78\n"},{"id":"287996","messageId":"20160601104519.16563-2-pclouds@gmail.com","threadId":"42492","inReplyTo":"20160601104519.16563-1-pclouds@gmail.com","subject":"[PATCH 1/4] revision.c: move read_cache() out of add_index_objects_to_pending()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-01T10:45:16Z","receivedAt":"2016-06-01T10:45:16Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"A library function like this should not change global state like\nthe_index. Granted, read_cache() will not re-read from $GIT_DIR/index\nand lose all in-core changes if the index is dirty (i.e. no bad side\neffects). But that sort of detail should not be relied on here.\n\nMake the function take the index object from outside instead. The\ncaller will be responsible for reading index files if necessary.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n reachable.c |  3 ++-\n revision.c  | 15 ++++++++-------\n revision.h  |  4 +++-\n 3 files changed, 13 insertions(+), 9 deletions(-)\n\ndiff --git a/reachable.c b/reachable.c\nindex d0199ca..15dbe60 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -170,7 +170,8 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \trevs->tree_objects = 1;\n \n \t/* Add all refs from the index file */\n-\tadd_index_objects_to_pending(revs, 0);\n+\tread_cache();\n+\tadd_index_objects_to_pending(revs, 0, &the_index);\n \n \t/* Add all external refs */\n \tfor_each_ref(add_one_ref, revs);\ndiff --git a/revision.c b/revision.c\nindex d30d1c4..bbb6ff1 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1275,13 +1275,13 @@ static void add_cache_tree(struct cache_tree *it, struct rev_info *revs,\n \n }\n \n-void add_index_objects_to_pending(struct rev_info *revs, unsigned flags)\n+void add_index_objects_to_pending(struct rev_info *revs, unsigned flags,\n+\t\t\t\t  const struct index_state *istate)\n {\n \tint i;\n \n-\tread_cache();\n-\tfor (i = 0; i < active_nr; i++) {\n-\t\tstruct cache_entry *ce = active_cache[i];\n+\tfor (i = 0; i < istate->cache_nr; i++) {\n+\t\tconst struct cache_entry *ce = istate->cache[i];\n \t\tstruct blob *blob;\n \n \t\tif (S_ISGITLINK(ce->ce_mode))\n@@ -1294,9 +1294,9 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned flags)\n \t\t\t\t\t     ce->ce_mode, ce->name);\n \t}\n \n-\tif (active_cache_tree) {\n+\tif (istate->cache_tree) {\n \t\tstruct strbuf path = STRBUF_INIT;\n-\t\tadd_cache_tree(active_cache_tree, revs, &path);\n+\t\tadd_cache_tree(istate->cache_tree, revs, &path);\n \t\tstrbuf_release(&path);\n \t}\n }\n@@ -2106,7 +2106,8 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \t} else if (!strcmp(arg, \"--reflog\")) {\n \t\tadd_reflogs_to_pending(revs, *flags);\n \t} else if (!strcmp(arg, \"--indexed-objects\")) {\n-\t\tadd_index_objects_to_pending(revs, *flags);\n+\t\tread_cache();\n+\t\tadd_index_objects_to_pending(revs, *flags, &the_index);\n \t} else if (!strcmp(arg, \"--not\")) {\n \t\t*flags ^= UNINTERESTING | BOTTOM;\n \t} else if (!strcmp(arg, \"--no-walk\")) {\ndiff --git a/revision.h b/revision.h\nindex 9fac1a6..d06d098 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -29,6 +29,7 @@ struct rev_info;\n struct log_info;\n struct string_list;\n struct saved_parents;\n+struct index_state;\n \n struct rev_cmdline_info {\n \tunsigned int nr;\n@@ -271,7 +272,8 @@ extern void add_pending_sha1(struct rev_info *revs,\n \n extern void add_head_to_pending(struct rev_info *);\n extern void add_reflogs_to_pending(struct rev_info *, unsigned int flags);\n-extern void add_index_objects_to_pending(struct rev_info *, unsigned int flags);\n+extern void add_index_objects_to_pending(struct rev_info *, unsigned int flags,\n+\t\t\t\t\t const struct index_state *);\n \n enum commit_action {\n \tcommit_ignore,\n-- \n2.8.2.524.g6ff3d78\n"},{"id":"287998","messageId":"20160601104519.16563-3-pclouds@gmail.com","threadId":"42492","inReplyTo":"20160601104519.16563-1-pclouds@gmail.com","subject":"[PATCH 2/4] reachable.c: mark reachable objects in index from all worktrees","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-01T10:45:17Z","receivedAt":"2016-06-01T10:45:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Current mark_reachable_objects() only marks objects from index from\n_current_ worktree as reachable instead of all worktrees. Because this\nfunction is used for pruning, there is a chance that objects referenced\nby other worktrees may be deleted. Fix that.\n\nSmall behavior change in \"one worktree\" case, the index is read again\nfrom file. In the current implementation, if the_index is already\nloaded, the index file will not be read from file again. This adds some\nmore cost to this operation, hopefully insignificant because\nreachability test is usually very expensive already.\n\nReported-by: Johannes Sixt <j6t@kdbg.org>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n reachable.c      | 33 +++++++++++++++++++++++++++++----\n t/t5304-prune.sh |  9 +++++++++\n 2 files changed, 38 insertions(+), 4 deletions(-)\n\ndiff --git a/reachable.c b/reachable.c\nindex 15dbe60..8f67242 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -9,6 +9,7 @@\n #include \"cache-tree.h\"\n #include \"progress.h\"\n #include \"list-objects.h\"\n+#include \"worktree.h\"\n \n struct connectivity_progress {\n \tstruct progress *progress;\n@@ -155,6 +156,32 @@ int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n \t\t\t\t      FOR_EACH_OBJECT_LOCAL_ONLY);\n }\n \n+static void add_objects_from_worktree(struct rev_info *revs)\n+{\n+\tstruct worktree **worktrees, **p;\n+\n+\tworktrees = get_worktrees();\n+\tfor (p = worktrees; *p; p++) {\n+\t\tstruct worktree *wt = *p;\n+\t\tstruct index_state istate;\n+\n+\t\tmemset(&istate, 0, sizeof(istate));\n+\t\tif (read_index_from(&istate,\n+\t\t\t\t    worktree_git_path(wt, \"index\")) > 0)\n+\t\t\tadd_index_objects_to_pending(revs, 0, &istate);\n+\t\tdiscard_index(&istate);\n+\t}\n+\tfree_worktrees(worktrees);\n+\n+\t/*\n+\t * this is in case the index is already updated but not\n+\t * written down in file yet, then add_index_... in the above\n+\t * loop will miss new objects that are just created or\n+\t * referenced.\n+\t */\n+\tadd_index_objects_to_pending(revs, 0, &the_index);\n+}\n+\n void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \t\t\t    unsigned long mark_recent,\n \t\t\t    struct progress *progress)\n@@ -169,10 +196,6 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \trevs->blob_objects = 1;\n \trevs->tree_objects = 1;\n \n-\t/* Add all refs from the index file */\n-\tread_cache();\n-\tadd_index_objects_to_pending(revs, 0, &the_index);\n-\n \t/* Add all external refs */\n \tfor_each_ref(add_one_ref, revs);\n \n@@ -183,6 +206,8 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \tif (mark_reflog)\n \t\tadd_reflogs_to_pending(revs, 0);\n \n+\tadd_objects_from_worktree(revs);\n+\n \tcp.progress = progress;\n \tcp.count = 0;\n \ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 133b584..cba45c7 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -283,4 +283,13 @@ test_expect_success 'prune: handle alternate object database' '\n \tgit -C B prune\n '\n \n+test_expect_success 'prune: handle index in multiple worktrees' '\n+\tgit worktree add second-worktree &&\n+\techo \"new blob for second-worktree\" >second-worktree/blob &&\n+\tgit -C second-worktree add blob &&\n+\tgit prune --expire=now &&\n+\tgit -C second-worktree show :blob >actual &&\n+\ttest_cmp second-worktree/blob actual\n+'\n+\n test_done\n-- \n2.8.2.524.g6ff3d78\n"},{"id":"288000","messageId":"20160601104519.16563-4-pclouds@gmail.com","threadId":"42492","inReplyTo":"20160601104519.16563-1-pclouds@gmail.com","subject":"[PATCH 3/4] reachable.c: mark reachable detached HEAD from all worktrees","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-01T10:45:18Z","receivedAt":"2016-06-01T10:45:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Same reason as the previous commit, this prevents potential object\ndeletion that other worktrees need.\n\nThere is a slight change in behavior. The current implementation always\nadds HEAD. The new one only adds _detached_ HEAD. But that's fine because\nall normal refs should be already covered by for_each_ref() in\nmark_reachable_objects().\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n reachable.c      | 14 +++++++++++---\n t/t5304-prune.sh | 12 ++++++++++++\n 2 files changed, 23 insertions(+), 3 deletions(-)\n\ndiff --git a/reachable.c b/reachable.c\nindex 8f67242..e5f9170 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -170,6 +170,13 @@ static void add_objects_from_worktree(struct rev_info *revs)\n \t\t\t\t    worktree_git_path(wt, \"index\")) > 0)\n \t\t\tadd_index_objects_to_pending(revs, 0, &istate);\n \t\tdiscard_index(&istate);\n+\n+\t\tif (wt->is_detached) {\n+\t\t\tstruct object *o;\n+\t\t\to = parse_object_or_die(wt->head_sha1, \"HEAD\");\n+\t\t\tadd_pending_object(revs, o, \"\");\n+\t\t}\n+\n \t}\n \tfree_worktrees(worktrees);\n \n@@ -199,13 +206,14 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \t/* Add all external refs */\n \tfor_each_ref(add_one_ref, revs);\n \n-\t/* detached HEAD is not included in the list above */\n-\thead_ref(add_one_ref, revs);\n-\n \t/* Add all reflog info */\n \tif (mark_reflog)\n \t\tadd_reflogs_to_pending(revs, 0);\n \n+\t/*\n+\t * Add all objects from the in-core index file and detached\n+\t * HEAD which is not included in the list above\n+\t */\n \tadd_objects_from_worktree(revs);\n \n \tcp.progress = progress;\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex cba45c7..683bdb0 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -292,4 +292,16 @@ test_expect_success 'prune: handle index in multiple worktrees' '\n \ttest_cmp second-worktree/blob actual\n '\n \n+test_expect_success 'prune: handle HEAD in multiple worktrees' '\n+\tgit worktree add --detach third-worktree &&\n+\techo \"new blob for third-worktree\" >third-worktree/blob &&\n+\tgit -C third-worktree add blob &&\n+\tgit -C third-worktree commit -m \"third\" &&\n+\trm .git/worktrees/third-worktree/index &&\n+\ttest_must_fail git -C third-worktree show :blob &&\n+\tgit prune --expire=now &&\n+\tgit -C third-worktree show HEAD:blob >actual &&\n+\ttest_cmp third-worktree/blob actual\n+'\n+\n test_done\n-- \n2.8.2.524.g6ff3d78\n"},{"id":"287999","messageId":"20160601104519.16563-5-pclouds@gmail.com","threadId":"42492","inReplyTo":"20160601104519.16563-1-pclouds@gmail.com","subject":"[PATCH 4/4] reachable.c: make reachable reflogs for all per-worktree reflogs","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-01T10:45:19Z","receivedAt":"2016-06-01T10:45:19Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Same reason as the previous commit, this is to avoid deleting objects\nbeing referenced by per-worktree reflogs from other worktrees.\n\"logs/HEAD\" is most important. \"logs/refs/bisect\" should not live long\nenough to matter, but let's add it too for safety.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n reachable.c      |  7 ++++---\n revision.c       | 19 +++++++++++++++++++\n revision.h       |  3 +++\n t/t5304-prune.sh | 19 +++++++++++++++++++\n 4 files changed, 45 insertions(+), 3 deletions(-)\n\ndiff --git a/reachable.c b/reachable.c\nindex e5f9170..73915e0 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -156,7 +156,7 @@ int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n \t\t\t\t      FOR_EACH_OBJECT_LOCAL_ONLY);\n }\n \n-static void add_objects_from_worktree(struct rev_info *revs)\n+static void add_objects_from_worktree(struct rev_info *revs, int mark_reflog)\n {\n \tstruct worktree **worktrees, **p;\n \n@@ -176,7 +176,8 @@ static void add_objects_from_worktree(struct rev_info *revs)\n \t\t\to = parse_object_or_die(wt->head_sha1, \"HEAD\");\n \t\t\tadd_pending_object(revs, o, \"\");\n \t\t}\n-\n+\t\tif (mark_reflog)\n+\t\t\tadd_worktree_reflogs_to_pending(revs, 0, wt);\n \t}\n \tfree_worktrees(worktrees);\n \n@@ -214,7 +215,7 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \t * Add all objects from the in-core index file and detached\n \t * HEAD which is not included in the list above\n \t */\n-\tadd_objects_from_worktree(revs);\n+\tadd_objects_from_worktree(revs, mark_reflog);\n \n \tcp.progress = progress;\n \tcp.count = 0;\ndiff --git a/revision.c b/revision.c\nindex bbb6ff1..6a197a4 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -19,6 +19,7 @@\n #include \"dir.h\"\n #include \"cache-tree.h\"\n #include \"bisect.h\"\n+#include \"worktree.h\"\n \n volatile show_early_output_fn_t show_early_output;\n \n@@ -1245,6 +1246,24 @@ static int handle_one_reflog(const char *path, const struct object_id *oid,\n \treturn 0;\n }\n \n+void add_worktree_reflogs_to_pending(struct rev_info *revs, unsigned flags,\n+\t\t\t\t     struct worktree *wt)\n+{\n+\tstruct all_refs_cb cb;\n+\tchar *path;\n+\n+\tcb.all_revs = revs;\n+\tcb.all_flags = flags;\n+\tpath = xstrdup(worktree_git_path(wt, \"logs/HEAD\"));\n+\tif (file_exists(path))\n+\t\thandle_one_reflog(path, NULL, 0, &cb);\n+\tfree(path);\n+\tpath = xstrdup(worktree_git_path(wt, \"logs/refs/bisect\"));\n+\tif (file_exists(path))\n+\t\thandle_one_reflog(path, NULL, 0, &cb);\n+\tfree(path);\n+}\n+\n void add_reflogs_to_pending(struct rev_info *revs, unsigned flags)\n {\n \tstruct all_refs_cb cb;\ndiff --git a/revision.h b/revision.h\nindex d06d098..9f3f148 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -30,6 +30,7 @@ struct log_info;\n struct string_list;\n struct saved_parents;\n struct index_state;\n+struct worktree;\n \n struct rev_cmdline_info {\n \tunsigned int nr;\n@@ -272,6 +273,8 @@ extern void add_pending_sha1(struct rev_info *revs,\n \n extern void add_head_to_pending(struct rev_info *);\n extern void add_reflogs_to_pending(struct rev_info *, unsigned int flags);\n+extern void add_worktree_reflogs_to_pending(struct rev_info *, unsigned int flags,\n+\t\t\t\t\t    struct worktree *);\n extern void add_index_objects_to_pending(struct rev_info *, unsigned int flags,\n \t\t\t\t\t const struct index_state *);\n \ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 683bdb0..6b1c456 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -304,4 +304,23 @@ test_expect_success 'prune: handle HEAD in multiple worktrees' '\n \ttest_cmp third-worktree/blob actual\n '\n \n+test_expect_success 'prune: handle HEAD reflog in multiple worktrees' '\n+\t(\n+\t\tcd third-worktree &&\n+\t\tgit config core.logAllRefUpdates true &&\n+\t\techo \"HEAD{1} blob for third-worktree\" >blob &&\n+\t\tgit add blob &&\n+\t\tgit commit -m \"second commit in third\" &&\n+\t\tcp blob expected &&\n+\t\techo \"HEAD{0} blob for third-worktree\" >blob &&\n+\t\tgit add blob &&\n+\t\tgit commit -m \"third commit in third\" &&\n+\t\tgit show HEAD@{1}:blob >actual &&\n+\t\ttest_cmp expected actual\n+\t) &&\n+\tgit prune --expire=now &&\n+\tgit -C third-worktree show HEAD@{1}:blob >actual &&\n+\ttest_cmp third-worktree/expected actual\n+'\n+\n test_done\n-- \n2.8.2.524.g6ff3d78\n"},{"id":"288009","messageId":"xmqqbn3l0wkv.fsf@gitster.mtv.corp.google.com","threadId":"42492","inReplyTo":"574EA364.60408@alum.mit.edu","subject":"Re: git gc and worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-01T15:15:28Z","receivedAt":"2016-06-01T15:15:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> I think reference stores are going to need two distinct types of\n> reference iteration: one to iterate over the *logical* reference space\n> of a single repo or worktree, and one to find all *local* references\n> and/or reachability roots (e.g., when run within a linked repo).\n\nThis is hard reason about without defining *logical* and *local*.\n\nFor protecting necessary objects from pruning by anchoring them as\n\"reachable\", and for avoiding unnecessary object transfer by showing\nthings via \".have\" lines, we're better consider references and\nreflogs that do not appear in the namespace visible from the current\nworktree.  I wanted to come up with an example of wanting to iterate\nover refs of the current worktree, but other than \"what are my\nbranches including HEAD?\" I didn't think of anything interesting.\n\n> The current approach, where subtrees of the reference namespace\n> are pasted ad-hoc into each other partly by manhandling the\n> ref_cache and partly via the hack in `git_path()` is not, I think,\n> a very good long-term design.\n\nThis I agree with.\n\nThanks.\n"},{"id":"288012","messageId":"574F0483.80803@alum.mit.edu","threadId":"42492","inReplyTo":"20160601104519.16563-5-pclouds@gmail.com","subject":"Re: [PATCH 4/4] reachable.c: make reachable reflogs for all per-worktree reflogs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-01T15:51:31Z","receivedAt":"2016-06-01T15:51:31Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/01/2016 12:45 PM, Nguyễn Thái Ngọc Duy wrote:\n> [...]\n> +void add_worktree_reflogs_to_pending(struct rev_info *revs, unsigned flags,\n> +\t\t\t\t     struct worktree *wt)\n> +{\n> +\tstruct all_refs_cb cb;\n> +\tchar *path;\n> +\n> +\tcb.all_revs = revs;\n> +\tcb.all_flags = flags;\n> +\tpath = xstrdup(worktree_git_path(wt, \"logs/HEAD\"));\n> +\tif (file_exists(path))\n> +\t\thandle_one_reflog(path, NULL, 0, &cb);\n> +\tfree(path);\n> +\tpath = xstrdup(worktree_git_path(wt, \"logs/refs/bisect\"));\n> +\tif (file_exists(path))\n> +\t\thandle_one_reflog(path, NULL, 0, &cb);\n> +\tfree(path);\n> +}\n\n`refs/bisect` is not a single reference. It is a namespace that contains\nreferences with names like `refs/bisect/bad` and\n`refs/bisect/good-66106691a1b71e445fe5e4d6b8b043dffc7dfe4c`.\n\n> [...]\n\nMichael\n"},{"id":"288014","messageId":"20160601160143.GA9219@sigill.intra.peff.net","threadId":"42492","inReplyTo":"20160601104519.16563-1-pclouds@gmail.com","subject":"Re: [PATCH 0/4] Fix prune/gc problem with multiple worktrees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-01T16:01:44Z","receivedAt":"2016-06-01T16:01:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 01, 2016 at 05:45:15PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> This series makes sure that objects referenced by all worktrees are\n> marked reachable so that we don't accidentally delete objects that are\n> being used. Previously per-worktree references in index, detached HEAD\n> or per-worktree reflogs come from current worktree only, not all\n> worktrees.\n> \n> The series deals with git-prune and git-gc specifically. I left out\n> \"git rev-list\". It shares the same problem because it will only\n> consider current worktree's HEAD, index and per-worktree reflogs. The\n> problem is I am not sure if we simply just change, say\n> --indexed-objects, to cover all indexes, or should we only do that\n> with \"--all-worktrees --indexed-objects\". I guess this is up for\n> discussion.\n\nI don't think touching reachable.c is enough. You also need to make sure\nthat calling \"git pack-objects --indexed-objects\" will get them, too\n(otherwise they will be either ejected loose or dropped completely\nwhen --unpack-unreachable=2.weeks.ago or similar is used).\n\nThat code path uses rev-list internally. So I think you need something\nlike \"--all-worktrees --indexed-objects\", and you need to pass the new\noption in from git-repack to pack-objects (or you need to simply make\n\"--indexed-objects\" cover all worktrees by default).\n\n-Peff\n"},{"id":"288015","messageId":"xmqqshwwzyee.fsf@gitster.mtv.corp.google.com","threadId":"42492","inReplyTo":"20160601104519.16563-1-pclouds@gmail.com","subject":"Re: [PATCH 0/4] Fix prune/gc problem with multiple worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-01T16:06:49Z","receivedAt":"2016-06-01T16:06:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> This series makes sure that objects referenced by all worktrees are\n> marked reachable so that we don't accidentally delete objects that are\n> being used. Previously per-worktree references in index, detached HEAD\n> or per-worktree reflogs come from current worktree only, not all\n> worktrees.\n\nI'll let this topic simmer on the list for now, instead of picking\nit up immediately to 'pu', as Michael in $gmane/296068 makes me\nwonder if we want to keep piling on the current \"worktree ref\niterations are bolted on\" or if we want to first clean it up, whose\nnatural fallout hopefully would eliminate the bug away.\n\nThanks.\n\n> The series deals with git-prune and git-gc specifically. I left out\n> \"git rev-list\". It shares the same problem because it will only\n> consider current worktree's HEAD, index and per-worktree reflogs. The\n> problem is I am not sure if we simply just change, say\n> --indexed-objects, to cover all indexes, or should we only do that\n> with \"--all-worktrees --indexed-objects\". I guess this is up for\n> discussion.\n>\n> Nguyễn Thái Ngọc Duy (4):\n>   revision.c: move read_cache() out of add_index_objects_to_pending()\n>   reachable.c: mark reachable objects in index from all worktrees\n>   reachable.c: mark reachable detached HEAD from all worktrees\n>   reachable.c: make reachable reflogs for all per-worktree reflogs\n>\n>  reachable.c      | 47 +++++++++++++++++++++++++++++++++++++++++------\n>  revision.c       | 34 +++++++++++++++++++++++++++-------\n>  revision.h       |  7 ++++++-\n>  t/t5304-prune.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n>  4 files changed, 114 insertions(+), 14 deletions(-)\n"},{"id":"288017","messageId":"574F0983.5030108@alum.mit.edu","threadId":"42492","inReplyTo":"xmqqbn3l0wkv.fsf@gitster.mtv.corp.google.com","subject":"Re: git gc and worktrees","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-01T16:12:51Z","receivedAt":"2016-06-01T16:12:51Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/01/2016 05:15 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> I think reference stores are going to need two distinct types of\n>> reference iteration: one to iterate over the *logical* reference space\n>> of a single repo or worktree, and one to find all *local* references\n>> and/or reachability roots (e.g., when run within a linked repo).\n> \n> This is hard reason about without defining *logical* and *local*.\n\nBy \"logical\" I mean the references that appear to be available in the\nspecified repository. Roughly, \"what reference names can I pass to `git\nrev-parse`?\" If you are in a linked repo, your \"logical\" references are\nmost of those in the main repo plus the \"HEAD\" and \"refs/bisect/*\" and\nprobably a couple other pseudo-refs that are stored in your worktree.\n\n\"Local\" is probably a bad name. What I mean is that we have multiple\nphysical places to store references. In particular, in the case of\nlinked repos we have the main repo's references plus we have the\nworktree's local references.\n\nMost Git commands care only about the \"logical\" references in the repo.\nBut some commands (mostly having to do with reachability) need to know\nabout all roots, and therefore have to know about the union of all of\nthe references stored in each of the linked repositories.\n\nNote that in the second case it is a little silly even talking about\n\"references\", because references in different linked repos might have\nthe same names. We're really talking about reachability roots that might\nhave a little metadata connected with them, like \"this SHA-1 came from\nreference 'HEAD' in the 'test' worktree\" or \"this SHA-1 came from the\nreflog of 'refs/heads/master' in the main repo\".\n\nI argue that the fundamental concept in terms of the implementation\nshould be the individual physical reference stores, and these should be\ncompounded together to form the logical reference collections and the\nsets of reachability roots that are interesting at the UI level.\n\n> For protecting necessary objects from pruning by anchoring them as\n> \"reachable\", and for avoiding unnecessary object transfer by showing\n> things via \".have\" lines, we're better consider references and\n> reflogs that do not appear in the namespace visible from the current\n> worktree.  I wanted to come up with an example of wanting to iterate\n> over refs of the current worktree, but other than \"what are my\n> branches including HEAD?\" I didn't think of anything interesting.\n\nIt doesn't show up much at the UI level, but there are some examples, like\n\n* git log --decorate\n* git for-each-ref --contains\n* gitk --all\n\n> [...]\n\nMichael\n"},{"id":"288063","messageId":"CAPig+cRzRa1uZ+2jD4_uCSisb+0dywPGV3AVE38-xZr3hauTtA@mail.gmail.com","threadId":"42492","inReplyTo":"20160601104519.16563-3-pclouds@gmail.com","subject":"Re: [PATCH 2/4] reachable.c: mark reachable objects in index from all worktrees","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-06-01T18:13:55Z","receivedAt":"2016-06-01T18:13:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jun 1, 2016 at 6:45 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:\n> Current mark_reachable_objects() only marks objects from index from\n> _current_ worktree as reachable instead of all worktrees. Because this\n> function is used for pruning, there is a chance that objects referenced\n> by other worktrees may be deleted. Fix that.\n>\n> Small behavior change in \"one worktree\" case, the index is read again\n> from file. In the current implementation, if the_index is already\n> loaded, the index file will not be read from file again. This adds some\n> more cost to this operation, hopefully insignificant because\n> reachability test is usually very expensive already.\n\nCould this extra index read be avoided by taking advantage of 'struct\nworktree::is_current'[1] and passing the already-loaded index to\nadd_index_objects_to_pending() if true?\n\nOr, am I misunderstanding the issue?\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/292194\n\n> Reported-by: Johannes Sixt <j6t@kdbg.org>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n> diff --git a/reachable.c b/reachable.c\n> @@ -155,6 +156,32 @@ int add_unseen_recent_objects_to_traversal(struct > +static void add_objects_from_worktree(struct rev_info *revs)\n> +{\n> +       struct worktree **worktrees, **p;\n> +\n> +       worktrees = get_worktrees();\n> +       for (p = worktrees; *p; p++) {\n> +               struct worktree *wt = *p;\n> +               struct index_state istate;\n> +\n> +               memset(&istate, 0, sizeof(istate));\n> +               if (read_index_from(&istate,\n> +                                   worktree_git_path(wt, \"index\")) > 0)\n> +                       add_index_objects_to_pending(revs, 0, &istate);\n> +               discard_index(&istate);\n> +       }\n> +       free_worktrees(worktrees);\n> +\n> +       /*\n> +        * this is in case the index is already updated but not\n> +        * written down in file yet, then add_index_... in the above\n> +        * loop will miss new objects that are just created or\n> +        * referenced.\n> +        */\n> +       add_index_objects_to_pending(revs, 0, &the_index);\n> +}\n"},{"id":"288070","messageId":"1464807435.3988.9.camel@twopensource.com","threadId":"42492","inReplyTo":"20160601104519.16563-3-pclouds@gmail.com","subject":"Re: [PATCH 2/4] reachable.c: mark reachable objects in index from all worktrees","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-06-01T18:57:15Z","receivedAt":"2016-06-01T18:57:15Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2016-06-01 at 17:45 +0700, Nguyễn Thái Ngọc Duy wrote:\n> Current mark_reachable_objects() only marks objects from index from\n> _current_ worktree as reachable instead of all worktrees. Because\n> this\n> function is used for pruning, there is a chance that objects\n> referenced\n> by other worktrees may be deleted. Fix that.\n> \n> Small behavior change in \"one worktree\" case, the index is read again\n> from file. In the current implementation, if the_index is already\n> loaded, the index file will not be read from file again. This adds\n> some\n> more cost to this operation, hopefully insignificant because\n> reachability test is usually very expensive already.\n> \n> Reported-by: Johannes Sixt <j6t@kdbg.org>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  reachable.c      | 33 +++++++++++++++++++++++++++++----\n>  t/t5304-prune.sh |  9 +++++++++\n>  2 files changed, 38 insertions(+), 4 deletions(-)\n> \n> diff --git a/reachable.c b/reachable.c\n> index 15dbe60..8f67242 100644\n> --- a/reachable.c\n> +++ b/reachable.c\n> @@ -9,6 +9,7 @@\n>  #include \"cache-tree.h\"\n>  #include \"progress.h\"\n>  #include \"list-objects.h\"\n> +#include \"worktree.h\"\n>  \n>  struct connectivity_progress {\n>  \tstruct progress *progress;\n> @@ -155,6 +156,32 @@ int\n> add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n>  \t\t\t\t      FOR_EACH_OBJECT_LOCAL_ONLY);\n>  }\n>  \n> +static void add_objects_from_worktree(struct rev_info *revs)\n> +{\n> +\tstruct worktree **worktrees, **p;\n> +\n> +\tworktrees = get_worktrees();\n> +\tfor (p = worktrees; *p; p++) {\n> +\t\tstruct worktree *wt = *p;\n> +\t\tstruct index_state istate;\n> +\n> +\t\tmemset(&istate, 0, sizeof(istate));\n\n\nWhy not just struct index_state istate = {0}; ?\n"},{"id":"288073","messageId":"xmqqmvn4y9zq.fsf@gitster.mtv.corp.google.com","threadId":"42492","inReplyTo":"574F0983.5030108@alum.mit.edu","subject":"Re: git gc and worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-01T19:39:21Z","receivedAt":"2016-06-01T19:39:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> I argue that the fundamental concept in terms of the implementation\n> should be the individual physical reference stores, and these should be\n> compounded together to form the logical reference collections and the\n> sets of reachability roots that are interesting at the UI level.\n\nThat is very good in principle.  How does that principle translate\nto the current setup (with possible enhancement with pluggable ref\nbackends) and multiple worktrees?  Let me try thinking it through\naloud.\n\n * Without pluggable ref backend or worktrees, we start from two\n   \"physical reference stores\"; packed-refs file lists refs that\n   will be covered (overridden) by loose refs in .git/refs/.\n   Symbolic refs always being in loose falls out as a natural\n   consequence that packed-refs file does not record symrefs.\n\n * Throw in multiple worktrees to the mix.  How?  Do we consider\n   selected refs/ hierarchies (like refs/bisect/*) as separate\n   physical store (even though it might be backed by the files in\n   the same .git/refs/ filesystem hierarchy) and represent the\n   \"logical\" view as an overlay across the traditional two types of\n   physical reference stores?  That is:\n\n   - loose refs in .git/HEAD, .git/refs/{bisect,...} for\n     per-worktree part form one physical store.  If a ref is found\n     here, that is what we use as a part of the logical view.\n\n   - loose refs in .git/refs/{branches,tags,notes,...} for common\n     part form one physical store.  For a ref that is not found\n     above but is found here becomes a part of the logical view.\n\n   - packed refs in .git/packed-refs is another physical store.  For\n     a ref that is not found in the above two but is found here\n     becomes a part of the logical view.\n\nUp to this point, I am all for your \"separate physical stores are\ncomposited to give a logical view\".  I can see how multi-worktree\nworld view fits within that framework.\n\n * With pluggable ref backend, we may gain yet another \"physical\n   reference store\" possibility, e.g. one backed by lmdb.  If it\n   supports symrefs, a repoitory may use lmdb backed reference store\n   without the traditional two.\n\n   But it is unclear how it would interact with the multi-worktree\n   world order.\n"},{"id":"288107","messageId":"574FB126.4090805@alum.mit.edu","threadId":"42492","inReplyTo":"xmqqmvn4y9zq.fsf@gitster.mtv.corp.google.com","subject":"Re: git gc and worktrees","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-02T04:08:06Z","receivedAt":"2016-06-02T04:08:06Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/01/2016 09:39 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> I argue that the fundamental concept in terms of the implementation\n>> should be the individual physical reference stores, and these should be\n>> compounded together to form the logical reference collections and the\n>> sets of reachability roots that are interesting at the UI level.\n> \n> That is very good in principle.  How does that principle translate\n> to the current setup (with possible enhancement with pluggable ref\n> backends) and multiple worktrees?  Let me try thinking it through\n> aloud.\n> \n>  * Without pluggable ref backend or worktrees, we start from two\n>    \"physical reference stores\"; packed-refs file lists refs that\n>    will be covered (overridden) by loose refs in .git/refs/.\n>    Symbolic refs always being in loose falls out as a natural\n>    consequence that packed-refs file does not record symrefs.\n> \n>  * Throw in multiple worktrees to the mix.  How?  Do we consider\n>    selected refs/ hierarchies (like refs/bisect/*) as separate\n>    physical store (even though it might be backed by the files in\n>    the same .git/refs/ filesystem hierarchy) and represent the\n>    \"logical\" view as an overlay across the traditional two types of\n>    physical reference stores?  That is:\n> \n>    - loose refs in .git/HEAD, .git/refs/{bisect,...} for\n>      per-worktree part form one physical store.  If a ref is found\n>      here, that is what we use as a part of the logical view.\n> \n>    - loose refs in .git/refs/{branches,tags,notes,...} for common\n>      part form one physical store.  For a ref that is not found\n>      above but is found here becomes a part of the logical view.\n> \n>    - packed refs in .git/packed-refs is another physical store.  For\n>      a ref that is not found in the above two but is found here\n>      becomes a part of the logical view.\n\nI think I would represent the logical store of a worktree repo as\nfollows. First, I would implement a cached_ref_store that introduces a\nlayer of caching around another ref_store. Then\n\n    def get_files_ref_store(dir) {\n        loose = create_cached_ref_store(get_loose_ref_store(dir))\n        packed = create_cached_ref_store(get_packed_ref_store(dir))\n        return create_files_ref_store(loose, packed)\n    }\n\n    common_ref_store = get_files_ref_store(common_dir)\n\n    /*\n     * I think we only allow loose refs in worktrees; otherwise\n     * this could be an overlay_ref_store too. Actually, we might\n     * want to omit the caching here.\n     */\n    local_ref_store = create_cached_ref_store(\n            get_loose_ref_store(git_dir))\n\n    logical_ref_store = create_worktree_ref_store(\n        local_ref_store, common_ref_store)\n\nWhere worktree_ref_store does something like\n\n    if (is_per_worktree_ref(refname))\n        lookup in local_ref_store\n    else\n        lookup in common_ref_store\n\nfor reading, and uses a merge_ref_iterator with a select function that\ndoes something similar for iterating.\n\nThe files_ref_store would do lookups by looking first in the\nloose_ref_store then in the packed_ref_store, would use an\noverlay_ref_iterator for iteration, and would know to do all writes in\nthe loose_ref_store (except for deletes, which also have to go to\npacked_ref_store). It would have a special \"pack-refs\" operation,\nspecific to files_ref_store, that shuffles references between its two\nbackends.\n\nWriting to a worktree_ref_store is a bit tricker, because we want to\nallow ref_transactions to span worktree and common refs (though we\nprobably need to give up atomicity for any such transaction). The\nworktree_ref_transaction_commit() method has to split the main\ntransaction into two sub-transactions, one for each of its component\nref_stores. I planned for this when designing split_under_lock and think\nit is possible, though I admit I haven't implemented it yet.\n\nOne nice thing about this design is that you can skip the\nworktree_ref_store layer and its overhead entirely for repositories that\nare not linked. The decision can be made once, at instantiation time,\nrather than every time a reference is looked up. See the pseudocode below.\n\n> Up to this point, I am all for your \"separate physical stores are\n> composited to give a logical view\".  I can see how multi-worktree\n> world view fits within that framework.\n> \n>  * With pluggable ref backend, we may gain yet another \"physical\n>    reference store\" possibility, e.g. one backed by lmdb.  If it\n>    supports symrefs, a repoitory may use lmdb backed reference store\n>    without the traditional two.\n> \n>    But it is unclear how it would interact with the multi-worktree\n>    world order.\n\nSince you could plug-and-play different ref_stores in the above scheme,\nI don't see any problem here.\n\n    def get_logical_ref_store() {\n        local_ref_store = get_local_ref_store(git_dir)\n        if (is_linked_repo) {\n            common_ref_store = get_ref_store(common_dir)\n            return worktree_ref_store(local_ref_store,\n                                      common_ref_store)\n        } else {\n            return local_ref_store;\n        }\n    }\n\nget_ref_store() would read the git config to decide what the ref store\nto use for the specified repository, which itself might be an\nlmdb_ref_store or an overlay_ref_store(loose_ref_store, packed_ref_store).\n\nMichael\n"},{"id":"288110","messageId":"CACsJy8BCrCYDpAXUrkFvFNXrua39Ou1r5=s_NK28Pm61O+u-rQ@mail.gmail.com","threadId":"42492","inReplyTo":"CAPig+cRzRa1uZ+2jD4_uCSisb+0dywPGV3AVE38-xZr3hauTtA@mail.gmail.com","subject":"Re: [PATCH 2/4] reachable.c: mark reachable objects in index from all worktrees","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-02T09:35:18Z","receivedAt":"2016-06-02T09:35:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jun 2, 2016 at 1:13 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Wed, Jun 1, 2016 at 6:45 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:\n>> Current mark_reachable_objects() only marks objects from index from\n>> _current_ worktree as reachable instead of all worktrees. Because this\n>> function is used for pruning, there is a chance that objects referenced\n>> by other worktrees may be deleted. Fix that.\n>>\n>> Small behavior change in \"one worktree\" case, the index is read again\n>> from file. In the current implementation, if the_index is already\n>> loaded, the index file will not be read from file again. This adds some\n>> more cost to this operation, hopefully insignificant because\n>> reachability test is usually very expensive already.\n>\n> Could this extra index read be avoided by taking advantage of 'struct\n> worktree::is_current'[1] and passing the already-loaded index to\n> add_index_objects_to_pending() if true?\n>\n> Or, am I misunderstanding the issue?\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/292194\n\nHah! I broke my worktree changes into many pieces and lost track of\nthem. I thought that one was still in some unsent series. Will use it.\n-- \nDuy\n"},{"id":"288111","messageId":"CACsJy8CsmeRgk49quPZe5vx5W_3X1T=ConhsfPBEcEwvJ1-b4Q@mail.gmail.com","threadId":"42492","inReplyTo":"1464807435.3988.9.camel@twopensource.com","subject":"Re: [PATCH 2/4] reachable.c: mark reachable objects in index from all worktrees","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-02T09:37:06Z","receivedAt":"2016-06-02T09:37:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jun 2, 2016 at 1:57 AM, David Turner <dturner@twopensource.com> wrote:\n>> +             struct index_state istate;\n>> +\n>> +             memset(&istate, 0, sizeof(istate));\n>\n>\n> Why not just struct index_state istate = {0}; ?\n>\n\nMy first thought was.. \"hmm.. C99?\" but then there are 24 of them in\nthe code base already. Will change.\n-- \nDuy\n"},{"id":"288113","messageId":"CACsJy8DjKo-HcyG6sKhhvx8vtySn6VTTP-E6vX2uKNEJgjouZg@mail.gmail.com","threadId":"42492","inReplyTo":"xmqqshwwzyee.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/4] Fix prune/gc problem with multiple worktrees","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-02T09:53:12Z","receivedAt":"2016-06-02T09:53:12Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"(from patch 4/4 mail)\n\nOn Wed, Jun 1, 2016 at 10:51 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>> +     path = xstrdup(worktree_git_path(wt, \"logs/refs/bisect\"));\n>> +     if (file_exists(path))\n>> +             handle_one_reflog(path, NULL, 0, &cb);\n>> +     free(path);\n>> +}\n>\n> `refs/bisect` is not a single reference. It is a namespace that contains\n> references with names like `refs/bisect/bad` and\n> `refs/bisect/good-66106691a1b71e445fe5e4d6b8b043dffc7dfe4c`.\n\nYeah I missed that. I'm not going to write another directory walker to\ncollect all logs/refs/bisect/*. I didn't add pending objects for\nrefs/bisect/* of other worktrees either. At that point waiting for the\nnew ref iterator makes more sense...\n\nOn Wed, Jun 1, 2016 at 11:06 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n>\n>> This series makes sure that objects referenced by all worktrees are\n>> marked reachable so that we don't accidentally delete objects that are\n>> being used. Previously per-worktree references in index, detached HEAD\n>> or per-worktree reflogs come from current worktree only, not all\n>> worktrees.\n>\n> I'll let this topic simmer on the list for now, instead of picking\n> it up immediately to 'pu', as Michael in $gmane/296068 makes me\n> wonder if we want to keep piling on the current \"worktree ref\n> iterations are bolted on\" or if we want to first clean it up, whose\n> natural fallout hopefully would eliminate the bug away.\n\nSo what should be the way forward? My intention was having something\nthat can fix the problem for now, even if a bit hacky while waiting\nfor ref iterator to be ready, then convert to use it and clean things\nup, because I don't how long ref-iterator would take and losing data\nis serous enough that I'd like to fix it soon. If we go with \"fix soon\nthen convert to ref-iterator later\", then I will drop the\nlogs/bisect/* check, checking logs/HEAD alone should be good enough.\nOtherwise I'll prepare a series on top of ref-iterator.\n-- \nDuy\n"},{"id":"288116","messageId":"575017FF.1020901@alum.mit.edu","threadId":"42492","inReplyTo":"CACsJy8DjKo-HcyG6sKhhvx8vtySn6VTTP-E6vX2uKNEJgjouZg@mail.gmail.com","subject":"Re: [PATCH 0/4] Fix prune/gc problem with multiple worktrees","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-02T11:26:55Z","receivedAt":"2016-06-02T11:26:55Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/02/2016 11:53 AM, Duy Nguyen wrote:\n> (from patch 4/4 mail)\n> \n> On Wed, Jun 1, 2016 at 10:51 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>>> +     path = xstrdup(worktree_git_path(wt, \"logs/refs/bisect\"));\n>>> +     if (file_exists(path))\n>>> +             handle_one_reflog(path, NULL, 0, &cb);\n>>> +     free(path);\n>>> +}\n>>\n>> `refs/bisect` is not a single reference. It is a namespace that contains\n>> references with names like `refs/bisect/bad` and\n>> `refs/bisect/good-66106691a1b71e445fe5e4d6b8b043dffc7dfe4c`.\n> \n> Yeah I missed that. I'm not going to write another directory walker to\n> collect all logs/refs/bisect/*. I didn't add pending objects for\n> refs/bisect/* of other worktrees either. At that point waiting for the\n> new ref iterator makes more sense...\n> \n> On Wed, Jun 1, 2016 at 11:06 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n>>\n>>> This series makes sure that objects referenced by all worktrees are\n>>> marked reachable so that we don't accidentally delete objects that are\n>>> being used. Previously per-worktree references in index, detached HEAD\n>>> or per-worktree reflogs come from current worktree only, not all\n>>> worktrees.\n>>\n>> I'll let this topic simmer on the list for now, instead of picking\n>> it up immediately to 'pu', as Michael in $gmane/296068 makes me\n>> wonder if we want to keep piling on the current \"worktree ref\n>> iterations are bolted on\" or if we want to first clean it up, whose\n>> natural fallout hopefully would eliminate the bug away.\n> \n> So what should be the way forward? My intention was having something\n> that can fix the problem for now, even if a bit hacky while waiting\n> for ref iterator to be ready, then convert to use it and clean things\n> up, because I don't how long ref-iterator would take and losing data\n> is serous enough that I'd like to fix it soon. If we go with \"fix soon\n> then convert to ref-iterator later\", then I will drop the\n> logs/bisect/* check, checking logs/HEAD alone should be good enough.\n> Otherwise I'll prepare a series on top of ref-iterator.\n\nFixing reachability via the index and detached HEADs feels relatively\nimportant.\n\nFixing refs/bisect/* feels a bit less urgent, because (1) usually\nbisections don't take very long, and (2) if the commits that one is\nbisecting are not otherwise reachable anymore, then the bisection is\nprobably not interesting anymore. However, these are real references, so\nif they get broken, the worktree will be seen as corrupt and recovery is\nnot super obvious (I guess most people would end up deleting the whole\nworktree).\n\nFixing the reflogs for HEAD and (especially) refs/bisect/* in worktrees\nfeels even less important, because reflogs are not nearly as important\nas current ref values, Git is relatively tolerant of broken reflog\nentries, and there are easy ways to prune them if breakage occurs.\n\nIt's hard for me to predict when the ref-iterator stuff will be merged.\nIt is a big change, but so far the feedback seems pretty good. I can\ntell you that pushing it and ref-stores forward is high on my priority list.\n\nMichael\n"},{"id":"288140","messageId":"xmqqy46ntrhk.fsf@gitster.mtv.corp.google.com","threadId":"42492","inReplyTo":"575017FF.1020901@alum.mit.edu","subject":"Re: [PATCH 0/4] Fix prune/gc problem with multiple worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-02T17:44:55Z","receivedAt":"2016-06-02T17:44:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> Fixing reachability via the index and detached HEADs feels relatively\n> important.\n> ...\n\nI agree with the order of importance above.  But \"relatively\" is a\nvery good keyword.  Just like bisection refs, what is in the index\nand the commit detached HEAD points at are expected to be tentative.\nAs a part of still-experimental feature, I'd rather see our\nbandwidth spent on fixing it the right way first time, instead of\npiling on an unproven quick-fix as a band aid, having to rip it off\nand fixing it properly later.\n\n> It's hard for me to predict when the ref-iterator stuff will be merged.\n> It is a big change, but so far the feedback seems pretty good. I can\n> tell you that pushing it and ref-stores forward is high on my priority list.\n\nThanks.\n"},{"id":"288266","messageId":"xmqqvb1qp6gi.fsf@gitster.mtv.corp.google.com","threadId":"42492","inReplyTo":"574FB126.4090805@alum.mit.edu","subject":"Re: git gc and worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-03T16:45:01Z","receivedAt":"2016-06-03T16:45:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> On 06/01/2016 09:39 PM, Junio C Hamano wrote:\n>> ...\n> I think I would represent the logical store of a worktree repo as\n> follows. First, ...\n> ...\n>> Up to this point, I am all for your \"separate physical stores are\n>> composited to give a logical view\".  I can see how multi-worktree\n>> world view fits within that framework.\n>> \n>>  * With pluggable ref backend, we may gain yet another \"physical\n>>    reference store\" possibility, e.g. one backed by lmdb.  If it\n>>    supports symrefs, a repoitory may use lmdb backed reference store\n>>    without the traditional two.\n>> \n>>    But it is unclear how it would interact with the multi-worktree\n>>    world order.\n>\n> Since you could plug-and-play different ref_stores in the above scheme,\n> I don't see any problem here.\n>\n>     def get_logical_ref_store() {\n>         local_ref_store = get_local_ref_store(git_dir)\n>         if (is_linked_repo) {\n>             common_ref_store = get_ref_store(common_dir)\n>             return worktree_ref_store(local_ref_store,\n>                                       common_ref_store)\n>         } else {\n>             return local_ref_store;\n>         }\n>     }\n>\n> get_ref_store() would read the git config to decide what the ref store\n> to use for the specified repository, which itself might be an\n> lmdb_ref_store or an overlay_ref_store(loose_ref_store, packed_ref_store).\n\nSounds all sensible.  Thanks.\n"}]}