Re: [PATCH v3 2/4] path: optimize common dir checking

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Aug 15, 2015, 18:12 UTC
Message-ID: <55CF811A.8060106@alum.mit.edu>
In-Reply-To: <xmqqtws1iyxn.fsf@gitster.dls.corp.google.com>

On 08/14/2015 07:04 PM, Junio C Hamano wrote:

Show 23 quoted lines

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Let's take a step back.
>>
>> We have always had a ton of code that uses `git_path()` and friends to
>> convert abstract things into filesystem paths. Let's take the
>> reference-handling code as an example:
>> ...
>> This seems crazy to me. It is the *reference* code that should know
>> whether a particular reference should be stored under `$GIT_DIR` or
>> `$GIT_COMMON_DIR`, or indeed whether it should be stored in a database.
> 
> It is more like:
> 
>  1. The system as a whole should decide if HEAD and refs/heads/
>     should be per workspace or shared across a repository (and we say
>     the former should be per workspace, the latter should be shared).
> 
>  2. The reference code should decide which ref-backend is used to
>     store refs.
> 
>  3. And any ref-backend should follow the decision made by the
>     system as a whole in 1.

If I understand correctly, you consider the decision of where a particular reference should be stored to be a kind of "business logic" decision that should live outside of the refs module. I don't think it is so important whether this knowledge is inside or outside of the refs module (I can live with it either way).

Show 9 quoted lines

> I'd imagine that David's ref-backend code inherited from Ronnie
> would still accept the string "refs/heads/master" from the rest of
> the system (i.e. callers that call into the ref API) to mean "the
> ref that represents the 'master' branch", and uses that as the key
> to decide "ok, that is shared across workspaces" to honor the
> system-wide decision made in 1.  The outside callers wouldn't pass
> the result of calling git_path("refs/heads/master") into the ref
> API, which may expand to "$somewhere_else/refs/heads/master" when
> run in a secondary workspace to point at the common location.

Definitely agreed.

Show 9 quoted lines

> I'd also imagine that the workspace API would give ways for the
> implementation of the reference API to ask these questions:
> 
>  - which workspace am I operating for?  where is the "common" thing?
>    how would I identify this workspace among the ones that share the
>    same "common" thing?
> 
>  - is this ref (or ref-like thing) supposed to be in common or per
>    workspace?

Yes, I especially like this last idea. For example, suppose the function is "is_common_reference(refname)". It's nice that this function doesn't have to know about all possible "things" like your is_common_thing() function. Therefore it can be simpler. Almost always the caller (and in this case the caller would usually be within the refs module) will know that the "thing" it wants to inquire about is a reference name, so it can spare the extra expense of calling is_common_thing().

If we still need is_common_thing() (e.g., for the implementation of `git rev-parse --git-path`), its definition would become something like

    return is_common_reference(thing) ||
           is_common_file(thing) ||
           is_common_flurg(thing) || ...;

In this construction I think it is is clear that is_common_reference() would fit pretty well in the refs module, though elsewhere would be OK too.

> [...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu