Re: [PATCH 04/21] gc: drop the_repository in log location

[Derrick Stolee <stolee@gmail.com>]

On 7/8/2020 10:22 PM, Jonathan Tan wrote:
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> The report_last_gc_error() method use git_pathdup() which implicitly
>> uses the_repository. Replace this with strbuf_repo_path() to get a
>> path buffer we control that uses a given repository pointer.
>>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> 
> Regarding the first 4 patches, it would be great if there was a test
> like the one in test_repository.c (together with a code coverage report,
> perhaps) that verifies that all code paths do not use the_repository.
> 
> But set aside that test for now - I don't think gc.c fully supports
> arbitrary repositories. In particular, when running a subprocess, it
> inherits the environment from the current process. I see that future
> patches try to resolve this by passing "-C", but that does not work if
> environment variables like GIT_DIR are set (because the environment
> variables override the "-C"). Perhaps we need a function that runs a
> subprocess in a specific repository. I ran into the same problem when
> attempting to make fetch-pack (which runs index-pack as a subprocess)
> support arbitrary repositories, but I haven't looked deeply into
> resolving this yet (and I haven't looked at that problem in a while).
> 
> Having said that, I'm fine with these patches being in the set - the
> only negative is that perhaps a reader would be misled into thinking
> that GC supports arbitrary repositories.

I agree. I hope that I do not give the impression that GC is now
safe for arbitrary repositories. I only thought that this was prudent
to do before I start taking new dependencies on the code.

It' probably time for someone to do a round of the_repository cleanups
again, and perhaps the RC window is a good time to think about that
(for submission after the release).

Thanks,
-Stolee