threads / rfc / 48962

RFC patch, 3 partsMigrate the refs API to take the repository argument

Subject: [RFC PATCH 0/3] Migrate the refs API to take the repository argument

## tl;dr

14 messages between Jul 27, 2018 and Jul 31, 2018. Diffs are folded; open one to read it.

replies: 13people: 4as markdown or json

Stefan Beller· Jul 27, 2018, 00:36 UTC · lore

The second patch is the real API proposal. Unlike the lookup_* series, which caused a lot of integration pain to Junio, I plan to structure this in a different way, by having multiple steps:

 (1) in this (later to be non-RFC) series, add the new API that passes thru
     the repository; for now do not replace refs_store argument by
     struct repository.
 (2) the last patch is a demo of converting one of the callers over
     to the new API; this would need to be done for all of them
     
 (3) After some time do a cleanup series to remove callers of the
     old API fromly introduced series that are currently in flight.
 (4) Remove the old API.
 (5) Introduce the final API removing the refs_store
 (6) convert all callers to the final API, using this same dual step approach
 (7) remove this API
 
Steps 1,2 will be done in this series (2 is done only as demo here
for one function, but the non-RFC would do it all)

Steps 3,4 would be done once there are no more series in flight using the old API.

Before continuing on step (2), I would want to ask for your thoughts of (1).

Also note that after step (1) before (4) refs.h looks messy as well as between (5) and (7).

Thanks, Stefan

Stefan Beller (3):
  refs.c: migrate internal ref iteration to pass thru repository
    argument
  refs: introduce new API, wrap old API shallowly around new API
  replace: migrate to for_each_replace_repo_ref
 builtin/replace.c    |   9 +-
 refs.c               | 187 ++++++++++++++--------
 refs.h               | 362 ++++++++++++++++++++++++++++++++++++++-----
 refs/iterator.c      |   6 +-
 refs/refs-internal.h |   5 +-
 replace-object.c     |   7 +-
 6 files changed, 464 insertions(+), 112 deletions(-)
-- 
2.18.0.345.g5c9ce644c3-goog
Stefan Beller· Jul 27, 2018, 00:36 UTC · re: Stefan Beller · lore

[PATCH 1/3] refs.c: migrate internal ref iteration to pass thru repository argument

In 60ce76d3581 (refs: add repository argument to for_each_replace_ref, 2018-04-11) and 0d296c57aec (refs: allow for_each_replace_ref to handle arbitrary repositories, 2018-04-11), for_each_replace_ref learned how to iterate over refs by a given arbitrary repository. New attempts in the object store conversion have shown that it is useful to have the repository handle available that the refs iteration is currently iterating over.

To achieve this goal we will need to add a repository argument to each_ref_fn in refs.h. However as many callers rely on the signature such a patch would be too large.

So convert the internals of the ref subsystem first to pass through a repository argument without exposing the change to the user. Assume the_repository for the passed through repository, although it is not used anywhere yet.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 refs.c               | 39 +++++++++++++++++++++++++++++++++++++--
 refs.h               | 10 ++++++++++
 refs/iterator.c      |  6 +++---
 refs/refs-internal.h |  5 +++--
 4 files changed, 53 insertions(+), 7 deletions(-)
Show changes to 4 files +53 −7

refs.c, refs.h, refs/iterator.c, refs/refs-internal.h

diff --git a/refs.c b/refs.c
index fcfd3171e83..2513f77acb3 100644
--- a/refs.c
+++ b/refs.c
@@ -1390,17 +1390,50 @@ struct ref_iterator *refs_ref_iterator_begin(
  * non-zero value, stop the iteration and return that value;
  * otherwise, return 0.
  */
+static int do_for_each_repo_ref(struct repository *r, const char *prefix,
+				each_repo_ref_fn fn, int trim, int flags,
+				void *cb_data)
+{
+	struct ref_iterator *iter;
+	struct ref_store *refs = get_main_ref_store(r);
+
+	if (!refs)
+		return 0;
+
+	iter = refs_ref_iterator_begin(refs, prefix, trim, flags);
+
+	return do_for_each_repo_ref_iterator(r, iter, fn, cb_data);
+}
+
+struct do_for_each_ref_help {
+	each_ref_fn *fn;
+	void *cb_data;
+};
+
+static int do_for_each_ref_helper(struct repository *r,
+				  const char *refname,
+				  const struct object_id *oid,
+				  int flags,
+				  void *cb_data)
+{
+	struct do_for_each_ref_help *hp = cb_data;
+
+	return hp->fn(refname, oid, flags, hp->cb_data);
+}
+
 static int do_for_each_ref(struct ref_store *refs, const char *prefix,
 			   each_ref_fn fn, int trim, int flags, void *cb_data)
 {
 	struct ref_iterator *iter;
+	struct do_for_each_ref_help hp = { fn, cb_data };
 
 	if (!refs)
 		return 0;
 
 	iter = refs_ref_iterator_begin(refs, prefix, trim, flags);
 
-	return do_for_each_ref_iterator(iter, fn, cb_data);
+	return do_for_each_repo_ref_iterator(the_repository, iter,
+					do_for_each_ref_helper, &hp);
 }
 
 int refs_for_each_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
@@ -2029,10 +2062,12 @@ int refs_verify_refname_available(struct ref_store *refs,
 int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data)
 {
 	struct ref_iterator *iter;
+	struct do_for_each_ref_help hp = { fn, cb_data };
 
 	iter = refs->be->reflog_iterator_begin(refs);
 
-	return do_for_each_ref_iterator(iter, fn, cb_data);
+	return do_for_each_repo_ref_iterator(the_repository, iter,
+					     do_for_each_ref_helper, &hp);
 }
 
 int for_each_reflog(each_ref_fn fn, void *cb_data)
diff --git a/refs.h b/refs.h
index cc2fb4c68c0..80eec8bbc68 100644
--- a/refs.h
+++ b/refs.h
@@ -274,6 +274,16 @@ struct ref_transaction;
 typedef int each_ref_fn(const char *refname,
 			const struct object_id *oid, int flags, void *cb_data);
 
+/*
+ * The same as each_ref_fn, but also with a repository argument that
+ * contains the repository associated with the callback.
+ */
+typedef int each_repo_ref_fn(struct repository *r,
+			     const char *refname,
+			     const struct object_id *oid,
+			     int flags,
+			     void *cb_data);
+
 /*
  * The following functions invoke the specified callback function for
  * each reference indicated.  If the function ever returns a nonzero
diff --git a/refs/iterator.c b/refs/iterator.c
index 2ac91ac3401..629e00a122a 100644
--- a/refs/iterator.c
+++ b/refs/iterator.c
@@ -407,15 +407,15 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,
 
 struct ref_iterator *current_ref_iter = NULL;
 
-int do_for_each_ref_iterator(struct ref_iterator *iter,
-			     each_ref_fn fn, void *cb_data)
+int do_for_each_repo_ref_iterator(struct repository *r, struct ref_iterator *iter,
+				  each_repo_ref_fn fn, void *cb_data)
 {
 	int retval = 0, ok;
 	struct ref_iterator *old_ref_iter = current_ref_iter;
 
 	current_ref_iter = iter;
 	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
-		retval = fn(iter->refname, iter->oid, iter->flags, cb_data);
+		retval = fn(r, iter->refname, iter->oid, iter->flags, cb_data);
 		if (retval) {
 			/*
 			 * If ref_iterator_abort() returns ITER_ERROR,
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index dd834314bd8..5c7414bf099 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -473,8 +473,9 @@ extern struct ref_iterator *current_ref_iter;
  * adapter between the callback style of reference iteration and the
  * iterator style.
  */
-int do_for_each_ref_iterator(struct ref_iterator *iter,
-			     each_ref_fn fn, void *cb_data);
+int do_for_each_repo_ref_iterator(struct repository *r,
+				  struct ref_iterator *iter,
+				  each_repo_ref_fn fn, void *cb_data);
 
 /*
  * Only include per-worktree refs in a do_for_each_ref*() iteration.

-- 
2.18.0.345.g5c9ce644c3-goog
Stefan Beller· Jul 27, 2018, 00:36 UTC · re: Stefan Beller · lore

[PATCH 2/3] refs: introduce new API, wrap old API shallowly around new API

Currently the refs API takes a 'ref_store' as an argument to specify which ref store to iterate over; however it is more useful to specify the repository instead (or later a specific worktree of a repository).

Introduce a new API, that takes a repository struct instead of a ref store; the repository argument is also passed through to the callback, which is of type 'each_repo_ref_fn' that is introduced in a previous patch and is an extension of the 'each_ref_fn' type with the additional repository argument.

We wrap the old API as in a very shallow way around the new API, by wrapping the callback and the callback data into a new callback to translate between the 'each_ref_fn' and 'each_repo_ref_fn' type.

The wrapping implementation could be done either in refs.c or as presented in this patch as a 'static inline' in the header file itself. This has the advantage that the line of the old API is changed (and not just its implementation in refs.c), such that it will show up in git-blame.

The new API is not perfect yet, as some of them take both a 'repository' and 'ref_store' argument. This is done for an easy migration: If the ref_store argument is non-NULL, prefer it over the repository to compute which refs to iterate over. That way we can ensure that this step of API migration doesn't confuse which ref store to work on.

Once all callers have migrated to this newly introduced API, we can get rid of the old API; a second migration step in the future will remove the then useless ref_store argument

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 refs.c | 158 +++++++++++++++-----------
 refs.h | 352 +++++++++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 407 insertions(+), 103 deletions(-)
Show changes to 2 files +407 −103

refs.c, refs.h

diff --git a/refs.c b/refs.c
index 2513f77acb3..27e3772fca9 100644
--- a/refs.c
+++ b/refs.c
@@ -217,7 +217,7 @@ char *resolve_refdup(const char *refname, int resolve_flags,
 /* The argument to filter_refs */
 struct ref_filter {
 	const char *pattern;
-	each_ref_fn *fn;
+	each_repo_ref_fn *fn;
 	void *cb_data;
 };
 
@@ -289,14 +289,15 @@ int ref_filter_match(const char *refname,
 	return 1;
 }
 
-static int filter_refs(const char *refname, const struct object_id *oid,
-			   int flags, void *data)
+static int filter_refs(struct repository *r,
+		       const char *refname, const struct object_id *oid,
+		       int flags, void *data)
 {
 	struct ref_filter *filter = (struct ref_filter *)data;
 
 	if (wildmatch(filter->pattern, refname, 0))
 		return 0;
-	return filter->fn(refname, oid, flags, filter->cb_data);
+	return filter->fn(r, refname, oid, flags, filter->cb_data);
 }
 
 enum peel_status peel_object(const struct object_id *name, struct object_id *oid)
@@ -371,46 +372,50 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct string_li
 	for_each_rawref(warn_if_dangling_symref, &data);
 }
 
-int refs_for_each_tag_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
+int refs_for_each_tag_repo_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data)
 {
-	return refs_for_each_ref_in(refs, "refs/tags/", fn, cb_data);
+	return refs_for_each_repo_ref_in(r, NULL, "refs/tags/", fn, cb_data);
 }
 
-int for_each_tag_ref(each_ref_fn fn, void *cb_data)
+int for_each_tag_repo_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data)
 {
-	return refs_for_each_tag_ref(get_main_ref_store(the_repository), fn, cb_data);
+	return refs_for_each_tag_repo_ref(r, fn, cb_data);
 }
 
-int refs_for_each_branch_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
+int refs_for_each_branch_repo_ref(struct repository *r, struct ref_store *refs,
+				  each_repo_ref_fn fn, void *cb_data)
 {
-	return refs_for_each_ref_in(refs, "refs/heads/", fn, cb_data);
+	return refs_for_each_repo_ref_in(r, refs, "refs/heads/", fn, cb_data);
 }
 
-int for_each_branch_ref(each_ref_fn fn, void *cb_data)
+int for_each_branch_repo_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data)
 {
-	return refs_for_each_branch_ref(get_main_ref_store(the_repository), fn, cb_data);
+	return refs_for_each_branch_repo_ref(r, NULL, fn, cb_data);
 }
 
-int refs_for_each_remote_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
+int refs_for_each_remote_repo_ref(struct repository *r, struct ref_store *refs,
+				  each_repo_ref_fn fn, void *cb_data)
 {
-	return refs_for_each_ref_in(refs, "refs/remotes/", fn, cb_data);
+	return refs_for_each_repo_ref_in(r, refs, "refs/remotes/", fn, cb_data);
 }
 
-int for_each_remote_ref(each_ref_fn fn, void *cb_data)
+int for_each_remote_repo_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data)
 {
-	return refs_for_each_remote_ref(get_main_ref_store(the_repository), fn, cb_data);
+	return refs_for_each_remote_repo_ref(the_repository, NULL, fn, cb_data);
 }
 
-int head_ref_namespaced(each_ref_fn fn, void *cb_data)
+int head_repo_ref_namespaced(struct repository *r, each_repo_ref_fn fn, void *cb_data)
 {
 	struct strbuf buf = STRBUF_INIT;
 	int ret = 0;
 	struct object_id oid;
 	int flag;
+	struct ref_store *refs = get_main_ref_store(r);
 
 	strbuf_addf(&buf, "%sHEAD", get_git_namespace());
-	if (!read_ref_full(buf.buf, RESOLVE_REF_READING, &oid, &flag))
-		ret = fn(buf.buf, &oid, flag, cb_data);
+
+	if (!refs_read_ref_full(refs, buf.buf, RESOLVE_REF_READING, &oid, &flag))
+		ret = fn(r, buf.buf, &oid, flag, cb_data);
 	strbuf_release(&buf);
 
 	return ret;
@@ -437,8 +442,8 @@ void normalize_glob_ref(struct string_list_item *item, const char *prefix,
 	strbuf_release(&normalized_pattern);
 }
 
-int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
-	const char *prefix, void *cb_data)
+int for_each_glob_repo_ref_in(struct repository *r, each_repo_ref_fn fn,
+	const char *pattern, const char *prefix, void *cb_data)
 {
 	struct strbuf real_pattern = STRBUF_INIT;
 	struct ref_filter filter;
@@ -460,15 +465,16 @@ int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
 	filter.pattern = real_pattern.buf;
 	filter.fn = fn;
 	filter.cb_data = cb_data;
-	ret = for_each_ref(filter_refs, &filter);
+	ret = for_each_repo_ref(r, filter_refs, &filter);
 
 	strbuf_release(&real_pattern);
 	return ret;
 }
 
-int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)
+int for_each_glob_repo_ref(struct repository *r, each_repo_ref_fn fn,
+			   const char *pattern, void *cb_data)
 {
-	return for_each_glob_ref_in(fn, pattern, NULL, cb_data);
+	return for_each_glob_repo_ref_in(r, fn, pattern, NULL, cb_data);
 }
 
 const char *prettify_refname(const char *name)
@@ -1337,21 +1343,25 @@ int refs_rename_ref_available(struct ref_store *refs,
 	return ok;
 }
 
-int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
+int refs_head_repo_ref(struct repository *r, struct ref_store *refs,
+		       each_repo_ref_fn fn, void *cb_data)
 {
 	struct object_id oid;
 	int flag;
 
+	if (!refs)
+		refs = get_main_ref_store(r);
+
 	if (!refs_read_ref_full(refs, "HEAD", RESOLVE_REF_READING,
 				&oid, &flag))
-		return fn("HEAD", &oid, flag, cb_data);
+		return fn(r, "HEAD", &oid, flag, cb_data);
 
 	return 0;
 }
 
-int head_ref(each_ref_fn fn, void *cb_data)
+int head_repo_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data)
 {
-	return refs_head_ref(get_main_ref_store(the_repository), fn, cb_data);
+	return refs_head_repo_ref(r, NULL, fn, cb_data);
 }
 
 struct ref_iterator *refs_ref_iterator_begin(
@@ -1390,12 +1400,15 @@ struct ref_iterator *refs_ref_iterator_begin(
  * non-zero value, stop the iteration and return that value;
  * otherwise, return 0.
  */
-static int do_for_each_repo_ref(struct repository *r, const char *prefix,
+static int do_for_each_repo_ref(struct repository *r, struct ref_store *refs,
+				const char *prefix,
 				each_repo_ref_fn fn, int trim, int flags,
 				void *cb_data)
 {
 	struct ref_iterator *iter;
-	struct ref_store *refs = get_main_ref_store(r);
+
+	if (!refs)
+		refs = get_main_ref_store(r);
 
 	if (!refs)
 		return 0;
@@ -1405,6 +1418,15 @@ static int do_for_each_repo_ref(struct repository *r, const char *prefix,
 	return do_for_each_repo_ref_iterator(r, iter, fn, cb_data);
 }
 
+int each_ref_fn_repository_wrapped(struct repository *r,
+				   const char *refname,
+				   const struct object_id *oid,
+				   int flags, void *cb_data)
+{
+	struct each_ref_fn_repository_wrapper *cb = cb_data;
+	return cb->fn(refname, oid, flags, cb->cb);
+}
+
 struct do_for_each_ref_help {
 	each_ref_fn *fn;
 	void *cb_data;
@@ -1436,76 +1458,78 @@ static int do_for_each_ref(struct ref_store *refs, const char *prefix,
 					do_for_each_ref_helper, &hp);
 }
 
-int refs_for_each_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
+int refs_for_each_repo_ref(struct repository *r, struct ref_store *refs,
+			   each_repo_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref(refs, "", fn, 0, 0, cb_data);
+	return do_for_each_repo_ref(r, refs, "", fn, 0, 0, cb_data);
 }
 
-int for_each_ref(each_ref_fn fn, void *cb_data)
+int for_each_repo_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data)
 {
-	return refs_for_each_ref(get_main_ref_store(the_repository), fn, cb_data);
+	return refs_for_each_repo_ref(r, NULL, fn, cb_data);
 }
 
-int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,
-			 each_ref_fn fn, void *cb_data)
+int refs_for_each_repo_ref_in(struct repository *r, struct ref_store *refs,
+			      const char *prefix, each_repo_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref(refs, prefix, fn, strlen(prefix), 0, cb_data);
+	return do_for_each_repo_ref(r, refs, prefix, fn, strlen(prefix), 0, cb_data);
 }
 
-int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data)
+int for_each_repo_ref_in(struct repository *r, const char *prefix, each_repo_ref_fn fn, void *cb_data)
 {
-	return refs_for_each_ref_in(get_main_ref_store(the_repository), prefix, fn, cb_data);
+	return refs_for_each_repo_ref_in(r, NULL, prefix, fn, cb_data);
 }
 
-int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsigned int broken)
+int refs_for_each_full_repo_ref_in(struct repository *r, struct ref_store *refs,
+				   const char *prefix,
+				   each_repo_ref_fn fn, void *cb_data,
+				   unsigned int broken)
 {
 	unsigned int flag = 0;
 
 	if (broken)
 		flag = DO_FOR_EACH_INCLUDE_BROKEN;
-	return do_for_each_ref(get_main_ref_store(the_repository),
-			       prefix, fn, 0, flag, cb_data);
+	return do_for_each_repo_ref(r, refs, prefix, fn, 0, flag, cb_data);
 }
 
-int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,
-			     each_ref_fn fn, void *cb_data,
-			     unsigned int broken)
+int for_each_full_repo_ref_in(struct repository *r, const char *prefix,
+			      each_repo_ref_fn fn, void *cb_data,
+			      unsigned int broken)
 {
 	unsigned int flag = 0;
 
 	if (broken)
 		flag = DO_FOR_EACH_INCLUDE_BROKEN;
-	return do_for_each_ref(refs, prefix, fn, 0, flag, cb_data);
+	return do_for_each_repo_ref(r, NULL, prefix, fn, 0, flag, cb_data);
 }
 
-int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data)
+int for_each_replace_repo_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref(get_main_ref_store(r),
-			       git_replace_ref_base, fn,
-			       strlen(git_replace_ref_base),
-			       DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
+	return do_for_each_repo_ref(r, NULL, git_replace_ref_base, fn,
+				    strlen(git_replace_ref_base),
+				    DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
 }
 
-int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)
+int for_each_namespaced_repo_ref(struct repository *r,
+				 each_repo_ref_fn fn, void *cb_data)
 {
 	struct strbuf buf = STRBUF_INIT;
 	int ret;
 	strbuf_addf(&buf, "%srefs/", get_git_namespace());
-	ret = do_for_each_ref(get_main_ref_store(the_repository),
-			      buf.buf, fn, 0, 0, cb_data);
+	ret = do_for_each_repo_ref(r, NULL, buf.buf, fn, 0, 0, cb_data);
 	strbuf_release(&buf);
 	return ret;
 }
 
-int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
+int refs_for_each_raw_repo_ref(struct repository *r, struct ref_store *refs, each_repo_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref(refs, "", fn, 0,
-			       DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
+	return do_for_each_repo_ref(r, refs, "", fn, 0,
+				    DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
 }
 
-int for_each_rawref(each_ref_fn fn, void *cb_data)
+int for_each_raw_repo_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data)
 {
-	return refs_for_each_rawref(get_main_ref_store(the_repository), fn, cb_data);
+	return refs_for_each_raw_repo_ref(r, NULL, fn, cb_data);
 }
 
 int refs_read_raw_ref(struct ref_store *ref_store,
@@ -2059,20 +2083,18 @@ int refs_verify_refname_available(struct ref_store *refs,
 	return ret;
 }
 
-int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data)
+int refs_for_each_repo_reflog(struct repository *r, struct ref_store *refs,
+			      each_repo_ref_fn fn, void *cb_data)
 {
 	struct ref_iterator *iter;
-	struct do_for_each_ref_help hp = { fn, cb_data };
 
-	iter = refs->be->reflog_iterator_begin(refs);
+	if (!refs)
+		refs = get_main_ref_store(r);
 
-	return do_for_each_repo_ref_iterator(the_repository, iter,
-					     do_for_each_ref_helper, &hp);
-}
+	iter = refs->be->reflog_iterator_begin(refs);
 
-int for_each_reflog(each_ref_fn fn, void *cb_data)
-{
-	return refs_for_each_reflog(get_main_ref_store(the_repository), fn, cb_data);
+	return do_for_each_repo_ref_iterator(r, iter,
+					     fn, cb_data);
 }
 
 int refs_for_each_reflog_ent_reverse(struct ref_store *refs,
diff --git a/refs.h b/refs.h
index 80eec8bbc68..ba50fb9152d 100644
--- a/refs.h
+++ b/refs.h
@@ -284,6 +284,16 @@ typedef int each_repo_ref_fn(struct repository *r,
 			     int flags,
 			     void *cb_data);
 
+struct each_ref_fn_repository_wrapper {
+	each_ref_fn *fn;
+	void *cb;
+};
+
+int each_ref_fn_repository_wrapped(struct repository *r,
+				   const char *refname,
+				   const struct object_id *oid,
+				   int flags, void *cb_data);
+
 /*
  * The following functions invoke the specified callback function for
  * each reference indicated.  If the function ever returns a nonzero
@@ -293,41 +303,292 @@ typedef int each_repo_ref_fn(struct repository *r,
  * 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,
-			 each_ref_fn fn, void *cb_data);
-int refs_for_each_tag_ref(struct ref_store *refs,
-			  each_ref_fn fn, void *cb_data);
-int refs_for_each_branch_ref(struct ref_store *refs,
-			     each_ref_fn fn, void *cb_data);
-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 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 refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,
-			     each_ref_fn fn, void *cb_data,
-			     unsigned int broken);
-int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,
-			unsigned int broken);
-int for_each_tag_ref(each_ref_fn fn, void *cb_data);
-int for_each_branch_ref(each_ref_fn fn, void *cb_data);
-int for_each_remote_ref(each_ref_fn fn, void *cb_data);
-int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data);
-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_namespaced(each_ref_fn fn, void *cb_data);
-int for_each_namespaced_ref(each_ref_fn fn, void *cb_data);
+int refs_head_repo_ref(struct repository *r, struct ref_store *refs,
+		       each_repo_ref_fn fn, void *cb_data);
+static inline int refs_head_ref(struct ref_store *refs,
+		  each_ref_fn fn, void *cb_data)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return refs_head_repo_ref(the_repository, refs,
+				  each_ref_fn_repository_wrapped, &cb);
+}
+
+int refs_for_each_repo_ref(struct repository *r, struct ref_store *refs,
+		      each_repo_ref_fn fn, void *cb_data);
+static inline int refs_for_each_ref(struct ref_store *refs,
+		      each_ref_fn fn, void *cb_data)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return refs_for_each_repo_ref(the_repository, refs,
+				      each_ref_fn_repository_wrapped, &cb);
+}
+
+int refs_for_each_repo_ref_in(struct repository *r, struct ref_store *refs,
+			 const char *prefix, each_repo_ref_fn fn, void *cb_data);
+static inline int refs_for_each_ref_in(struct ref_store *refs,
+				       const char *prefix,
+				       each_ref_fn fn, void *cb_data)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return refs_for_each_repo_ref_in(the_repository, refs, prefix,
+					 each_ref_fn_repository_wrapped, &cb);
+}
+
+int refs_for_each_tag_repo_ref(struct repository *r,
+			       each_repo_ref_fn fn, void *cb_data);
+static inline int refs_for_each_tag_ref(struct ref_store *refs,
+				 each_ref_fn fn, void *cb_data)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return refs_for_each_tag_repo_ref(the_repository,
+					  each_ref_fn_repository_wrapped, &cb);
+}
+
+int refs_for_each_branch_repo_ref(struct repository *r, struct ref_store *refs,
+				  each_repo_ref_fn fn,
+				  void *cb_data);
+static inline int refs_for_each_branch_ref(struct ref_store *refs,
+					   each_ref_fn fn, void *cb_data)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return refs_for_each_branch_repo_ref(the_repository, refs,
+					     each_ref_fn_repository_wrapped, &cb);
+}
+
+int refs_for_each_remote_repo_ref(struct repository *r, struct ref_store *refs,
+				  each_repo_ref_fn fn,
+				  void *cb_data);
+static inline int refs_for_each_remote_ref(struct ref_store *refs,
+					   each_ref_fn fn, void *cb_data)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return refs_for_each_remote_repo_ref(the_repository, refs,
+					     each_ref_fn_repository_wrapped, &cb);
+}
+
+int head_repo_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data);
+static inline int head_ref(each_ref_fn fn, void *cb_data)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return head_repo_ref(the_repository,
+			     each_ref_fn_repository_wrapped, &cb);
+}
+
+int for_each_repo_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data);
+static inline int for_each_ref(each_ref_fn fn, void *cb_data)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return for_each_repo_ref(the_repository,
+				 each_ref_fn_repository_wrapped, &cb);
+}
+
+int for_each_repo_ref_in(struct repository *r, const char *prefix,
+			 each_repo_ref_fn fn, void *cb_data);
+static inline int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return for_each_repo_ref_in(the_repository, prefix,
+				    each_ref_fn_repository_wrapped, &cb);
+}
+
+int refs_for_each_full_repo_ref_in(struct repository *r, struct ref_store *refs,
+				   const char *prefix,
+				   each_repo_ref_fn fn, void *cb_data,
+				   unsigned int broken);
+static inline int refs_for_each_fullref_in(struct ref_store *refs,
+					   const char *prefix, each_ref_fn fn,
+					   void *cb_data, unsigned int broken)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return refs_for_each_full_repo_ref_in(the_repository, refs, prefix,
+					      each_ref_fn_repository_wrapped,
+					      &cb, broken);
+}
+
+int for_each_full_repo_ref_in(struct repository *r, const char *prefix,
+			      each_repo_ref_fn fn, void *cb_data,
+			      unsigned int broken);
+static inline int for_each_fullref_in(const char *prefix, each_ref_fn fn,
+				      void *cb_data, unsigned int broken)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return for_each_full_repo_ref_in(the_repository, prefix,
+					      each_ref_fn_repository_wrapped,
+					      &cb, broken);
+}
+
+int for_each_tag_repo_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data);
+static inline int for_each_tag_ref(each_ref_fn fn, void *cb_data)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return for_each_tag_repo_ref(the_repository,
+				     each_ref_fn_repository_wrapped, &cb);
+}
+
+int for_each_branch_repo_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data);
+static inline int for_each_branch_ref(each_ref_fn fn, void *cb_data)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return for_each_branch_repo_ref(the_repository,
+			each_ref_fn_repository_wrapped, &cb);
+}
+
+int for_each_remote_repo_ref(struct repository *r, each_repo_ref_fn fn, void *cb);
+static inline int for_each_remote_ref(each_ref_fn fn, void *cb_data)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return for_each_remote_repo_ref(the_repository,
+					each_ref_fn_repository_wrapped, &cb);
+}
+
+int for_each_replace_repo_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data);
+static inline int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return for_each_replace_repo_ref(r, each_ref_fn_repository_wrapped, &cb);
+}
+
+int for_each_glob_repo_ref(struct repository *r, each_repo_ref_fn fn,
+			   const char *pattern, void *cb_data);
+static inline int for_each_glob_ref(each_ref_fn fn, const char *pattern,
+				    void *cb_data)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return for_each_glob_repo_ref(the_repository,
+				      each_ref_fn_repository_wrapped,
+				      pattern, &cb);
+}
+
+int for_each_glob_repo_ref_in(struct repository *r,
+			      each_repo_ref_fn fn, const char *pattern,
+			      const char *prefix, void *cb_data);
+static inline int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
+			 const char *prefix, void *cb_data)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return for_each_glob_repo_ref_in(the_repository,
+					 each_ref_fn_repository_wrapped,
+					 pattern, prefix, &cb);
+}
+
+int head_repo_ref_namespaced(struct repository *r, each_repo_ref_fn fn, void *cb_data);
+static inline int head_ref_namespaced(each_ref_fn fn, void *cb_data)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return head_repo_ref_namespaced(the_repository,
+					each_ref_fn_repository_wrapped, &cb);
+}
+
+int for_each_namespaced_repo_ref(struct repository *r, each_repo_ref_fn fn,
+				 void *cb_data);
+static inline int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return for_each_namespaced_repo_ref(the_repository,
+					    each_ref_fn_repository_wrapped, &cb);
+}
 
 /* can be used to learn about broken ref and symref */
-int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data);
-int for_each_rawref(each_ref_fn fn, void *cb_data);
+int refs_for_each_raw_repo_ref(struct repository *r, struct ref_store *refs,
+			       each_repo_ref_fn fn, void *cb_data);
+static inline int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return refs_for_each_raw_repo_ref(the_repository, refs,
+					  each_ref_fn_repository_wrapped, &cb);
+}
+
+int for_each_raw_repo_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data);
+static inline int for_each_rawref(each_ref_fn fn, void *cb_data)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return for_each_raw_repo_ref(the_repository,
+				     each_ref_fn_repository_wrapped, &cb);
+}
 
 /*
  * Normalizes partial refs to their fully qualified form.
@@ -442,8 +703,29 @@ int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn, void
  * Calls the specified function for each reflog file until it returns nonzero,
  * and returns the value. Reflog file order is unspecified.
  */
-int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data);
-int for_each_reflog(each_ref_fn fn, void *cb_data);
+int refs_for_each_repo_reflog(struct repository *r, struct ref_store *refs,
+			 each_repo_ref_fn fn, void *cb_data);
+static inline int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn,
+				       void *cb_data)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return refs_for_each_repo_reflog(the_repository, refs,
+					 each_ref_fn_repository_wrapped, &cb);
+}
+static inline int for_each_reflog(each_ref_fn fn, void *cb_data)
+{
+	/*
+	 * NEEDSWORK: remove this function when there are no
+	 * series in flight using this function.
+	 */
+	struct each_ref_fn_repository_wrapper cb = {fn, cb_data};
+	return refs_for_each_repo_reflog(the_repository, NULL,
+					 each_ref_fn_repository_wrapped, &cb);
+}
 
 #define REFNAME_ALLOW_ONELEVEL 1
 #define REFNAME_REFSPEC_PATTERN 2

-- 
2.18.0.345.g5c9ce644c3-goog
Duy Nguyen· Jul 27, 2018, 16:07 UTC · re: Stefan Beller · lore

Re: [PATCH 2/3] refs: introduce new API, wrap old API shallowly around new API

On Fri, Jul 27, 2018 at 2:40 AM Stefan Beller <sbeller@google.com> wrote:
>
> Currently the refs API takes a 'ref_store' as an argument to specify
> which ref store to iterate over; however it is more useful to specify
> the repository instead (or later a specific worktree of a repository).

There is no 'later'. worktrees.c already passes a worktree specific ref store. If you make this move you have to also design a way to give a specific ref store now.

Frankly I still dislike the decision to pass repo everywhere, especially when refs code already has a nice ref-store abstraction. Some people frown upon back pointers. But I think adding a back pointer in ref-store, pointing back to the repository is the right move.

-- 
Duy
Brandon Williams· Jul 27, 2018, 17:19 UTC · re: Duy Nguyen · lore

Re: [PATCH 2/3] refs: introduce new API, wrap old API shallowly around new API

On 07/27, Duy Nguyen wrote:
Show 15 quoted lines
> On Fri, Jul 27, 2018 at 2:40 AM Stefan Beller <sbeller@google.com> wrote:
> >
> > Currently the refs API takes a 'ref_store' as an argument to specify
> > which ref store to iterate over; however it is more useful to specify
> > the repository instead (or later a specific worktree of a repository).
> 
> There is no 'later'. worktrees.c already passes a worktree specific
> ref store. If you make this move you have to also design a way to give
> a specific ref store now.
> 
> Frankly I still dislike the decision to pass repo everywhere,
> especially when refs code already has a nice ref-store abstraction.
> Some people frown upon back pointers. But I think adding a back
> pointer in ref-store, pointing back to the repository is the right
> move.

I don't quite understand why the refs code would need a whole repository and not just the ref-store it self. I thought the refs code was self contained enough that all its state was based on the passed in ref-store. If its not, then we've done a terrible job at avoiding layering violations (well actually we're really really bad at this in general, and I *think* we're trying to make this better though the object store/index refactoring).

If anything I would expect that the actual ref-store code would remain untouched by any refactoring and that instead the higher-level API that hasn't already been converted to explicitly use a ref-store (and instead just calls the underlying impl with get_main_ref_store()). Am I missing something here?

-- 
Brandon Williams
Stefan Beller· Jul 27, 2018, 17:30 UTC · re: Brandon Williams · lore

Re: [PATCH 2/3] refs: introduce new API, wrap old API shallowly around new API

On Fri, Jul 27, 2018 at 10:19 AM Brandon Williams <bmwill@google.com> wrote:
Show 31 quoted lines
>
> On 07/27, Duy Nguyen wrote:
> > On Fri, Jul 27, 2018 at 2:40 AM Stefan Beller <sbeller@google.com> wrote:
> > >
> > > Currently the refs API takes a 'ref_store' as an argument to specify
> > > which ref store to iterate over; however it is more useful to specify
> > > the repository instead (or later a specific worktree of a repository).
> >
> > There is no 'later'. worktrees.c already passes a worktree specific
> > ref store. If you make this move you have to also design a way to give
> > a specific ref store now.
> >
> > Frankly I still dislike the decision to pass repo everywhere,
> > especially when refs code already has a nice ref-store abstraction.
> > Some people frown upon back pointers. But I think adding a back
> > pointer in ref-store, pointing back to the repository is the right
> > move.
>
> I don't quite understand why the refs code would need a whole repository
> and not just the ref-store it self.  I thought the refs code was self
> contained enough that all its state was based on the passed in
> ref-store.  If its not, then we've done a terrible job at avoiding
> layering violations (well actually we're really really bad at this in
> general, and I *think* we're trying to make this better though the
> object store/index refactoring).
>
> If anything I would expect that the actual ref-store code would remain
> untouched by any refactoring and that instead the higher-level API that
> hasn't already been converted to explicitly use a ref-store (and instead
> just calls the underlying impl with get_main_ref_store()).  Am I missing
> something here?

Then I think we might want to go with the original in Stolees proposal https://github.com/gitgitgadget/git/pull/11/commits/300db80140dacc927db0d46c804ca0ef4dcc1be1 but there the call to for_each_replace_ref just looks ugly, as it takes the repository as both the repository where to obtain the ref store from as well as the back pointer.

I anticipate that we need to have a lot of back pointers to the repository in question, hence I think we should have the repository pointer promoted to not just a back pointer.

Duy Nguyen· Jul 27, 2018, 18:04 UTC · re: Stefan Beller · lore

Re: [PATCH 2/3] refs: introduce new API, wrap old API shallowly around new API

On Fri, Jul 27, 2018 at 7:31 PM Stefan Beller <sbeller@google.com> wrote:
Show 43 quoted lines
>
> On Fri, Jul 27, 2018 at 10:19 AM Brandon Williams <bmwill@google.com> wrote:
> >
> > On 07/27, Duy Nguyen wrote:
> > > On Fri, Jul 27, 2018 at 2:40 AM Stefan Beller <sbeller@google.com> wrote:
> > > >
> > > > Currently the refs API takes a 'ref_store' as an argument to specify
> > > > which ref store to iterate over; however it is more useful to specify
> > > > the repository instead (or later a specific worktree of a repository).
> > >
> > > There is no 'later'. worktrees.c already passes a worktree specific
> > > ref store. If you make this move you have to also design a way to give
> > > a specific ref store now.
> > >
> > > Frankly I still dislike the decision to pass repo everywhere,
> > > especially when refs code already has a nice ref-store abstraction.
> > > Some people frown upon back pointers. But I think adding a back
> > > pointer in ref-store, pointing back to the repository is the right
> > > move.
> >
> > I don't quite understand why the refs code would need a whole repository
> > and not just the ref-store it self.  I thought the refs code was self
> > contained enough that all its state was based on the passed in
> > ref-store.  If its not, then we've done a terrible job at avoiding
> > layering violations (well actually we're really really bad at this in
> > general, and I *think* we're trying to make this better though the
> > object store/index refactoring).
> >
> > If anything I would expect that the actual ref-store code would remain
> > untouched by any refactoring and that instead the higher-level API that
> > hasn't already been converted to explicitly use a ref-store (and instead
> > just calls the underlying impl with get_main_ref_store()).  Am I missing
> > something here?
>
> Then I think we might want to go with the original in Stolees proposal
> https://github.com/gitgitgadget/git/pull/11/commits/300db80140dacc927db0d46c804ca0ef4dcc1be1
> but there the call to for_each_replace_ref just looks ugly, as it takes the
> repository as both the repository where to obtain the ref store from
> as well as the back pointer.
>
> I anticipate that we need to have a lot of back pointers to the repository
> in question, hence I think we should have the repository pointer promoted
> to not just a back pointer.

I will probably need more time to study that commit and maybe the mail archive for the history of this series. But if I remember correctly some of these for_each_ api is quite a pain (perhaps it's the for_each version of reflog?) and it's probably better to redesign it (again talking without real understanding of the problem).

-- 
Duy
Stefan Beller· Jul 30, 2018, 19:47 UTC · re: Duy Nguyen · lore

[PATCH 0/2] Cleanup refs API [WAS: Re: [PATCH 2/3] refs: introduce new API, wrap old API shallowly around new API]

Show 9 quoted lines
> > I anticipate that we need to have a lot of back pointers to the repository
> > in question, hence I think we should have the repository pointer promoted
> > to not just a back pointer.
>
> I will probably need more time to study that commit and maybe the mail
> archive for the history of this series. But if I remember correctly
> some of these for_each_ api is quite a pain (perhaps it's the for_each
> version of reflog?) and it's probably better to redesign it (again
> talking without real understanding of the problem).

I stepped back a bit and reconsidered the point made above, and I do not think that the repository argument is any special. If you need a repository (for e.g. lookup_commit or friends), you'll have to pass it through the callback cookie, whether directly or as part of a struct tailored to your purpose.

Instead we should strive to make the refs API smaller and cleaner, omitting the repository argument at all, and instead should be focussing on a ref_store argument instead.

This series applies on master; when we decide to go this direction we can drop origin/sb/refs-in-repo.

Thanks, Stefan

Derrick Stolee (1):
  replace-objects: use arbitrary repositories
Stefan Beller (1):
  refs: switch for_each_replace_ref back to use a ref_store
 builtin/replace.c | 4 +---
 refs.c            | 4 ++--
 refs.h            | 2 +-
 replace-object.c  | 5 +++--
 4 files changed, 7 insertions(+), 8 deletions(-)
-- 
2.18.0.132.g195c49a2227
Stefan Beller· Jul 30, 2018, 19:47 UTC · re: Stefan Beller · lore

[PATCH 1/2] replace-objects: use arbitrary repositories

From: Derrick Stolee <dstolee@microsoft.com>

This is the smallest possible change that makes prepare_replace_objects work properly with arbitrary repositories. By supplying the repository as the cb_data, we do not need to modify any code in the ref iterator logic. We will likely want to do a full replacement of the ref iterator logic to provide a repository struct as a concrete parameter.

[sb: original commit message left as-is. I disagree with it. We want to keep the ref store API clean and focussed on struct ref_store. There is no need to treat a repository any special for pass-through by the callback cookie. So instead let's just pass the repository as a cb cookie and cleanup the API in follow up patches]

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 replace-object.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Show changes to replace-object.c +3 −2
diff --git a/replace-object.c b/replace-object.c
index 801b5c16789..e99fcd1ff6e 100644
--- a/replace-object.c
+++ b/replace-object.c
@@ -14,6 +14,7 @@ static int register_replace_ref(const char *refname,
 	const char *slash = strrchr(refname, '/');
 	const char *hash = slash ? slash + 1 : refname;
 	struct replace_object *repl_obj = xmalloc(sizeof(*repl_obj));
+	struct repository *r = (struct repository *)cb_data;
 
 	if (get_oid_hex(hash, &repl_obj->original.oid)) {
 		free(repl_obj);
@@ -25,7 +26,7 @@ static int register_replace_ref(const char *refname,
 	oidcpy(&repl_obj->replacement, oid);
 
 	/* Register new object */
-	if (oidmap_put(the_repository->objects->replace_map, repl_obj))
+	if (oidmap_put(r->objects->replace_map, repl_obj))
 		die("duplicate replace ref: %s", refname);
 
 	return 0;
@@ -40,7 +41,7 @@ static void prepare_replace_object(struct repository *r)
 		xmalloc(sizeof(*r->objects->replace_map));
 	oidmap_init(r->objects->replace_map, 0);
 
-	for_each_replace_ref(r, register_replace_ref, NULL);
+	for_each_replace_ref(r, register_replace_ref, r);
 }
 
 /* We allow "recursive" replacement. Only within reason, though */

-- 
2.18.0.132.g195c49a2227
Stefan Beller· Jul 30, 2018, 19:47 UTC · re: Stefan Beller · lore

[PATCH 2/2] refs: switch for_each_replace_ref back to use a ref_store

This effectively reverts commit 0d296c57ae (refs: allow for_each_replace_ref to handle arbitrary repositories, 2018-04-11) and 60ce76d3581 (refs: add repository argument to for_each_replace_ref, 2018-04-11).

The repository argument is not any special from the ref-store's point of life. If you need a repository (for e.g. lookup_commit or friends), you'll have to pass it through the callback cookie, whether directly or as part of a struct tailored to your purpose.

So let's go back to the clean API, just requiring a ref_store as an argument.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/replace.c | 2 +-
 refs.c            | 4 ++--
 refs.h            | 2 +-
 replace-object.c  | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)
Show changes to 4 files +5 −5

builtin/replace.c, refs.c, refs.h, replace-object.c

diff --git a/builtin/replace.c b/builtin/replace.c
index deabda21012..52dc371eafc 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -87,7 +87,7 @@ static int list_replace_refs(const char *pattern, const char *format)
 			     "valid formats are 'short', 'medium' and 'long'\n",
 			     format);
 
-	for_each_replace_ref(the_repository, show_reference, (void *)&data);
+	for_each_replace_ref(show_reference, (void *)&data);
 
 	return 0;
 }
diff --git a/refs.c b/refs.c
index 08fb5a99148..2d713499125 100644
--- a/refs.c
+++ b/refs.c
@@ -1441,9 +1441,9 @@ int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,
 	return do_for_each_ref(refs, prefix, fn, 0, flag, cb_data);
 }
 
-int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data)
+int for_each_replace_ref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref(get_main_ref_store(r),
+	return do_for_each_ref(get_main_ref_store(the_repository),
 			       git_replace_ref_base, fn,
 			       strlen(git_replace_ref_base),
 			       DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
diff --git a/refs.h b/refs.h
index cc2fb4c68c0..48d5ffd2082 100644
--- a/refs.h
+++ b/refs.h
@@ -307,7 +307,7 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,
 int for_each_tag_ref(each_ref_fn fn, void *cb_data);
 int for_each_branch_ref(each_ref_fn fn, void *cb_data);
 int for_each_remote_ref(each_ref_fn fn, void *cb_data);
-int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data);
+int for_each_replace_ref(each_ref_fn fn, void *cb_data);
 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);
diff --git a/replace-object.c b/replace-object.c
index e99fcd1ff6e..ee3374ab59b 100644
--- a/replace-object.c
+++ b/replace-object.c
@@ -41,7 +41,7 @@ static void prepare_replace_object(struct repository *r)
 		xmalloc(sizeof(*r->objects->replace_map));
 	oidmap_init(r->objects->replace_map, 0);
 
-	for_each_replace_ref(r, register_replace_ref, r);
+	for_each_replace_ref(register_replace_ref, r);
 }
 
 /* We allow "recursive" replacement. Only within reason, though */

-- 
2.18.0.132.g195c49a2227
Jonathan Tan· Jul 31, 2018, 00:18 UTC · re: Stefan Beller · lore

Re: [PATCH 2/2] refs: switch for_each_replace_ref back to use a ref_store

> So let's go back to the clean API, just requiring a ref_store as an
> argument.
Here, you say that we want ref_store as an argument...
Show 8 quoted lines
> -int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data)
> +int for_each_replace_ref(each_ref_fn fn, void *cb_data)
>  {
> -	return do_for_each_ref(get_main_ref_store(r),
> +	return do_for_each_ref(get_main_ref_store(the_repository),
>  			       git_replace_ref_base, fn,
>  			       strlen(git_replace_ref_base),
>  			       DO_FOR_EACH_INCLUDE_BROKEN, cb_data);

...but there is no ref_store as an argument here - instead, the repository argument is deleted with no replacement. I presume you meant to replace it with a ref_store instead? (This will also fix the issue that for_each_replace_ref only works on the_repository.)

Taking a step back, was there anything that prompted these patches? Maybe at least the 2nd one should wait until we have a situation that warrants it (for example, if we want to for_each_replace_ref(), but we only have a ref_store, not a repository).

Stefan Beller· Jul 31, 2018, 00:41 UTC · re: Jonathan Tan · lore

Re: [PATCH 2/2] refs: switch for_each_replace_ref back to use a ref_store

On Mon, Jul 30, 2018 at 5:19 PM Jonathan Tan <jonathantanmy@google.com> wrote:
Show 5 quoted lines
>
> > So let's go back to the clean API, just requiring a ref_store as an
> > argument.
>
> Here, you say that we want ref_store as an argument...
I do.
Show 14 quoted lines
>
> > -int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data)
> > +int for_each_replace_ref(each_ref_fn fn, void *cb_data)
> >  {
> > -     return do_for_each_ref(get_main_ref_store(r),
> > +     return do_for_each_ref(get_main_ref_store(the_repository),
> >                              git_replace_ref_base, fn,
> >                              strlen(git_replace_ref_base),
> >                              DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
>
> ...but there is no ref_store as an argument here - instead, the
> repository argument is deleted with no replacement. I presume you meant
> to replace it with a ref_store instead? (This will also fix the issue
> that for_each_replace_ref only works on the_repository.)

Yes, I would want to pass in a ref_store and use that as the first argument in do_for_each_ref for now.

That would reduce the API uncleanliness to have to pass the repository twice.
> Taking a step back, was there anything that prompted these patches?
I am flailing around on how to approach the ref store and the repository:
* I dislike having to pass a repository 'r' twice. (current situation after
  patch 1. That patch itself is part of Stolees larger series to address
  commit graphs and replace refs, so we will have that one way or another)
* So I sent out some RFC patches to have the_repository in the ref store
  and pass the repo through to all the call backs to make it easy for
  users inside the callback to do basic things like looking up commits.
* both Duy (on list) and Brandon (privately) expressed their dislike for
  having the refs API bloated with the repository, as the repository is
  not needed per se in the ref store.
* After some reflection I agreed with their concerns, which let me
  to re-examine the refs API: all but a few select functions take a
  ref_store as the first argument (or imply to work on the ref store
  in the_repository, then neither a repo nor a ref store argument is
  there)
* I want to bring back the cleanliness of the API, which is to take a
  ref store when needed instead of the repository, which is rather
  bloated.
> Maybe at least the 2nd one should wait until we have a situation that
> warrants it (for example, if we want to for_each_replace_ref(), but we
> only have a ref_store, not a repository).

okay, then let's drop this series for now and I'll re-examine what is needed to have submodule handling in-core.

Thanks, Stefan

Duy Nguyen· Jul 31, 2018, 16:17 UTC · re: Stefan Beller · lore

Re: [PATCH 2/2] refs: switch for_each_replace_ref back to use a ref_store

On Tue, Jul 31, 2018 at 2:41 AM Stefan Beller <sbeller@google.com> wrote:
Show 17 quoted lines
> > Taking a step back, was there anything that prompted these patches?
>
> I am flailing around on how to approach the ref store and the repository:
> * I dislike having to pass a repository 'r' twice. (current situation after
>   patch 1. That patch itself is part of Stolees larger series to address
>   commit graphs and replace refs, so we will have that one way or another)
> * So I sent out some RFC patches to have the_repository in the ref store
>   and pass the repo through to all the call backs to make it easy for
>   users inside the callback to do basic things like looking up commits.
> * both Duy (on list) and Brandon (privately) expressed their dislike for
>   having the refs API bloated with the repository, as the repository is
>   not needed per se in the ref store.
> * After some reflection I agreed with their concerns, which let me
>   to re-examine the refs API: all but a few select functions take a
>   ref_store as the first argument (or imply to work on the ref store
>   in the_repository, then neither a repo nor a ref store argument is
>   there)

Since I'm the one who added the refs_* variants (which take ref_store as the first argument). There's one thing that I should have done but did not: making each_ref_fn takes the ref store.

If a callback is given a refname and wants to do something about it (other that just printing it), chances are you need the same ref-store that triggers the callback and you should not need to pass a separate ref-store around by yourself because you would have the same "passing twice" problem that you disliked. This is more obvious with refs_for_each_reflog() because you will very likely want to parse the ref from the callback.

Then, even ref store code needs access to object database and I don't think we want to pass a pair of "struct repository *", "struct ref_store *" in every API. We know the ref store has to be associated with one repository and we do save that information (notice that ref_store_init_fn takes gitdir and the "files" backend does save it). Once refs code is adapted to struct repository, I think it will take a 'struct repository *' instead of the gitdir string and store the pointer to the repository too for internal use.

Then if a ref callback needs access to the same repository, we could just provide this repo via refs api. Since callbacks should already have access to the ref store (preferably without having to carrying it via cb_data), it has access to the repository as well and you don't need to explicitly pass the repository.

> * I want to bring back the cleanliness of the API, which is to take a
>   ref store when needed instead of the repository, which is rather
>   bloated.
-- 
Duy;
Stefan Beller· Jul 27, 2018, 00:36 UTC · re: Stefan Beller · lore

[PATCH 3/3] replace: migrate to for_each_replace_repo_ref

By upgrading the replace mechanism works well for all repositories
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/replace.c | 9 +++++----
 replace-object.c  | 7 ++++---
 2 files changed, 9 insertions(+), 7 deletions(-)
Show changes to 2 files +9 −7

builtin/replace.c, replace-object.c

diff --git a/builtin/replace.c b/builtin/replace.c
index ef22d724bbc..fd8a935eb77 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -39,7 +39,8 @@ struct show_data {
 	enum replace_format format;
 };
 
-static int show_reference(const char *refname, const struct object_id *oid,
+static int show_reference(struct repository *r, const char *refname,
+			  const struct object_id *oid,
 			  int flag, void *cb_data)
 {
 	struct show_data *data = cb_data;
@@ -56,9 +57,9 @@ static int show_reference(const char *refname, const struct object_id *oid,
 			if (get_oid(refname, &object))
 				return error("Failed to resolve '%s' as a valid ref.", refname);
 
-			obj_type = oid_object_info(the_repository, &object,
+			obj_type = oid_object_info(r, &object,
 						   NULL);
-			repl_type = oid_object_info(the_repository, oid, NULL);
+			repl_type = oid_object_info(r, oid, NULL);
 
 			printf("%s (%s) -> %s (%s)\n", refname, type_name(obj_type),
 			       oid_to_hex(oid), type_name(repl_type));
@@ -87,7 +88,7 @@ static int list_replace_refs(const char *pattern, const char *format)
 			     "valid formats are 'short', 'medium' and 'long'\n",
 			     format);
 
-	for_each_replace_ref(the_repository, show_reference, (void *)&data);
+	for_each_replace_repo_ref(the_repository, show_reference, (void *)&data);
 
 	return 0;
 }
diff --git a/replace-object.c b/replace-object.c
index 801b5c16789..c0457b8048c 100644
--- a/replace-object.c
+++ b/replace-object.c
@@ -6,7 +6,8 @@
 #include "repository.h"
 #include "commit.h"
 
-static int register_replace_ref(const char *refname,
+static int register_replace_ref(struct repository *r,
+				const char *refname,
 				const struct object_id *oid,
 				int flag, void *cb_data)
 {
@@ -25,7 +26,7 @@ static int register_replace_ref(const char *refname,
 	oidcpy(&repl_obj->replacement, oid);
 
 	/* Register new object */
-	if (oidmap_put(the_repository->objects->replace_map, repl_obj))
+	if (oidmap_put(r->objects->replace_map, repl_obj))
 		die("duplicate replace ref: %s", refname);
 
 	return 0;
@@ -40,7 +41,7 @@ static void prepare_replace_object(struct repository *r)
 		xmalloc(sizeof(*r->objects->replace_map));
 	oidmap_init(r->objects->replace_map, 0);
 
-	for_each_replace_ref(r, register_replace_ref, NULL);
+	for_each_replace_repo_ref(r, register_replace_ref, NULL);
 }
 
 /* We allow "recursive" replacement. Only within reason, though */

-- 
2.18.0.345.g5c9ce644c3-goog

← back to recent threads