Re: [PATCH 05/38] refs: create a base class "ref_store" for files_ref_store

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 7, 2016, 17:03 UTC
Message-ID: <xmqqbn3drkwd.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <8ee7d7359f6763cba29dc788aba2f236204bb76e.1464983301.git.mhagger@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

> We want ref_stores to be polymorphic, so invent a base class of which
> files_ref_store is a derived class. For now there is a one-to-one
> relationship between ref_stores and submodules.

The mention of "submodules" made me go "Huh?" but thinking about it for a second it is clear and obvious. We often peek into refs in a different repository that is a submodule, and we do not mix them with our own refs. Logically that is what a "ref store" is, and one-to-one relationship is expected.

Show 8 quoted lines

> @@ -1284,3 +1288,90 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
>  	errno = ELOOP;
>  	return NULL;
>  }
> +
> +static struct ref_store *main_ref_store = NULL;
> +
> +static struct ref_store *submodule_ref_stores = NULL;

Let's let BSS take care of these initialization.

Show 39 quoted lines

> +/*
> + * Downcast ref_store to files_ref_store. Die if ref_store is not a
> + * files_ref_store. If submodule_allowed is not true, then also die if
> + * files_ref_store is for a submodule (i.e., not for the main
> + * repository). caller is used in any necessary error messages.
> + */
> +static struct files_ref_store *files_downcast(
> +		struct ref_store *ref_store, int submodule_allowed,
> +		const char *caller)
>  {
>  	struct files_ref_store *refs;
>  
> +	if (ref_store->be != &refs_be_files)
> +		die("BUG: ref_store is type \"%s\" not \"files\" in %s",
> +		    ref_store->be->name, caller);
>  
> +	refs = (struct files_ref_store *)ref_store;
> +
> +	if (!submodule_allowed)
> +		assert_main_repository(ref_store, caller);
> +
> +	return refs;
>  }
>
>  /*
> + * Return a pointer to the reference store for the specified
> + * submodule. For the main repository, use submodule==NULL; such a
> + * call cannot fail. For a submodule, the submodule must exist and be
> + * a nonbare repository, otherwise return NULL. Verify that the
> + * reference store is a files_ref_store, and cast it to that type
> + * before returning it.
>   */
> +static struct files_ref_store *get_files_ref_store(const char *submodule,
> +						   const char *caller)
>  {
> +	struct ref_store *refs = get_ref_store(submodule);
>  
> +	return refs ? files_downcast(refs, 1, caller) : NULL;
>  }

This comment may be barking up a wrong tree, but the support for class inheritance makes me wonder if I can do something along this line:

 * implement a filesystem based ref_store, that is very similar to
   what you have as files_ref_store, but

   - when storing a ref as a loose ref, or when checking if a ref
     exists as a loose ref, quote them somehow (e.g. a branch
     "Branch" is not stored as a file "$GIT_DIR/refs/heads/branch"
     but by using "^" as a single shift marker, i.e. as
     "$GIT_DIR/refs/heads/^branch");

   - when enumerating what refs we have as loose refs, unquote what
     readdir(3) gave us, e.g. seeing "$GIT_DIR/refs/heads/^branch",
     I say "there is a branch whose name is 'Branch'".

 * as locking and other 'methods' are likely to be very similar to
   your files_ref_store, make this new backend as a subclass of it,
   i.e. create a new class but function pointers to many methods are
   copied from files ref_store vtable.

Would the strict "when downcasting to 'files', we make sure vtable is that of 'files' backend" interfere with such an approach?