Subject: Re: [PATCH v2 3/3] repository: move 'repository_format_worktree_config' to repo scope

From: Junio C Hamano <[email-masked]>
Date: Thu, 01 Jun 2023 13:43:25 +0900

Glen Choo <[email-masked]> writes:

>> @@ -1423,6 +1422,9 @@ int discover_git_directory(struct strbuf *commondir,
>>  		return -1;
>>  	}
>>  
>> +	the_repository->repository_format_worktree_config =
>> +		candidate.worktree_config;
>> +
>>  	/* take ownership of candidate.partial_clone */
>>  	the_repository->repository_format_partial_clone =
>>  		candidate.partial_clone;
>
> This hunk does not copy .hash_algo. I initially wondered if it is safe
> to just copy .hash_algo here too, but I now suspect that we shouldn't
> have done the_repository setup in discover_git_directory() in the first
> place.

That's quite a departure from the established practice, isn't it?
Due to recent and not so recent header shuffling (moving everything
out of cache.h, dropping "extern", etc.), "git blame" is a bit hard
to follow, but ever since 16ac8b8d (setup: introduce the
discover_git_directory() function, 2017-03-13) added the function,
we do execute the "setup" when we know we are in a repository.

It would probably be worth mentioning that the "global state" Dscho
refers to in that commit is primarily about the current directory of
the Git process.  During the discovery, we used to go up one level
at a time and tried to see if the current directory is either the
top of the working tree (i.e.  has ".git/" that is a git repository)
or the top of a GIT_DIR-looking directory.  That was changed in
ce9b8aab (setup_git_directory_1(): avoid changing global state,
2017-03-13) in the same series and discusses what "global state" the
series addresses.

If a relatively recent and oddball caller calls the function when it
does not want any of the setup donw after finding out that we could
use the directory as a repository, a new early "pure discovery" part
should be split out of the function, and both the function itself
and the oddball caller should be taught to call that pure-discovery
helper, I think.

> If I'm wrong and we _should_ be doing the_repository setup, then I'm
> guessing it's safe to copy .hash_algo here too. So either way, I think
> we should introduce a helper function to do the copying, especially
> because we will probably need to repeat this process yet again for
> "repository_format_precious_objects".

I do not know (or care in the context of this thread) about the
"precious objects" bit, but .worktree-config is the third one on top
of .hash_algo and .partial_clone, and it generally is a good time to
refactor when you find yourself adding the third instance of
repetitive code.  So I agree with you that it is time to introduce a
helper function to copy from a "struct repository_format" to the
repository instance.