On Tue, Aug 29, 2023 at 04:55:30PM +0100, Phillip Wood wrote:
> > We could change the path helper to use REPO_GIT_PATH_FUNC(), but doing
> > so piecemeal is not great. There are 41 uses of GIT_PATH_FUNC() in
> > sequencer.c,
>
> Wow, I knew there were quite a few but I hadn't realized there were that
> many. Changing them all to take a struct repository will be a big change and
> will make struct repo_cache_path a lot larger.
Yeah. One of the things that gave me pause is that some of them may need
to be renamed, as well. Most of the existing ones are static local
within sequencer.c, so names like git_path_head_file() are OK. But
REPO_GIT_PATH_FUNC() requires a pointer in the global repo_cache_path,
so the names need to be appropriate for a global view.
> > Note that we could also drop this parameter entirely, as the function is
> > always called directly, and not as a callback that has to conform to
> > some external interface. But since we'd eventually want to use the
> > repository parameter, let's leave it in place to avoid disrupting the
> > callers twice.
>
> I think that makes sense as we're going to need that argument eventually. I
> was curious as to why this function takes a repository argument. When the
> function was added in 4a72486de97 (fix cherry-pick/revert status after
> commit, 2019-04-16) it called parse_insn_line() which takes a repository
> argument. It was refactored in ed5b1ca10b (status: do not report errors in
> sequencer/todo, 2019-06-27) and I failed to notice that the repository was
> unused afterwards.
Thanks for digging there. I usually do something similar for these
patches (to help verify that there's no extra bug lurking), but with
"struct repository" ones, the answer was usually uninteresting ("it
should be used, but the underlying functions don't support it" kind of
thing).
Since I'm re-rolling anyway, I'll throw your research into the commit
message.
-Peff
