Re: [PATCH v2 0/3] remote: replace static variables with struct remote_state

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 13, 2021, 20:11 UTC
Message-ID: <xmqqtuhkfzw8.fsf@gitster.g>
In-Reply-To: <20211013193127.76537-1-chooglen@google.com>

Glen Choo <chooglen@google.com> writes:

Show 16 quoted lines

> While this concern is valid, I decided to keep this interface for a few
> reasons:
>
> 1. The correct way of calling the function is 'obvious'.
> 2. It is relatively easy to get the contained object (struct branch/remote)
>    and its containing struct repository/remote_state (e.g. we don't pass
>    struct branch or remote through long call chains). For "struct
>    branch", callers usually get the branch from the repo and use it
>    immediately. For "struct remote", we don't use container objects
>    outside of static functions. If you are interested in seeing all of
>    the call sites, you can see a sample commit in [3].
> 3. The separation between container/contained objects allows us to
>    reason better about what the correct interface is. e.g. we might be
>    tempted to include a backpointer from struct branch to struct
>    remote_state so that we can pass around struct branch and be
>    confident that struct branch has all of the information it needs.

I am not following. None of the above reasons argue for forcing the functions that take contained object to also take its container.

Show 7 quoted lines

>    However, I believe that some questions *shouldn't* be answered by
>    just struct branch. The prime example in this series is
>    branch_get_push() - it conceptually answers 'What is the pushremote
>    of this branch', but the behavior we want is closer to 'If
>    configured, give me the pushremote for the branch. Otherwise, give me
>    the default pushremote of the repo.'. This is arguably a relevant
>    detail that should be exposed to callers.

It is a good example why such a function can just take an instance of branch, and the caller,

 (1) who does care about the fallback, can be assured that the logic
     falls back to the correct repository; and

 (2) who does not care about the fallback and sees it a mere
     implementation detail of "I am on this branch; give me the
     remote to push to", do not have to know what, other than the
     branch object, needs to be passed.

if we explicitly record a branch object which repository it was taken from.

There may be some other (real) reason where the resistance comes from, that you may not be telling us, though. But in what was described in the message I am responding to, I didn't see much convincing reason to argue _for_ keeping the contained objects ignorant of the container and forcing callers to pass both to functions that use both the container and contained to compute something.

Thanks.