From: Derrick Stolee <stolee@gmail.com>
Date: Wed, 04 Sep 2024 20:03:34 GMT
Subject: Re: [PATCH] remote: prefetch config
Message-ID: <e8232cdc-7a5d-485e-84f7-2c05e17f9907@gmail.com>
In-Reply-To: <pull.1779.git.1725472799637.gitgitgadget@gmail.com>

On 9/4/24 1:59 PM, Shubham Kanodia via GitGitGadget wrote:
> From: Shubham Kanodia <shubham.kanodia10@gmail.com>
> 
> Large repositories often contain numerous branches and refs, many of
> which individual users may not need. This commit introduces a new
> configuration option (`remote.<remote>.prefetch`) to allow
> users to specify which remotes to prefetch during
> the maintenance task.
> 
> Key behaviors:
> 1. If `remote.<remote>.prefetch` is unset or true, running
>     `git-maintenance` will prefetch all refs for the remote.
> 2. If `remote.<remote>.prefetch` is set to false, the remote
>     will be ignored for prefetching.

Thanks for this contribution. I think this is a good idea for extra
flexibility of the prefetch task.

> In a future change, we could also allow restricting the refs that are
> prefetched per remote using the `prefetchref` config option per remote.

I agree that this would also be of interest, but more complicated. Thanks
for starting with this simpler modification.

> +	if (remote->prefetch == 0)
> +		return 0;

In the Git codebase, this would normally be written as

	if (!remote->prefetch)
		return 0;

> +		# Run maintenance prefetch task
> +		GIT_TRACE2_EVENT="$(pwd)/prefetch.txt" git maintenance run --task=prefetch 2>/dev/null &&
> +
> +		# Check that remote1 was not fetched (prefetch=false)
> +		test_subcommand ! git fetch remote1 --prefetch --prune --no-tags \
> +			--no-write-fetch-head --recurse-submodules=no --quiet \
> +			<prefetch.txt &&

I'm happy to see this use of test_subcommand to validate the behavior
of this patch!

This is a very good patch and I only have the one style nit.

Thanks,
-Stolee