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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 1, 2023, 04:43 UTC
Message-ID: <xmqqedmveqs2.fsf@gitster.g>
In-Reply-To: <kl6lr0qwno2q.fsf@chooglen-macbookpro.roam.corp.google.com>

Glen Choo <chooglen@google.com> writes:

Show 15 quoted lines

>> @@ -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.

Show 5 quoted lines

> 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.