{"thread":{"id":"45745","subject":"[PATCH v3 00/12] Fix git-gc losing objects in multi worktree","startedAt":"2017-04-19T11:02:05Z","lastAt":"2017-05-17T13:59:43Z","messageCount":30,"participants":["Nguyễn Thái Ngọc Duy","Ramsay Jones","Johannes Sixt","Duy Nguyen","Junio C Hamano","Michael Haggerty","Stefan Beller"],"isPatch":true,"patchVersion":3,"patchTotal":12},"messages":[{"id":"317212","messageId":"20170419110145.5086-1-pclouds@gmail.com","threadId":"45745","inReplyTo":null,"subject":"[PATCH v3 00/12] Fix git-gc losing objects in multi worktree","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-19T11:01:33Z","receivedAt":"2017-04-19T11:02:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Changes since v2 [1] is relatively small. It still needs\nnd/worktree-kill-parse-ref of course.\n\n[1] http://public-inbox.org/git/20170318101153.6901-1-pclouds@gmail.com/\n\ndiff --git a/Documentation/technical/api-ref-iteration.txt b/Documentation/technical/api-ref-iteration.txt\nindex 37379d8337..c9e9a60dbd 100644\n--- a/Documentation/technical/api-ref-iteration.txt\n+++ b/Documentation/technical/api-ref-iteration.txt\n@@ -32,11 +32,8 @@ Iteration functions\n \n * `for_each_glob_ref_in()` the previous and `for_each_ref_in()` combined.\n \n-* `head_ref_submodule()`, `for_each_ref_submodule()`,\n-  `for_each_ref_in_submodule()`, `for_each_tag_ref_submodule()`,\n-  `for_each_branch_ref_submodule()`, `for_each_remote_ref_submodule()`\n-  do the same as the functions described above but for a specified\n-  submodule.\n+* Use `refs_` API for accessing submodules. The submodule ref store could\n+  be obtained with `get_submodule_ref_store().\n \n * `for_each_rawref()` can be used to learn about broken ref and symref.\n \ndiff --git a/refs.c b/refs.c\nindex c39058adce..23e3607674 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1208,11 +1208,6 @@ int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n \treturn 0;\n }\n \n-int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)\n-{\n-\treturn refs_head_ref(get_submodule_ref_store(submodule), fn, cb_data);\n-}\n-\n int head_ref(each_ref_fn fn, void *cb_data)\n {\n \treturn refs_head_ref(get_main_ref_store(), fn, cb_data);\ndiff --git a/revision.c b/revision.c\nindex dc32e99c54..79ce8a007f 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1336,7 +1336,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)\n \tworktrees = get_worktrees(0);\n \tfor (p = worktrees; *p; p++) {\n \t\tstruct worktree *wt = *p;\n-\t\tstruct index_state istate = {0};\n+\t\tstruct index_state istate = { NULL };\n \n \t\tif (wt->is_current)\n \t\t\tcontinue; /* current index already taken care of */\n\nNguyễn Thái Ngọc Duy (12):\n  revision.h: new flag in struct rev_info wrt. worktree-related refs\n  revision.c: refactor add_index_objects_to_pending()\n  revision.c: --indexed-objects add objects from all worktrees\n  refs.c: refactor get_submodule_ref_store(), share common free block\n  refs: move submodule slash stripping code to get_submodule_ref_store\n  refs: add refs_head_ref()\n  revision.c: use refs_for_each*() instead of for_each_*_submodule()\n  refs: remove dead for_each_*_submodule()\n  revision.c: --all adds HEAD from all worktrees\n  files-backend: make reflog iterator go through per-worktree reflog\n  revision.c: --reflog add HEAD reflog from all worktrees\n  rev-list: expose and document --single-worktree\n\n Documentation/rev-list-options.txt            |   8 ++\n Documentation/technical/api-ref-iteration.txt |   7 +-\n reachable.c                                   |   1 +\n refs.c                                        | 105 ++++++++++-----------\n refs.h                                        |  12 +--\n refs/files-backend.c                          |  46 ++++++---\n revision.c                                    | 130 +++++++++++++++++++++-----\n revision.h                                    |   1 +\n submodule.c                                   |   2 +\n t/t1407-worktree-ref-store.sh                 |  30 ++++++\n t/t5304-prune.sh                              |  37 ++++++++\n 11 files changed, 274 insertions(+), 105 deletions(-)\n\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317213","messageId":"20170419110145.5086-2-pclouds@gmail.com","threadId":"45745","inReplyTo":"20170419110145.5086-1-pclouds@gmail.com","subject":"[PATCH v3 01/12] revision.h: new flag in struct rev_info wrt. worktree-related refs","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-19T11:01:34Z","receivedAt":"2017-04-19T11:02:12Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The revision walker can walk through per-worktree refs like HEAD or\nSHA-1 references in the index. These currently are from the current\nworktree only. This new flag is added to change rev-list behavior in\nthis regard:\n\nWhen single_worktree is set, only current worktree is considered. When\nit is not set (which is the default), all worktrees are considered.\n\nThe default is chosen so because the two big components that rev-list\nworks with are object database (entirely shared between worktrees) and\nrefs (mostly shared). It makes sense that default behavior goes per-repo\ntoo instead of per-worktree.\n\nThe flag will eventually be exposed as a rev-list argument with\ndocuments. For now it stays internal until the new behavior is fully\nimplemented.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n revision.h | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/revision.h b/revision.h\nindex 9fac1a607d..c851b94ad8 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -88,6 +88,7 @@ struct rev_info {\n \t\t\ttopo_order:1,\n \t\t\tsimplify_merges:1,\n \t\t\tsimplify_by_decoration:1,\n+\t\t\tsingle_worktree:1,\n \t\t\ttag_objects:1,\n \t\t\ttree_objects:1,\n \t\t\tblob_objects:1,\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317214","messageId":"20170419110145.5086-3-pclouds@gmail.com","threadId":"45745","inReplyTo":"20170419110145.5086-1-pclouds@gmail.com","subject":"[PATCH v3 02/12] revision.c: refactor add_index_objects_to_pending()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-19T11:01:35Z","receivedAt":"2017-04-19T11:02:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The core code is factored out and take 'struct index_state *' instead so\nthat we can reuse it to add objects from index files other than .git/index\nin the next patch.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n revision.c | 18 ++++++++++++------\n 1 file changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 7ff61ff5f7..98146f179f 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1263,13 +1263,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+static void do_add_index_objects_to_pending(struct rev_info *revs,\n+\t\t\t\t\t    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\tstruct cache_entry *ce = istate->cache[i];\n \t\tstruct blob *blob;\n \n \t\tif (S_ISGITLINK(ce->ce_mode))\n@@ -1282,13 +1282,19 @@ 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 \n+void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)\n+{\n+\tread_cache();\n+\tdo_add_index_objects_to_pending(revs, &the_index);\n+}\n+\n static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,\n \t\t\t    int exclude_parent)\n {\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317215","messageId":"20170419110145.5086-4-pclouds@gmail.com","threadId":"45745","inReplyTo":"20170419110145.5086-1-pclouds@gmail.com","subject":"[PATCH v3 03/12] revision.c: --indexed-objects add objects from all worktrees","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-19T11:01:36Z","receivedAt":"2017-04-19T11:02:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This is the result of single_worktree flag never being set (no way to up\nuntil now). To get objects from current index only, set single_worktree.\n\nThe other add_index_objects_to_pending's caller is mark_reachable_objects()\n(e.g. \"git prune\") which also mark objects from all indexes.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n revision.c       | 21 +++++++++++++++++++++\n t/t5304-prune.sh |  9 +++++++++\n 2 files changed, 30 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex 98146f179f..295d4f8205 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@@ -1291,8 +1292,28 @@ static void do_add_index_objects_to_pending(struct rev_info *revs,\n \n void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)\n {\n+\tstruct worktree **worktrees, **p;\n+\n \tread_cache();\n \tdo_add_index_objects_to_pending(revs, &the_index);\n+\n+\tif (revs->single_worktree)\n+\t\treturn;\n+\n+\tworktrees = get_worktrees(0);\n+\tfor (p = worktrees; *p; p++) {\n+\t\tstruct worktree *wt = *p;\n+\t\tstruct index_state istate = { NULL };\n+\n+\t\tif (wt->is_current)\n+\t\t\tcontinue; /* current index already taken care of */\n+\n+\t\tif (read_index_from(&istate,\n+\t\t\t\t    worktree_git_path(wt, \"index\")) > 0)\n+\t\t\tdo_add_index_objects_to_pending(revs, &istate);\n+\t\tdiscard_index(&istate);\n+\t}\n+\tfree_worktrees(worktrees);\n }\n \n static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 133b5842b1..cba45c7be9 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.11.0.157.gd943d85\n\n"},{"id":"317216","messageId":"20170419110145.5086-5-pclouds@gmail.com","threadId":"45745","inReplyTo":"20170419110145.5086-1-pclouds@gmail.com","subject":"[PATCH v3 04/12] refs.c: refactor get_submodule_ref_store(), share common free block","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-19T11:01:37Z","receivedAt":"2017-04-19T11:02:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n refs.c | 12 +++++-------\n 1 file changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 5d31fb6bcf..5902a3d9e5 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1570,19 +1570,16 @@ struct ref_store *get_submodule_ref_store(const char *submodule)\n \n \trefs = lookup_ref_store_map(&submodule_ref_stores, submodule);\n \tif (refs)\n-\t\treturn refs;\n+\t\tgoto done;\n \n \tstrbuf_addstr(&submodule_sb, submodule);\n \tret = is_nonbare_repository_dir(&submodule_sb);\n-\tstrbuf_release(&submodule_sb);\n \tif (!ret)\n-\t\treturn NULL;\n+\t\tgoto done;\n \n \tret = submodule_to_gitdir(&submodule_sb, submodule);\n-\tif (ret) {\n-\t\tstrbuf_release(&submodule_sb);\n-\t\treturn NULL;\n-\t}\n+\tif (ret)\n+\t\tgoto done;\n \n \t/* assume that add_submodule_odb() has been called */\n \trefs = ref_store_init(submodule_sb.buf,\n@@ -1590,6 +1587,7 @@ struct ref_store *get_submodule_ref_store(const char *submodule)\n \tregister_ref_store_map(&submodule_ref_stores, \"submodule\",\n \t\t\t       refs, submodule);\n \n+done:\n \tstrbuf_release(&submodule_sb);\n \treturn refs;\n }\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317217","messageId":"20170419110145.5086-6-pclouds@gmail.com","threadId":"45745","inReplyTo":"20170419110145.5086-1-pclouds@gmail.com","subject":"[PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-19T11:01:38Z","receivedAt":"2017-04-19T11:02:34Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This is a better place that will benefit all submodule callers instead\nof just resolve_gitlink_ref()\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n refs.c | 33 +++++++++++++++++----------------\n 1 file changed, 17 insertions(+), 16 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 5902a3d9e5..26474cb62a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1422,25 +1422,10 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,\n int resolve_gitlink_ref(const char *submodule, const char *refname,\n \t\t\tunsigned char *sha1)\n {\n-\tsize_t len = strlen(submodule);\n \tstruct ref_store *refs;\n \tint flags;\n \n-\twhile (len && submodule[len - 1] == '/')\n-\t\tlen--;\n-\n-\tif (!len)\n-\t\treturn -1;\n-\n-\tif (submodule[len]) {\n-\t\t/* We need to strip off one or more trailing slashes */\n-\t\tchar *stripped = xmemdupz(submodule, len);\n-\n-\t\trefs = get_submodule_ref_store(stripped);\n-\t\tfree(stripped);\n-\t} else {\n-\t\trefs = get_submodule_ref_store(submodule);\n-\t}\n+\trefs = get_submodule_ref_store(submodule);\n \n \tif (!refs)\n \t\treturn -1;\n@@ -1558,7 +1543,17 @@ struct ref_store *get_submodule_ref_store(const char *submodule)\n {\n \tstruct strbuf submodule_sb = STRBUF_INIT;\n \tstruct ref_store *refs;\n+\tchar *to_free = NULL;\n \tint ret;\n+\tsize_t len;\n+\n+\tif (submodule) {\n+\t\tlen = strlen(submodule);\n+\t\twhile (len && submodule[len - 1] == '/')\n+\t\t\tlen--;\n+\t\tif (!len)\n+\t\t\tsubmodule = NULL;\n+\t}\n \n \tif (!submodule || !*submodule) {\n \t\t/*\n@@ -1568,6 +1563,10 @@ struct ref_store *get_submodule_ref_store(const char *submodule)\n \t\treturn get_main_ref_store();\n \t}\n \n+\tif (submodule[len])\n+\t\t/* We need to strip off one or more trailing slashes */\n+\t\tsubmodule = to_free = xmemdupz(submodule, len);\n+\n \trefs = lookup_ref_store_map(&submodule_ref_stores, submodule);\n \tif (refs)\n \t\tgoto done;\n@@ -1589,6 +1588,8 @@ struct ref_store *get_submodule_ref_store(const char *submodule)\n \n done:\n \tstrbuf_release(&submodule_sb);\n+\tfree(to_free);\n+\n \treturn refs;\n }\n \n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317218","messageId":"20170419110145.5086-7-pclouds@gmail.com","threadId":"45745","inReplyTo":"20170419110145.5086-1-pclouds@gmail.com","subject":"[PATCH v3 06/12] refs: add refs_head_ref()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-19T11:01:39Z","receivedAt":"2017-04-19T11:02:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n refs.c | 19 +++++++++----------\n refs.h |  2 ++\n 2 files changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 26474cb62a..a252ae43ee 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1208,27 +1208,26 @@ int refs_rename_ref_available(struct ref_store *refs,\n \treturn ok;\n }\n \n-int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)\n+int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n {\n \tstruct object_id oid;\n \tint flag;\n \n-\tif (submodule) {\n-\t\tif (resolve_gitlink_ref(submodule, \"HEAD\", oid.hash) == 0)\n-\t\t\treturn fn(\"HEAD\", &oid, 0, cb_data);\n-\n-\t\treturn 0;\n-\t}\n-\n-\tif (!read_ref_full(\"HEAD\", RESOLVE_REF_READING, oid.hash, &flag))\n+\tif (!refs_read_ref_full(refs, \"HEAD\", RESOLVE_REF_READING,\n+\t\t\t\toid.hash, &flag))\n \t\treturn fn(\"HEAD\", &oid, flag, cb_data);\n \n \treturn 0;\n }\n \n+int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)\n+{\n+\treturn refs_head_ref(get_submodule_ref_store(submodule), fn, cb_data);\n+}\n+\n int head_ref(each_ref_fn fn, void *cb_data)\n {\n-\treturn head_ref_submodule(NULL, fn, cb_data);\n+\treturn refs_head_ref(get_main_ref_store(), fn, cb_data);\n }\n \n /*\ndiff --git a/refs.h b/refs.h\nindex 447381d378..0572473ef7 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -233,6 +233,8 @@ typedef int each_ref_fn(const char *refname,\n  * modifies the reference also returns a nonzero value to immediately\n  * stop the iteration. Returned references are sorted.\n  */\n+int refs_head_ref(struct ref_store *refs,\n+\t\t  each_ref_fn fn, void *cb_data);\n int refs_for_each_ref(struct ref_store *refs,\n \t\t      each_ref_fn fn, void *cb_data);\n int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317219","messageId":"20170419110145.5086-8-pclouds@gmail.com","threadId":"45745","inReplyTo":"20170419110145.5086-1-pclouds@gmail.com","subject":"[PATCH v3 07/12] revision.c: use refs_for_each*() instead of for_each_*_submodule()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-19T11:01:40Z","receivedAt":"2017-04-19T11:02:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n revision.c | 48 ++++++++++++++++++++++++++++++++----------------\n 1 file changed, 32 insertions(+), 16 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 295d4f8205..c329070c89 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1189,12 +1189,19 @@ void add_ref_exclusion(struct string_list **ref_excludes_p, const char *exclude)\n \tstring_list_append(*ref_excludes_p, exclude);\n }\n \n-static void handle_refs(const char *submodule, struct rev_info *revs, unsigned flags,\n-\t\tint (*for_each)(const char *, each_ref_fn, void *))\n+static void handle_refs(struct ref_store *refs,\n+\t\t\tstruct rev_info *revs, unsigned flags,\n+\t\t\tint (*for_each)(struct ref_store *, each_ref_fn, void *))\n {\n \tstruct all_refs_cb cb;\n+\n+\tif (!refs) {\n+\t\t/* this could happen with uninitialized submodules */\n+\t\treturn;\n+\t}\n+\n \tinit_all_refs_cb(&cb, revs, flags);\n-\tfor_each(submodule, handle_one_ref, &cb);\n+\tfor_each(refs, handle_one_ref, &cb);\n }\n \n static void handle_one_reflog_commit(struct object_id *oid, void *cb_data)\n@@ -2067,23 +2074,25 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,\n \tctx->argc -= n;\n }\n \n-static int for_each_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data, const char *term) {\n+static int for_each_bisect_ref(struct ref_store *refs, each_ref_fn fn,\n+\t\t\t       void *cb_data, const char *term)\n+{\n \tstruct strbuf bisect_refs = STRBUF_INIT;\n \tint status;\n \tstrbuf_addf(&bisect_refs, \"refs/bisect/%s\", term);\n-\tstatus = for_each_ref_in_submodule(submodule, bisect_refs.buf, fn, cb_data);\n+\tstatus = refs_for_each_ref_in(refs, bisect_refs.buf, fn, cb_data);\n \tstrbuf_release(&bisect_refs);\n \treturn status;\n }\n \n-static int for_each_bad_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data)\n+static int for_each_bad_bisect_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n {\n-\treturn for_each_bisect_ref(submodule, fn, cb_data, term_bad);\n+\treturn for_each_bisect_ref(refs, fn, cb_data, term_bad);\n }\n \n-static int for_each_good_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data)\n+static int for_each_good_bisect_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n {\n-\treturn for_each_bisect_ref(submodule, fn, cb_data, term_good);\n+\treturn for_each_bisect_ref(refs, fn, cb_data, term_good);\n }\n \n static int handle_revision_pseudo_opt(const char *submodule,\n@@ -2092,8 +2101,14 @@ static int handle_revision_pseudo_opt(const char *submodule,\n {\n \tconst char *arg = argv[0];\n \tconst char *optarg;\n+\tstruct ref_store *refs;\n \tint argcount;\n \n+\tif (submodule) {\n+\t\trefs = get_submodule_ref_store(submodule);\n+\t} else\n+\t\trefs = get_main_ref_store();\n+\n \t/*\n \t * NOTE!\n \t *\n@@ -2105,22 +2120,23 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \t * register it in the list at the top of handle_revision_opt.\n \t */\n \tif (!strcmp(arg, \"--all\")) {\n-\t\thandle_refs(submodule, revs, *flags, for_each_ref_submodule);\n-\t\thandle_refs(submodule, revs, *flags, head_ref_submodule);\n+\t\thandle_refs(refs, revs, *flags, refs_for_each_ref);\n+\t\thandle_refs(refs, revs, *flags, refs_head_ref);\n \t\tclear_ref_exclusion(&revs->ref_excludes);\n \t} else if (!strcmp(arg, \"--branches\")) {\n-\t\thandle_refs(submodule, revs, *flags, for_each_branch_ref_submodule);\n+\t\thandle_refs(refs, revs, *flags, refs_for_each_branch_ref);\n \t\tclear_ref_exclusion(&revs->ref_excludes);\n \t} else if (!strcmp(arg, \"--bisect\")) {\n \t\tread_bisect_terms(&term_bad, &term_good);\n-\t\thandle_refs(submodule, revs, *flags, for_each_bad_bisect_ref);\n-\t\thandle_refs(submodule, revs, *flags ^ (UNINTERESTING | BOTTOM), for_each_good_bisect_ref);\n+\t\thandle_refs(refs, revs, *flags, for_each_bad_bisect_ref);\n+\t\thandle_refs(refs, revs, *flags ^ (UNINTERESTING | BOTTOM),\n+\t\t\t    for_each_good_bisect_ref);\n \t\trevs->bisect = 1;\n \t} else if (!strcmp(arg, \"--tags\")) {\n-\t\thandle_refs(submodule, revs, *flags, for_each_tag_ref_submodule);\n+\t\thandle_refs(refs, revs, *flags, refs_for_each_tag_ref);\n \t\tclear_ref_exclusion(&revs->ref_excludes);\n \t} else if (!strcmp(arg, \"--remotes\")) {\n-\t\thandle_refs(submodule, revs, *flags, for_each_remote_ref_submodule);\n+\t\thandle_refs(refs, revs, *flags, refs_for_each_remote_ref);\n \t\tclear_ref_exclusion(&revs->ref_excludes);\n \t} else if ((argcount = parse_long_opt(\"glob\", argv, &optarg))) {\n \t\tstruct all_refs_cb cb;\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317220","messageId":"20170419110145.5086-9-pclouds@gmail.com","threadId":"45745","inReplyTo":"20170419110145.5086-1-pclouds@gmail.com","subject":"[PATCH v3 08/12] refs: remove dead for_each_*_submodule()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-19T11:01:41Z","receivedAt":"2017-04-19T11:02:58Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"These are used in revision.c. After the last patch they are replaced\nwith the refs_ version. Delete them (except for_each_remote_ref_submodule\nwhich is still used by submodule.c)\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Documentation/technical/api-ref-iteration.txt |  7 ++-----\n refs.c                                        | 29 ---------------------------\n refs.h                                        |  9 ---------\n 3 files changed, 2 insertions(+), 43 deletions(-)\n\ndiff --git a/Documentation/technical/api-ref-iteration.txt b/Documentation/technical/api-ref-iteration.txt\nindex 37379d8337..c9e9a60dbd 100644\n--- a/Documentation/technical/api-ref-iteration.txt\n+++ b/Documentation/technical/api-ref-iteration.txt\n@@ -32,11 +32,8 @@ Iteration functions\n \n * `for_each_glob_ref_in()` the previous and `for_each_ref_in()` combined.\n \n-* `head_ref_submodule()`, `for_each_ref_submodule()`,\n-  `for_each_ref_in_submodule()`, `for_each_tag_ref_submodule()`,\n-  `for_each_branch_ref_submodule()`, `for_each_remote_ref_submodule()`\n-  do the same as the functions described above but for a specified\n-  submodule.\n+* Use `refs_` API for accessing submodules. The submodule ref store could\n+  be obtained with `get_submodule_ref_store().\n \n * `for_each_rawref()` can be used to learn about broken ref and symref.\n \ndiff --git a/refs.c b/refs.c\nindex a252ae43ee..537052f7ba 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -316,12 +316,6 @@ int for_each_tag_ref(each_ref_fn fn, void *cb_data)\n \treturn refs_for_each_tag_ref(get_main_ref_store(), fn, cb_data);\n }\n \n-int for_each_tag_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)\n-{\n-\treturn refs_for_each_tag_ref(get_submodule_ref_store(submodule),\n-\t\t\t\t     fn, cb_data);\n-}\n-\n int refs_for_each_branch_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n {\n \treturn refs_for_each_ref_in(refs, \"refs/heads/\", fn, cb_data);\n@@ -332,12 +326,6 @@ int for_each_branch_ref(each_ref_fn fn, void *cb_data)\n \treturn refs_for_each_branch_ref(get_main_ref_store(), fn, cb_data);\n }\n \n-int for_each_branch_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)\n-{\n-\treturn refs_for_each_branch_ref(get_submodule_ref_store(submodule),\n-\t\t\t\t\tfn, cb_data);\n-}\n-\n int refs_for_each_remote_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n {\n \treturn refs_for_each_ref_in(refs, \"refs/remotes/\", fn, cb_data);\n@@ -1220,11 +1208,6 @@ int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n \treturn 0;\n }\n \n-int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)\n-{\n-\treturn refs_head_ref(get_submodule_ref_store(submodule), fn, cb_data);\n-}\n-\n int head_ref(each_ref_fn fn, void *cb_data)\n {\n \treturn refs_head_ref(get_main_ref_store(), fn, cb_data);\n@@ -1263,11 +1246,6 @@ int for_each_ref(each_ref_fn fn, void *cb_data)\n \treturn refs_for_each_ref(get_main_ref_store(), fn, cb_data);\n }\n \n-int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)\n-{\n-\treturn refs_for_each_ref(get_submodule_ref_store(submodule), fn, cb_data);\n-}\n-\n int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,\n \t\t\t each_ref_fn fn, void *cb_data)\n {\n@@ -1289,13 +1267,6 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsig\n \t\t\t       prefix, fn, 0, flag, cb_data);\n }\n \n-int for_each_ref_in_submodule(const char *submodule, const char *prefix,\n-\t\t\t      each_ref_fn fn, void *cb_data)\n-{\n-\treturn refs_for_each_ref_in(get_submodule_ref_store(submodule),\n-\t\t\t\t    prefix, fn, cb_data);\n-}\n-\n int for_each_replace_ref(each_ref_fn fn, void *cb_data)\n {\n \treturn do_for_each_ref(get_main_ref_store(),\ndiff --git a/refs.h b/refs.h\nindex 0572473ef7..e06db37118 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -259,15 +259,6 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);\n int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,\n \t\t\t const char *prefix, void *cb_data);\n \n-int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);\n-int for_each_ref_submodule(const char *submodule,\n-\t\t\t   each_ref_fn fn, void *cb_data);\n-int for_each_ref_in_submodule(const char *submodule, const char *prefix,\n-\t\teach_ref_fn fn, void *cb_data);\n-int for_each_tag_ref_submodule(const char *submodule,\n-\t\t\t       each_ref_fn fn, void *cb_data);\n-int for_each_branch_ref_submodule(const char *submodule,\n-\t\t\t\t  each_ref_fn fn, void *cb_data);\n int for_each_remote_ref_submodule(const char *submodule,\n \t\t\t\t  each_ref_fn fn, void *cb_data);\n \n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317221","messageId":"20170419110145.5086-10-pclouds@gmail.com","threadId":"45745","inReplyTo":"20170419110145.5086-1-pclouds@gmail.com","subject":"[PATCH v3 09/12] revision.c: --all adds HEAD from all worktrees","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-19T11:01:42Z","receivedAt":"2017-04-19T11:03:00Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Unless single_worktree is set, --all now adds HEAD from all worktrees.\n\nSince reachable.c code does not use setup_revisions(), we need to call\nother_head_refs_submodule() explicitly there to have the same effect on\n\"git prune\", so that we won't accidentally delete objects needed by some\nother HEADs.\n\nA new FIXME is added because we would need something like\n\n    int refs_other_head_refs(struct ref_store *, each_ref_fn, cb_data);\n\nin addition to other_head_refs() to handle it, which might require\n\n    int get_submodule_worktrees(const char *submodule, int flags);\n\nIt could be a separate topic to reduce the scope of this one.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n reachable.c      |  1 +\n refs.c           | 22 ++++++++++++++++++++++\n refs.h           |  1 +\n revision.c       | 13 +++++++++++++\n submodule.c      |  2 ++\n t/t5304-prune.sh | 12 ++++++++++++\n 6 files changed, 51 insertions(+)\n\ndiff --git a/reachable.c b/reachable.c\nindex a8a979bd4f..a3b938b46c 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -177,6 +177,7 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \n \t/* detached HEAD is not included in the list above */\n \thead_ref(add_one_ref, revs);\n+\tother_head_refs(add_one_ref, revs);\n \n \t/* Add all reflog info */\n \tif (mark_reflog)\ndiff --git a/refs.c b/refs.c\nindex 537052f7ba..23e3607674 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1780,3 +1780,25 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)\n {\n \treturn refs_rename_ref(get_main_ref_store(), oldref, newref, logmsg);\n }\n+\n+int other_head_refs(each_ref_fn fn, void *cb_data)\n+{\n+\tstruct worktree **worktrees, **p;\n+\tint ret = 0;\n+\n+\tworktrees = get_worktrees(0);\n+\tfor (p = worktrees; *p; p++) {\n+\t\tstruct worktree *wt = *p;\n+\t\tstruct ref_store *refs;\n+\n+\t\tif (wt->is_current)\n+\t\t\tcontinue;\n+\n+\t\trefs = get_worktree_ref_store(wt);\n+\t\tret = refs_head_ref(refs, fn, cb_data);\n+\t\tif (ret)\n+\t\t\tbreak;\n+\t}\n+\tfree_worktrees(worktrees);\n+\treturn ret;\n+}\ndiff --git a/refs.h b/refs.h\nindex e06db37118..cc71b6c7a0 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -247,6 +247,7 @@ int refs_for_each_remote_ref(struct ref_store *refs,\n \t\t\t     each_ref_fn fn, void *cb_data);\n \n int head_ref(each_ref_fn fn, void *cb_data);\n+int other_head_refs(each_ref_fn fn, void *cb_data);\n int for_each_ref(each_ref_fn fn, void *cb_data);\n int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data);\n int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,\ndiff --git a/revision.c b/revision.c\nindex c329070c89..040a0064f6 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2105,6 +2105,13 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \tint argcount;\n \n \tif (submodule) {\n+\t\t/*\n+\t\t * We need some something like get_submodule_worktrees()\n+\t\t * before we can go through all worktrees of a submodule,\n+\t\t * .e.g with adding all HEADs from --all, which is not\n+\t\t * supported right now, so stick to single worktree.\n+\t\t */\n+\t\tassert(revs->single_worktree != 0);\n \t\trefs = get_submodule_ref_store(submodule);\n \t} else\n \t\trefs = get_main_ref_store();\n@@ -2122,6 +2129,12 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \tif (!strcmp(arg, \"--all\")) {\n \t\thandle_refs(refs, revs, *flags, refs_for_each_ref);\n \t\thandle_refs(refs, revs, *flags, refs_head_ref);\n+\t\tif (!revs->single_worktree) {\n+\t\t\tstruct all_refs_cb cb;\n+\n+\t\t\tinit_all_refs_cb(&cb, revs, *flags);\n+\t\t\tother_head_refs(handle_one_ref, &cb);\n+\t\t}\n \t\tclear_ref_exclusion(&revs->ref_excludes);\n \t} else if (!strcmp(arg, \"--branches\")) {\n \t\thandle_refs(refs, revs, *flags, refs_for_each_branch_ref);\ndiff --git a/submodule.c b/submodule.c\nindex a31f68812c..8c5af6e7f3 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1225,6 +1225,8 @@ static int find_first_merges(struct object_array *result, const char *path,\n \t\t\toid_to_hex(&a->object.oid));\n \tinit_revisions(&revs, NULL);\n \trev_opts.submodule = path;\n+\t/* FIXME: can't handle linked worktrees in submodules yet */\n+\trevs.single_worktree = path != NULL;\n \tsetup_revisions(ARRAY_SIZE(rev_args)-1, rev_args, &revs, &rev_opts);\n \n \t/* save all revisions from the above list that contain b */\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex cba45c7be9..683bdb031c 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.11.0.157.gd943d85\n\n"},{"id":"317222","messageId":"20170419110145.5086-11-pclouds@gmail.com","threadId":"45745","inReplyTo":"20170419110145.5086-1-pclouds@gmail.com","subject":"[PATCH v3 10/12] files-backend: make reflog iterator go through per-worktree reflog","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-19T11:01:43Z","receivedAt":"2017-04-19T11:03:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"refs/bisect is unfortunately per-worktree, so we need to look in\nper-worktree logs/refs/bisect in addition to per-repo logs/refs. The\ncurrent iterator only goes through per-repo logs/refs.\n\nIdeally we should have something like merge_ref_iterator_begin (and\nmaybe with a predicate), but for dir_iterator. Since there's only one\nuse case for this pattern, let's not add a bunch more code for\nmerge_dir_iterator_begin just yet.\n\nPS. Note the unsorted order of for_each_reflog in the test. This is\nsupposed to be OK, for now. If we enforce order on for_each_reflog()\nthen some more work will be required.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n refs/files-backend.c          | 46 ++++++++++++++++++++++++++++++++-----------\n t/t1407-worktree-ref-store.sh | 30 ++++++++++++++++++++++++++++\n 2 files changed, 65 insertions(+), 11 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 4149943a6e..fce380679c 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1171,15 +1171,6 @@ static void files_reflog_path(struct files_ref_store *refs,\n \t\t\t      struct strbuf *sb,\n \t\t\t      const char *refname)\n {\n-\tif (!refname) {\n-\t\t/*\n-\t\t * FIXME: of course this is wrong in multi worktree\n-\t\t * setting. To be fixed real soon.\n-\t\t */\n-\t\tstrbuf_addf(sb, \"%s/logs\", refs->gitcommondir);\n-\t\treturn;\n-\t}\n-\n \tswitch (ref_type(refname)) {\n \tcase REF_TYPE_PER_WORKTREE:\n \tcase REF_TYPE_PSEUDOREF:\n@@ -3368,6 +3359,7 @@ struct files_reflog_iterator {\n \n \tstruct ref_store *ref_store;\n \tstruct dir_iterator *dir_iterator;\n+\tstruct dir_iterator *worktree_dir_iterator;\n \tstruct object_id oid;\n };\n \n@@ -3388,6 +3380,21 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \t\tif (ends_with(diter->basename, \".lock\"))\n \t\t\tcontinue;\n \n+\t\tif (iter->worktree_dir_iterator) {\n+\t\t\tconst char *refname = diter->relative_path;\n+\n+\t\t\tswitch (ref_type(refname)) {\n+\t\t\tcase REF_TYPE_PER_WORKTREE:\n+\t\t\tcase REF_TYPE_PSEUDOREF:\n+\t\t\t\tcontinue;\n+\t\t\tcase REF_TYPE_NORMAL:\n+\t\t\t\tbreak;\n+\t\t\tdefault:\n+\t\t\t\tdie(\"BUG: unknown ref type %d of ref %s\",\n+\t\t\t\t    ref_type(refname), refname);\n+\t\t\t}\n+\t\t}\n+\n \t\tif (refs_read_ref_full(iter->ref_store,\n \t\t\t\t       diter->relative_path, 0,\n \t\t\t\t       iter->oid.hash, &flags)) {\n@@ -3401,7 +3408,11 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \t\treturn ITER_OK;\n \t}\n \n-\titer->dir_iterator = NULL;\n+\titer->dir_iterator = iter->worktree_dir_iterator;\n+\tif (iter->worktree_dir_iterator) {\n+\t\titer->worktree_dir_iterator = NULL;\n+\t\treturn files_reflog_iterator_advance(ref_iterator);\n+\t}\n \tif (ref_iterator_abort(ref_iterator) == ITER_ERROR)\n \t\tok = ITER_ERROR;\n \treturn ok;\n@@ -3422,6 +3433,12 @@ static int files_reflog_iterator_abort(struct ref_iterator *ref_iterator)\n \tif (iter->dir_iterator)\n \t\tok = dir_iterator_abort(iter->dir_iterator);\n \n+\tif (iter->worktree_dir_iterator) {\n+\t\tint ok2 = dir_iterator_abort(iter->worktree_dir_iterator);\n+\t\tif (ok2 == ITER_ERROR)\n+\t\t\tok = ok2;\n+\t}\n+\n \tbase_ref_iterator_free(ref_iterator);\n \treturn ok;\n }\n@@ -3442,10 +3459,17 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st\n \tstruct strbuf sb = STRBUF_INIT;\n \n \tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable);\n-\tfiles_reflog_path(refs, &sb, NULL);\n+\tstrbuf_addf(&sb, \"%s/logs\", refs->gitcommondir);\n \titer->dir_iterator = dir_iterator_begin(sb.buf);\n \titer->ref_store = ref_store;\n \tstrbuf_release(&sb);\n+\n+\tif (strcmp(refs->gitdir, refs->gitcommondir)) {\n+\t\tstrbuf_addf(&sb, \"%s/logs\", refs->gitdir);\n+\t\titer->worktree_dir_iterator = dir_iterator_begin(sb.buf);\n+\t\tstrbuf_release(&sb);\n+\t}\n+\n \treturn ref_iterator;\n }\n \ndiff --git a/t/t1407-worktree-ref-store.sh b/t/t1407-worktree-ref-store.sh\nindex 5df06f3556..8842d0329f 100755\n--- a/t/t1407-worktree-ref-store.sh\n+++ b/t/t1407-worktree-ref-store.sh\n@@ -49,4 +49,34 @@ test_expect_success 'create_symref(FOO, refs/heads/master)' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'for_each_reflog()' '\n+\techo $_z40 > .git/logs/PSEUDO-MAIN &&\n+\tmkdir -p     .git/logs/refs/bisect &&\n+\techo $_z40 > .git/logs/refs/bisect/random &&\n+\n+\techo $_z40 > .git/worktrees/wt/logs/PSEUDO-WT &&\n+\tmkdir -p     .git/worktrees/wt/logs/refs/bisect &&\n+\techo $_z40 > .git/worktrees/wt/logs/refs/bisect/wt-random &&\n+\n+\t$RWT for-each-reflog | cut -c 42- | sort >actual &&\n+\tcat >expected <<-\\EOF &&\n+\tHEAD 0x1\n+\tPSEUDO-WT 0x0\n+\trefs/bisect/wt-random 0x0\n+\trefs/heads/master 0x0\n+\trefs/heads/wt-master 0x0\n+\tEOF\n+\ttest_cmp expected actual &&\n+\n+\t$RMAIN for-each-reflog | cut -c 42- | sort >actual &&\n+\tcat >expected <<-\\EOF &&\n+\tHEAD 0x1\n+\tPSEUDO-MAIN 0x0\n+\trefs/bisect/random 0x0\n+\trefs/heads/master 0x0\n+\trefs/heads/wt-master 0x0\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317223","messageId":"20170419110145.5086-12-pclouds@gmail.com","threadId":"45745","inReplyTo":"20170419110145.5086-1-pclouds@gmail.com","subject":"[PATCH v3 11/12] revision.c: --reflog add HEAD reflog from all worktrees","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-19T11:01:44Z","receivedAt":"2017-04-19T11:03:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Note that add_other_reflogs_to_pending() is a bit inefficient, since\nit scans reflog for all refs of each worktree, including shared refs,\nso the shared ref's reflog is scanned over and over again.\n\nWe could update refs API to pass \"per-worktree only\" flag to avoid\nthat. But long term we should be able to obtain a \"per-worktree only\"\nref store and would need to revert the changes in reflog iteration\nAPI. So let's just wait until then.\n\nadd_reflogs_to_pending() is called by reachable.c so by default \"git\nprune\" will examine reflog from all worktrees.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n revision.c       | 28 +++++++++++++++++++++++++++-\n t/t5304-prune.sh | 16 ++++++++++++++++\n 2 files changed, 43 insertions(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex 040a0064f6..f4bc9bc65c 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1134,6 +1134,7 @@ struct all_refs_cb {\n \tint warned_bad_reflog;\n \tstruct rev_info *all_revs;\n \tconst char *name_for_errormsg;\n+\tstruct ref_store *refs;\n };\n \n int ref_excluded(struct string_list *ref_excludes, const char *path)\n@@ -1169,6 +1170,7 @@ static void init_all_refs_cb(struct all_refs_cb *cb, struct rev_info *revs,\n {\n \tcb->all_revs = revs;\n \tcb->all_flags = flags;\n+\tcb->refs = NULL;\n }\n \n void clear_ref_exclusion(struct string_list **ref_excludes_p)\n@@ -1237,17 +1239,41 @@ static int handle_one_reflog(const char *path, const struct object_id *oid,\n \tstruct all_refs_cb *cb = cb_data;\n \tcb->warned_bad_reflog = 0;\n \tcb->name_for_errormsg = path;\n-\tfor_each_reflog_ent(path, handle_one_reflog_ent, cb_data);\n+\trefs_for_each_reflog_ent(cb->refs, path,\n+\t\t\t\t handle_one_reflog_ent, cb_data);\n \treturn 0;\n }\n \n+static void add_other_reflogs_to_pending(struct all_refs_cb *cb)\n+{\n+\tstruct worktree **worktrees, **p;\n+\n+\tworktrees = get_worktrees(0);\n+\tfor (p = worktrees; *p; p++) {\n+\t\tstruct worktree *wt = *p;\n+\n+\t\tif (wt->is_current)\n+\t\t\tcontinue;\n+\n+\t\tcb->refs = get_worktree_ref_store(wt);\n+\t\trefs_for_each_reflog(cb->refs,\n+\t\t\t\t     handle_one_reflog,\n+\t\t\t\t     cb);\n+\t}\n+\tfree_worktrees(worktrees);\n+}\n+\n void add_reflogs_to_pending(struct rev_info *revs, unsigned flags)\n {\n \tstruct all_refs_cb cb;\n \n \tcb.all_revs = revs;\n \tcb.all_flags = flags;\n+\tcb.refs = get_main_ref_store();\n \tfor_each_reflog(handle_one_reflog, &cb);\n+\n+\tif (!revs->single_worktree)\n+\t\tadd_other_reflogs_to_pending(&cb);\n }\n \n static void add_cache_tree(struct cache_tree *it, struct rev_info *revs,\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 683bdb031c..6694c19a1e 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -304,4 +304,20 @@ 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+\tgit config core.logAllRefUpdates true &&\n+\techo \"lost blob for third-worktree\" >expected &&\n+\t(\n+\t\tcd third-worktree &&\n+\t\tcat ../expected >blob &&\n+\t\tgit add blob &&\n+\t\tgit commit -m \"second commit in third\" &&\n+\t\tgit reset --hard HEAD^\n+\t) &&\n+\tgit prune --expire=now &&\n+\tSHA1=`git hash-object expected` &&\n+\tgit -C third-worktree show \"$SHA1\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317224","messageId":"20170419110145.5086-13-pclouds@gmail.com","threadId":"45745","inReplyTo":"20170419110145.5086-1-pclouds@gmail.com","subject":"[PATCH v3 12/12] rev-list: expose and document --single-worktree","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-19T11:01:45Z","receivedAt":"2017-04-19T11:03:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Documentation/rev-list-options.txt | 8 ++++++++\n revision.c                         | 2 ++\n 2 files changed, 10 insertions(+)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex a02f7324c0..c71e94b2d0 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -179,6 +179,14 @@ explicitly.\n \tPretend as if all objects mentioned by reflogs are listed on the\n \tcommand line as `<commit>`.\n \n+--single-worktree::\n+\tBy default, all working trees will be examined by the\n+\tfollowing options when there are more than one (see\n+\tlinkgit:git-worktree[1]): `--all`, `--reflog` and\n+\t`--indexed-objects`.\n+\tThis option forces them to examine the current working tree\n+\tonly.\n+\n --ignore-missing::\n \tUpon seeing an invalid object name in the input, pretend as if\n \tthe bad input was not given.\ndiff --git a/revision.c b/revision.c\nindex f4bc9bc65c..79ce8a007f 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2222,6 +2222,8 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \t\t\treturn error(\"invalid argument to --no-walk\");\n \t} else if (!strcmp(arg, \"--do-walk\")) {\n \t\trevs->no_walk = 0;\n+\t} else if (!strcmp(arg, \"--single-worktree\")) {\n+\t\trevs->single_worktree = 1;\n \t} else {\n \t\treturn 0;\n \t}\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317244","messageId":"bc3795da-4d9e-ac57-a011-28017494a71d@ramsayjones.plus.com","threadId":"45745","inReplyTo":"20170419110145.5086-9-pclouds@gmail.com","subject":"Re: [PATCH v3 08/12] refs: remove dead for_each_*_submodule()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-04-19T16:07:28Z","receivedAt":"2017-04-19T16:09:09Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 19/04/17 12:01, Nguyễn Thái Ngọc Duy wrote:\n> These are used in revision.c. After the last patch they are replaced\n> with the refs_ version. Delete them (except for_each_remote_ref_submodule\n> which is still used by submodule.c)\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  Documentation/technical/api-ref-iteration.txt |  7 ++-----\n>  refs.c                                        | 29 ---------------------------\n>  refs.h                                        |  9 ---------\n>  3 files changed, 2 insertions(+), 43 deletions(-)\n> \n> diff --git a/Documentation/technical/api-ref-iteration.txt b/Documentation/technical/api-ref-iteration.txt\n> index 37379d8337..c9e9a60dbd 100644\n> --- a/Documentation/technical/api-ref-iteration.txt\n> +++ b/Documentation/technical/api-ref-iteration.txt\n> @@ -32,11 +32,8 @@ Iteration functions\n>  \n>  * `for_each_glob_ref_in()` the previous and `for_each_ref_in()` combined.\n>  \n> -* `head_ref_submodule()`, `for_each_ref_submodule()`,\n> -  `for_each_ref_in_submodule()`, `for_each_tag_ref_submodule()`,\n> -  `for_each_branch_ref_submodule()`, `for_each_remote_ref_submodule()`\n> -  do the same as the functions described above but for a specified\n> -  submodule.\n> +* Use `refs_` API for accessing submodules. The submodule ref store could\n> +  be obtained with `get_submodule_ref_store().\n\nmissing ` ? ------------------------------------^\n\nATB,\nRamsay Jones\n\n"},{"id":"317275","messageId":"a74cf309-fb16-2f45-8189-d1d0c655dea4@kdbg.org","threadId":"45745","inReplyTo":"20170419110145.5086-6-pclouds@gmail.com","subject":"Re: [PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-04-19T22:02:08Z","receivedAt":"2017-04-19T22:02:17Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 19.04.2017 um 13:01 schrieb Nguyễn Thái Ngọc Duy:\n> @@ -1558,7 +1543,17 @@ struct ref_store *get_submodule_ref_store(const char *submodule)\n>  {\n>  \tstruct strbuf submodule_sb = STRBUF_INIT;\n>  \tstruct ref_store *refs;\n> +\tchar *to_free = NULL;\n>  \tint ret;\n> +\tsize_t len;\n> +\n> +\tif (submodule) {\n> +\t\tlen = strlen(submodule);\n> +\t\twhile (len && submodule[len - 1] == '/')\n\nWhat is the source of the value of 'submodule'? Is it an index entry? Or \ndid it pass through parse_pathspec? In these cases it is correct to \ncompare against literal '/'. Otherwise, is_dir_sep() is preferred.\n\n> +\t\t\tlen--;\n> +\t\tif (!len)\n> +\t\t\tsubmodule = NULL;\n> +\t}\n>\n>  \tif (!submodule || !*submodule) {\n>  \t\t/*\n\n-- Hannes\n\n"},{"id":"317304","messageId":"20170420020147.GA8879@duynguyen.vn.dektech.internal","threadId":"45745","inReplyTo":"a74cf309-fb16-2f45-8189-d1d0c655dea4@kdbg.org","subject":"Re: [PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T02:01:47Z","receivedAt":"2017-04-20T02:01:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Apr 20, 2017 at 12:02:08AM +0200, Johannes Sixt wrote:\n> Am 19.04.2017 um 13:01 schrieb Nguyễn Thái Ngọc Duy:\n> > @@ -1558,7 +1543,17 @@ struct ref_store *get_submodule_ref_store(const char *submodule)\n> >  {\n> >  \tstruct strbuf submodule_sb = STRBUF_INIT;\n> >  \tstruct ref_store *refs;\n> > +\tchar *to_free = NULL;\n> >  \tint ret;\n> > +\tsize_t len;\n> > +\n> > +\tif (submodule) {\n> > +\t\tlen = strlen(submodule);\n> > +\t\twhile (len && submodule[len - 1] == '/')\n> \n> What is the source of the value of 'submodule'? Is it an index entry? Or \n> did it pass through parse_pathspec? In these cases it is correct to \n> compare against literal '/'. Otherwise, is_dir_sep() is preferred.\n\nThis is a code move from resolve_gitlink_ref(), which goes back to\n0ebde32c87 (Add 'resolve_gitlink_ref()' helper function - 2007-04-09)\nand it looks like a dir separator back then.\n\nCan I convert that in a separate topic? I think Michael even wanted to\nkill all these path manipulation in refs code, which makes sense, but\nI would need to audit the callers carefully before making that move.\n--\nDuy\n"},{"id":"317306","messageId":"xmqqo9vrai99.fsf@gitster.mtv.corp.google.com","threadId":"45745","inReplyTo":"bc3795da-4d9e-ac57-a011-28017494a71d@ramsayjones.plus.com","subject":"Re: [PATCH v3 08/12] refs: remove dead for_each_*_submodule()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-20T03:08:02Z","receivedAt":"2017-04-20T03:08:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n\n> On 19/04/17 12:01, Nguyễn Thái Ngọc Duy wrote:\n>> These are used in revision.c. After the last patch they are replaced\n>> with the refs_ version. Delete them (except for_each_remote_ref_submodule\n>> which is still used by submodule.c)\n>> \n>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n>> ---\n>>  Documentation/technical/api-ref-iteration.txt |  7 ++-----\n>>  refs.c                                        | 29 ---------------------------\n>>  refs.h                                        |  9 ---------\n>>  3 files changed, 2 insertions(+), 43 deletions(-)\n>> \n>> diff --git a/Documentation/technical/api-ref-iteration.txt b/Documentation/technical/api-ref-iteration.txt\n>> index 37379d8337..c9e9a60dbd 100644\n>> --- a/Documentation/technical/api-ref-iteration.txt\n>> +++ b/Documentation/technical/api-ref-iteration.txt\n>> @@ -32,11 +32,8 @@ Iteration functions\n>>  \n>>  * `for_each_glob_ref_in()` the previous and `for_each_ref_in()` combined.\n>>  \n>> -* `head_ref_submodule()`, `for_each_ref_submodule()`,\n>> -  `for_each_ref_in_submodule()`, `for_each_tag_ref_submodule()`,\n>> -  `for_each_branch_ref_submodule()`, `for_each_remote_ref_submodule()`\n>> -  do the same as the functions described above but for a specified\n>> -  submodule.\n>> +* Use `refs_` API for accessing submodules. The submodule ref store could\n>> +  be obtained with `get_submodule_ref_store().\n>\n> missing ` ? ------------------------------------^\n\nIndeed.  Will tweak while queuing.\n"},{"id":"317343","messageId":"CACsJy8A1aR-=QqqZdw+i5Hv2kXptTzYcn5tFsMBDYk6vBwUwZg@mail.gmail.com","threadId":"45745","inReplyTo":"a74cf309-fb16-2f45-8189-d1d0c655dea4@kdbg.org","subject":"Re: [PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T11:56:25Z","receivedAt":"2017-04-20T11:57:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Apr 20, 2017 at 5:02 AM, Johannes Sixt <j6t@kdbg.org> wrote:\n> Am 19.04.2017 um 13:01 schrieb Nguyễn Thái Ngọc Duy:\n>>\n>> @@ -1558,7 +1543,17 @@ struct ref_store *get_submodule_ref_store(const\n>> char *submodule)\n>>  {\n>>         struct strbuf submodule_sb = STRBUF_INIT;\n>>         struct ref_store *refs;\n>> +       char *to_free = NULL;\n>>         int ret;\n>> +       size_t len;\n>> +\n>> +       if (submodule) {\n>> +               len = strlen(submodule);\n>> +               while (len && submodule[len - 1] == '/')\n>\n>\n> What is the source of the value of 'submodule'? Is it an index entry? Or did\n> it pass through parse_pathspec? In these cases it is correct to compare\n> against literal '/'. Otherwise, is_dir_sep() is preferred.\n\nI've looked at the callers. Yes it is a path and is_dir_sep() should\nbe used. Since this has been like this in the current code, unless\nthere's some more changes in this series or you insist, I would hold\nthis change back, wait for this series to settle and submit it later\n(I'll have to do that anyway to kill the get_main_store() call in this\nfunction).\n-- \nDuy\n"},{"id":"317351","messageId":"fa37a980-d984-1834-6d2e-cce60486ec33@kdbg.org","threadId":"45745","inReplyTo":"CACsJy8A1aR-=QqqZdw+i5Hv2kXptTzYcn5tFsMBDYk6vBwUwZg@mail.gmail.com","subject":"Re: [PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-04-20T16:30:28Z","receivedAt":"2017-04-20T16:31:29Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 20.04.2017 um 13:56 schrieb Duy Nguyen:\n> On Thu, Apr 20, 2017 at 5:02 AM, Johannes Sixt <j6t@kdbg.org> wrote:\n>> What is the source of the value of 'submodule'? Is it an index entry? Or did\n>> it pass through parse_pathspec? In these cases it is correct to compare\n>> against literal '/'. Otherwise, is_dir_sep() is preferred.\n>\n> I've looked at the callers. Yes it is a path and is_dir_sep() should\n> be used. Since this has been like this in the current code, unless\n> there's some more changes in this series or you insist, I would hold\n> this change back,\n\nI won't insist because we have it the old way since a decade without \nissue reports.\n\n-- Hannes\n\n"},{"id":"317532","messageId":"c50e5ff0-52aa-aeaa-168a-9ac56c71626b@alum.mit.edu","threadId":"45745","inReplyTo":"20170419110145.5086-5-pclouds@gmail.com","subject":"Re: [PATCH v3 04/12] refs.c: refactor get_submodule_ref_store(), share common free block","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-04-22T05:13:50Z","receivedAt":"2017-04-22T05:14:03Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  refs.c | 12 +++++-------\n>  1 file changed, 5 insertions(+), 7 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index 5d31fb6bcf..5902a3d9e5 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1570,19 +1570,16 @@ struct ref_store *get_submodule_ref_store(const char *submodule)\n>  \n>  \trefs = lookup_ref_store_map(&submodule_ref_stores, submodule);\n>  \tif (refs)\n> -\t\treturn refs;\n> +\t\tgoto done;\n>  \n>  \tstrbuf_addstr(&submodule_sb, submodule);\n>  \tret = is_nonbare_repository_dir(&submodule_sb);\n> -\tstrbuf_release(&submodule_sb);\n>  \tif (!ret)\n> -\t\treturn NULL;\n> +\t\tgoto done;\n>  \n>  \tret = submodule_to_gitdir(&submodule_sb, submodule);\n> -\tif (ret) {\n> -\t\tstrbuf_release(&submodule_sb);\n> -\t\treturn NULL;\n> -\t}\n> +\tif (ret)\n> +\t\tgoto done;\n\nAfter this change, the temporary variable `ret` could be eliminated.\n\n> [...]\n\nMichael\n\n"},{"id":"317533","messageId":"378b8e50-562a-a311-2e0f-505de036aaf1@alum.mit.edu","threadId":"45745","inReplyTo":"20170419110145.5086-6-pclouds@gmail.com","subject":"Re: [PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-04-22T05:27:01Z","receivedAt":"2017-04-22T05:27:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:\n> This is a better place that will benefit all submodule callers instead\n> of just resolve_gitlink_ref()\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  refs.c | 33 +++++++++++++++++----------------\n>  1 file changed, 17 insertions(+), 16 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index 5902a3d9e5..26474cb62a 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1422,25 +1422,10 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,\n>  int resolve_gitlink_ref(const char *submodule, const char *refname,\n>  \t\t\tunsigned char *sha1)\n>  {\n> -\tsize_t len = strlen(submodule);\n>  \tstruct ref_store *refs;\n>  \tint flags;\n>  \n> -\twhile (len && submodule[len - 1] == '/')\n> -\t\tlen--;\n> -\n> -\tif (!len)\n> -\t\treturn -1;\n> -\n> -\tif (submodule[len]) {\n> -\t\t/* We need to strip off one or more trailing slashes */\n> -\t\tchar *stripped = xmemdupz(submodule, len);\n> -\n> -\t\trefs = get_submodule_ref_store(stripped);\n> -\t\tfree(stripped);\n> -\t} else {\n> -\t\trefs = get_submodule_ref_store(submodule);\n> -\t}\n> +\trefs = get_submodule_ref_store(submodule);\n>  \n>  \tif (!refs)\n>  \t\treturn -1;\n> @@ -1558,7 +1543,17 @@ struct ref_store *get_submodule_ref_store(const char *submodule)\n>  {\n>  \tstruct strbuf submodule_sb = STRBUF_INIT;\n>  \tstruct ref_store *refs;\n> +\tchar *to_free = NULL;\n>  \tint ret;\n> +\tsize_t len;\n> +\n> +\tif (submodule) {\n> +\t\tlen = strlen(submodule);\n> +\t\twhile (len && submodule[len - 1] == '/')\n> +\t\t\tlen--;\n> +\t\tif (!len)\n> +\t\t\tsubmodule = NULL;\n> +\t}\n\nUgh. Should a submodule named \"///\" *really* be considered to refer to\nthe main ref_store? I understand that's what the code did before this\npatch, but it seems to me more like an accident of the old design rather\nthan something worth supporting. In other words, if a caller would\nreally pass us such a string, it seems like we could declare the caller\nbuggy, no?\n\n> [...]\n\nOtherwise, looks good and makes a lot of sense.\n\nMichael\n\n"},{"id":"317534","messageId":"1f3c633b-fc05-36f9-ff9f-8327e2e90b73@alum.mit.edu","threadId":"45745","inReplyTo":"20170419110145.5086-9-pclouds@gmail.com","subject":"Re: [PATCH v3 08/12] refs: remove dead for_each_*_submodule()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-04-22T05:35:00Z","receivedAt":"2017-04-22T05:35:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:\n> These are used in revision.c. After the last patch they are replaced\n> with the refs_ version. Delete them (except for_each_remote_ref_submodule\n> which is still used by submodule.c)\n\n❤❤❤❤❤❤\n\nI love the way this is going.\n\nMichael\n\n"},{"id":"317537","messageId":"b0bd826e-8394-08f7-131e-8dfa44d0f647@alum.mit.edu","threadId":"45745","inReplyTo":"20170419110145.5086-10-pclouds@gmail.com","subject":"Re: [PATCH v3 09/12] revision.c: --all adds HEAD from all worktrees","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-04-22T05:48:42Z","receivedAt":"2017-04-22T05:48:55Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:\n> Unless single_worktree is set, --all now adds HEAD from all worktrees.\n> \n> Since reachable.c code does not use setup_revisions(), we need to call\n> other_head_refs_submodule() explicitly there to have the same effect on\n> \"git prune\", so that we won't accidentally delete objects needed by some\n> other HEADs.\n> \n> A new FIXME is added because we would need something like\n> \n>     int refs_other_head_refs(struct ref_store *, each_ref_fn, cb_data);\n> \n> in addition to other_head_refs() to handle it, which might require\n> \n>     int get_submodule_worktrees(const char *submodule, int flags);\n> \n> It could be a separate topic to reduce the scope of this one.\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  reachable.c      |  1 +\n>  refs.c           | 22 ++++++++++++++++++++++\n>  refs.h           |  1 +\n>  revision.c       | 13 +++++++++++++\n>  submodule.c      |  2 ++\n>  t/t5304-prune.sh | 12 ++++++++++++\n>  6 files changed, 51 insertions(+)\n> \n> diff --git a/reachable.c b/reachable.c\n> index a8a979bd4f..a3b938b46c 100644\n> --- a/reachable.c\n> +++ b/reachable.c\n> @@ -177,6 +177,7 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n>  \n>  \t/* detached HEAD is not included in the list above */\n>  \thead_ref(add_one_ref, revs);\n> +\tother_head_refs(add_one_ref, revs);\n>  \n>  \t/* Add all reflog info */\n>  \tif (mark_reflog)\n> diff --git a/refs.c b/refs.c\n> index 537052f7ba..23e3607674 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1780,3 +1780,25 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)\n>  {\n>  \treturn refs_rename_ref(get_main_ref_store(), oldref, newref, logmsg);\n>  }\n> +\n> +int other_head_refs(each_ref_fn fn, void *cb_data)\n> +{\n> +\tstruct worktree **worktrees, **p;\n> +\tint ret = 0;\n> +\n> +\tworktrees = get_worktrees(0);\n> +\tfor (p = worktrees; *p; p++) {\n> +\t\tstruct worktree *wt = *p;\n> +\t\tstruct ref_store *refs;\n> +\n> +\t\tif (wt->is_current)\n> +\t\t\tcontinue;\n> +\n> +\t\trefs = get_worktree_ref_store(wt);\n> +\t\tret = refs_head_ref(refs, fn, cb_data);\n> +\t\tif (ret)\n> +\t\t\tbreak;\n> +\t}\n> +\tfree_worktrees(worktrees);\n> +\treturn ret;\n> +}\n\nThis function is mainly about iterating through all worktrees. Therefore\nit feels out of place in the refs module. But I don't have a strong\nfeeling about it.\n\n> diff --git a/refs.h b/refs.h\n> index e06db37118..cc71b6c7a0 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -247,6 +247,7 @@ int refs_for_each_remote_ref(struct ref_store *refs,\n>  \t\t\t     each_ref_fn fn, void *cb_data);\n>  \n>  int head_ref(each_ref_fn fn, void *cb_data);\n> +int other_head_refs(each_ref_fn fn, void *cb_data);\n>  int for_each_ref(each_ref_fn fn, void *cb_data);\n>  int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data);\n>  int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,\n> diff --git a/revision.c b/revision.c\n> index c329070c89..040a0064f6 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2105,6 +2105,13 @@ static int handle_revision_pseudo_opt(const char *submodule,\n>  \tint argcount;\n>  \n>  \tif (submodule) {\n> +\t\t/*\n> +\t\t * We need some something like get_submodule_worktrees()\n> +\t\t * before we can go through all worktrees of a submodule,\n> +\t\t * .e.g with adding all HEADs from --all, which is not\n> +\t\t * supported right now, so stick to single worktree.\n> +\t\t */\n> +\t\tassert(revs->single_worktree != 0);\n\nYou don't need `!= 0` above.\n\nWe usually don't use `assert(foo)` but rather `if (!foo)\ndie(\"BUG:...\")`, which gives a better error message and isn't switched\noff if a distribution compiles with NDEBUG.\n\nBut here I'm confused about whether this check failing really indicates\na bug within git, or whether it could indicate a situation that the user\nhas set up that git just can't handle right now. For example, maybe the\nuser will set up a submodule that itself uses worktrees (even if that's\nnot possible now, maybe they will have done it with some future version\nof git or with another tool). If the problem is only that this version\nof git is too stupid to handle pruning safely in that situation, it\nwould be more appropriate to use something more like\n\n\tif (!refs->single_worktree)\n\t\tdie(\"error: git is currently unable to handle submodules that use\nlinked worktrees\");\n\n>  \t\trefs = get_submodule_ref_store(submodule);\n>  \t} else\n>  \t\trefs = get_main_ref_store();\n> [...]\n\nMichael\n\n"},{"id":"317538","messageId":"4519cd91-80e1-a416-658e-e10e542b41ff@alum.mit.edu","threadId":"45745","inReplyTo":"20170419110145.5086-7-pclouds@gmail.com","subject":"Re: [PATCH v3 06/12] refs: add refs_head_ref()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-04-22T06:37:37Z","receivedAt":"2017-04-22T06:37:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  refs.c | 19 +++++++++----------\n>  refs.h |  2 ++\n>  2 files changed, 11 insertions(+), 10 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index 26474cb62a..a252ae43ee 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1208,27 +1208,26 @@ int refs_rename_ref_available(struct ref_store *refs,\n>  \treturn ok;\n>  }\n>  \n> -int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)\n> +int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n>  {\n>  \tstruct object_id oid;\n>  \tint flag;\n>  \n> -\tif (submodule) {\n> -\t\tif (resolve_gitlink_ref(submodule, \"HEAD\", oid.hash) == 0)\n> -\t\t\treturn fn(\"HEAD\", &oid, 0, cb_data);\n> -\n> -\t\treturn 0;\n> -\t}\n> -\n> -\tif (!read_ref_full(\"HEAD\", RESOLVE_REF_READING, oid.hash, &flag))\n> +\tif (!refs_read_ref_full(refs, \"HEAD\", RESOLVE_REF_READING,\n> +\t\t\t\toid.hash, &flag))\n>  \t\treturn fn(\"HEAD\", &oid, flag, cb_data);\n>  \n>  \treturn 0;\n>  }\n>  \n> +int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)\n> +{\n> +\treturn refs_head_ref(get_submodule_ref_store(submodule), fn, cb_data);\n> +}\n> +\n>  int head_ref(each_ref_fn fn, void *cb_data)\n>  {\n> -\treturn head_ref_submodule(NULL, fn, cb_data);\n> +\treturn refs_head_ref(get_main_ref_store(), fn, cb_data);\n>  }\n>  \n>  /*\n> diff --git a/refs.h b/refs.h\n> index 447381d378..0572473ef7 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -233,6 +233,8 @@ typedef int each_ref_fn(const char *refname,\n>   * modifies the reference also returns a nonzero value to immediately\n>   * stop the iteration. Returned references are sorted.\n>   */\n> +int refs_head_ref(struct ref_store *refs,\n> +\t\t  each_ref_fn fn, void *cb_data);\n>  int refs_for_each_ref(struct ref_store *refs,\n>  \t\t      each_ref_fn fn, void *cb_data);\n>  int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,\n> \n\nI'm seeing segfaults in t3600 after this patch, apparently because\n`refs==NULL` gets passed from `head_ref_submodule()` to `refs_head_ref()`.\n\nMichael\n\n"},{"id":"317539","messageId":"d3428e5e-9ac2-0426-31fd-92d29a414b3d@alum.mit.edu","threadId":"45745","inReplyTo":"20170419110145.5086-11-pclouds@gmail.com","subject":"Re: [PATCH v3 10/12] files-backend: make reflog iterator go through per-worktree reflog","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-04-22T08:05:02Z","receivedAt":"2017-04-22T08:05:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:\n> refs/bisect is unfortunately per-worktree, so we need to look in\n> per-worktree logs/refs/bisect in addition to per-repo logs/refs. The\n> current iterator only goes through per-repo logs/refs.\n> \n> Ideally we should have something like merge_ref_iterator_begin (and\n> maybe with a predicate), but for dir_iterator. Since there's only one\n> use case for this pattern, let's not add a bunch more code for\n> merge_dir_iterator_begin just yet.\n> \n> PS. Note the unsorted order of for_each_reflog in the test. This is\n> supposed to be OK, for now. If we enforce order on for_each_reflog()\n> then some more work will be required.\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  refs/files-backend.c          | 46 ++++++++++++++++++++++++++++++++-----------\n>  t/t1407-worktree-ref-store.sh | 30 ++++++++++++++++++++++++++++\n>  2 files changed, 65 insertions(+), 11 deletions(-)\n> \n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 4149943a6e..fce380679c 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1171,15 +1171,6 @@ static void files_reflog_path(struct files_ref_store *refs,\n>  \t\t\t      struct strbuf *sb,\n>  \t\t\t      const char *refname)\n>  {\n> -\tif (!refname) {\n> -\t\t/*\n> -\t\t * FIXME: of course this is wrong in multi worktree\n> -\t\t * setting. To be fixed real soon.\n> -\t\t */\n> -\t\tstrbuf_addf(sb, \"%s/logs\", refs->gitcommondir);\n> -\t\treturn;\n> -\t}\n> -\n>  \tswitch (ref_type(refname)) {\n>  \tcase REF_TYPE_PER_WORKTREE:\n>  \tcase REF_TYPE_PSEUDOREF:\n> @@ -3368,6 +3359,7 @@ struct files_reflog_iterator {\n>  \n>  \tstruct ref_store *ref_store;\n>  \tstruct dir_iterator *dir_iterator;\n> +\tstruct dir_iterator *worktree_dir_iterator;\n>  \tstruct object_id oid;\n>  };\n>  \n> @@ -3388,6 +3380,21 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n>  \t\tif (ends_with(diter->basename, \".lock\"))\n>  \t\t\tcontinue;\n>  \n> +\t\tif (iter->worktree_dir_iterator) {\n> +\t\t\tconst char *refname = diter->relative_path;\n> +\n> +\t\t\tswitch (ref_type(refname)) {\n> +\t\t\tcase REF_TYPE_PER_WORKTREE:\n> +\t\t\tcase REF_TYPE_PSEUDOREF:\n> +\t\t\t\tcontinue;\n> +\t\t\tcase REF_TYPE_NORMAL:\n> +\t\t\t\tbreak;\n> +\t\t\tdefault:\n> +\t\t\t\tdie(\"BUG: unknown ref type %d of ref %s\",\n> +\t\t\t\t    ref_type(refname), refname);\n> +\t\t\t}\n> +\t\t}\n> +\n>  \t\tif (refs_read_ref_full(iter->ref_store,\n>  \t\t\t\t       diter->relative_path, 0,\n>  \t\t\t\t       iter->oid.hash, &flags)) {\n> @@ -3401,7 +3408,11 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n>  \t\treturn ITER_OK;\n>  \t}\n>  \n> -\titer->dir_iterator = NULL;\n> +\titer->dir_iterator = iter->worktree_dir_iterator;\n> +\tif (iter->worktree_dir_iterator) {\n> +\t\titer->worktree_dir_iterator = NULL;\n> +\t\treturn files_reflog_iterator_advance(ref_iterator);\n> +\t}\n\nI find this implementation confusing:\n\n* `if (iter->worktree_dir_iterator)` sounds like it should mean\n  that we are iterating over worktree references but it really means\n  that we are iterating over the common references in a repository\n  that is a linked worktree.\n* `files_reflog_iterator_advance()` is called recursively, but only\n  for the first worktree reference.\n* `iter->worktree_dir_iterator` is moved over to `iter->dir_iterator`\n  when the common refs are exhausted.\n\nDo you find it more readable as follows?:\n\nstatic int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n{\n\tstruct files_reflog_iterator *iter =\n\t\t(struct files_reflog_iterator *)ref_iterator;\n\tint ok;\n\n\twhile (1) {\n\t\tstruct dir_iterator **diter;\n\t\tint normal_only, flags;\n\n\t\tif (iter->dir_iterator) {\n\t\t\tditer = &iter->dir_iterator;\n\t\t\t/*\n\t\t\t * If we are in a worktree, then we only\n\t\t\t * include \"normal\" common references:\n\t\t\t */\n\t\t\tnormal_only = !!iter->worktree_dir_iterator;\n\t\t} else if (iter->worktree_dir_iterator) {\n\t\t\tditer = &iter->worktree_dir_iterator;\n\t\t\tnormal_only = 0;\n\t\t} else {\n\t\t\tok = ITER_DONE;\n\t\t\tbreak;\n\t\t}\n\n\t\tok = dir_iterator_advance(*diter);\n\t\tif (ok == ITER_ERROR) {\n\t\t\t*diter = NULL;\n\t\t\tbreak;\n\t\t} else if (ok == ITER_DONE) {\n\t\t\t*diter = NULL;\n\t\t\t/* There might still be worktree refs left: */\n\t\t\tcontinue;\n\t\t}\n\n\t\tif (!S_ISREG((*diter)->st.st_mode))\n\t\t\tcontinue;\n\t\tif ((*diter)->basename[0] == '.')\n\t\t\tcontinue;\n\t\tif (ends_with((*diter)->basename, \".lock\"))\n\t\t\tcontinue;\n\n\t\titer->base.refname = (*diter)->relative_path;\n\n\t\tif (normal_only &&\n\t\t    ref_type(iter->base.refname) != REF_TYPE_NORMAL)\n\t\t\tcontinue;\n\n\t\tif (refs_read_ref_full(iter->ref_store,\n\t\t\t\t       iter->base.refname, 0,\n\t\t\t\t       iter->oid.hash, &flags)) {\n\t\t\terror(\"bad ref for %s\", (*diter)->path.buf);\n\t\t\tcontinue;\n\t\t}\n\n\t\titer->base.oid = &iter->oid;\n\t\titer->base.flags = flags;\n\t\treturn ITER_OK;\n\t}\n\n\tif (ref_iterator_abort(ref_iterator) == ITER_ERROR)\n\t\treturn ITER_ERROR;\n\n\treturn ok;\n}\n\n>  \tif (ref_iterator_abort(ref_iterator) == ITER_ERROR)\n>  \t\tok = ITER_ERROR;\n>  \treturn ok;\n> [...]\n\nMichael\n\n"},{"id":"317540","messageId":"024e7bbd-16ad-04e1-a565-ac7b150c11e4@alum.mit.edu","threadId":"45745","inReplyTo":"20170419110145.5086-1-pclouds@gmail.com","subject":"Re: [PATCH v3 00/12] Fix git-gc losing objects in multi worktree","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-04-22T08:14:01Z","receivedAt":"2017-04-22T08:14:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:\n> Changes since v2 [1] is relatively small. It still needs\n> nd/worktree-kill-parse-ref of course.\n\nI read the whole series. Aside from the comments I made, it looks good\nto me.\n\nThis will be able to be cleaned up once we have a cleaner distinction\nbetween \"logical\" and \"physical\" ref_stores. But given the current state\nof the code, your implementation is reasonable.\n\nMichael\n\n"},{"id":"317541","messageId":"CACsJy8ADCVBiLoPg_Tz0L6CMdh_eFmK4RYzfQ-PmUgBK7w9e=A@mail.gmail.com","threadId":"45745","inReplyTo":"4519cd91-80e1-a416-658e-e10e542b41ff@alum.mit.edu","subject":"Re: [PATCH v3 06/12] refs: add refs_head_ref()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-22T08:15:47Z","receivedAt":"2017-04-22T08:16:28Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Apr 22, 2017 at 1:37 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:\n>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n>> ---\n>>  refs.c | 19 +++++++++----------\n>>  refs.h |  2 ++\n>>  2 files changed, 11 insertions(+), 10 deletions(-)\n>>\n>> diff --git a/refs.c b/refs.c\n>> index 26474cb62a..a252ae43ee 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -1208,27 +1208,26 @@ int refs_rename_ref_available(struct ref_store *refs,\n>>       return ok;\n>>  }\n>>\n>> -int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)\n>> +int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n>>  {\n>>       struct object_id oid;\n>>       int flag;\n>>\n>> -     if (submodule) {\n>> -             if (resolve_gitlink_ref(submodule, \"HEAD\", oid.hash) == 0)\n>> -                     return fn(\"HEAD\", &oid, 0, cb_data);\n>> -\n>> -             return 0;\n>> -     }\n>> -\n>> -     if (!read_ref_full(\"HEAD\", RESOLVE_REF_READING, oid.hash, &flag))\n>> +     if (!refs_read_ref_full(refs, \"HEAD\", RESOLVE_REF_READING,\n>> +                             oid.hash, &flag))\n>>               return fn(\"HEAD\", &oid, flag, cb_data);\n>>\n>>       return 0;\n>>  }\n>>\n>> +int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)\n>> +{\n>> +     return refs_head_ref(get_submodule_ref_store(submodule), fn, cb_data);\n>> +}\n>> +\n>>  int head_ref(each_ref_fn fn, void *cb_data)\n>>  {\n>> -     return head_ref_submodule(NULL, fn, cb_data);\n>> +     return refs_head_ref(get_main_ref_store(), fn, cb_data);\n>>  }\n>>\n>>  /*\n>> diff --git a/refs.h b/refs.h\n>> index 447381d378..0572473ef7 100644\n>> --- a/refs.h\n>> +++ b/refs.h\n>> @@ -233,6 +233,8 @@ typedef int each_ref_fn(const char *refname,\n>>   * modifies the reference also returns a nonzero value to immediately\n>>   * stop the iteration. Returned references are sorted.\n>>   */\n>> +int refs_head_ref(struct ref_store *refs,\n>> +               each_ref_fn fn, void *cb_data);\n>>  int refs_for_each_ref(struct ref_store *refs,\n>>                     each_ref_fn fn, void *cb_data);\n>>  int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,\n>>\n>\n> I'm seeing segfaults in t3600 after this patch, apparently because\n> `refs==NULL` gets passed from `head_ref_submodule()` to `refs_head_ref()`.\n\nEventually it's the caller's responsibility not to pass NULL ref store\nto refs_* functions. That happens in \"revision.c: use refs_for_each*()\ninstead of for_each_*_submodule()\" patch, which is too late and would\nbreak bisect. Will fix.\n-- \nDuy\n"},{"id":"317554","messageId":"20170423044420.GA28419@ash","threadId":"45745","inReplyTo":"d3428e5e-9ac2-0426-31fd-92d29a414b3d@alum.mit.edu","subject":"Re: [PATCH v3 10/12] files-backend: make reflog iterator go through per-worktree reflog","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-23T04:44:21Z","receivedAt":"2017-04-23T04:44:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Apr 22, 2017 at 10:05:02AM +0200, Michael Haggerty wrote:\n> I find this implementation confusing:\n> \n> * `if (iter->worktree_dir_iterator)` sounds like it should mean\n>   that we are iterating over worktree references but it really means\n>   that we are iterating over the common references in a repository\n>   that is a linked worktree.\n> * `files_reflog_iterator_advance()` is called recursively, but only\n>   for the first worktree reference.\n> * `iter->worktree_dir_iterator` is moved over to `iter->dir_iterator`\n>   when the common refs are exhausted.\n> \n> Do you find it more readable as follows?:\n\nIt's a bit better, but while we're at it, why not take full advantage\nof iterator abstraction?\n\nThis replacement patch (with some unrelated bits removed to reduce\ndistraction) adds a new meta ref-iterator that combine a per-repo and\na per-worktree iterators together. The new iterator walks through both\nsub-iterators and drop the per-worktree results from the per-repo\niterator, which will be replaced with results from per-worktree\niterator.\n\nYou probably see where I'm going with this. When the new \"linked\nworktree ref store\" comes, it will combine two per-worktree and\nper-repo ref stores together and this iterator will come handy.\n\nAt that point, files-backend can go back to being oblivious about\n$GIT_DIR vs $GIT_COMMON_DIR and files_reflog_iterator_begin() will be\nreverted back to the version before this patch.\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 4149943a6e..817b7b5d5e 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3432,23 +3423,37 @@ static struct ref_iterator_vtable files_reflog_iterator_vtable = {\n \tfiles_reflog_iterator_abort\n };\n \n-static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_store)\n+static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,\n+\t\t\t\t\t\t  const char *gitdir)\n {\n-\tstruct files_ref_store *refs =\n-\t\tfiles_downcast(ref_store, REF_STORE_READ,\n-\t\t\t       \"reflog_iterator_begin\");\n \tstruct files_reflog_iterator *iter = xcalloc(1, sizeof(*iter));\n \tstruct ref_iterator *ref_iterator = &iter->base;\n \tstruct strbuf sb = STRBUF_INIT;\n \n \tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable);\n-\tfiles_reflog_path(refs, &sb, NULL);\n+\tstrbuf_addf(&sb, \"%s/logs\", gitdir);\n \titer->dir_iterator = dir_iterator_begin(sb.buf);\n \titer->ref_store = ref_store;\n \tstrbuf_release(&sb);\n+\n \treturn ref_iterator;\n }\n \n+static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_store)\n+{\n+\tstruct files_ref_store *refs =\n+\t\tfiles_downcast(ref_store, REF_STORE_READ,\n+\t\t\t       \"reflog_iterator_begin\");\n+\n+\tif (!strcmp(refs->gitdir, refs->gitcommondir)) {\n+\t\treturn reflog_iterator_begin(ref_store, refs->gitcommondir);\n+\t} else {\n+\t\treturn worktree_ref_iterator_begin(\n+\t\t\treflog_iterator_begin(ref_store, refs->gitcommondir),\n+\t\t\treflog_iterator_begin(ref_store, refs->gitdir));\n+\t}\n+}\n+\n static int ref_update_reject_duplicates(struct string_list *refnames,\n \t\t\t\t\tstruct strbuf *err)\n {\ndiff --git a/refs/iterator.c b/refs/iterator.c\nindex bce1f192f7..93243a00c4 100644\n--- a/refs/iterator.c\n+++ b/refs/iterator.c\n@@ -40,6 +40,14 @@ void base_ref_iterator_free(struct ref_iterator *iter)\n \tfree(iter);\n }\n \n+static void ref_iterator_copy_result(struct ref_iterator *dst,\n+\t\t\t\t     const struct ref_iterator *src)\n+{\n+\tdst->refname = src->refname;\n+\tdst->oid = src->oid;\n+\tdst->flags = src->flags;\n+}\n+\n struct empty_ref_iterator {\n \tstruct ref_iterator base;\n };\n@@ -382,3 +390,100 @@ int do_for_each_ref_iterator(struct ref_iterator *iter,\n \t\treturn -1;\n \treturn retval;\n }\n+\n+struct worktree_ref_iterator {\n+\tstruct ref_iterator base;\n+\tstruct ref_iterator *per_repo_iterator;\n+\tstruct ref_iterator *per_worktree_iterator;\n+};\n+\n+static int worktree_ref_iterator_advance(struct ref_iterator *ref_iterator)\n+{\n+\tstruct worktree_ref_iterator *iter =\n+\t\t(struct worktree_ref_iterator *)ref_iterator;\n+\tint ok;\n+\n+\twhile (1) {\n+\t\tstruct ref_iterator **subiter;\n+\t\tint normal_only;\n+\n+\t\tif (iter->per_repo_iterator) {\n+\t\t\tsubiter = &iter->per_repo_iterator;\n+\t\t\t/*\n+\t\t\t * If we are in a worktree, then we only\n+\t\t\t * include \"normal\" common references:\n+\t\t\t */\n+\t\t\tnormal_only = !!iter->per_worktree_iterator;\n+\t\t} else if (iter->per_worktree_iterator) {\n+\t\t\tsubiter = &iter->per_worktree_iterator;\n+\t\t\tnormal_only = 0;\n+\t\t} else {\n+\t\t\tok = ITER_DONE;\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\tok = ref_iterator_advance(*subiter);\n+\t\tif (ok == ITER_ERROR) {\n+\t\t\t*subiter = NULL;\n+\t\t\tbreak;\n+\t\t} else if (ok == ITER_DONE) {\n+\t\t\t*subiter = NULL;\n+\t\t\t/* There might still be worktree refs left: */\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (normal_only &&\n+\t\t    ref_type((*subiter)->refname) != REF_TYPE_NORMAL)\n+\t\t\tcontinue;\n+\n+\t\tref_iterator_copy_result(&iter->base, *subiter);\n+\t\treturn ITER_OK;\n+\t}\n+\n+\tif (ref_iterator_abort(ref_iterator) == ITER_ERROR)\n+\t\treturn ITER_ERROR;\n+\n+\treturn ok;\n+}\n+\n+static int worktree_ref_iterator_peel(struct ref_iterator *ref_iterator,\n+\t\t\t\t\t struct object_id *peeled)\n+{\n+\tdie(\"BUG: ref_iterator_peel() called for reflog_iterator\");\n+}\n+\n+static int worktree_ref_iterator_abort(struct ref_iterator *ref_iterator)\n+{\n+\tstruct worktree_ref_iterator *iter =\n+\t\t(struct worktree_ref_iterator *)ref_iterator;\n+\tint ok = ITER_DONE;\n+\n+\tif (iter->per_repo_iterator)\n+\t\tok = ref_iterator_abort(iter->per_repo_iterator);\n+\n+\tif (iter->per_worktree_iterator) {\n+\t\tint ok2 = ref_iterator_abort(iter->per_worktree_iterator);\n+\t\tif (ok2 == ITER_ERROR)\n+\t\t\tok = ok2;\n+\t}\n+\n+\tbase_ref_iterator_free(ref_iterator);\n+\treturn ok;\n+}\n+\n+static struct ref_iterator_vtable worktree_ref_iterator_vtable = {\n+\tworktree_ref_iterator_advance,\n+\tworktree_ref_iterator_peel,\n+\tworktree_ref_iterator_abort\n+};\n+\n+struct ref_iterator *worktree_ref_iterator_begin(struct ref_iterator *per_repo,\n+\t\t\t\t\t\t struct ref_iterator *per_worktree)\n+{\n+\tstruct worktree_ref_iterator *iter = xcalloc(1, sizeof(*iter));\n+\n+\tbase_ref_iterator_init(&iter->base, &worktree_ref_iterator_vtable);\n+\titer->per_repo_iterator = per_repo;\n+\titer->per_worktree_iterator = per_worktree;\n+\treturn &iter->base;\n+}\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 690498698e..dcb1f1d73d 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -387,6 +387,14 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,\n \t\t\t\t\t       const char *prefix,\n \t\t\t\t\t       int trim);\n \n+/*\n+ * Wrap per_repo and per_worktree iterators. Traverse per_repo\n+ * iterator, drop per-worktree refs. Then traverse per_worktree\n+ * iterator.\n+ */\n+struct ref_iterator *worktree_ref_iterator_begin(struct ref_iterator *per_repo,\n+\t\t\t\t\t\t struct ref_iterator *per_worktree);\n+\n /* Internal implementation of reference iteration: */\n \n /*\n\n\n"},{"id":"317710","messageId":"CAGZ79kb-8zUibxCMNxkg2jbGLuXPGm0ZjqZ_KtLKV4YTrn0TsQ@mail.gmail.com","threadId":"45745","inReplyTo":"378b8e50-562a-a311-2e0f-505de036aaf1@alum.mit.edu","subject":"Re: [PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-24T18:12:22Z","receivedAt":"2017-04-24T18:12:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Apr 21, 2017 at 10:27 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:\n>> This is a better place that will benefit all submodule callers instead\n>> of just resolve_gitlink_ref()\n>>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n>> ---\n>>  refs.c | 33 +++++++++++++++++----------------\n>>  1 file changed, 17 insertions(+), 16 deletions(-)\n>>\n>> diff --git a/refs.c b/refs.c\n>> index 5902a3d9e5..26474cb62a 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -1422,25 +1422,10 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,\n>>  int resolve_gitlink_ref(const char *submodule, const char *refname,\n>>                       unsigned char *sha1)\n>>  {\n>> -     size_t len = strlen(submodule);\n>>       struct ref_store *refs;\n>>       int flags;\n>>\n>> -     while (len && submodule[len - 1] == '/')\n>> -             len--;\n>> -\n>> -     if (!len)\n>> -             return -1;\n>> -\n>> -     if (submodule[len]) {\n>> -             /* We need to strip off one or more trailing slashes */\n>> -             char *stripped = xmemdupz(submodule, len);\n>> -\n>> -             refs = get_submodule_ref_store(stripped);\n>> -             free(stripped);\n>> -     } else {\n>> -             refs = get_submodule_ref_store(submodule);\n>> -     }\n>> +     refs = get_submodule_ref_store(submodule);\n>>\n>>       if (!refs)\n>>               return -1;\n>> @@ -1558,7 +1543,17 @@ struct ref_store *get_submodule_ref_store(const char *submodule)\n>>  {\n>>       struct strbuf submodule_sb = STRBUF_INIT;\n>>       struct ref_store *refs;\n>> +     char *to_free = NULL;\n>>       int ret;\n>> +     size_t len;\n>> +\n>> +     if (submodule) {\n>> +             len = strlen(submodule);\n>> +             while (len && submodule[len - 1] == '/')\n>> +                     len--;\n>> +             if (!len)\n>> +                     submodule = NULL;\n>> +     }\n>\n> Ugh. Should a submodule named \"///\" *really* be considered to refer to\n> the main ref_store? I understand that's what the code did before this\n> patch, but it seems to me more like an accident of the old design rather\n> than something worth supporting. In other words, if a caller would\n> really pass us such a string, it seems like we could declare the caller\n> buggy, no?\n\nIn a nearby thread we discussed whether we want to tighten the\nsubmodule names and paths as some of the code path do funny\nthings on funny input. As 'submodule' here refers to a path (and not\na name), I would think '///' is not possible/buggy, rather we'd want to\ndiscuss if we want to extend the check to is_dir_sep, which would\ninclude '\\' on Windows.\n\nThanks,\nStefan\n"},{"id":"320075","messageId":"00720e90-ed85-e8d8-a2e4-f42f93a33d33@alum.mit.edu","threadId":"45745","inReplyTo":"20170423044420.GA28419@ash","subject":"Re: [PATCH v3 10/12] files-backend: make reflog iterator go through per-worktree reflog","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T13:59:32Z","receivedAt":"2017-05-17T13:59:43Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Hi,\n\nI put off reviewing this patch, thinking that it would appear in a\nre-roll, then never came back to it. :-(\n\nOn 04/23/2017 06:44 AM, Duy Nguyen wrote:\n> On Sat, Apr 22, 2017 at 10:05:02AM +0200, Michael Haggerty wrote:\n>> I find this implementation confusing:\n>>\n>> * `if (iter->worktree_dir_iterator)` sounds like it should mean\n>>   that we are iterating over worktree references but it really means\n>>   that we are iterating over the common references in a repository\n>>   that is a linked worktree.\n>> * `files_reflog_iterator_advance()` is called recursively, but only\n>>   for the first worktree reference.\n>> * `iter->worktree_dir_iterator` is moved over to `iter->dir_iterator`\n>>   when the common refs are exhausted.\n>>\n>> Do you find it more readable as follows?:\n> \n> It's a bit better, but while we're at it, why not take full advantage\n> of iterator abstraction?\n> \n> This replacement patch (with some unrelated bits removed to reduce\n> distraction) adds a new meta ref-iterator that combine a per-repo and\n> a per-worktree iterators together. The new iterator walks through both\n> sub-iterators and drop the per-worktree results from the per-repo\n> iterator, which will be replaced with results from per-worktree\n> iterator.\n> \n> You probably see where I'm going with this. When the new \"linked\n> worktree ref store\" comes, it will combine two per-worktree and\n> per-repo ref stores together and this iterator will come handy.\n> \n> At that point, files-backend can go back to being oblivious about\n> $GIT_DIR vs $GIT_COMMON_DIR and files_reflog_iterator_begin() will be\n> reverted back to the version before this patch.\n\nYes, that's even better.\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 4149943a6e..817b7b5d5e 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -3432,23 +3423,37 @@ static struct ref_iterator_vtable files_reflog_iterator_vtable = {\n>  \tfiles_reflog_iterator_abort\n>  };\n>  \n> -static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_store)\n> +static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,\n> +\t\t\t\t\t\t  const char *gitdir)\n>  {\n> -\tstruct files_ref_store *refs =\n> -\t\tfiles_downcast(ref_store, REF_STORE_READ,\n> -\t\t\t       \"reflog_iterator_begin\");\n>  \tstruct files_reflog_iterator *iter = xcalloc(1, sizeof(*iter));\n>  \tstruct ref_iterator *ref_iterator = &iter->base;\n>  \tstruct strbuf sb = STRBUF_INIT;\n>  \n>  \tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable);\n> -\tfiles_reflog_path(refs, &sb, NULL);\n> +\tstrbuf_addf(&sb, \"%s/logs\", gitdir);\n>  \titer->dir_iterator = dir_iterator_begin(sb.buf);\n>  \titer->ref_store = ref_store;\n>  \tstrbuf_release(&sb);\n> +\n>  \treturn ref_iterator;\n>  }\n\nMakes sense.\n\n> +static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_store)\n> +{\n> +\tstruct files_ref_store *refs =\n> +\t\tfiles_downcast(ref_store, REF_STORE_READ,\n> +\t\t\t       \"reflog_iterator_begin\");\n> +\n> +\tif (!strcmp(refs->gitdir, refs->gitcommondir)) {\n> +\t\treturn reflog_iterator_begin(ref_store, refs->gitcommondir);\n> +\t} else {\n> +\t\treturn worktree_ref_iterator_begin(\n> +\t\t\treflog_iterator_begin(ref_store, refs->gitcommondir),\n> +\t\t\treflog_iterator_begin(ref_store, refs->gitdir));\n> +\t}\n> +}\n> +\n\nYes.\n\n>  static int ref_update_reject_duplicates(struct string_list *refnames,\n>  \t\t\t\t\tstruct strbuf *err)\n>  {\n> diff --git a/refs/iterator.c b/refs/iterator.c\n> index bce1f192f7..93243a00c4 100644\n> --- a/refs/iterator.c\n> +++ b/refs/iterator.c\n> @@ -40,6 +40,14 @@ void base_ref_iterator_free(struct ref_iterator *iter)\n>  \tfree(iter);\n>  }\n>  \n> +static void ref_iterator_copy_result(struct ref_iterator *dst,\n> +\t\t\t\t     const struct ref_iterator *src)\n> +{\n> +\tdst->refname = src->refname;\n> +\tdst->oid = src->oid;\n> +\tdst->flags = src->flags;\n> +}\n> +\n>  struct empty_ref_iterator {\n>  \tstruct ref_iterator base;\n>  };\n> @@ -382,3 +390,100 @@ int do_for_each_ref_iterator(struct ref_iterator *iter,\n>  \t\treturn -1;\n>  \treturn retval;\n>  }\n> +\n> +struct worktree_ref_iterator {\n> +\tstruct ref_iterator base;\n> +\tstruct ref_iterator *per_repo_iterator;\n> +\tstruct ref_iterator *per_worktree_iterator;\n> +};\n> +\n> +static int worktree_ref_iterator_advance(struct ref_iterator *ref_iterator)\n> +{\n> +\tstruct worktree_ref_iterator *iter =\n> +\t\t(struct worktree_ref_iterator *)ref_iterator;\n> +\tint ok;\n> +\n> +\twhile (1) {\n> +\t\tstruct ref_iterator **subiter;\n> +\t\tint normal_only;\n> +\n> +\t\tif (iter->per_repo_iterator) {\n> +\t\t\tsubiter = &iter->per_repo_iterator;\n> +\t\t\t/*\n> +\t\t\t * If we are in a worktree, then we only\n> +\t\t\t * include \"normal\" common references:\n> +\t\t\t */\n> +\t\t\tnormal_only = !!iter->per_worktree_iterator;\n> +\t\t} else if (iter->per_worktree_iterator) {\n> +\t\t\tsubiter = &iter->per_worktree_iterator;\n> +\t\t\tnormal_only = 0;\n> +\t\t} else {\n> +\t\t\tok = ITER_DONE;\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tok = ref_iterator_advance(*subiter);\n> +\t\tif (ok == ITER_ERROR) {\n> +\t\t\t*subiter = NULL;\n> +\t\t\tbreak;\n> +\t\t} else if (ok == ITER_DONE) {\n> +\t\t\t*subiter = NULL;\n> +\t\t\t/* There might still be worktree refs left: */\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tif (normal_only &&\n> +\t\t    ref_type((*subiter)->refname) != REF_TYPE_NORMAL)\n> +\t\t\tcontinue;\n> +\n> +\t\tref_iterator_copy_result(&iter->base, *subiter);\n> +\t\treturn ITER_OK;\n> +\t}\n> +\n> +\tif (ref_iterator_abort(ref_iterator) == ITER_ERROR)\n> +\t\treturn ITER_ERROR;\n> +\n> +\treturn ok;\n> +}\n> +\n> +static int worktree_ref_iterator_peel(struct ref_iterator *ref_iterator,\n> +\t\t\t\t\t struct object_id *peeled)\n> +{\n> +\tdie(\"BUG: ref_iterator_peel() called for reflog_iterator\");\n> +}\n> +\n> +static int worktree_ref_iterator_abort(struct ref_iterator *ref_iterator)\n> +{\n> +\tstruct worktree_ref_iterator *iter =\n> +\t\t(struct worktree_ref_iterator *)ref_iterator;\n> +\tint ok = ITER_DONE;\n> +\n> +\tif (iter->per_repo_iterator)\n> +\t\tok = ref_iterator_abort(iter->per_repo_iterator);\n> +\n> +\tif (iter->per_worktree_iterator) {\n> +\t\tint ok2 = ref_iterator_abort(iter->per_worktree_iterator);\n> +\t\tif (ok2 == ITER_ERROR)\n> +\t\t\tok = ok2;\n> +\t}\n> +\n> +\tbase_ref_iterator_free(ref_iterator);\n> +\treturn ok;\n> +}\n> +\n> +static struct ref_iterator_vtable worktree_ref_iterator_vtable = {\n> +\tworktree_ref_iterator_advance,\n> +\tworktree_ref_iterator_peel,\n> +\tworktree_ref_iterator_abort\n> +};\n> +\n> +struct ref_iterator *worktree_ref_iterator_begin(struct ref_iterator *per_repo,\n> +\t\t\t\t\t\t struct ref_iterator *per_worktree)\n> +{\n> +\tstruct worktree_ref_iterator *iter = xcalloc(1, sizeof(*iter));\n> +\n> +\tbase_ref_iterator_init(&iter->base, &worktree_ref_iterator_vtable);\n> +\titer->per_repo_iterator = per_repo;\n> +\titer->per_worktree_iterator = per_worktree;\n> +\treturn &iter->base;\n> +}\n> diff --git a/refs/refs-internal.h b/refs/refs-internal.h\n> index 690498698e..dcb1f1d73d 100644\n> --- a/refs/refs-internal.h\n> +++ b/refs/refs-internal.h\n> @@ -387,6 +387,14 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,\n>  \t\t\t\t\t       const char *prefix,\n>  \t\t\t\t\t       int trim);\n>  \n> +/*\n> + * Wrap per_repo and per_worktree iterators. Traverse per_repo\n> + * iterator, drop per-worktree refs. Then traverse per_worktree\n> + * iterator.\n> + */\n> +struct ref_iterator *worktree_ref_iterator_begin(struct ref_iterator *per_repo,\n> +\t\t\t\t\t\t struct ref_iterator *per_worktree);\n> +\n>  /* Internal implementation of reference iteration: */\n>  \n>  /*\n> \n> \n\nI think `worktree_ref_iterator_begin()` could be implemented with a lot\nless code via a `merge_ref_iterator`. You would need to define a\n`ref_iterator_select_fn`, which could look like something like this:\n\nenum iterator_selection worktree_iterator_select_fn(\n\t\tstruct ref_iterator *iter0, struct ref_iterator *iter1,\n\t\tvoid *cb_data)\n{\n\tif (iter0) {\n\t\tif (ref_type(iter0->refname) != REF_TYPE_NORMAL)\n\t\t\treturn ITER_SKIP_0;\n\t\treturn ITER_SELECT_0;\n\t} else if (iter1) {\n\t\treturn ITER_SELECT_1;\n\t} else {\n\t\treturn ITER_SELECT_DONE;\n\t}\n}\n\nThen `worktree_ref_iterator_begin()` is simply\n\nstruct ref_iterator *worktree_ref_iterator_begin(struct ref_iterator\n*per_repo,\n\t\t\t\t\t\t struct ref_iterator *per_worktree)\n{\n\treturn merge_ref_iterator_begin(per_repo, per_worktree,\n\t\t\t\t\tworktree_iterator_select_fn, NULL);\n}\n\nFor references (as opposed to reflogs) you would want to interleave the\noutputs from the two input iterators in the correct order. See\n`overlay_iterator_select()` for how that can be done.\n\nMichael\n\n"}]}