On Thu, 8 Jun 2017 16:40:28 -0700 Brandon Williams <bmwill@xxxxxxxxxx> wrote: > When I sent out my RFC series there seemed to be a lot of interest but I > haven't seen many people jump to review this series. Despite lack of review I > wanted to get out another version which includes some changes to fix things > that were bugging me about the series. Hopfully this v2 will prod some more > people to take a look. > > The meat of the series is really from 04-15 and patch 32 which converts > ls-files to recurse using a repository object. So if you're pressed for time > you can focus on those patches. Thanks - I can see in patch 32 that one immediate benefit is that ls-files can now recurse into submodules without needing to invoke an extra process just to change the GIT_DIR. Before I get into the details, I have some questions: 1. I am concerned that "struct repository" will end up growing without bounds as we store more and more repo-specific concerns in it. Could it be restricted to just the fields populated by repo_init()? repo_read_index() will then return the index itself, instead of using "struct repository" as a cache. This means that code using repo_read_index() will need to maintain its own variable holding the returned index, but that is likely a positive - it's better for code to just pass around the specific thing needed between functions anyway, as opposed to passing a giant "struct repository" (which partially defeats the purpose of eliminating the usage of globals). 2. If we do the above, another potential benefit is that (I think) the repo_config_get_.* functions would no longer be necessary, since we would have a function that generates a "struct config_set" given a repo, and then we just use the git_configset_get_.* functions on it. This would also reduce the urgency of extracting the config methods into its own header file, and reduce the size of this patch set. (I was also going to suggest that you remove the "convert" patches, but that is not possible - ls-files uses get_cached_convert_stats_ascii() from convert.h despite not #include-ing it.)