From: Lessley Dennington <lessleydennington@gmail.com>
Date: Mon, 29 Nov 2021 23:38:03 GMT
Subject: Re: [PATCH v4 1/4] sparse index: enable only for git repos
Message-ID: <6db27c92-6ea6-fa5f-8aa7-e1fef7d5ff5a@gmail.com>
In-Reply-To: <xmqqzgpt2z0h.fsf@gitster.g>



On 11/24/21 10:23 AM, Junio C Hamano wrote:
> Lessley Dennington <lessleydennington@gmail.com> writes:
> 
>>>> @@ -1588,8 +1588,10 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>>>>      	git_config(git_checkout_config, opts);
>>>>    -	prepare_repo_settings(the_repository);
>>>> -	the_repository->settings.command_requires_full_index = 0;
>>>> +	if (startup_info->have_repository) {
>>>> +		prepare_repo_settings(the_repository);
>>>> +		the_repository->settings.command_requires_full_index = 0;
>>>> +	}
>>> I am kind-a surprised if the control reaches this deep if you are
>>> not in a repository.  In git.c::commands[] list, all three primary
>>> entry points that call checkout_main(), namely, cmd_checkout(),
>>> cmd_restore(), and cmd_switch(), are marked with RUN_SETUP bit,
>>> which makes us call setup_git_directory() even before we call the
>>> cmd_X() function.  And setup_git_directory() dies with "not a git
>>> repository (or any of the parent directories)" outside a repository.
>>> So, how can startup_info->have_repository bit be false if the
>>> control flow reaches here?  Or am I grossly misunderstanding what
>>> you are trying to do?
>>>
>> This was in reaction to the t0012-help.sh tests failing with the
>> new BUG in prepare_repo_settings. However, Elijah pointed out that
>> it's a better idea to move prepare_repo_settings farther down
>> (after parse_options) instead. So I will be issuing that change as
>> part of v5.
> 
> I forgot that "git foo -h" for a builtin command 'foo' calls
> run_builtin() but bypasses the RUN_SETUP and NEED_WORK_TREE handling
> before it in turn calls cmd_foo().  We expect a call in cmd_foo() to
> parse_options() emit a short-help and exit.
> 
> So, yes, there is a way to reach this point in the codeflow without
> being in a repository (or even when in a repository, we may have
> chosen not to realize it).  Feels ugly.
> 
> Now a bit of tangent.
> 
> I wonder if it is a problem to completely bypass RUN_SETUP in such a
> case.  In general, we read the configuration to tweak the hardcoded
> default behaviour, and then further override it by parsing command
> line options.  In order to read configuration fully, we'd need to
> know where the repository is.  So the start-up sequence must be in
> this order:
> 
>   - discover where the repository is (either gently or with a hard failure)
>   - read the configuration files
>   - call parse_options()
> 
> And by completly bypassing RUN_SETUP, we are not reading per-repo
> settings from the configuration files.
> 
> Something along this line (note: there is an always-taken if block
> to reduce the patch noise for this illustration), perhaps.
> 
> 
> 
> 
>   git.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git i/git.c w/git.c
> index 5ff21be21f..50e258508e 100644
> --- i/git.c
> +++ w/git.c
> @@ -421,25 +421,30 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
>   	int status, help;
>   	struct stat st;
>   	const char *prefix;
> +	int run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));
>   
>   	prefix = NULL;
>   	help = argc == 2 && !strcmp(argv[1], "-h");
> -	if (!help) {
> -		if (p->option & RUN_SETUP)
> +	if (help && (run_setup & RUN_SETUP))
> +		/* demote to GENTLY to allow 'git cmd -h' outside repo */
> +		run_setup = RUN_SETUP_GENTLY;
> +
> +	if (1) {
> +		if (run_setup & RUN_SETUP)
>   			prefix = setup_git_directory();
> -		else if (p->option & RUN_SETUP_GENTLY) {
> +		else if (run_setup & RUN_SETUP_GENTLY) {
>   			int nongit_ok;
>   			prefix = setup_git_directory_gently(&nongit_ok);
>   		}
>   		precompose_argv_prefix(argc, argv, NULL);
> -		if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&
> +		if (use_pager == -1 && run_setup &&
>   		    !(p->option & DELAY_PAGER_CONFIG))
>   			use_pager = check_pager_config(p->cmd);
>   		if (use_pager == -1 && p->option & USE_PAGER)
>   			use_pager = 1;
>   
> -		if ((p->option & (RUN_SETUP | RUN_SETUP_GENTLY)) &&
> -		    startup_info->have_repository) /* get_git_dir() may set up repo, avoid that */
> +		if (run_setup && startup_info->have_repository)
> +			/* get_git_dir() may set up repo, avoid that */
>   			trace_repo_setup(prefix);
>   	}
>   	commit_pager_choice();
> 
This is cool! I applied it locally, and it seems to be working well. I
will plan to replace my changes to checkout and pack-objects with this
for v5.

Lessley