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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Jun 9, 2016, 14:10 UTC
Message-ID: <575978DA.30608@alum.mit.edu>
In-Reply-To: <xmqqbn3drkwd.fsf@gitster.mtv.corp.google.com>

On 06/07/2016 07:03 PM, Junio C Hamano wrote:

Show 22 quoted lines

> 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.
> 
>> @@ -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.

I like the `= NULL` because it expresses "yes, I care about the initial values of these variables", which to me is more valuable than saving a few bytes in the size of the executable. But in fact, GCC does the obvious optimization: it detects that these variables are being initialized to zero, and puts them in BSS anyway. I'd be surprised if other compilers don't do the same. So I'd prefer to leave this as-is, if it's OK with you.

Show 38 quoted lines

>> [...]
>>  /*
>> + * 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.

That is definitely something we could implement in the future. If I were going to design an extension like this, I think I'd go straight to something more expressive; maybe something like URL-encoding. For example, we might like a system that can record refnames with a Unicode encoding that is determined by us rather than by the filesystem.

Depending on the details, it might be preferable to implement the new ref-store as an encoding layer that delegates to an old-fashioned files backend rather than using inheritance. In fact, you'd only want to do the translation for the loose storage part, so in the end the implemention might look something like

    overlay_ref_store(
        encoding_ref_store(loose_ref_store()),
        packed_ref_store())

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

True, the simple approach that I use above doesn't generalize to implementation inheritance. But I'd rather cross that bridge when we come to it. Implementing an RTTI system in C is a bit ambitious and, I think, comes with runtime costs.

Michael