Re: [PATCH] fetch set_head: fix non-mirror remotes in bare repositories

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 24, 2025, 05:56 UTC
Message-ID: <Z5Mrk02wMdABtrVZ@pks.im>
In-Reply-To: <20250112165125.130400-1-bence@ferdinandy.com>

On Sun, Jan 12, 2025 at 05:51:22PM +0100, Bence Ferdinandy wrote:

Show 14 quoted lines

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index fe2b26c74a..625d45be8b 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1925,7 +1925,8 @@ static int do_fetch(struct transport *transport,
>  		}
>  	}
>  	if (set_head(remote_refs, transport->remote->follow_remote_head,
> -		transport->remote->no_warn_branch))
> +		transport->remote->no_warn_branch,
> +		transport->remote->mirror))
>  		;
>  		/*
>  		 * Way too many cases where this can go wrong

Nit: At this point it might be sensible to simply pass in the remote
itself, which would allow for an easier callsite and less risk of
getting the order of parameters wrong.

Show 10 quoted lines

> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> index 519f7973e3..c75cfe968f 100755
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -589,6 +589,16 @@ test_expect_success 'add --mirror setting HEAD' '
>  	)
>  '
>  
> +test_expect_success 'non-mirror fetch does not interfere with mirror' '
> +	mkdir headnotmain &&

Nit: this can be simplified into `git init --bare -b notmain
headnotmain` so that you don't have to create an empty directory first.
Also, do we want to `test_when_finished rm -rf headnotmain` to clean up
after ourselves?

Show 23 quoted lines

> +	(
> +		cd headnotmain &&
> +		git init --bare -b notmain &&
> +		git remote add -f other ../two &&
> +		test "$(git symbolic-ref HEAD)" = "refs/heads/notmain"
> +	)
> +'
> +
>  test_expect_success 'add --mirror=fetch' '
>  	mkdir mirror-fetch &&
>  	git init -b main mirror-fetch/parent &&
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 2d9587059f..cfa63ae086 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -84,6 +84,19 @@ test_expect_success "fetch test remote HEAD" '
>  	branch=$(git rev-parse refs/remotes/origin/main) &&
>  	test "z$head" = "z$branch"'
>  
> +test_expect_success "fetch test remote HEAD in bare repository" '
> +	cd "$D" &&
> +	git init --bare barerepo &&
> +	cd barerepo &&

The `cd` needs to happen in a subshell. ALso, the same comment here regarding whether we want to have `test_when_finished` to clean up state.

Show 7 quoted lines

> +	git remote add upstream ../two &&
> +	git fetch upstream &&
> +	git rev-parse --verify refs/remotes/upstream/HEAD &&
> +	git rev-parse --verify refs/remotes/upstream/main &&
> +	head=$(git rev-parse refs/remotes/upstream/HEAD) &&
> +	branch=$(git rev-parse refs/remotes/upstream/main) &&
> +	test "z$head" = "z$branch"'

The closing single-quote should be on its own line.

I see though that you simply follow existing code style, both for the call to cd(1) and for the single-quote, so these are fine. This test file could use a makeover, but that is obviously outside of the scope of this patch series.

Patrick