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

[David Turner <dturner@twopensource.com>]

On Thu, 2016-02-11 at 09:42 +0100, Michael Haggerty wrote:
> On 02/05/2016 08:44 PM, David Turner wrote:
> > From: Ronnie Sahlberg <sahlberg@google.com>
> > 
> > Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> > Signed-off-by: David Turner <dturner@twopensource.com>
> > ---
> >  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.