Re: [PATCH v4 03/21] refs: add methods for the ref iterators

On Thu, 2016-02-11 at 09:42 +0100, Michael Haggerty wrote:

On 02/05/2016 08:44 PM, David Turner wrote:

quoted

From: Ronnie Sahlberg <redacted>

Signed-off-by: Ronnie Sahlberg <redacted>
Signed-off-by: David Turner <redacted>
---
 refs.c               | 54
++++++++++++++++++++++++++++++++++++++++++++++++++++
 refs/files-backend.c | 41 +++++++++++++++++++++++++++------------
 refs/refs-internal.h | 29 ++++++++++++++++++++++++++++
 3 files changed, 112 insertions(+), 12 deletions(-)

diff --git a/refs.c b/refs.c
index afdde7d..e598b73 100644
--- a/refs.c
+++ b/refs.c

@@ -1158,3 +1158,57 @@ int resolve_gitlink_ref(const char *path,

const char *refname,
 {
 	return the_refs_backend->resolve_gitlink_ref(path,
refname, sha1);
 }
+
+int head_ref(each_ref_fn fn, void *cb_data)
+{
+	return the_refs_backend->head_ref(fn, cb_data);
+}
+
+int head_ref_submodule(const char *submodule, each_ref_fn fn, void
*cb_data)
+{
+	return the_refs_backend->head_ref_submodule(submodule, fn,
cb_data);
+}
+

I think it is unnecessary to have so many virtual functions. For
example, here you have made head_ref_submodule() virtual. But the
files
and lmdb implementations of this function are identical. They both
call
do_head_ref(), which are (confusingly) two independent static
functions.
But those functions are *also* defined identically, namely

After all, what else would they possibly want to do? And both
resolve_gitlink_ref() and read_ref_full() are already virtual
functions
(actually read_ref_full() only has a single definition but it calls
the
virtualized resolve_ref_unsafe()).

So it seems to me that it is unnecessary for head_ref_submodule() to
be
virtual, and that there only needs to be one definition of
do_head_ref().

(Off-topic: for that matter, do_head_ref() is itself almost
pointless.
It could easily be inlined if we were sure that nobody passes it
submodule==NULL.)

Added a commit to move these to common code and inline do_head_ref.

Similarly, I bet that if do_for_each_ref() were just a little bit
more
capable, then the following functions could also remain non-virtual
(i.e., one definition could be shared across all backend
implementations):

* for_each_ref()
...

Yeah, that makes sense.  I think we've grown a few more of those funcs
since Ronnie's version, so virtualizing do_for_each_ref makes more
sense.