From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 15 Jul 2026 06:35:46 GMT
Subject: Re: [PATCH v1] repository: move fetch_if_missing into struct repository
Message-ID: <alcqQp0lkwRIIE1t@pks.im>
In-Reply-To: <20260715011850.3181131-1-cat@malon.dev>

On Wed, Jul 15, 2026 at 09:18:50AM +0800, Tian Yuchen wrote:
> The global variable 'fetch_if_missing' controls whether a missing
> object check should prompt a lazy fetch from a promisor remote.
> In order to continue the libification effort, move it into
> 'struct repository' and initialize it to 1 by default to keep the
> previous behavior.

Right. I was also thinking about moving this into a non-global scope
multiple times. I was approaching this a bit differently though: it's
ultimately a property of the object database whether or not we want to
accept missing objects, so I moved it in there instead.

I don't really think there's a downside with your version, though. Quite
on the contrary: we can really only perform the backfill fetches with a
whole repository at hand anyway. So conceptually your version might even
be more sensible.

> Subsystems that already pass around a repository pointer, are
> updated to read this flag directly from their respective 'repo'
> instances. For the rest, we access 'the_repository'.
> 
> Note that in builtin/fsck.c and builtin/index-pack.c, when running
> related commands with the '-h' parameter, the 'repo' pointer is not
> passed in. To prevent null pointer dereferences, we defer
> operations on the repo in until after parameter parsing is complete.

s/on the repo in/on the repo/

> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 0793dc595c..721d576938 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1898,15 +1898,16 @@ int cmd_index_pack(int argc,
>  	int report_end_of_input = 0;
>  	int hash_algo = 0;
>  
> +	show_usage_if_asked(argc, argv, index_pack_usage);
> +
>  	/*
>  	 * index-pack never needs to fetch missing objects except when
>  	 * REF_DELTA bases are missing (which are explicitly handled). It only
>  	 * accesses the repo to do hash collision checks and to check which
>  	 * REF_DELTA bases need to be fetched.
>  	 */
> -	fetch_if_missing = 0;
> -
> -	show_usage_if_asked(argc, argv, index_pack_usage);
> +	if (repo)
> +		repo->fetch_if_missing = 0;
>  
>  	disable_replace_refs();
>  

Okay. This command can run without a repository, in which case we'll end
up just indexing the pack. My assumption is that we'll probably end up
using `the_repository` if so, as we still use `the_repository` in this
file. So could this here cause a change in behaviour?

If the answer is "maybe" I'd propose that we simply continue to use
`the_repository` here.

> diff --git a/revision.c b/revision.c
> index e91d7e1f11..bb645654c3 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2714,7 +2714,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  		revs->ignore_missing = 1;
>  	} else if (opt && opt->allow_exclude_promisor_objects &&
>  		   !strcmp(arg, "--exclude-promisor-objects")) {
> -		if (fetch_if_missing)
> +		if (revs->repo->fetch_if_missing)
>  			BUG("exclude_promisor_objects can only be used when fetch_if_missing is 0");
>  		revs->exclude_promisor_objects = 1;
>  	} else {

This one here also makes me wonder whether it could cause weird
interactions in case a caller passes a repository other than
`the_repository`. It ideally _shouldn't_, but it's hard to tell because
we still use `the_repository` in lots of places here.

Thanks!

Patrick