[PATCH v3 00/12] Fix git-gc losing objects in multi worktree

[PATCH v3 00/12] Fix git-gc losing objects in multi worktree

[Nguyễn Thái Ngọc Duy]

Changes since v2 [1] is relatively small. It still needs
nd/worktree-kill-parse-ref of course.

[1] http://public-inbox.org/git/20170318101153.6901-1-pclouds@gmail.com/

diff --git a/Documentation/technical/api-ref-iteration.txt b/Documentation/technical/api-ref-iteration.txt
index 37379d8337..c9e9a60dbd 100644
--- a/Documentation/technical/api-ref-iteration.txt
+++ b/Documentation/technical/api-ref-iteration.txt
@@ -32,11 +32,8 @@ Iteration functions
 
 * `for_each_glob_ref_in()` the previous and `for_each_ref_in()` combined.
 
-* `head_ref_submodule()`, `for_each_ref_submodule()`,
-  `for_each_ref_in_submodule()`, `for_each_tag_ref_submodule()`,
-  `for_each_branch_ref_submodule()`, `for_each_remote_ref_submodule()`
-  do the same as the functions described above but for a specified
-  submodule.
+* Use `refs_` API for accessing submodules. The submodule ref store could
+  be obtained with `get_submodule_ref_store().
 
 * `for_each_rawref()` can be used to learn about broken ref and symref.
 
diff --git a/refs.c b/refs.c
index c39058adce..23e3607674 100644
--- a/refs.c
+++ b/refs.c
@@ -1208,11 +1208,6 @@ int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
 	return 0;
 }
 
-int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
-{
-	return refs_head_ref(get_submodule_ref_store(submodule), fn, cb_data);
-}
-
 int head_ref(each_ref_fn fn, void *cb_data)
 {
 	return refs_head_ref(get_main_ref_store(), fn, cb_data);
diff --git a/revision.c b/revision.c
index dc32e99c54..79ce8a007f 100644
--- a/revision.c
+++ b/revision.c
@@ -1336,7 +1336,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 	worktrees = get_worktrees(0);
 	for (p = worktrees; *p; p++) {
 		struct worktree *wt = *p;
-		struct index_state istate = {0};
+		struct index_state istate = { NULL };
 
 		if (wt->is_current)
 			continue; /* current index already taken care of */

Nguyễn Thái Ngọc Duy (12):
  revision.h: new flag in struct rev_info wrt. worktree-related refs
  revision.c: refactor add_index_objects_to_pending()
  revision.c: --indexed-objects add objects from all worktrees
  refs.c: refactor get_submodule_ref_store(), share common free block
  refs: move submodule slash stripping code to get_submodule_ref_store
  refs: add refs_head_ref()
  revision.c: use refs_for_each*() instead of for_each_*_submodule()
  refs: remove dead for_each_*_submodule()
  revision.c: --all adds HEAD from all worktrees
  files-backend: make reflog iterator go through per-worktree reflog
  revision.c: --reflog add HEAD reflog from all worktrees
  rev-list: expose and document --single-worktree

 Documentation/rev-list-options.txt            |   8 ++
 Documentation/technical/api-ref-iteration.txt |   7 +-
 reachable.c                                   |   1 +
 refs.c                                        | 105 ++++++++++-----------
 refs.h                                        |  12 +--
 refs/files-backend.c                          |  46 ++++++---
 revision.c                                    | 130 +++++++++++++++++++++-----
 revision.h                                    |   1 +
 submodule.c                                   |   2 +
 t/t1407-worktree-ref-store.sh                 |  30 ++++++
 t/t5304-prune.sh                              |  37 ++++++++
 11 files changed, 274 insertions(+), 105 deletions(-)

-- 
2.11.0.157.gd943d85

[PATCH v3 01/12] revision.h: new flag in struct rev_info wrt. worktree-related refs

[Nguyễn Thái Ngọc Duy]

The revision walker can walk through per-worktree refs like HEAD or
SHA-1 references in the index. These currently are from the current
worktree only. This new flag is added to change rev-list behavior in
this regard:

When single_worktree is set, only current worktree is considered. When
it is not set (which is the default), all worktrees are considered.

The default is chosen so because the two big components that rev-list
works with are object database (entirely shared between worktrees) and
refs (mostly shared). It makes sense that default behavior goes per-repo
too instead of per-worktree.

The flag will eventually be exposed as a rev-list argument with
documents. For now it stays internal until the new behavior is fully
implemented.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 revision.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/revision.h b/revision.h
index 9fac1a607d..c851b94ad8 100644
--- a/revision.h
+++ b/revision.h
@@ -88,6 +88,7 @@ struct rev_info {
 			topo_order:1,
 			simplify_merges:1,
 			simplify_by_decoration:1,
+			single_worktree:1,
 			tag_objects:1,
 			tree_objects:1,
 			blob_objects:1,
-- 
2.11.0.157.gd943d85

[PATCH v3 02/12] revision.c: refactor add_index_objects_to_pending()

[Nguyễn Thái Ngọc Duy]

The core code is factored out and take 'struct index_state *' instead so
that we can reuse it to add objects from index files other than .git/index
in the next patch.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 revision.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/revision.c b/revision.c
index 7ff61ff5f7..98146f179f 100644
--- a/revision.c
+++ b/revision.c
@@ -1263,13 +1263,13 @@ static void add_cache_tree(struct cache_tree *it, struct rev_info *revs,
 
 }
 
-void add_index_objects_to_pending(struct rev_info *revs, unsigned flags)
+static void do_add_index_objects_to_pending(struct rev_info *revs,
+					    struct index_state *istate)
 {
 	int i;
 
-	read_cache();
-	for (i = 0; i < active_nr; i++) {
-		struct cache_entry *ce = active_cache[i];
+	for (i = 0; i < istate->cache_nr; i++) {
+		struct cache_entry *ce = istate->cache[i];
 		struct blob *blob;
 
 		if (S_ISGITLINK(ce->ce_mode))
@@ -1282,13 +1282,19 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned flags)
 					     ce->ce_mode, ce->name);
 	}
 
-	if (active_cache_tree) {
+	if (istate->cache_tree) {
 		struct strbuf path = STRBUF_INIT;
-		add_cache_tree(active_cache_tree, revs, &path);
+		add_cache_tree(istate->cache_tree, revs, &path);
 		strbuf_release(&path);
 	}
 }
 
+void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
+{
+	read_cache();
+	do_add_index_objects_to_pending(revs, &the_index);
+}
+
 static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
 			    int exclude_parent)
 {
-- 
2.11.0.157.gd943d85

[PATCH v3 03/12] revision.c: --indexed-objects add objects from all worktrees

[Nguyễn Thái Ngọc Duy]

This is the result of single_worktree flag never being set (no way to up
until now). To get objects from current index only, set single_worktree.

The other add_index_objects_to_pending's caller is mark_reachable_objects()
(e.g. "git prune") which also mark objects from all indexes.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 revision.c       | 21 +++++++++++++++++++++
 t/t5304-prune.sh |  9 +++++++++
 2 files changed, 30 insertions(+)

diff --git a/revision.c b/revision.c
index 98146f179f..295d4f8205 100644
--- a/revision.c
+++ b/revision.c
@@ -19,6 +19,7 @@
 #include "dir.h"
 #include "cache-tree.h"
 #include "bisect.h"
+#include "worktree.h"
 
 volatile show_early_output_fn_t show_early_output;
 
@@ -1291,8 +1292,28 @@ static void do_add_index_objects_to_pending(struct rev_info *revs,
 
 void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 {
+	struct worktree **worktrees, **p;
+
 	read_cache();
 	do_add_index_objects_to_pending(revs, &the_index);
+
+	if (revs->single_worktree)
+		return;
+
+	worktrees = get_worktrees(0);
+	for (p = worktrees; *p; p++) {
+		struct worktree *wt = *p;
+		struct index_state istate = { NULL };
+
+		if (wt->is_current)
+			continue; /* current index already taken care of */
+
+		if (read_index_from(&istate,
+				    worktree_git_path(wt, "index")) > 0)
+			do_add_index_objects_to_pending(revs, &istate);
+		discard_index(&istate);
+	}
+	free_worktrees(worktrees);
 }
 
 static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 133b5842b1..cba45c7be9 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -283,4 +283,13 @@ test_expect_success 'prune: handle alternate object database' '
 	git -C B prune
 '
 
+test_expect_success 'prune: handle index in multiple worktrees' '
+	git worktree add second-worktree &&
+	echo "new blob for second-worktree" >second-worktree/blob &&
+	git -C second-worktree add blob &&
+	git prune --expire=now &&
+	git -C second-worktree show :blob >actual &&
+	test_cmp second-worktree/blob actual
+'
+
 test_done
-- 
2.11.0.157.gd943d85

[PATCH v3 04/12] refs.c: refactor get_submodule_ref_store(), share common free block

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index 5d31fb6bcf..5902a3d9e5 100644
--- a/refs.c
+++ b/refs.c
@@ -1570,19 +1570,16 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 
 	refs = lookup_ref_store_map(&submodule_ref_stores, submodule);
 	if (refs)
-		return refs;
+		goto done;
 
 	strbuf_addstr(&submodule_sb, submodule);
 	ret = is_nonbare_repository_dir(&submodule_sb);
-	strbuf_release(&submodule_sb);
 	if (!ret)
-		return NULL;
+		goto done;
 
 	ret = submodule_to_gitdir(&submodule_sb, submodule);
-	if (ret) {
-		strbuf_release(&submodule_sb);
-		return NULL;
-	}
+	if (ret)
+		goto done;
 
 	/* assume that add_submodule_odb() has been called */
 	refs = ref_store_init(submodule_sb.buf,
@@ -1590,6 +1587,7 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 	register_ref_store_map(&submodule_ref_stores, "submodule",
 			       refs, submodule);
 
+done:
 	strbuf_release(&submodule_sb);
 	return refs;
 }
-- 
2.11.0.157.gd943d85

[PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store

[Nguyễn Thái Ngọc Duy]

This is a better place that will benefit all submodule callers instead
of just resolve_gitlink_ref()

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/refs.c b/refs.c
index 5902a3d9e5..26474cb62a 100644
--- a/refs.c
+++ b/refs.c
@@ -1422,25 +1422,10 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
 int resolve_gitlink_ref(const char *submodule, const char *refname,
 			unsigned char *sha1)
 {
-	size_t len = strlen(submodule);
 	struct ref_store *refs;
 	int flags;
 
-	while (len && submodule[len - 1] == '/')
-		len--;
-
-	if (!len)
-		return -1;
-
-	if (submodule[len]) {
-		/* We need to strip off one or more trailing slashes */
-		char *stripped = xmemdupz(submodule, len);
-
-		refs = get_submodule_ref_store(stripped);
-		free(stripped);
-	} else {
-		refs = get_submodule_ref_store(submodule);
-	}
+	refs = get_submodule_ref_store(submodule);
 
 	if (!refs)
 		return -1;
@@ -1558,7 +1543,17 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 {
 	struct strbuf submodule_sb = STRBUF_INIT;
 	struct ref_store *refs;
+	char *to_free = NULL;
 	int ret;
+	size_t len;
+
+	if (submodule) {
+		len = strlen(submodule);
+		while (len && submodule[len - 1] == '/')
+			len--;
+		if (!len)
+			submodule = NULL;
+	}
 
 	if (!submodule || !*submodule) {
 		/*
@@ -1568,6 +1563,10 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 		return get_main_ref_store();
 	}
 
+	if (submodule[len])
+		/* We need to strip off one or more trailing slashes */
+		submodule = to_free = xmemdupz(submodule, len);
+
 	refs = lookup_ref_store_map(&submodule_ref_stores, submodule);
 	if (refs)
 		goto done;
@@ -1589,6 +1588,8 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 
 done:
 	strbuf_release(&submodule_sb);
+	free(to_free);
+
 	return refs;
 }
 
-- 
2.11.0.157.gd943d85

[PATCH v3 06/12] refs: add refs_head_ref()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c | 19 +++++++++----------
 refs.h |  2 ++
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index 26474cb62a..a252ae43ee 100644
--- a/refs.c
+++ b/refs.c
@@ -1208,27 +1208,26 @@ int refs_rename_ref_available(struct ref_store *refs,
 	return ok;
 }
 
-int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
+int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
 {
 	struct object_id oid;
 	int flag;
 
-	if (submodule) {
-		if (resolve_gitlink_ref(submodule, "HEAD", oid.hash) == 0)
-			return fn("HEAD", &oid, 0, cb_data);
-
-		return 0;
-	}
-
-	if (!read_ref_full("HEAD", RESOLVE_REF_READING, oid.hash, &flag))
+	if (!refs_read_ref_full(refs, "HEAD", RESOLVE_REF_READING,
+				oid.hash, &flag))
 		return fn("HEAD", &oid, flag, cb_data);
 
 	return 0;
 }
 
+int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
+{
+	return refs_head_ref(get_submodule_ref_store(submodule), fn, cb_data);
+}
+
 int head_ref(each_ref_fn fn, void *cb_data)
 {
-	return head_ref_submodule(NULL, fn, cb_data);
+	return refs_head_ref(get_main_ref_store(), fn, cb_data);
 }
 
 /*
diff --git a/refs.h b/refs.h
index 447381d378..0572473ef7 100644
--- a/refs.h
+++ b/refs.h
@@ -233,6 +233,8 @@ typedef int each_ref_fn(const char *refname,
  * modifies the reference also returns a nonzero value to immediately
  * stop the iteration. Returned references are sorted.
  */
+int refs_head_ref(struct ref_store *refs,
+		  each_ref_fn fn, void *cb_data);
 int refs_for_each_ref(struct ref_store *refs,
 		      each_ref_fn fn, void *cb_data);
 int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,
-- 
2.11.0.157.gd943d85

[PATCH v3 07/12] revision.c: use refs_for_each*() instead of for_each_*_submodule()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 revision.c | 48 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/revision.c b/revision.c
index 295d4f8205..c329070c89 100644
--- a/revision.c
+++ b/revision.c
@@ -1189,12 +1189,19 @@ void add_ref_exclusion(struct string_list **ref_excludes_p, const char *exclude)
 	string_list_append(*ref_excludes_p, exclude);
 }
 
-static void handle_refs(const char *submodule, struct rev_info *revs, unsigned flags,
-		int (*for_each)(const char *, each_ref_fn, void *))
+static void handle_refs(struct ref_store *refs,
+			struct rev_info *revs, unsigned flags,
+			int (*for_each)(struct ref_store *, each_ref_fn, void *))
 {
 	struct all_refs_cb cb;
+
+	if (!refs) {
+		/* this could happen with uninitialized submodules */
+		return;
+	}
+
 	init_all_refs_cb(&cb, revs, flags);
-	for_each(submodule, handle_one_ref, &cb);
+	for_each(refs, handle_one_ref, &cb);
 }
 
 static void handle_one_reflog_commit(struct object_id *oid, void *cb_data)
@@ -2067,23 +2074,25 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
 	ctx->argc -= n;
 }
 
-static int for_each_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data, const char *term) {
+static int for_each_bisect_ref(struct ref_store *refs, each_ref_fn fn,
+			       void *cb_data, const char *term)
+{
 	struct strbuf bisect_refs = STRBUF_INIT;
 	int status;
 	strbuf_addf(&bisect_refs, "refs/bisect/%s", term);
-	status = for_each_ref_in_submodule(submodule, bisect_refs.buf, fn, cb_data);
+	status = refs_for_each_ref_in(refs, bisect_refs.buf, fn, cb_data);
 	strbuf_release(&bisect_refs);
 	return status;
 }
 
-static int for_each_bad_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data)
+static int for_each_bad_bisect_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
 {
-	return for_each_bisect_ref(submodule, fn, cb_data, term_bad);
+	return for_each_bisect_ref(refs, fn, cb_data, term_bad);
 }
 
-static int for_each_good_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data)
+static int for_each_good_bisect_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
 {
-	return for_each_bisect_ref(submodule, fn, cb_data, term_good);
+	return for_each_bisect_ref(refs, fn, cb_data, term_good);
 }
 
 static int handle_revision_pseudo_opt(const char *submodule,
@@ -2092,8 +2101,14 @@ static int handle_revision_pseudo_opt(const char *submodule,
 {
 	const char *arg = argv[0];
 	const char *optarg;
+	struct ref_store *refs;
 	int argcount;
 
+	if (submodule) {
+		refs = get_submodule_ref_store(submodule);
+	} else
+		refs = get_main_ref_store();
+
 	/*
 	 * NOTE!
 	 *
@@ -2105,22 +2120,23 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	 * register it in the list at the top of handle_revision_opt.
 	 */
 	if (!strcmp(arg, "--all")) {
-		handle_refs(submodule, revs, *flags, for_each_ref_submodule);
-		handle_refs(submodule, revs, *flags, head_ref_submodule);
+		handle_refs(refs, revs, *flags, refs_for_each_ref);
+		handle_refs(refs, revs, *flags, refs_head_ref);
 		clear_ref_exclusion(&revs->ref_excludes);
 	} else if (!strcmp(arg, "--branches")) {
-		handle_refs(submodule, revs, *flags, for_each_branch_ref_submodule);
+		handle_refs(refs, revs, *flags, refs_for_each_branch_ref);
 		clear_ref_exclusion(&revs->ref_excludes);
 	} else if (!strcmp(arg, "--bisect")) {
 		read_bisect_terms(&term_bad, &term_good);
-		handle_refs(submodule, revs, *flags, for_each_bad_bisect_ref);
-		handle_refs(submodule, revs, *flags ^ (UNINTERESTING | BOTTOM), for_each_good_bisect_ref);
+		handle_refs(refs, revs, *flags, for_each_bad_bisect_ref);
+		handle_refs(refs, revs, *flags ^ (UNINTERESTING | BOTTOM),
+			    for_each_good_bisect_ref);
 		revs->bisect = 1;
 	} else if (!strcmp(arg, "--tags")) {
-		handle_refs(submodule, revs, *flags, for_each_tag_ref_submodule);
+		handle_refs(refs, revs, *flags, refs_for_each_tag_ref);
 		clear_ref_exclusion(&revs->ref_excludes);
 	} else if (!strcmp(arg, "--remotes")) {
-		handle_refs(submodule, revs, *flags, for_each_remote_ref_submodule);
+		handle_refs(refs, revs, *flags, refs_for_each_remote_ref);
 		clear_ref_exclusion(&revs->ref_excludes);
 	} else if ((argcount = parse_long_opt("glob", argv, &optarg))) {
 		struct all_refs_cb cb;
-- 
2.11.0.157.gd943d85

[PATCH v3 08/12] refs: remove dead for_each_*_submodule()

[Nguyễn Thái Ngọc Duy]

These are used in revision.c. After the last patch they are replaced
with the refs_ version. Delete them (except for_each_remote_ref_submodule
which is still used by submodule.c)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/technical/api-ref-iteration.txt |  7 ++-----
 refs.c                                        | 29 ---------------------------
 refs.h                                        |  9 ---------
 3 files changed, 2 insertions(+), 43 deletions(-)

diff --git a/Documentation/technical/api-ref-iteration.txt b/Documentation/technical/api-ref-iteration.txt
index 37379d8337..c9e9a60dbd 100644
--- a/Documentation/technical/api-ref-iteration.txt
+++ b/Documentation/technical/api-ref-iteration.txt
@@ -32,11 +32,8 @@ Iteration functions
 
 * `for_each_glob_ref_in()` the previous and `for_each_ref_in()` combined.
 
-* `head_ref_submodule()`, `for_each_ref_submodule()`,
-  `for_each_ref_in_submodule()`, `for_each_tag_ref_submodule()`,
-  `for_each_branch_ref_submodule()`, `for_each_remote_ref_submodule()`
-  do the same as the functions described above but for a specified
-  submodule.
+* Use `refs_` API for accessing submodules. The submodule ref store could
+  be obtained with `get_submodule_ref_store().
 
 * `for_each_rawref()` can be used to learn about broken ref and symref.
 
diff --git a/refs.c b/refs.c
index a252ae43ee..537052f7ba 100644
--- a/refs.c
+++ b/refs.c
@@ -316,12 +316,6 @@ int for_each_tag_ref(each_ref_fn fn, void *cb_data)
 	return refs_for_each_tag_ref(get_main_ref_store(), fn, cb_data);
 }
 
-int for_each_tag_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
-{
-	return refs_for_each_tag_ref(get_submodule_ref_store(submodule),
-				     fn, cb_data);
-}
-
 int refs_for_each_branch_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
 {
 	return refs_for_each_ref_in(refs, "refs/heads/", fn, cb_data);
@@ -332,12 +326,6 @@ int for_each_branch_ref(each_ref_fn fn, void *cb_data)
 	return refs_for_each_branch_ref(get_main_ref_store(), fn, cb_data);
 }
 
-int for_each_branch_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
-{
-	return refs_for_each_branch_ref(get_submodule_ref_store(submodule),
-					fn, cb_data);
-}
-
 int refs_for_each_remote_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
 {
 	return refs_for_each_ref_in(refs, "refs/remotes/", fn, cb_data);
@@ -1220,11 +1208,6 @@ int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
 	return 0;
 }
 
-int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
-{
-	return refs_head_ref(get_submodule_ref_store(submodule), fn, cb_data);
-}
-
 int head_ref(each_ref_fn fn, void *cb_data)
 {
 	return refs_head_ref(get_main_ref_store(), fn, cb_data);
@@ -1263,11 +1246,6 @@ int for_each_ref(each_ref_fn fn, void *cb_data)
 	return refs_for_each_ref(get_main_ref_store(), fn, cb_data);
 }
 
-int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
-{
-	return refs_for_each_ref(get_submodule_ref_store(submodule), fn, cb_data);
-}
-
 int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,
 			 each_ref_fn fn, void *cb_data)
 {
@@ -1289,13 +1267,6 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsig
 			       prefix, fn, 0, flag, cb_data);
 }
 
-int for_each_ref_in_submodule(const char *submodule, const char *prefix,
-			      each_ref_fn fn, void *cb_data)
-{
-	return refs_for_each_ref_in(get_submodule_ref_store(submodule),
-				    prefix, fn, cb_data);
-}
-
 int for_each_replace_ref(each_ref_fn fn, void *cb_data)
 {
 	return do_for_each_ref(get_main_ref_store(),
diff --git a/refs.h b/refs.h
index 0572473ef7..e06db37118 100644
--- a/refs.h
+++ b/refs.h
@@ -259,15 +259,6 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);
 int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
 			 const char *prefix, void *cb_data);
 
-int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);
-int for_each_ref_submodule(const char *submodule,
-			   each_ref_fn fn, void *cb_data);
-int for_each_ref_in_submodule(const char *submodule, const char *prefix,
-		each_ref_fn fn, void *cb_data);
-int for_each_tag_ref_submodule(const char *submodule,
-			       each_ref_fn fn, void *cb_data);
-int for_each_branch_ref_submodule(const char *submodule,
-				  each_ref_fn fn, void *cb_data);
 int for_each_remote_ref_submodule(const char *submodule,
 				  each_ref_fn fn, void *cb_data);
 
-- 
2.11.0.157.gd943d85

[PATCH v3 09/12] revision.c: --all adds HEAD from all worktrees

[Nguyễn Thái Ngọc Duy]

Unless single_worktree is set, --all now adds HEAD from all worktrees.

Since reachable.c code does not use setup_revisions(), we need to call
other_head_refs_submodule() explicitly there to have the same effect on
"git prune", so that we won't accidentally delete objects needed by some
other HEADs.

A new FIXME is added because we would need something like

    int refs_other_head_refs(struct ref_store *, each_ref_fn, cb_data);

in addition to other_head_refs() to handle it, which might require

    int get_submodule_worktrees(const char *submodule, int flags);

It could be a separate topic to reduce the scope of this one.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 reachable.c      |  1 +
 refs.c           | 22 ++++++++++++++++++++++
 refs.h           |  1 +
 revision.c       | 13 +++++++++++++
 submodule.c      |  2 ++
 t/t5304-prune.sh | 12 ++++++++++++
 6 files changed, 51 insertions(+)

diff --git a/reachable.c b/reachable.c
index a8a979bd4f..a3b938b46c 100644
--- a/reachable.c
+++ b/reachable.c
@@ -177,6 +177,7 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,
 
 	/* detached HEAD is not included in the list above */
 	head_ref(add_one_ref, revs);
+	other_head_refs(add_one_ref, revs);
 
 	/* Add all reflog info */
 	if (mark_reflog)
diff --git a/refs.c b/refs.c
index 537052f7ba..23e3607674 100644
--- a/refs.c
+++ b/refs.c
@@ -1780,3 +1780,25 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 {
 	return refs_rename_ref(get_main_ref_store(), oldref, newref, logmsg);
 }
+
+int other_head_refs(each_ref_fn fn, void *cb_data)
+{
+	struct worktree **worktrees, **p;
+	int ret = 0;
+
+	worktrees = get_worktrees(0);
+	for (p = worktrees; *p; p++) {
+		struct worktree *wt = *p;
+		struct ref_store *refs;
+
+		if (wt->is_current)
+			continue;
+
+		refs = get_worktree_ref_store(wt);
+		ret = refs_head_ref(refs, fn, cb_data);
+		if (ret)
+			break;
+	}
+	free_worktrees(worktrees);
+	return ret;
+}
diff --git a/refs.h b/refs.h
index e06db37118..cc71b6c7a0 100644
--- a/refs.h
+++ b/refs.h
@@ -247,6 +247,7 @@ int refs_for_each_remote_ref(struct ref_store *refs,
 			     each_ref_fn fn, void *cb_data);
 
 int head_ref(each_ref_fn fn, void *cb_data);
+int other_head_refs(each_ref_fn fn, void *cb_data);
 int for_each_ref(each_ref_fn fn, void *cb_data);
 int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data);
 int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,
diff --git a/revision.c b/revision.c
index c329070c89..040a0064f6 100644
--- a/revision.c
+++ b/revision.c
@@ -2105,6 +2105,13 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	int argcount;
 
 	if (submodule) {
+		/*
+		 * We need some something like get_submodule_worktrees()
+		 * before we can go through all worktrees of a submodule,
+		 * .e.g with adding all HEADs from --all, which is not
+		 * supported right now, so stick to single worktree.
+		 */
+		assert(revs->single_worktree != 0);
 		refs = get_submodule_ref_store(submodule);
 	} else
 		refs = get_main_ref_store();
@@ -2122,6 +2129,12 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	if (!strcmp(arg, "--all")) {
 		handle_refs(refs, revs, *flags, refs_for_each_ref);
 		handle_refs(refs, revs, *flags, refs_head_ref);
+		if (!revs->single_worktree) {
+			struct all_refs_cb cb;
+
+			init_all_refs_cb(&cb, revs, *flags);
+			other_head_refs(handle_one_ref, &cb);
+		}
 		clear_ref_exclusion(&revs->ref_excludes);
 	} else if (!strcmp(arg, "--branches")) {
 		handle_refs(refs, revs, *flags, refs_for_each_branch_ref);
diff --git a/submodule.c b/submodule.c
index a31f68812c..8c5af6e7f3 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1225,6 +1225,8 @@ static int find_first_merges(struct object_array *result, const char *path,
 			oid_to_hex(&a->object.oid));
 	init_revisions(&revs, NULL);
 	rev_opts.submodule = path;
+	/* FIXME: can't handle linked worktrees in submodules yet */
+	revs.single_worktree = path != NULL;
 	setup_revisions(ARRAY_SIZE(rev_args)-1, rev_args, &revs, &rev_opts);
 
 	/* save all revisions from the above list that contain b */
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index cba45c7be9..683bdb031c 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -292,4 +292,16 @@ test_expect_success 'prune: handle index in multiple worktrees' '
 	test_cmp second-worktree/blob actual
 '
 
+test_expect_success 'prune: handle HEAD in multiple worktrees' '
+	git worktree add --detach third-worktree &&
+	echo "new blob for third-worktree" >third-worktree/blob &&
+	git -C third-worktree add blob &&
+	git -C third-worktree commit -m "third" &&
+	rm .git/worktrees/third-worktree/index &&
+	test_must_fail git -C third-worktree show :blob &&
+	git prune --expire=now &&
+	git -C third-worktree show HEAD:blob >actual &&
+	test_cmp third-worktree/blob actual
+'
+
 test_done
-- 
2.11.0.157.gd943d85

[PATCH v3 10/12] files-backend: make reflog iterator go through per-worktree reflog

[Nguyễn Thái Ngọc Duy]

refs/bisect is unfortunately per-worktree, so we need to look in
per-worktree logs/refs/bisect in addition to per-repo logs/refs. The
current iterator only goes through per-repo logs/refs.

Ideally we should have something like merge_ref_iterator_begin (and
maybe with a predicate), but for dir_iterator. Since there's only one
use case for this pattern, let's not add a bunch more code for
merge_dir_iterator_begin just yet.

PS. Note the unsorted order of for_each_reflog in the test. This is
supposed to be OK, for now. If we enforce order on for_each_reflog()
then some more work will be required.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs/files-backend.c          | 46 ++++++++++++++++++++++++++++++++-----------
 t/t1407-worktree-ref-store.sh | 30 ++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 11 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4149943a6e..fce380679c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1171,15 +1171,6 @@ static void files_reflog_path(struct files_ref_store *refs,
 			      struct strbuf *sb,
 			      const char *refname)
 {
-	if (!refname) {
-		/*
-		 * FIXME: of course this is wrong in multi worktree
-		 * setting. To be fixed real soon.
-		 */
-		strbuf_addf(sb, "%s/logs", refs->gitcommondir);
-		return;
-	}
-
 	switch (ref_type(refname)) {
 	case REF_TYPE_PER_WORKTREE:
 	case REF_TYPE_PSEUDOREF:
@@ -3368,6 +3359,7 @@ struct files_reflog_iterator {
 
 	struct ref_store *ref_store;
 	struct dir_iterator *dir_iterator;
+	struct dir_iterator *worktree_dir_iterator;
 	struct object_id oid;
 };
 
@@ -3388,6 +3380,21 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
 		if (ends_with(diter->basename, ".lock"))
 			continue;
 
+		if (iter->worktree_dir_iterator) {
+			const char *refname = diter->relative_path;
+
+			switch (ref_type(refname)) {
+			case REF_TYPE_PER_WORKTREE:
+			case REF_TYPE_PSEUDOREF:
+				continue;
+			case REF_TYPE_NORMAL:
+				break;
+			default:
+				die("BUG: unknown ref type %d of ref %s",
+				    ref_type(refname), refname);
+			}
+		}
+
 		if (refs_read_ref_full(iter->ref_store,
 				       diter->relative_path, 0,
 				       iter->oid.hash, &flags)) {
@@ -3401,7 +3408,11 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
 		return ITER_OK;
 	}
 
-	iter->dir_iterator = NULL;
+	iter->dir_iterator = iter->worktree_dir_iterator;
+	if (iter->worktree_dir_iterator) {
+		iter->worktree_dir_iterator = NULL;
+		return files_reflog_iterator_advance(ref_iterator);
+	}
 	if (ref_iterator_abort(ref_iterator) == ITER_ERROR)
 		ok = ITER_ERROR;
 	return ok;
@@ -3422,6 +3433,12 @@ static int files_reflog_iterator_abort(struct ref_iterator *ref_iterator)
 	if (iter->dir_iterator)
 		ok = dir_iterator_abort(iter->dir_iterator);
 
+	if (iter->worktree_dir_iterator) {
+		int ok2 = dir_iterator_abort(iter->worktree_dir_iterator);
+		if (ok2 == ITER_ERROR)
+			ok = ok2;
+	}
+
 	base_ref_iterator_free(ref_iterator);
 	return ok;
 }
@@ -3442,10 +3459,17 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st
 	struct strbuf sb = STRBUF_INIT;
 
 	base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable);
-	files_reflog_path(refs, &sb, NULL);
+	strbuf_addf(&sb, "%s/logs", refs->gitcommondir);
 	iter->dir_iterator = dir_iterator_begin(sb.buf);
 	iter->ref_store = ref_store;
 	strbuf_release(&sb);
+
+	if (strcmp(refs->gitdir, refs->gitcommondir)) {
+		strbuf_addf(&sb, "%s/logs", refs->gitdir);
+		iter->worktree_dir_iterator = dir_iterator_begin(sb.buf);
+		strbuf_release(&sb);
+	}
+
 	return ref_iterator;
 }
 
diff --git a/t/t1407-worktree-ref-store.sh b/t/t1407-worktree-ref-store.sh
index 5df06f3556..8842d0329f 100755
--- a/t/t1407-worktree-ref-store.sh
+++ b/t/t1407-worktree-ref-store.sh
@@ -49,4 +49,34 @@ test_expect_success 'create_symref(FOO, refs/heads/master)' '
 	test_cmp expected actual
 '
 
+test_expect_success 'for_each_reflog()' '
+	echo $_z40 > .git/logs/PSEUDO-MAIN &&
+	mkdir -p     .git/logs/refs/bisect &&
+	echo $_z40 > .git/logs/refs/bisect/random &&
+
+	echo $_z40 > .git/worktrees/wt/logs/PSEUDO-WT &&
+	mkdir -p     .git/worktrees/wt/logs/refs/bisect &&
+	echo $_z40 > .git/worktrees/wt/logs/refs/bisect/wt-random &&
+
+	$RWT for-each-reflog | cut -c 42- | sort >actual &&
+	cat >expected <<-\EOF &&
+	HEAD 0x1
+	PSEUDO-WT 0x0
+	refs/bisect/wt-random 0x0
+	refs/heads/master 0x0
+	refs/heads/wt-master 0x0
+	EOF
+	test_cmp expected actual &&
+
+	$RMAIN for-each-reflog | cut -c 42- | sort >actual &&
+	cat >expected <<-\EOF &&
+	HEAD 0x1
+	PSEUDO-MAIN 0x0
+	refs/bisect/random 0x0
+	refs/heads/master 0x0
+	refs/heads/wt-master 0x0
+	EOF
+	test_cmp expected actual
+'
+
 test_done
-- 
2.11.0.157.gd943d85

[PATCH v3 11/12] revision.c: --reflog add HEAD reflog from all worktrees

[Nguyễn Thái Ngọc Duy]

Note that add_other_reflogs_to_pending() is a bit inefficient, since
it scans reflog for all refs of each worktree, including shared refs,
so the shared ref's reflog is scanned over and over again.

We could update refs API to pass "per-worktree only" flag to avoid
that. But long term we should be able to obtain a "per-worktree only"
ref store and would need to revert the changes in reflog iteration
API. So let's just wait until then.

add_reflogs_to_pending() is called by reachable.c so by default "git
prune" will examine reflog from all worktrees.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 revision.c       | 28 +++++++++++++++++++++++++++-
 t/t5304-prune.sh | 16 ++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index 040a0064f6..f4bc9bc65c 100644
--- a/revision.c
+++ b/revision.c
@@ -1134,6 +1134,7 @@ struct all_refs_cb {
 	int warned_bad_reflog;
 	struct rev_info *all_revs;
 	const char *name_for_errormsg;
+	struct ref_store *refs;
 };
 
 int ref_excluded(struct string_list *ref_excludes, const char *path)
@@ -1169,6 +1170,7 @@ static void init_all_refs_cb(struct all_refs_cb *cb, struct rev_info *revs,
 {
 	cb->all_revs = revs;
 	cb->all_flags = flags;
+	cb->refs = NULL;
 }
 
 void clear_ref_exclusion(struct string_list **ref_excludes_p)
@@ -1237,17 +1239,41 @@ static int handle_one_reflog(const char *path, const struct object_id *oid,
 	struct all_refs_cb *cb = cb_data;
 	cb->warned_bad_reflog = 0;
 	cb->name_for_errormsg = path;
-	for_each_reflog_ent(path, handle_one_reflog_ent, cb_data);
+	refs_for_each_reflog_ent(cb->refs, path,
+				 handle_one_reflog_ent, cb_data);
 	return 0;
 }
 
+static void add_other_reflogs_to_pending(struct all_refs_cb *cb)
+{
+	struct worktree **worktrees, **p;
+
+	worktrees = get_worktrees(0);
+	for (p = worktrees; *p; p++) {
+		struct worktree *wt = *p;
+
+		if (wt->is_current)
+			continue;
+
+		cb->refs = get_worktree_ref_store(wt);
+		refs_for_each_reflog(cb->refs,
+				     handle_one_reflog,
+				     cb);
+	}
+	free_worktrees(worktrees);
+}
+
 void add_reflogs_to_pending(struct rev_info *revs, unsigned flags)
 {
 	struct all_refs_cb cb;
 
 	cb.all_revs = revs;
 	cb.all_flags = flags;
+	cb.refs = get_main_ref_store();
 	for_each_reflog(handle_one_reflog, &cb);
+
+	if (!revs->single_worktree)
+		add_other_reflogs_to_pending(&cb);
 }
 
 static void add_cache_tree(struct cache_tree *it, struct rev_info *revs,
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 683bdb031c..6694c19a1e 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -304,4 +304,20 @@ test_expect_success 'prune: handle HEAD in multiple worktrees' '
 	test_cmp third-worktree/blob actual
 '
 
+test_expect_success 'prune: handle HEAD reflog in multiple worktrees' '
+	git config core.logAllRefUpdates true &&
+	echo "lost blob for third-worktree" >expected &&
+	(
+		cd third-worktree &&
+		cat ../expected >blob &&
+		git add blob &&
+		git commit -m "second commit in third" &&
+		git reset --hard HEAD^
+	) &&
+	git prune --expire=now &&
+	SHA1=`git hash-object expected` &&
+	git -C third-worktree show "$SHA1" >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.11.0.157.gd943d85

[PATCH v3 12/12] rev-list: expose and document --single-worktree

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/rev-list-options.txt | 8 ++++++++
 revision.c                         | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index a02f7324c0..c71e94b2d0 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -179,6 +179,14 @@ explicitly.
 	Pretend as if all objects mentioned by reflogs are listed on the
 	command line as `<commit>`.
 
+--single-worktree::
+	By default, all working trees will be examined by the
+	following options when there are more than one (see
+	linkgit:git-worktree[1]): `--all`, `--reflog` and
+	`--indexed-objects`.
+	This option forces them to examine the current working tree
+	only.
+
 --ignore-missing::
 	Upon seeing an invalid object name in the input, pretend as if
 	the bad input was not given.
diff --git a/revision.c b/revision.c
index f4bc9bc65c..79ce8a007f 100644
--- a/revision.c
+++ b/revision.c
@@ -2222,6 +2222,8 @@ static int handle_revision_pseudo_opt(const char *submodule,
 			return error("invalid argument to --no-walk");
 	} else if (!strcmp(arg, "--do-walk")) {
 		revs->no_walk = 0;
+	} else if (!strcmp(arg, "--single-worktree")) {
+		revs->single_worktree = 1;
 	} else {
 		return 0;
 	}
-- 
2.11.0.157.gd943d85

Re: [PATCH v3 08/12] refs: remove dead for_each_*_submodule()

[Ramsay Jones]

On 19/04/17 12:01, Nguyễn Thái Ngọc Duy wrote:
> These are used in revision.c. After the last patch they are replaced
> with the refs_ version. Delete them (except for_each_remote_ref_submodule
> which is still used by submodule.c)
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation/technical/api-ref-iteration.txt |  7 ++-----
>  refs.c                                        | 29 ---------------------------
>  refs.h                                        |  9 ---------
>  3 files changed, 2 insertions(+), 43 deletions(-)
> 
> diff --git a/Documentation/technical/api-ref-iteration.txt b/Documentation/technical/api-ref-iteration.txt
> index 37379d8337..c9e9a60dbd 100644
> --- a/Documentation/technical/api-ref-iteration.txt
> +++ b/Documentation/technical/api-ref-iteration.txt
> @@ -32,11 +32,8 @@ Iteration functions
>  
>  * `for_each_glob_ref_in()` the previous and `for_each_ref_in()` combined.
>  
> -* `head_ref_submodule()`, `for_each_ref_submodule()`,
> -  `for_each_ref_in_submodule()`, `for_each_tag_ref_submodule()`,
> -  `for_each_branch_ref_submodule()`, `for_each_remote_ref_submodule()`
> -  do the same as the functions described above but for a specified
> -  submodule.
> +* Use `refs_` API for accessing submodules. The submodule ref store could
> +  be obtained with `get_submodule_ref_store().

missing ` ? ------------------------------------^

ATB,
Ramsay Jones

Re: [PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store

[Johannes Sixt]

Am 19.04.2017 um 13:01 schrieb Nguyễn Thái Ngọc Duy:
> @@ -1558,7 +1543,17 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
>  {
>  	struct strbuf submodule_sb = STRBUF_INIT;
>  	struct ref_store *refs;
> +	char *to_free = NULL;
>  	int ret;
> +	size_t len;
> +
> +	if (submodule) {
> +		len = strlen(submodule);
> +		while (len && submodule[len - 1] == '/')

What is the source of the value of 'submodule'? Is it an index entry? Or 
did it pass through parse_pathspec? In these cases it is correct to 
compare against literal '/'. Otherwise, is_dir_sep() is preferred.

> +			len--;
> +		if (!len)
> +			submodule = NULL;
> +	}
>
>  	if (!submodule || !*submodule) {
>  		/*

-- Hannes

Re: [PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store

[Duy Nguyen]

On Thu, Apr 20, 2017 at 12:02:08AM +0200, Johannes Sixt wrote:
> Am 19.04.2017 um 13:01 schrieb Nguyễn Thái Ngọc Duy:
> > @@ -1558,7 +1543,17 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
> >  {
> >  	struct strbuf submodule_sb = STRBUF_INIT;
> >  	struct ref_store *refs;
> > +	char *to_free = NULL;
> >  	int ret;
> > +	size_t len;
> > +
> > +	if (submodule) {
> > +		len = strlen(submodule);
> > +		while (len && submodule[len - 1] == '/')
> 
> What is the source of the value of 'submodule'? Is it an index entry? Or 
> did it pass through parse_pathspec? In these cases it is correct to 
> compare against literal '/'. Otherwise, is_dir_sep() is preferred.

This is a code move from resolve_gitlink_ref(), which goes back to
0ebde32c87 (Add 'resolve_gitlink_ref()' helper function - 2007-04-09)
and it looks like a dir separator back then.

Can I convert that in a separate topic? I think Michael even wanted to
kill all these path manipulation in refs code, which makes sense, but
I would need to audit the callers carefully before making that move.
--
Duy

Re: [PATCH v3 08/12] refs: remove dead for_each_*_submodule()

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> On 19/04/17 12:01, Nguyễn Thái Ngọc Duy wrote:
>> These are used in revision.c. After the last patch they are replaced
>> with the refs_ version. Delete them (except for_each_remote_ref_submodule
>> which is still used by submodule.c)
>> 
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  Documentation/technical/api-ref-iteration.txt |  7 ++-----
>>  refs.c                                        | 29 ---------------------------
>>  refs.h                                        |  9 ---------
>>  3 files changed, 2 insertions(+), 43 deletions(-)
>> 
>> diff --git a/Documentation/technical/api-ref-iteration.txt b/Documentation/technical/api-ref-iteration.txt
>> index 37379d8337..c9e9a60dbd 100644
>> --- a/Documentation/technical/api-ref-iteration.txt
>> +++ b/Documentation/technical/api-ref-iteration.txt
>> @@ -32,11 +32,8 @@ Iteration functions
>>  
>>  * `for_each_glob_ref_in()` the previous and `for_each_ref_in()` combined.
>>  
>> -* `head_ref_submodule()`, `for_each_ref_submodule()`,
>> -  `for_each_ref_in_submodule()`, `for_each_tag_ref_submodule()`,
>> -  `for_each_branch_ref_submodule()`, `for_each_remote_ref_submodule()`
>> -  do the same as the functions described above but for a specified
>> -  submodule.
>> +* Use `refs_` API for accessing submodules. The submodule ref store could
>> +  be obtained with `get_submodule_ref_store().
>
> missing ` ? ------------------------------------^

Indeed.  Will tweak while queuing.

Re: [PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store

[Duy Nguyen]

On Thu, Apr 20, 2017 at 5:02 AM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 19.04.2017 um 13:01 schrieb Nguyễn Thái Ngọc Duy:
>>
>> @@ -1558,7 +1543,17 @@ struct ref_store *get_submodule_ref_store(const
>> char *submodule)
>>  {
>>         struct strbuf submodule_sb = STRBUF_INIT;
>>         struct ref_store *refs;
>> +       char *to_free = NULL;
>>         int ret;
>> +       size_t len;
>> +
>> +       if (submodule) {
>> +               len = strlen(submodule);
>> +               while (len && submodule[len - 1] == '/')
>
>
> What is the source of the value of 'submodule'? Is it an index entry? Or did
> it pass through parse_pathspec? In these cases it is correct to compare
> against literal '/'. Otherwise, is_dir_sep() is preferred.

I've looked at the callers. Yes it is a path and is_dir_sep() should
be used. Since this has been like this in the current code, unless
there's some more changes in this series or you insist, I would hold
this change back, wait for this series to settle and submit it later
(I'll have to do that anyway to kill the get_main_store() call in this
function).
-- 
Duy

Re: [PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store

[Johannes Sixt]

Am 20.04.2017 um 13:56 schrieb Duy Nguyen:
> On Thu, Apr 20, 2017 at 5:02 AM, Johannes Sixt <j6t@kdbg.org> wrote:
>> What is the source of the value of 'submodule'? Is it an index entry? Or did
>> it pass through parse_pathspec? In these cases it is correct to compare
>> against literal '/'. Otherwise, is_dir_sep() is preferred.
>
> I've looked at the callers. Yes it is a path and is_dir_sep() should
> be used. Since this has been like this in the current code, unless
> there's some more changes in this series or you insist, I would hold
> this change back,

I won't insist because we have it the old way since a decade without 
issue reports.

-- Hannes

Re: [PATCH v3 04/12] refs.c: refactor get_submodule_ref_store(), share common free block

[Michael Haggerty]

On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  refs.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 5d31fb6bcf..5902a3d9e5 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1570,19 +1570,16 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
>  
>  	refs = lookup_ref_store_map(&submodule_ref_stores, submodule);
>  	if (refs)
> -		return refs;
> +		goto done;
>  
>  	strbuf_addstr(&submodule_sb, submodule);
>  	ret = is_nonbare_repository_dir(&submodule_sb);
> -	strbuf_release(&submodule_sb);
>  	if (!ret)
> -		return NULL;
> +		goto done;
>  
>  	ret = submodule_to_gitdir(&submodule_sb, submodule);
> -	if (ret) {
> -		strbuf_release(&submodule_sb);
> -		return NULL;
> -	}
> +	if (ret)
> +		goto done;

After this change, the temporary variable `ret` could be eliminated.

> [...]

Michael

Re: [PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store

[Michael Haggerty]

On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:
> This is a better place that will benefit all submodule callers instead
> of just resolve_gitlink_ref()
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  refs.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 5902a3d9e5..26474cb62a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1422,25 +1422,10 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
>  int resolve_gitlink_ref(const char *submodule, const char *refname,
>  			unsigned char *sha1)
>  {
> -	size_t len = strlen(submodule);
>  	struct ref_store *refs;
>  	int flags;
>  
> -	while (len && submodule[len - 1] == '/')
> -		len--;
> -
> -	if (!len)
> -		return -1;
> -
> -	if (submodule[len]) {
> -		/* We need to strip off one or more trailing slashes */
> -		char *stripped = xmemdupz(submodule, len);
> -
> -		refs = get_submodule_ref_store(stripped);
> -		free(stripped);
> -	} else {
> -		refs = get_submodule_ref_store(submodule);
> -	}
> +	refs = get_submodule_ref_store(submodule);
>  
>  	if (!refs)
>  		return -1;
> @@ -1558,7 +1543,17 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
>  {
>  	struct strbuf submodule_sb = STRBUF_INIT;
>  	struct ref_store *refs;
> +	char *to_free = NULL;
>  	int ret;
> +	size_t len;
> +
> +	if (submodule) {
> +		len = strlen(submodule);
> +		while (len && submodule[len - 1] == '/')
> +			len--;
> +		if (!len)
> +			submodule = NULL;
> +	}

Ugh. Should a submodule named "///" *really* be considered to refer to
the main ref_store? I understand that's what the code did before this
patch, but it seems to me more like an accident of the old design rather
than something worth supporting. In other words, if a caller would
really pass us such a string, it seems like we could declare the caller
buggy, no?

> [...]

Otherwise, looks good and makes a lot of sense.

Michael

Re: [PATCH v3 08/12] refs: remove dead for_each_*_submodule()

[Michael Haggerty]

On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:
> These are used in revision.c. After the last patch they are replaced
> with the refs_ version. Delete them (except for_each_remote_ref_submodule
> which is still used by submodule.c)

❤❤❤❤❤❤

I love the way this is going.

Michael

Re: [PATCH v3 09/12] revision.c: --all adds HEAD from all worktrees

[Michael Haggerty]

On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:
> Unless single_worktree is set, --all now adds HEAD from all worktrees.
> 
> Since reachable.c code does not use setup_revisions(), we need to call
> other_head_refs_submodule() explicitly there to have the same effect on
> "git prune", so that we won't accidentally delete objects needed by some
> other HEADs.
> 
> A new FIXME is added because we would need something like
> 
>     int refs_other_head_refs(struct ref_store *, each_ref_fn, cb_data);
> 
> in addition to other_head_refs() to handle it, which might require
> 
>     int get_submodule_worktrees(const char *submodule, int flags);
> 
> It could be a separate topic to reduce the scope of this one.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  reachable.c      |  1 +
>  refs.c           | 22 ++++++++++++++++++++++
>  refs.h           |  1 +
>  revision.c       | 13 +++++++++++++
>  submodule.c      |  2 ++
>  t/t5304-prune.sh | 12 ++++++++++++
>  6 files changed, 51 insertions(+)
> 
> diff --git a/reachable.c b/reachable.c
> index a8a979bd4f..a3b938b46c 100644
> --- a/reachable.c
> +++ b/reachable.c
> @@ -177,6 +177,7 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,
>  
>  	/* detached HEAD is not included in the list above */
>  	head_ref(add_one_ref, revs);
> +	other_head_refs(add_one_ref, revs);
>  
>  	/* Add all reflog info */
>  	if (mark_reflog)
> diff --git a/refs.c b/refs.c
> index 537052f7ba..23e3607674 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1780,3 +1780,25 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
>  {
>  	return refs_rename_ref(get_main_ref_store(), oldref, newref, logmsg);
>  }
> +
> +int other_head_refs(each_ref_fn fn, void *cb_data)
> +{
> +	struct worktree **worktrees, **p;
> +	int ret = 0;
> +
> +	worktrees = get_worktrees(0);
> +	for (p = worktrees; *p; p++) {
> +		struct worktree *wt = *p;
> +		struct ref_store *refs;
> +
> +		if (wt->is_current)
> +			continue;
> +
> +		refs = get_worktree_ref_store(wt);
> +		ret = refs_head_ref(refs, fn, cb_data);
> +		if (ret)
> +			break;
> +	}
> +	free_worktrees(worktrees);
> +	return ret;
> +}

This function is mainly about iterating through all worktrees. Therefore
it feels out of place in the refs module. But I don't have a strong
feeling about it.

> diff --git a/refs.h b/refs.h
> index e06db37118..cc71b6c7a0 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -247,6 +247,7 @@ int refs_for_each_remote_ref(struct ref_store *refs,
>  			     each_ref_fn fn, void *cb_data);
>  
>  int head_ref(each_ref_fn fn, void *cb_data);
> +int other_head_refs(each_ref_fn fn, void *cb_data);
>  int for_each_ref(each_ref_fn fn, void *cb_data);
>  int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data);
>  int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,
> diff --git a/revision.c b/revision.c
> index c329070c89..040a0064f6 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2105,6 +2105,13 @@ static int handle_revision_pseudo_opt(const char *submodule,
>  	int argcount;
>  
>  	if (submodule) {
> +		/*
> +		 * We need some something like get_submodule_worktrees()
> +		 * before we can go through all worktrees of a submodule,
> +		 * .e.g with adding all HEADs from --all, which is not
> +		 * supported right now, so stick to single worktree.
> +		 */
> +		assert(revs->single_worktree != 0);

You don't need `!= 0` above.

We usually don't use `assert(foo)` but rather `if (!foo)
die("BUG:...")`, which gives a better error message and isn't switched
off if a distribution compiles with NDEBUG.

But here I'm confused about whether this check failing really indicates
a bug within git, or whether it could indicate a situation that the user
has set up that git just can't handle right now. For example, maybe the
user will set up a submodule that itself uses worktrees (even if that's
not possible now, maybe they will have done it with some future version
of git or with another tool). If the problem is only that this version
of git is too stupid to handle pruning safely in that situation, it
would be more appropriate to use something more like

	if (!refs->single_worktree)
		die("error: git is currently unable to handle submodules that use
linked worktrees");

>  		refs = get_submodule_ref_store(submodule);
>  	} else
>  		refs = get_main_ref_store();
> [...]

Michael

Re: [PATCH v3 06/12] refs: add refs_head_ref()

[Michael Haggerty]

On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  refs.c | 19 +++++++++----------
>  refs.h |  2 ++
>  2 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 26474cb62a..a252ae43ee 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1208,27 +1208,26 @@ int refs_rename_ref_available(struct ref_store *refs,
>  	return ok;
>  }
>  
> -int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
> +int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
>  {
>  	struct object_id oid;
>  	int flag;
>  
> -	if (submodule) {
> -		if (resolve_gitlink_ref(submodule, "HEAD", oid.hash) == 0)
> -			return fn("HEAD", &oid, 0, cb_data);
> -
> -		return 0;
> -	}
> -
> -	if (!read_ref_full("HEAD", RESOLVE_REF_READING, oid.hash, &flag))
> +	if (!refs_read_ref_full(refs, "HEAD", RESOLVE_REF_READING,
> +				oid.hash, &flag))
>  		return fn("HEAD", &oid, flag, cb_data);
>  
>  	return 0;
>  }
>  
> +int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
> +{
> +	return refs_head_ref(get_submodule_ref_store(submodule), fn, cb_data);
> +}
> +
>  int head_ref(each_ref_fn fn, void *cb_data)
>  {
> -	return head_ref_submodule(NULL, fn, cb_data);
> +	return refs_head_ref(get_main_ref_store(), fn, cb_data);
>  }
>  
>  /*
> diff --git a/refs.h b/refs.h
> index 447381d378..0572473ef7 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -233,6 +233,8 @@ typedef int each_ref_fn(const char *refname,
>   * modifies the reference also returns a nonzero value to immediately
>   * stop the iteration. Returned references are sorted.
>   */
> +int refs_head_ref(struct ref_store *refs,
> +		  each_ref_fn fn, void *cb_data);
>  int refs_for_each_ref(struct ref_store *refs,
>  		      each_ref_fn fn, void *cb_data);
>  int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,
> 

I'm seeing segfaults in t3600 after this patch, apparently because
`refs==NULL` gets passed from `head_ref_submodule()` to `refs_head_ref()`.

Michael

Re: [PATCH v3 10/12] files-backend: make reflog iterator go through per-worktree reflog

[Michael Haggerty]

On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:
> refs/bisect is unfortunately per-worktree, so we need to look in
> per-worktree logs/refs/bisect in addition to per-repo logs/refs. The
> current iterator only goes through per-repo logs/refs.
> 
> Ideally we should have something like merge_ref_iterator_begin (and
> maybe with a predicate), but for dir_iterator. Since there's only one
> use case for this pattern, let's not add a bunch more code for
> merge_dir_iterator_begin just yet.
> 
> PS. Note the unsorted order of for_each_reflog in the test. This is
> supposed to be OK, for now. If we enforce order on for_each_reflog()
> then some more work will be required.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  refs/files-backend.c          | 46 ++++++++++++++++++++++++++++++++-----------
>  t/t1407-worktree-ref-store.sh | 30 ++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+), 11 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 4149943a6e..fce380679c 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1171,15 +1171,6 @@ static void files_reflog_path(struct files_ref_store *refs,
>  			      struct strbuf *sb,
>  			      const char *refname)
>  {
> -	if (!refname) {
> -		/*
> -		 * FIXME: of course this is wrong in multi worktree
> -		 * setting. To be fixed real soon.
> -		 */
> -		strbuf_addf(sb, "%s/logs", refs->gitcommondir);
> -		return;
> -	}
> -
>  	switch (ref_type(refname)) {
>  	case REF_TYPE_PER_WORKTREE:
>  	case REF_TYPE_PSEUDOREF:
> @@ -3368,6 +3359,7 @@ struct files_reflog_iterator {
>  
>  	struct ref_store *ref_store;
>  	struct dir_iterator *dir_iterator;
> +	struct dir_iterator *worktree_dir_iterator;
>  	struct object_id oid;
>  };
>  
> @@ -3388,6 +3380,21 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
>  		if (ends_with(diter->basename, ".lock"))
>  			continue;
>  
> +		if (iter->worktree_dir_iterator) {
> +			const char *refname = diter->relative_path;
> +
> +			switch (ref_type(refname)) {
> +			case REF_TYPE_PER_WORKTREE:
> +			case REF_TYPE_PSEUDOREF:
> +				continue;
> +			case REF_TYPE_NORMAL:
> +				break;
> +			default:
> +				die("BUG: unknown ref type %d of ref %s",
> +				    ref_type(refname), refname);
> +			}
> +		}
> +
>  		if (refs_read_ref_full(iter->ref_store,
>  				       diter->relative_path, 0,
>  				       iter->oid.hash, &flags)) {
> @@ -3401,7 +3408,11 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
>  		return ITER_OK;
>  	}
>  
> -	iter->dir_iterator = NULL;
> +	iter->dir_iterator = iter->worktree_dir_iterator;
> +	if (iter->worktree_dir_iterator) {
> +		iter->worktree_dir_iterator = NULL;
> +		return files_reflog_iterator_advance(ref_iterator);
> +	}

I find this implementation confusing:

* `if (iter->worktree_dir_iterator)` sounds like it should mean
  that we are iterating over worktree references but it really means
  that we are iterating over the common references in a repository
  that is a linked worktree.
* `files_reflog_iterator_advance()` is called recursively, but only
  for the first worktree reference.
* `iter->worktree_dir_iterator` is moved over to `iter->dir_iterator`
  when the common refs are exhausted.

Do you find it more readable as follows?:

static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
{
	struct files_reflog_iterator *iter =
		(struct files_reflog_iterator *)ref_iterator;
	int ok;

	while (1) {
		struct dir_iterator **diter;
		int normal_only, flags;

		if (iter->dir_iterator) {
			diter = &iter->dir_iterator;
			/*
			 * If we are in a worktree, then we only
			 * include "normal" common references:
			 */
			normal_only = !!iter->worktree_dir_iterator;
		} else if (iter->worktree_dir_iterator) {
			diter = &iter->worktree_dir_iterator;
			normal_only = 0;
		} else {
			ok = ITER_DONE;
			break;
		}

		ok = dir_iterator_advance(*diter);
		if (ok == ITER_ERROR) {
			*diter = NULL;
			break;
		} else if (ok == ITER_DONE) {
			*diter = NULL;
			/* There might still be worktree refs left: */
			continue;
		}

		if (!S_ISREG((*diter)->st.st_mode))
			continue;
		if ((*diter)->basename[0] == '.')
			continue;
		if (ends_with((*diter)->basename, ".lock"))
			continue;

		iter->base.refname = (*diter)->relative_path;

		if (normal_only &&
		    ref_type(iter->base.refname) != REF_TYPE_NORMAL)
			continue;

		if (refs_read_ref_full(iter->ref_store,
				       iter->base.refname, 0,
				       iter->oid.hash, &flags)) {
			error("bad ref for %s", (*diter)->path.buf);
			continue;
		}

		iter->base.oid = &iter->oid;
		iter->base.flags = flags;
		return ITER_OK;
	}

	if (ref_iterator_abort(ref_iterator) == ITER_ERROR)
		return ITER_ERROR;

	return ok;
}

>  	if (ref_iterator_abort(ref_iterator) == ITER_ERROR)
>  		ok = ITER_ERROR;
>  	return ok;
> [...]

Michael

Re: [PATCH v3 00/12] Fix git-gc losing objects in multi worktree

[Michael Haggerty]

On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:
> Changes since v2 [1] is relatively small. It still needs
> nd/worktree-kill-parse-ref of course.

I read the whole series. Aside from the comments I made, it looks good
to me.

This will be able to be cleaned up once we have a cleaner distinction
between "logical" and "physical" ref_stores. But given the current state
of the code, your implementation is reasonable.

Michael

Re: [PATCH v3 06/12] refs: add refs_head_ref()

[Duy Nguyen]

On Sat, Apr 22, 2017 at 1:37 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  refs.c | 19 +++++++++----------
>>  refs.h |  2 ++
>>  2 files changed, 11 insertions(+), 10 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index 26474cb62a..a252ae43ee 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1208,27 +1208,26 @@ int refs_rename_ref_available(struct ref_store *refs,
>>       return ok;
>>  }
>>
>> -int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
>> +int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
>>  {
>>       struct object_id oid;
>>       int flag;
>>
>> -     if (submodule) {
>> -             if (resolve_gitlink_ref(submodule, "HEAD", oid.hash) == 0)
>> -                     return fn("HEAD", &oid, 0, cb_data);
>> -
>> -             return 0;
>> -     }
>> -
>> -     if (!read_ref_full("HEAD", RESOLVE_REF_READING, oid.hash, &flag))
>> +     if (!refs_read_ref_full(refs, "HEAD", RESOLVE_REF_READING,
>> +                             oid.hash, &flag))
>>               return fn("HEAD", &oid, flag, cb_data);
>>
>>       return 0;
>>  }
>>
>> +int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
>> +{
>> +     return refs_head_ref(get_submodule_ref_store(submodule), fn, cb_data);
>> +}
>> +
>>  int head_ref(each_ref_fn fn, void *cb_data)
>>  {
>> -     return head_ref_submodule(NULL, fn, cb_data);
>> +     return refs_head_ref(get_main_ref_store(), fn, cb_data);
>>  }
>>
>>  /*
>> diff --git a/refs.h b/refs.h
>> index 447381d378..0572473ef7 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -233,6 +233,8 @@ typedef int each_ref_fn(const char *refname,
>>   * modifies the reference also returns a nonzero value to immediately
>>   * stop the iteration. Returned references are sorted.
>>   */
>> +int refs_head_ref(struct ref_store *refs,
>> +               each_ref_fn fn, void *cb_data);
>>  int refs_for_each_ref(struct ref_store *refs,
>>                     each_ref_fn fn, void *cb_data);
>>  int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,
>>
>
> I'm seeing segfaults in t3600 after this patch, apparently because
> `refs==NULL` gets passed from `head_ref_submodule()` to `refs_head_ref()`.

Eventually it's the caller's responsibility not to pass NULL ref store
to refs_* functions. That happens in "revision.c: use refs_for_each*()
instead of for_each_*_submodule()" patch, which is too late and would
break bisect. Will fix.
-- 
Duy

Re: [PATCH v3 10/12] files-backend: make reflog iterator go through per-worktree reflog

[Duy Nguyen]

On Sat, Apr 22, 2017 at 10:05:02AM +0200, Michael Haggerty wrote:
> I find this implementation confusing:
> 
> * `if (iter->worktree_dir_iterator)` sounds like it should mean
>   that we are iterating over worktree references but it really means
>   that we are iterating over the common references in a repository
>   that is a linked worktree.
> * `files_reflog_iterator_advance()` is called recursively, but only
>   for the first worktree reference.
> * `iter->worktree_dir_iterator` is moved over to `iter->dir_iterator`
>   when the common refs are exhausted.
> 
> Do you find it more readable as follows?:

It's a bit better, but while we're at it, why not take full advantage
of iterator abstraction?

This replacement patch (with some unrelated bits removed to reduce
distraction) adds a new meta ref-iterator that combine a per-repo and
a per-worktree iterators together. The new iterator walks through both
sub-iterators and drop the per-worktree results from the per-repo
iterator, which will be replaced with results from per-worktree
iterator.

You probably see where I'm going with this. When the new "linked
worktree ref store" comes, it will combine two per-worktree and
per-repo ref stores together and this iterator will come handy.

At that point, files-backend can go back to being oblivious about
$GIT_DIR vs $GIT_COMMON_DIR and files_reflog_iterator_begin() will be
reverted back to the version before this patch.

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4149943a6e..817b7b5d5e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3432,23 +3423,37 @@ static struct ref_iterator_vtable files_reflog_iterator_vtable = {
 	files_reflog_iterator_abort
 };
 
-static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_store)
+static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
+						  const char *gitdir)
 {
-	struct files_ref_store *refs =
-		files_downcast(ref_store, REF_STORE_READ,
-			       "reflog_iterator_begin");
 	struct files_reflog_iterator *iter = xcalloc(1, sizeof(*iter));
 	struct ref_iterator *ref_iterator = &iter->base;
 	struct strbuf sb = STRBUF_INIT;
 
 	base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable);
-	files_reflog_path(refs, &sb, NULL);
+	strbuf_addf(&sb, "%s/logs", gitdir);
 	iter->dir_iterator = dir_iterator_begin(sb.buf);
 	iter->ref_store = ref_store;
 	strbuf_release(&sb);
+
 	return ref_iterator;
 }
 
+static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_store)
+{
+	struct files_ref_store *refs =
+		files_downcast(ref_store, REF_STORE_READ,
+			       "reflog_iterator_begin");
+
+	if (!strcmp(refs->gitdir, refs->gitcommondir)) {
+		return reflog_iterator_begin(ref_store, refs->gitcommondir);
+	} else {
+		return worktree_ref_iterator_begin(
+			reflog_iterator_begin(ref_store, refs->gitcommondir),
+			reflog_iterator_begin(ref_store, refs->gitdir));
+	}
+}
+
 static int ref_update_reject_duplicates(struct string_list *refnames,
 					struct strbuf *err)
 {
diff --git a/refs/iterator.c b/refs/iterator.c
index bce1f192f7..93243a00c4 100644
--- a/refs/iterator.c
+++ b/refs/iterator.c
@@ -40,6 +40,14 @@ void base_ref_iterator_free(struct ref_iterator *iter)
 	free(iter);
 }
 
+static void ref_iterator_copy_result(struct ref_iterator *dst,
+				     const struct ref_iterator *src)
+{
+	dst->refname = src->refname;
+	dst->oid = src->oid;
+	dst->flags = src->flags;
+}
+
 struct empty_ref_iterator {
 	struct ref_iterator base;
 };
@@ -382,3 +390,100 @@ int do_for_each_ref_iterator(struct ref_iterator *iter,
 		return -1;
 	return retval;
 }
+
+struct worktree_ref_iterator {
+	struct ref_iterator base;
+	struct ref_iterator *per_repo_iterator;
+	struct ref_iterator *per_worktree_iterator;
+};
+
+static int worktree_ref_iterator_advance(struct ref_iterator *ref_iterator)
+{
+	struct worktree_ref_iterator *iter =
+		(struct worktree_ref_iterator *)ref_iterator;
+	int ok;
+
+	while (1) {
+		struct ref_iterator **subiter;
+		int normal_only;
+
+		if (iter->per_repo_iterator) {
+			subiter = &iter->per_repo_iterator;
+			/*
+			 * If we are in a worktree, then we only
+			 * include "normal" common references:
+			 */
+			normal_only = !!iter->per_worktree_iterator;
+		} else if (iter->per_worktree_iterator) {
+			subiter = &iter->per_worktree_iterator;
+			normal_only = 0;
+		} else {
+			ok = ITER_DONE;
+			break;
+		}
+
+		ok = ref_iterator_advance(*subiter);
+		if (ok == ITER_ERROR) {
+			*subiter = NULL;
+			break;
+		} else if (ok == ITER_DONE) {
+			*subiter = NULL;
+			/* There might still be worktree refs left: */
+			continue;
+		}
+
+		if (normal_only &&
+		    ref_type((*subiter)->refname) != REF_TYPE_NORMAL)
+			continue;
+
+		ref_iterator_copy_result(&iter->base, *subiter);
+		return ITER_OK;
+	}
+
+	if (ref_iterator_abort(ref_iterator) == ITER_ERROR)
+		return ITER_ERROR;
+
+	return ok;
+}
+
+static int worktree_ref_iterator_peel(struct ref_iterator *ref_iterator,
+					 struct object_id *peeled)
+{
+	die("BUG: ref_iterator_peel() called for reflog_iterator");
+}
+
+static int worktree_ref_iterator_abort(struct ref_iterator *ref_iterator)
+{
+	struct worktree_ref_iterator *iter =
+		(struct worktree_ref_iterator *)ref_iterator;
+	int ok = ITER_DONE;
+
+	if (iter->per_repo_iterator)
+		ok = ref_iterator_abort(iter->per_repo_iterator);
+
+	if (iter->per_worktree_iterator) {
+		int ok2 = ref_iterator_abort(iter->per_worktree_iterator);
+		if (ok2 == ITER_ERROR)
+			ok = ok2;
+	}
+
+	base_ref_iterator_free(ref_iterator);
+	return ok;
+}
+
+static struct ref_iterator_vtable worktree_ref_iterator_vtable = {
+	worktree_ref_iterator_advance,
+	worktree_ref_iterator_peel,
+	worktree_ref_iterator_abort
+};
+
+struct ref_iterator *worktree_ref_iterator_begin(struct ref_iterator *per_repo,
+						 struct ref_iterator *per_worktree)
+{
+	struct worktree_ref_iterator *iter = xcalloc(1, sizeof(*iter));
+
+	base_ref_iterator_init(&iter->base, &worktree_ref_iterator_vtable);
+	iter->per_repo_iterator = per_repo;
+	iter->per_worktree_iterator = per_worktree;
+	return &iter->base;
+}
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 690498698e..dcb1f1d73d 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -387,6 +387,14 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,
 					       const char *prefix,
 					       int trim);
 
+/*
+ * Wrap per_repo and per_worktree iterators. Traverse per_repo
+ * iterator, drop per-worktree refs. Then traverse per_worktree
+ * iterator.
+ */
+struct ref_iterator *worktree_ref_iterator_begin(struct ref_iterator *per_repo,
+						 struct ref_iterator *per_worktree);
+
 /* Internal implementation of reference iteration: */
 
 /*

Re: [PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store

[Stefan Beller]

On Fri, Apr 21, 2017 at 10:27 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:
>> This is a better place that will benefit all submodule callers instead
>> of just resolve_gitlink_ref()
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  refs.c | 33 +++++++++++++++++----------------
>>  1 file changed, 17 insertions(+), 16 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index 5902a3d9e5..26474cb62a 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1422,25 +1422,10 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
>>  int resolve_gitlink_ref(const char *submodule, const char *refname,
>>                       unsigned char *sha1)
>>  {
>> -     size_t len = strlen(submodule);
>>       struct ref_store *refs;
>>       int flags;
>>
>> -     while (len && submodule[len - 1] == '/')
>> -             len--;
>> -
>> -     if (!len)
>> -             return -1;
>> -
>> -     if (submodule[len]) {
>> -             /* We need to strip off one or more trailing slashes */
>> -             char *stripped = xmemdupz(submodule, len);
>> -
>> -             refs = get_submodule_ref_store(stripped);
>> -             free(stripped);
>> -     } else {
>> -             refs = get_submodule_ref_store(submodule);
>> -     }
>> +     refs = get_submodule_ref_store(submodule);
>>
>>       if (!refs)
>>               return -1;
>> @@ -1558,7 +1543,17 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
>>  {
>>       struct strbuf submodule_sb = STRBUF_INIT;
>>       struct ref_store *refs;
>> +     char *to_free = NULL;
>>       int ret;
>> +     size_t len;
>> +
>> +     if (submodule) {
>> +             len = strlen(submodule);
>> +             while (len && submodule[len - 1] == '/')
>> +                     len--;
>> +             if (!len)
>> +                     submodule = NULL;
>> +     }
>
> Ugh. Should a submodule named "///" *really* be considered to refer to
> the main ref_store? I understand that's what the code did before this
> patch, but it seems to me more like an accident of the old design rather
> than something worth supporting. In other words, if a caller would
> really pass us such a string, it seems like we could declare the caller
> buggy, no?

In a nearby thread we discussed whether we want to tighten the
submodule names and paths as some of the code path do funny
things on funny input. As 'submodule' here refers to a path (and not
a name), I would think '///' is not possible/buggy, rather we'd want to
discuss if we want to extend the check to is_dir_sep, which would
include '\' on Windows.

Thanks,
Stefan

Re: [PATCH v3 10/12] files-backend: make reflog iterator go through per-worktree reflog

[Michael Haggerty]

Hi,

I put off reviewing this patch, thinking that it would appear in a
re-roll, then never came back to it. :-(

On 04/23/2017 06:44 AM, Duy Nguyen wrote:
> On Sat, Apr 22, 2017 at 10:05:02AM +0200, Michael Haggerty wrote:
>> I find this implementation confusing:
>>
>> * `if (iter->worktree_dir_iterator)` sounds like it should mean
>>   that we are iterating over worktree references but it really means
>>   that we are iterating over the common references in a repository
>>   that is a linked worktree.
>> * `files_reflog_iterator_advance()` is called recursively, but only
>>   for the first worktree reference.
>> * `iter->worktree_dir_iterator` is moved over to `iter->dir_iterator`
>>   when the common refs are exhausted.
>>
>> Do you find it more readable as follows?:
> 
> It's a bit better, but while we're at it, why not take full advantage
> of iterator abstraction?
> 
> This replacement patch (with some unrelated bits removed to reduce
> distraction) adds a new meta ref-iterator that combine a per-repo and
> a per-worktree iterators together. The new iterator walks through both
> sub-iterators and drop the per-worktree results from the per-repo
> iterator, which will be replaced with results from per-worktree
> iterator.
> 
> You probably see where I'm going with this. When the new "linked
> worktree ref store" comes, it will combine two per-worktree and
> per-repo ref stores together and this iterator will come handy.
> 
> At that point, files-backend can go back to being oblivious about
> $GIT_DIR vs $GIT_COMMON_DIR and files_reflog_iterator_begin() will be
> reverted back to the version before this patch.

Yes, that's even better.

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 4149943a6e..817b7b5d5e 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3432,23 +3423,37 @@ static struct ref_iterator_vtable files_reflog_iterator_vtable = {
>  	files_reflog_iterator_abort
>  };
>  
> -static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_store)
> +static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
> +						  const char *gitdir)
>  {
> -	struct files_ref_store *refs =
> -		files_downcast(ref_store, REF_STORE_READ,
> -			       "reflog_iterator_begin");
>  	struct files_reflog_iterator *iter = xcalloc(1, sizeof(*iter));
>  	struct ref_iterator *ref_iterator = &iter->base;
>  	struct strbuf sb = STRBUF_INIT;
>  
>  	base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable);
> -	files_reflog_path(refs, &sb, NULL);
> +	strbuf_addf(&sb, "%s/logs", gitdir);
>  	iter->dir_iterator = dir_iterator_begin(sb.buf);
>  	iter->ref_store = ref_store;
>  	strbuf_release(&sb);
> +
>  	return ref_iterator;
>  }

Makes sense.

> +static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_store)
> +{
> +	struct files_ref_store *refs =
> +		files_downcast(ref_store, REF_STORE_READ,
> +			       "reflog_iterator_begin");
> +
> +	if (!strcmp(refs->gitdir, refs->gitcommondir)) {
> +		return reflog_iterator_begin(ref_store, refs->gitcommondir);
> +	} else {
> +		return worktree_ref_iterator_begin(
> +			reflog_iterator_begin(ref_store, refs->gitcommondir),
> +			reflog_iterator_begin(ref_store, refs->gitdir));
> +	}
> +}
> +

Yes.

>  static int ref_update_reject_duplicates(struct string_list *refnames,
>  					struct strbuf *err)
>  {
> diff --git a/refs/iterator.c b/refs/iterator.c
> index bce1f192f7..93243a00c4 100644
> --- a/refs/iterator.c
> +++ b/refs/iterator.c
> @@ -40,6 +40,14 @@ void base_ref_iterator_free(struct ref_iterator *iter)
>  	free(iter);
>  }
>  
> +static void ref_iterator_copy_result(struct ref_iterator *dst,
> +				     const struct ref_iterator *src)
> +{
> +	dst->refname = src->refname;
> +	dst->oid = src->oid;
> +	dst->flags = src->flags;
> +}
> +
>  struct empty_ref_iterator {
>  	struct ref_iterator base;
>  };
> @@ -382,3 +390,100 @@ int do_for_each_ref_iterator(struct ref_iterator *iter,
>  		return -1;
>  	return retval;
>  }
> +
> +struct worktree_ref_iterator {
> +	struct ref_iterator base;
> +	struct ref_iterator *per_repo_iterator;
> +	struct ref_iterator *per_worktree_iterator;
> +};
> +
> +static int worktree_ref_iterator_advance(struct ref_iterator *ref_iterator)
> +{
> +	struct worktree_ref_iterator *iter =
> +		(struct worktree_ref_iterator *)ref_iterator;
> +	int ok;
> +
> +	while (1) {
> +		struct ref_iterator **subiter;
> +		int normal_only;
> +
> +		if (iter->per_repo_iterator) {
> +			subiter = &iter->per_repo_iterator;
> +			/*
> +			 * If we are in a worktree, then we only
> +			 * include "normal" common references:
> +			 */
> +			normal_only = !!iter->per_worktree_iterator;
> +		} else if (iter->per_worktree_iterator) {
> +			subiter = &iter->per_worktree_iterator;
> +			normal_only = 0;
> +		} else {
> +			ok = ITER_DONE;
> +			break;
> +		}
> +
> +		ok = ref_iterator_advance(*subiter);
> +		if (ok == ITER_ERROR) {
> +			*subiter = NULL;
> +			break;
> +		} else if (ok == ITER_DONE) {
> +			*subiter = NULL;
> +			/* There might still be worktree refs left: */
> +			continue;
> +		}
> +
> +		if (normal_only &&
> +		    ref_type((*subiter)->refname) != REF_TYPE_NORMAL)
> +			continue;
> +
> +		ref_iterator_copy_result(&iter->base, *subiter);
> +		return ITER_OK;
> +	}
> +
> +	if (ref_iterator_abort(ref_iterator) == ITER_ERROR)
> +		return ITER_ERROR;
> +
> +	return ok;
> +}
> +
> +static int worktree_ref_iterator_peel(struct ref_iterator *ref_iterator,
> +					 struct object_id *peeled)
> +{
> +	die("BUG: ref_iterator_peel() called for reflog_iterator");
> +}
> +
> +static int worktree_ref_iterator_abort(struct ref_iterator *ref_iterator)
> +{
> +	struct worktree_ref_iterator *iter =
> +		(struct worktree_ref_iterator *)ref_iterator;
> +	int ok = ITER_DONE;
> +
> +	if (iter->per_repo_iterator)
> +		ok = ref_iterator_abort(iter->per_repo_iterator);
> +
> +	if (iter->per_worktree_iterator) {
> +		int ok2 = ref_iterator_abort(iter->per_worktree_iterator);
> +		if (ok2 == ITER_ERROR)
> +			ok = ok2;
> +	}
> +
> +	base_ref_iterator_free(ref_iterator);
> +	return ok;
> +}
> +
> +static struct ref_iterator_vtable worktree_ref_iterator_vtable = {
> +	worktree_ref_iterator_advance,
> +	worktree_ref_iterator_peel,
> +	worktree_ref_iterator_abort
> +};
> +
> +struct ref_iterator *worktree_ref_iterator_begin(struct ref_iterator *per_repo,
> +						 struct ref_iterator *per_worktree)
> +{
> +	struct worktree_ref_iterator *iter = xcalloc(1, sizeof(*iter));
> +
> +	base_ref_iterator_init(&iter->base, &worktree_ref_iterator_vtable);
> +	iter->per_repo_iterator = per_repo;
> +	iter->per_worktree_iterator = per_worktree;
> +	return &iter->base;
> +}
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 690498698e..dcb1f1d73d 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -387,6 +387,14 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,
>  					       const char *prefix,
>  					       int trim);
>  
> +/*
> + * Wrap per_repo and per_worktree iterators. Traverse per_repo
> + * iterator, drop per-worktree refs. Then traverse per_worktree
> + * iterator.
> + */
> +struct ref_iterator *worktree_ref_iterator_begin(struct ref_iterator *per_repo,
> +						 struct ref_iterator *per_worktree);
> +
>  /* Internal implementation of reference iteration: */
>  
>  /*
> 
> 

I think `worktree_ref_iterator_begin()` could be implemented with a lot
less code via a `merge_ref_iterator`. You would need to define a
`ref_iterator_select_fn`, which could look like something like this:

enum iterator_selection worktree_iterator_select_fn(
		struct ref_iterator *iter0, struct ref_iterator *iter1,
		void *cb_data)
{
	if (iter0) {
		if (ref_type(iter0->refname) != REF_TYPE_NORMAL)
			return ITER_SKIP_0;
		return ITER_SELECT_0;
	} else if (iter1) {
		return ITER_SELECT_1;
	} else {
		return ITER_SELECT_DONE;
	}
}

Then `worktree_ref_iterator_begin()` is simply

struct ref_iterator *worktree_ref_iterator_begin(struct ref_iterator
*per_repo,
						 struct ref_iterator *per_worktree)
{
	return merge_ref_iterator_begin(per_repo, per_worktree,
					worktree_iterator_select_fn, NULL);
}

For references (as opposed to reflogs) you would want to interleave the
outputs from the two input iterators in the correct order. See
`overlay_iterator_select()` for how that can be done.

Michael