Re: [PATCH v6 05/32] refs: add a backend method structure with transaction functions

From: Jeff King <peff@peff.net>
Date: Feb 27, 2016, 04:06 UTC
Message-ID: <20160227040643.GB10829@sigill.intra.peff.net>
In-Reply-To: <1456354744-8022-6-git-send-email-dturner@twopensource.com>

On Wed, Feb 24, 2016 at 05:58:37PM -0500, David Turner wrote:

Show 11 quoted lines

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

Show 11 quoted lines

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

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.

-Peff