On Fri, 2016-02-26 at 23:06 -0500, Jeff King wrote:
> On Wed, Feb 24, 2016 at 05:58:37PM -0500, David Turner wrote:
>
> > +int set_ref_storage_backend(const char *name)
> > +{
> > + struct ref_storage_be *be;
> > +
> > + for (be = refs_backends; be; be = be->next)
> > + if (!strcmp(be->name, name)) {
> > + the_refs_backend = be;
> > + return 0;
> > + }
> > + return 1;
> > +}
>
> So we search through the list and assign the_refs_backend if we find
> something, returning 0 for success, and 1 if we found nothing. OK
> (though our usual convention is that if 0 is success, -1 is the error
> case).
Will fix.
> > +int ref_storage_backend_exists(const char *name)
> > +{
> > + struct ref_storage_be *be;
> > +
> > + for (be = refs_backends; be; be = be->next)
> > + if (!strcmp(be->name, name)) {
> > + the_refs_backend = be;
> > + return 1;
> > + }
> > + return 0;
> > +}
>
> Here we do the same thing, but we get "1" for exists, and "0"
> otherwise. That makes sense if this is about querying for existence.
> But
> why does the query function still set the_refs_backend?
Yeah, that's wrong.
> I'm guessing the assignment in the second one is just copy-pasta, but
> maybe the whole thing would be simpler if they could share the
> implementation, like:
>
> struct ref_storage_be *find_ref_storage_backend(const char *name)
> {
> struct ref_storage *be;
> for (be = refs_backends; be; be = be->next)
> if (!strcmp(be->name, name))
> return be;
> return NULL;
> }
>
> int set_ref_storage_backend(const char *name)
> {
> struct ref_storage *be = find_ref_storage_backend(name);
> if (!be)
> return -1;
> the_refs_backend = be;
> return 0;
> }
>
> You don't really need an "exists", as you can check that "find"
> doesn't
> return NULL, but you could wrap it, of course.
I'd rather wrap it to keep the backends firmly inside the refs-internal
barrier.
Thanks.
--
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