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

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

[Stefan Beller]

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

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

[Stefan Beller]

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(-)

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

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

[Stefan Beller]

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(-)

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

[PATCH 3/3] replace: migrate to for_each_replace_repo_ref

[Stefan Beller]

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(-)

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

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

[Duy Nguyen]

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

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

[Brandon Williams]

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?

-- 
Brandon Williams

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

[Stefan Beller]

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.

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

[Duy Nguyen]

On Fri, Jul 27, 2018 at 7:31 PM Stefan Beller <sbeller@google.com> wrote:
>
> 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

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

[Stefan Beller]

> > 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

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

[Stefan Beller]

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(-)

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

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

[Stefan Beller]

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(-)

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

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

[Jonathan Tan]

> 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...

> -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).

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

[Stefan Beller]

On Mon, Jul 30, 2018 at 5:19 PM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> > 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.

>
> > -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

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

[Duy Nguyen]

On Tue, Jul 31, 2018 at 2:41 AM Stefan Beller <sbeller@google.com> wrote:
> > 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;