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@xxxxxxxxxx>
> >
> > Signed-off-by: Ronnie Sahlberg <sahlberg@xxxxxxxxxx>
> > Signed-off-by: David Turner <dturner@xxxxxxxxxxxxxxxx>
> > ---
> > 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html