Re: [PATCH 07/16] refs: add repository argument to get_main_ref_store

From: Stefan Beller <sbeller@google.com>
Date: Apr 10, 2018, 18:27 UTC
Message-ID: <CAGZ79kbAaHVAGXPhL9zBmkQp9f4=su7NdXn7RSy_PP=5Gd-Z5w@mail.gmail.com>
In-Reply-To: <d338f068-4ccd-292d-c65b-342fab4cd018@alum.mit.edu>

Hi Michael,

On Tue, Apr 10, 2018 at 6:36 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:

Show 13 quoted lines

> On 04/10/2018 12:45 AM, Stefan Beller wrote:
>> Add a repository argument to allow the get_main_ref_store caller
>> to be more specific about which repository to handle. This is a small
>> mechanical change; it doesn't change the implementation to handle
>> repositories other than the_repository yet.
>>
>> As with the previous commits, use a macro to catch callers passing a
>> repository other than the_repository at compile time.
>
> This seems OK to me from a refs perspective.
>
> The macro trick is surprising. I guess it gets you a compile-time check,
> under the assumption that nothing else is called `the_repository`.

Yes. Credit goes to Jonathan Tan for this trick.

> But
> why actually commit the macro, as opposed to compiling once locally to
> check for correctness, then maybe add something like `assert(r ==
> the_repository)` for the actual commit?

The eternal struggle of contributing patches that are easy to review. ;)

With the assert we'll have a run time check, which is not desirable compared to a compile time check. And from a reviewers point of view running a "rebase -x make" on the series that Junio queued is easier than to reason about the "assert(r = the_repository)" IMHO.

> But I don't care either way, since the macro disappears again soon.

Glad you're ok with this approach.

Thanks for looking at the refs specific code, Stefan