git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Duy Nguyen <pclouds@gmail.com>
Date
Apr 23, 2017, 04:44 UTC
Message-ID
<20170423044420.GA28419@ash>
In-Reply-To
<d3428e5e-9ac2-0426-31fd-92d29a414b3d@alum.mit.edu>
On Sat, Apr 22, 2017 at 10:05:02AM +0200, Michael Haggerty wrote:
Show 12 quoted lines
> 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: */
 
 /*
Previous: Michael HaggertyNext: Michael Haggerty
Message 26 of 30 in “Fix git-gc losing objects in multi worktree”
  1. 00/12 Fix git-gc losing objects in multi worktreeNguyễn Thái Ngọc Duy, Apr 19, 2017
  2. 01/12 revision.h: new flag in struct rev_info wrt. worktree-related refsNguyễn Thái Ngọc Duy, Apr 19, 2017
  3. 02/12 revision.c: refactor add_index_objects_to_pending()Nguyễn Thái Ngọc Duy, Apr 19, 2017
  4. 03/12 revision.c: --indexed-objects add objects from all worktreesNguyễn Thái Ngọc Duy, Apr 19, 2017
  5. 04/12 refs.c: refactor get_submodule_ref_store(), share common free blockNguyễn Thái Ngọc Duy, Apr 19, 2017
  6. Michael HaggertyApr 22, 2017
  7. 05/12 refs: move submodule slash stripping code to get_submodule_ref_storeNguyễn Thái Ngọc Duy, Apr 19, 2017
  8. Johannes SixtApr 19, 2017
  9. Duy NguyenApr 20, 2017
  10. Duy NguyenApr 20, 2017
  11. Johannes SixtApr 20, 2017
  12. Michael HaggertyApr 22, 2017
  13. Stefan BellerApr 24, 2017
  14. 06/12 refs: add refs_head_ref()Nguyễn Thái Ngọc Duy, Apr 19, 2017
  15. Michael HaggertyApr 22, 2017
  16. Duy NguyenApr 22, 2017
  17. 07/12 revision.c: use refs_for_each*() instead of for_each_*_submodule()Nguyễn Thái Ngọc Duy, Apr 19, 2017
  18. 08/12 refs: remove dead for_each_*_submodule()Nguyễn Thái Ngọc Duy, Apr 19, 2017
  19. Ramsay JonesApr 19, 2017
  20. Junio C HamanoApr 20, 2017
  21. Michael HaggertyApr 22, 2017
  22. 09/12 revision.c: --all adds HEAD from all worktreesNguyễn Thái Ngọc Duy, Apr 19, 2017
  23. Michael HaggertyApr 22, 2017
  24. 10/12 files-backend: make reflog iterator go through per-worktree reflogNguyễn Thái Ngọc Duy, Apr 19, 2017
  25. Michael HaggertyApr 22, 2017
  26. Duy NguyenApr 23, 2017
  27. Michael HaggertyMay 17, 2017
  28. 11/12 revision.c: --reflog add HEAD reflog from all worktreesNguyễn Thái Ngọc Duy, Apr 19, 2017
  29. 12/12 rev-list: expose and document --single-worktreeNguyễn Thái Ngọc Duy, Apr 19, 2017
  30. Michael HaggertyApr 22, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.