On Mon, Mar 20, 2017 at 05:33:09PM +0100, Michael Haggerty wrote:
> 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.
> 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