From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 05 Nov 2024 14:45:17 GMT
Subject: Re: [PATCH v4] remote: allow specifying refs to prefetch
Message-ID: <8c4e5911-d908-437a-8eec-385e272c8847@gmail.com>
In-Reply-To: <pull.1782.v4.git.1728073292874.gitgitgadget@gmail.com>

Hi Shubham

On 04/10/2024 21:21, Shubham Kanodia via GitGitGadget wrote:
> From: Shubham Kanodia <shubham.kanodia10@gmail.com>
> 

I agree with the Patrick's comments on the implementation, I've left a 
couple of test comments below.

> +test_expect_success 'prefetch with positive prefetch ref patterns' '
> +	test_create_repo filter-prefetch-positive &&
> +	(
> +		cd filter-prefetch-positive &&
> +		test_commit initial &&
> +		git clone . clone2 &&
> +		git remote add remote2 "file://$(pwd)/clone2" &&
> +
> +		cd clone2 &&
> +		git checkout -b feature && test_commit feature-commit-2 &&
> +		git checkout -b wip/test && test_commit wip-test-commit-2 &&
> +		git checkout -b topic/x && test_commit topic-x-commit-2 &&
> +		git push -f origin feature wip/test topic/x &&
> +		cd .. &&

I think it would make sense to have a blank line before this rather than 
after it so "cd" is grouped with the commands executed in that directory.

> +		git config remote.remote2.prefetchref "refs/heads/feature" &&
> +		fetchargs="--prefetch --prune --no-tags --no-write-fetch-head \
> +			--recurse-submodules=no --quiet" &&
> +		GIT_TRACE2_EVENT="$(pwd)/prefetch-positive.txt" \
> +			git maintenance run --task=prefetch 2>/dev/null &&
> +		test_subcommand git fetch remote2 $fetchargs <prefetch-positive.txt &&

This seems to be testing what "git maintenance" runs which is not really 
related to testing the prefetch ref pattern matching. I think just

	git maintenance run --task=prefetch &&

would be sufficient. We certainly should not be redirecting stderr to 
/dev/null as that hides any error messages that are helpful when 
debugging test failures.

> +		git rev-parse refs/prefetch/remotes/remote2/feature &&
> +		test_must_fail git rev-parse refs/prefetch/remotes/remote2/wip/test &&
> +		test_must_fail git rev-parse refs/prefetch/remotes/remote2/topic/x

these are the important tests for checking the prefetch pattern 
matching. We should perhaps be using "git rev-parse --verify"

The test coverage looks good

Best Wishes

Phillip

> +	)
> +'
> +
> +test_expect_success 'prefetch with negative prefetch ref patterns' '
> +	test_create_repo filter-prefetch-negative &&
> +	(
> +		cd filter-prefetch-negative &&
> +		test_commit initial &&
> +		git clone . clone3 &&
> +		git remote add remote3 "file://$(pwd)/clone3" &&
> +		cat .git/config &&
> +
> +		cd clone3 &&
> +		git checkout -b feature && test_commit feature-commit-3 &&
> +		git checkout -b wip/test && test_commit wip-test-commit-3 &&
> +		git checkout -b topic/x && test_commit topic-x-commit-3 &&
> +		git push -f origin feature wip/test topic/x &&
> +		cd .. &&
> +
> +		git config remote.remote3.prefetchref "!refs/heads/wip/*" &&
> +		fetchargs="--prefetch --prune --no-tags --no-write-fetch-head \
> +			--recurse-submodules=no --quiet" &&
> +		GIT_TRACE2_EVENT="$(pwd)/prefetch-negative.txt" \
> +			git maintenance run --task=prefetch 2>/dev/null &&
> +		test_subcommand git fetch remote3 $fetchargs <prefetch-negative.txt &&
> +		git rev-parse refs/prefetch/remotes/remote3/feature &&
> +		git rev-parse refs/prefetch/remotes/remote3/topic/x &&
> +		test_must_fail git rev-parse refs/prefetch/remotes/remote3/wip/test
> +	)
> +'
> +
> +test_expect_success 'prefetch with positive & negative prefetch ref patterns' '
> +	test_create_repo filter-prefetch-mixed &&
> +	(
> +		cd filter-prefetch-mixed &&
> +		test_commit initial &&
> +		git clone . clone4 &&
> +		git remote add remote4 "file://$(pwd)/clone4" &&
> +
> +		cd clone4 &&
> +		git checkout -b feature && test_commit feature-commit-4 &&
> +		git checkout -b topic/x && test_commit topic-x-commit-4 &&
> +		git checkout -b topic/y && test_commit topic-y-commit-4 &&
> +		git push -f origin feature topic/x topic/y &&
> +		cd .. &&
> +
> +		git config remote.remote4.prefetchref \
> +			"refs/heads/topic/* !refs/heads/topic/y" &&
> +		fetchargs="--prefetch --prune --no-tags --no-write-fetch-head \
> +			--recurse-submodules=no --quiet" &&
> +		GIT_TRACE2_EVENT="$(pwd)/prefetch-mixed.txt" \
> +			git maintenance run --task=prefetch 2>/dev/null &&
> +		test_subcommand git fetch remote4 $fetchargs <prefetch-mixed.txt &&
> +
> +		test_must_fail git rev-parse refs/prefetch/remotes/remote4/feature &&
> +		test_must_fail git rev-parse refs/prefetch/remotes/remote4/topic/y &&
> +		git rev-parse refs/prefetch/remotes/remote4/topic/x
> +	)
> +'
> +
>   test_expect_success 'loose-objects task' '
>   	# Repack everything so we know the state of the object dir
>   	git repack -adk &&
> 
> base-commit: 2e7b89e038c0c888acf61f1b4ee5a43d4dd5e94c