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

Glen Choo <chooglen@google.com> writes:

meaning, that the paragraphs below will explain why the reference is unnecessary?

Given that the API has been evolved over time from a "we deal only with a single repository and everybody else can just fork into a separate repository" to "now this function can work in the specified repository", it is understandable that some still do take repository as a separate parameter, even though the original counterpart that did not take a repository pointer took an object that clearly can belong to only one repository at a time. They would need to be cleaned up, and they do not make a good excuse to add more of them, when we know we are dealing with objects that belong only to one repository, like the "remote.c" contents we have been discussing.

IOW, we need to start somewhere to clean things up, so either we teach multi-repository support to remote.c from day one, or we pass a repository as a separate and redundant parameter, with the intention to later clean up. I suspect that this piece is isolated enough that it is simpler to use as a starting point, and then the callers into remote.c API can then be cleaned up, gradually extending the circle.

The original counerpart may not have been about "the other object" to begin with (e.g. "we have this string (not an object); resolve it to an object name in the context of the given repository"), in which case they are fine as they are, but I think most of the contents of "remote.c" do not fall into this category.

I do not think it is meant to be internal to begin with. If we declare, after careful consideration, that an in-core branch object given by remote.c API always belongs to one and only one repository, then following the branch->repository pointer should be the documented and kosher way to find out the repository by anybody who is given the branch object. A function that takes repo and branch to pretend as if it can work with an inconsistent pair is an invidation for the interface misuse, but I do not think it is a misuse of the API for a codepath that obtained a branch instance inspecting what repository it came from.

A function that takes a pair of remote and a branch object for whatever reason may want to have an assert() that makes sure they came from the same repository, by the way.

Also, some of the functions involved in this topic may need to say "I have this string, please resolve it as an object name in the context of this repository", and call a function that takes a repository as "context". The repository they use must be given by their caller somehow---is there any valid case where it must be different from the other parameter (i.e. contained object) they are dealing with? I do not think so. So such a function would have to say "resolve this string in the context of the repository branch->repo" because the function was given an in-core branch instance.

This is true. It however is the only valid excuse I can fully agree with for being hesitant to have "where did I come from" information. We do grow each in-core branch and remote instance by an 8-byte pointer (but we reduce the stackframe consumption by 8-byte along each step in the callchain as we do not have to pass a repository as a separate pointer parameter).

Sufficiently separated in the sense that you take a branch from the parent repository and throwing it at a function with a pointer to an in-core repository for a submodule would make some sort of sense?

Sorry, but I do not follow this part of your argument, at all.