Re: [PATCH 04/20] refs_verify_refname_available(): implement once for all backends

From: Jeff King <peff@peff.net>
Date: Mar 20, 2017, 17:42 UTC
Message-ID: <20170320174240.gykldqzbqyva6kbs@sigill.intra.peff.net>
In-Reply-To: <22abd274bfdada94b3654a811ee209822640765f.1490026594.git.mhagger@alum.mit.edu>

On Mon, Mar 20, 2017 at 05:33:09PM +0100, Michael Haggerty wrote:

Show 5 quoted lines

> It turns out that we can now implement
> `refs_verify_refname_available()` based on the other virtual
> functions, so there is no need for it to be defined at the backend
> level. Instead, define it once in `refs.c` and remove the
> `files_backend` definition.

Does this mean that backends can no longer provide storage for D/F-conflicted refnames (i.e., "refs/foo" and "refs/foo/bar")? It looks like the global verify_refname_available() has that logic baked in.

I know that was a complex subject because of the potential compatibility issues (e.g., fetching from a server with a more capable backend), but I'd just worry we are shutting a door on some options. OTOH, it probably wouldn't be that hard for the global function to check a flag specific to the ref_store, and allow such refs when it is set.

Show 21 quoted lines

>  int refs_verify_refname_available(struct ref_store *refs,
>  				  const char *refname,
> -				  const struct string_list *extra,
> +				  const struct string_list *extras,
>  				  const struct string_list *skip,
>  				  struct strbuf *err)
>  {
> [...]
> +		/*
> +		 * We are still at a leading dir of the refname (e.g.,
> +		 * "refs/foo"; if there is a reference with that name,
> +		 * it is a conflict, *unless* it is in skip.
> +		 */
> +		if (skip && string_list_has_string(skip, dirname.buf))
> +			continue;
> +
> +		if (!refs_read_raw_ref(refs, dirname.buf, oid.hash, &referent, &type)) {
> +			strbuf_addf(err, "'%s' exists; cannot create '%s'",
> +				    dirname.buf, refname);
> +			goto cleanup;
> +		}

We don't really care about reading the ref value here; we just care if it exists. Does that matter for efficiency (e.g., for the files backend it's a stat() versus an open/read/close)? I guess the difference only matters when it _does_ exist, which is the uncommon error case.

(Also, I suspect the loose ref cache always just reads everything in the current code, though with the iterator approach in theory we could stop doing that).

-Peff