Re: [PATCH v2] fetch: pass --no-write-fetch-head to subprocesses

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 8, 2023, 23:13 UTC
Message-ID: <xmqqttyurg4w.fsf@gitster.g>
In-Reply-To: <20230308222205.M679514@dcvr>

Eric Wong <e@80x24.org> writes:

Show 6 quoted lines

> +test_expect_success 'git fetch --all --no-write-fetch-head' '
> +	(cd test &&
> +	rm -f .git/FETCH_HEAD &&
> +	git fetch --all --no-write-fetch-head &&
> +	test_path_is_missing .git/FETCH_HEAD)
> +'

The style used in the other script might be more modern, but given that the existing one (in the post context) uses the same older style, I think that would be OK.

Show 16 quoted lines

>  test_expect_success 'git fetch --all should continue if a remote has errors' '
>  	(git clone one test2 &&
>  	 cd test2 &&
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index b9546ef8e5..8ffb300f2d 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -167,6 +167,19 @@ test_expect_success "fetch --recurse-submodules recurses into submodules" '
>  	verify_fetch_result actual.err
>  '
>  
> +test_expect_success "fetch --recurse-submodules honors --no-write-fetch-head" '
> +	(
> +		cd downstream &&
> +		fh=$(find . -name FETCH_HEAD -type f) &&
> +		rm -f $fh &&

I do not like this part. The "rm -f" we saw in the "fetch --all" test was "make sure it is missing, so that we can be sure that presence after running 'git fetch' *is* a bug". But using $fh later ...

Show 5 quoted lines

> +		git fetch --recurse-submodules --no-write-fetch-head &&
> +		for f in $fh
> +		do
> +			test_path_is_missing $f || return 1
> +		done

... like this means now we depend on FETCH_HEAD being in all submodule repositories before we start this step.

I think we should instead enumerate submodule repositories, instead of enumerating existing .git/FETCH_HEAD files.

Show 6 quoted lines

> +	)
> +'
> +
>  test_expect_success "submodule.recurse option triggers recursive fetch" '
>  	add_submodule_commits &&
>  	(