Re: [PATCH v4 6/6] remote: add struct repository parameter to external functions

From: Jonathan Tan <jonathantanmy@google.com>
To: chooglen@google.com
Cc: git@vger.kernel.org, gitster@pobox.com,
	Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH v4 6/6] remote: add struct repository parameter to external functions
Date: Mon, 15 Nov 2021 10:55:45 -0800	[thread overview]
Message-ID: <20211115185545.1082815-1-jonathantanmy@google.com> (raw)
In-Reply-To: <20211028183101.41013-7-chooglen@google.com>

> Finish plumbing remote_state by adding a struct repository
> parameter to repo_* functions. This removes all references to
> the_repository->remote_state in remote.c - explicit references in the
> function bodies and implicit references via calls to non-repo_*
> functions. However, certain functions still use the_repository to parse
> oids.
> 
> Where a non-static function has the option to call either the static
> "remotes_*" variant or the non-static "repo_*" variant, use "remotes_*"
> in order to be consistent with the static functions. A desirable
> side-effect of this is that it minimizes the number of calls to
> read_config().

I don't feel as confident about this patch as I do about the patches
that remove the global variables - as you said, some functions might use
the_repository to parse OIDs (or do other things). I think that part of
this patch should go into the next series you have planned (as you
describe in the original cover letter [1], when you make future
submodule commands be run in-process), so that they can be tested.
(Removing the global variables was more of a mechanical code change, so
it was easier to see what's going on by just looking at "before" and
"after".)

Overall this patch series looks good with only minor comments from me,
except that I think that this patch (patch 6) should be dropped.

[1] https://lore.kernel.org/git/pull.1103.git.git.1633633635.gitgitgadget@gmail.com/