Re: [PATCH v2 1/3] remote: move static variables into per-repository struct

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 14, 2021, 18:33 UTC
Message-ID: <xmqqr1cnbgmw.fsf@gitster.g>
In-Reply-To: <kl6lk0if1pt3.fsf@chooglen-macbookpro.roam.corp.google.com>

Glen Choo <chooglen@google.com> writes:

> By "here", I assume you mean "the restructuring doesn't belong in
> this topic" vs "the restructuring belongs in this topic but not this
> patch".

Hmph, it is unclear which one you assumed ;-), but anyway I do not think it would help to make such a clean-up as a part of this series.

Adding a pointer to the container to contained types, if done from the beginning without disrupting the function signature (i.e. a function that takes a branch instance can now work on a branch that was taken from any repository, by simply following branch->repo and using it as a parameter to repo_foo_blah() calls it would make instead of existing foo_blah() calls), may help reduce the size of the series greatly, because you wouldn't have to touch may existing callers. That would make a great part of this topic.

But if done as a post clean-up step (i.e. first we sprinkle "struct repo *" pointer as an additional parameter all over the functions that take an object that clearly belongs to a repository, and then later shrink the API to remove the extra pointer parameter by teaching the contained object which repository it belongs to), it would probably not a good idea to have it as part of this series. We'd instead stop at "we add a new parameter to functions to tell in which repository it now should work" in the topic itself.

The "post clean-up" series would be like "the previous series made it 'working', now let's clean-up the API" follow-up that is done separately. One benefit of going that route would be that it would allow us notice there are cases where contained object may have to be used in the context of the container that is different from the container it belongs to, before locking ourselves out of the possibility by limiting the API. I personally do not think we'd reap that benefit as I do not think of a case where it makes sense to take a branch or a remote from a repository and use it in the context of another repository (hence my suggestion to teach objects the repository they belong to), but if others think it is easier to proceed, then...

Thanks.