fatal: Not a valid object name HEAD

fatal: Not a valid object name HEAD

[Christian Hesse]

[-- Attachment #1: Type: text/plain, Size: 1871 bytes --]

Hello everybody,

starting with Git 2.48.0 I see some trouble with some mirrored bare
repositories. Try this:

box ~ % git clone --mirror https://github.com/codership/galera.git
Cloning into bare repository 'galera.git'...
remote: Enumerating objects: 49768, done.
remote: Counting objects: 100% (2251/2251), done.
remote: Compressing objects: 100% (553/553), done.
remote: Total 49768 (delta 1986), reused 1716 (delta 1698), pack-reused 47517 (from 3)
Receiving objects: 100% (49768/49768), 25.20 MiB | 4.92 MiB/s, done.
Resolving deltas: 100% (37386/37386), done.
box ~ % cd galera.git 
box ~/galera.git (git)-[4.x] % git describe 
release_26.4.5-345-gd811a577
box ~/galera.git (git)-[4.x] % git remote add mariadb https://github.com/MariaDB/galera.git
box ~/galera.git (git)-[4.x] % git fetch --all
Fetching origin
Fetching mariadb
remote: Enumerating objects: 638, done.
remote: Counting objects: 100% (517/517), done.
remote: Compressing objects: 100% (125/125), done.
remote: Total 638 (delta 415), reused 454 (delta 392), pack-reused 121 (from 3)
Receiving objects: 100% (638/638), 386.30 KiB | 5.08 MiB/s, done.
Resolving deltas: 100% (440/440), completed with 105 local objects.
From https://github.com/MariaDB/galera
 * [new branch]        0.6 -> mariadb/0.6
[ snipped some more branches and tags ]
box ~/galera.git (git)-[mariadb-4.x] % git describe                                                
fatal: Not a valid object name HEAD

Guess that's not expected... Why does that happen?

Possibly fallout caused by https://github.com/git/git/commit/5f212684abb66c9604e745a2296af8c4bb99961c
-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Best regards             my address:    */=0;b=c[a++];)
putchar(b-1/(/*    Chris            cc -ox -xc - && ./x    */b/42*2-3)*42);}

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: fatal: Not a valid object name HEAD

[Bence Ferdinandy]

On Sat Jan 11, 2025 at 20:26, Christian Hesse <list@eworm.de> wrote:
> Hello everybody,
>
> starting with Git 2.48.0 I see some trouble with some mirrored bare
> repositories. Try this:
>
> box ~ % git clone --mirror https://github.com/codership/galera.git
> Cloning into bare repository 'galera.git'...
> remote: Enumerating objects: 49768, done.
> remote: Counting objects: 100% (2251/2251), done.
> remote: Compressing objects: 100% (553/553), done.
> remote: Total 49768 (delta 1986), reused 1716 (delta 1698), pack-reused 47517 (from 3)
> Receiving objects: 100% (49768/49768), 25.20 MiB | 4.92 MiB/s, done.
> Resolving deltas: 100% (37386/37386), done.
> box ~ % cd galera.git 
> box ~/galera.git (git)-[4.x] % git describe 
> release_26.4.5-345-gd811a577
> box ~/galera.git (git)-[4.x] % git remote add mariadb https://github.com/MariaDB/galera.git
> box ~/galera.git (git)-[4.x] % git fetch --all
> Fetching origin
> Fetching mariadb
> remote: Enumerating objects: 638, done.
> remote: Counting objects: 100% (517/517), done.
> remote: Compressing objects: 100% (125/125), done.
> remote: Total 638 (delta 415), reused 454 (delta 392), pack-reused 121 (from 3)
> Receiving objects: 100% (638/638), 386.30 KiB | 5.08 MiB/s, done.
> Resolving deltas: 100% (440/440), completed with 105 local objects.
> From https://github.com/MariaDB/galera
>  * [new branch]        0.6 -> mariadb/0.6
> [ snipped some more branches and tags ]
> box ~/galera.git (git)-[mariadb-4.x] % git describe                                                
> fatal: Not a valid object name HEAD
>
> Guess that's not expected... Why does that happen?
>
> Possibly fallout caused by https://github.com/git/git/commit/5f212684abb66c9604e745a2296af8c4bb99961c

More specifically: https://github.com/git/git/commit/b1b713f722894d7f66e9ec64bc934ca32004d3d1

So what happened before the series is that HEAD was set on cloning to 

	ref: refs/heads/4.x 

and the adding the new remote did not change this. After the series fetching
the new remote will overwrite HEAD to 

	ref: refs/heads/mariadb-4.x 

which does not exist in the repository. Note that the older version does not
set up `refs/remotes/mariadb/HEAD` which is interesting?

I think the correct way to handle this would be to check if the remote has
`mirror=true` set, and if we're running in a bare repository we should overwrite
HEAD and if it's not a mirror we should instead set up
`refs/remotes/nonmirrorremote/HEAD`. (I think a remote can be a mirror without
the repository being bare, I'm not sure.)

I can probably send a patch late next week to fix this.

Best,
Bence

Re: fatal: Not a valid object name HEAD

[Junio C Hamano]

"Bence Ferdinandy" <bence@ferdinandy.com> writes:

> I think the correct way to handle this would be to check if the remote has
> `mirror=true` set, and if we're running in a bare repository we should overwrite
> HEAD and if it's not a mirror we should instead set up
> `refs/remotes/nonmirrorremote/HEAD`. (I think a remote can be a mirror without
> the repository being bare, I'm not sure.)

A non-bare repository can technically be a mirror but such a thing
is only useful when (1) it never fetches from anywhere afterwards,
(2) it stays on a branch that never changes by convention, or (3) it
stays on a detached HEAD.  Otherwise "git fetch" in it would almost
always fail.

Practically (1) and (2) are not all that useful---such a static
checkout does not even have to be a Git repository but a tarball
extract.  If you overwrite HEAD upon fetch, you will render the last
remaining useful usage, (3), also useless.

Thanks.

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

[Bence Ferdinandy]

In b1b713f722 (fetch set_head: handle mirrored bare repositories,
2024-11-22) it was implicitly assumed that all remotes will be mirrors
in a bare repository, thus fetching a non-mirrored remote could lead to
HEAD pointing to a non-existent reference. Make sure we only overwrite
HEAD if we are in a bare repository and fetching from a mirror.
Otherwise, proceed as normally, and create
refs/remotes/<nonmirrorremote>/HEAD instead.

Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com>
Reported-by: Christian Hesse <list@eworm.de>
---
 builtin/fetch.c   | 15 ++++++++-------
 t/t5505-remote.sh | 10 ++++++++++
 t/t5510-fetch.sh  | 13 +++++++++++++
 3 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index fe2b26c74a..625d45be8b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1618,9 +1618,9 @@ static void report_set_head(const char *remote, const char *head_name,
 }
 
 static int set_head(const struct ref *remote_refs, int follow_remote_head,
-		const char *no_warn_branch)
+		const char *no_warn_branch, int mirror)
 {
-	int result = 0, create_only, is_bare, was_detached;
+	int result = 0, create_only, baremirror, was_detached;
 	struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT,
 		      b_local_head = STRBUF_INIT;
 	const char *remote = gtransport->remote->name;
@@ -1655,9 +1655,9 @@ static int set_head(const struct ref *remote_refs, int follow_remote_head,
 
 	if (!head_name)
 		goto cleanup;
-	is_bare = is_bare_repository();
-	create_only = follow_remote_head == FOLLOW_REMOTE_ALWAYS ? 0 : !is_bare;
-	if (is_bare) {
+	baremirror = is_bare_repository() && mirror;
+	create_only = follow_remote_head == FOLLOW_REMOTE_ALWAYS ? 0 : !baremirror;
+	if (baremirror) {
 		strbuf_addstr(&b_head, "HEAD");
 		strbuf_addf(&b_remote_head, "refs/heads/%s", head_name);
 	} else {
@@ -1665,7 +1665,7 @@ static int set_head(const struct ref *remote_refs, int follow_remote_head,
 		strbuf_addf(&b_remote_head, "refs/remotes/%s/%s", remote, head_name);
 	}
 		/* make sure it's valid */
-	if (!is_bare && !refs_ref_exists(refs, b_remote_head.buf)) {
+	if (!baremirror && !refs_ref_exists(refs, b_remote_head.buf)) {
 		result = 1;
 		goto cleanup;
 	}
@@ -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
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 &&
+	(
+		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 &&
+	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"'
+
+
 test_expect_success "fetch test remote HEAD change" '
 	cd "$D" &&
 	cd two &&

base-commit: fbe8d3079d4a96aeb4e4529cc93cc0043b759a05
-- 
2.48.0.1.g32c193c8ca

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

[Junio C Hamano]

Bence Ferdinandy <bence@ferdinandy.com> writes:

> In b1b713f722 (fetch set_head: handle mirrored bare repositories,
> 2024-11-22) it was implicitly assumed that all remotes will be mirrors
> in a bare repository, thus fetching a non-mirrored remote could lead to
> HEAD pointing to a non-existent reference. Make sure we only overwrite
> HEAD if we are in a bare repository and fetching from a mirror.
> Otherwise, proceed as normally, and create
> refs/remotes/<nonmirrorremote>/HEAD instead.
>
> Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com>
> Reported-by: Christian Hesse <list@eworm.de>

These should be chronological; somebody reports an issue, the patch
gets written, and finally it is sent out with a Sign-off to certify
that the patch is not a stolen property.

> ---
>  builtin/fetch.c   | 15 ++++++++-------
>  t/t5505-remote.sh | 10 ++++++++++
>  t/t5510-fetch.sh  | 13 +++++++++++++
>  3 files changed, 31 insertions(+), 7 deletions(-)

We haven't heard from Chritian; has this been tested OK?

What the patch does does look sensible.  Thanks.

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

[Bence Ferdinandy]

On Thu Jan 23, 2025 at 22:00, Junio C Hamano <gitster@pobox.com> wrote:
> Bence Ferdinandy <bence@ferdinandy.com> writes:
>
>> In b1b713f722 (fetch set_head: handle mirrored bare repositories,
>> 2024-11-22) it was implicitly assumed that all remotes will be mirrors
>> in a bare repository, thus fetching a non-mirrored remote could lead to
>> HEAD pointing to a non-existent reference. Make sure we only overwrite
>> HEAD if we are in a bare repository and fetching from a mirror.
>> Otherwise, proceed as normally, and create
>> refs/remotes/<nonmirrorremote>/HEAD instead.
>>
>> Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com>
>> Reported-by: Christian Hesse <list@eworm.de>
>
> These should be chronological; somebody reports an issue, the patch
> gets written, and finally it is sent out with a Sign-off to certify
> that the patch is not a stolen property.

Makes sense, I'll send a v2 in that case.

>
>> ---
>>  builtin/fetch.c   | 15 ++++++++-------
>>  t/t5505-remote.sh | 10 ++++++++++
>>  t/t5510-fetch.sh  | 13 +++++++++++++
>>  3 files changed, 31 insertions(+), 7 deletions(-)
>
> We haven't heard from Chritian; has this been tested OK?

To the extent of the tests I've added, but I'm not aware of anybody else,
especially Christian trying it out.

>
> What the patch does does look sensible.  Thanks.

Thanks,
Bence

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

[Eric Sunshine]

On Thu, Jan 23, 2025 at 4:48 PM Bence Ferdinandy <bence@ferdinandy.com> wrote:
> On Thu Jan 23, 2025 at 22:00, Junio C Hamano <gitster@pobox.com> wrote:
> > Bence Ferdinandy <bence@ferdinandy.com> writes:
> >> Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com>
> >> Reported-by: Christian Hesse <list@eworm.de>
> >
> > These should be chronological; somebody reports an issue, the patch
> > gets written, and finally it is sent out with a Sign-off to certify
> > that the patch is not a stolen property.
>
> Makes sense, I'll send a v2 in that case.

It's a good idea to check whether Junio has himself fixed this sort of
thing when queuing your patch. (In this case, it appears that he has.)

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

[Christian Hesse]

[-- Attachment #1: Type: text/plain, Size: 1554 bytes --]

Junio C Hamano <gitster@pobox.com> on Thu, 2025/01/23 13:00:
> Bence Ferdinandy <bence@ferdinandy.com> writes:
> 
> > In b1b713f722 (fetch set_head: handle mirrored bare repositories,
> > 2024-11-22) it was implicitly assumed that all remotes will be mirrors
> > in a bare repository, thus fetching a non-mirrored remote could lead to
> > HEAD pointing to a non-existent reference. Make sure we only overwrite
> > HEAD if we are in a bare repository and fetching from a mirror.
> > Otherwise, proceed as normally, and create
> > refs/remotes/<nonmirrorremote>/HEAD instead.
> >
> > Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com>
> > Reported-by: Christian Hesse <list@eworm.de>  
> 
> These should be chronological; somebody reports an issue, the patch
> gets written, and finally it is sent out with a Sign-off to certify
> that the patch is not a stolen property.
> 
> > ---
> >  builtin/fetch.c   | 15 ++++++++-------
> >  t/t5505-remote.sh | 10 ++++++++++
> >  t/t5510-fetch.sh  | 13 +++++++++++++
> >  3 files changed, 31 insertions(+), 7 deletions(-)  
> 
> We haven't heard from Chritian; has this been tested OK?

Sorry for the late reply...

Yes, with this patch applied git behaves as expected for me.
Thanks a lot!

> What the patch does does look sensible.  Thanks.
-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Best regards             my address:    */=0;b=c[a++];)
putchar(b-1/(/*    Chris            cc -ox -xc - && ./x    */b/42*2-3)*42);}

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Junio C Hamano]

Christian Hesse <list@eworm.de> writes:

>> > ---
>> >  builtin/fetch.c   | 15 ++++++++-------
>> >  t/t5505-remote.sh | 10 ++++++++++
>> >  t/t5510-fetch.sh  | 13 +++++++++++++
>> >  3 files changed, 31 insertions(+), 7 deletions(-)  
>> 
>> We haven't heard from Chritian; has this been tested OK?
>
> Sorry for the late reply...
>
> Yes, with this patch applied git behaves as expected for me.
> Thanks a lot!

Thanks.

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

[Patrick Steinhardt]

On Sun, Jan 12, 2025 at 05:51:22PM +0100, Bence Ferdinandy wrote:
> 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.

> 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?

> +	(
> +		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.

> +	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

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

[Eric Sunshine]

On Fri, Jan 24, 2025 at 12:56 AM Patrick Steinhardt <ps@pks.im> wrote:
> On Sun, Jan 12, 2025 at 05:51:22PM +0100, Bence Ferdinandy wrote:
> > +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.

By way of explanation regarding `cd` in a subshell, see [*].

[*]: https://lore.kernel.org/git/CAPig+cRsAPp1APNJ7W337UNtunETr+Lnn-RcGrAXEFUhN1APyA@mail.gmail.com/

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> 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.

Yes, indeed.  The change to another script we saw earlier followed
the "chdir around only in a subshell" pattern.

> 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.

Terminating quote can stay, but chdir is a correctness issue that
may want to be addressed minimally (i.e. not making things worse,
while leaving it for later to clean up the existing ones).

Thanks.

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

[Bence Ferdinandy]

On Fri Jan 24, 2025 at 17:07, Junio C Hamano <gitster@pobox.com> wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
>>> 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.
>
> Yes, indeed.  The change to another script we saw earlier followed
> the "chdir around only in a subshell" pattern.
>
>> 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.
>
> Terminating quote can stay, but chdir is a correctness issue that
> may want to be addressed minimally (i.e. not making things worse,
> while leaving it for later to clean up the existing ones).

I'll be sending the fix with also the terminating quote fixed as well. Actually
tests after this which I added for followremotehead followed the correct style
so why not here as well ...

5505 remote also has some style issues that came up before so if I find some
time I'll probably send a patches cleaning the up both.

>
> Thanks.




-- 
bence.ferdinandy.com

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

[Bence Ferdinandy]

On Fri Jan 24, 2025 at 06:56, Patrick Steinhardt <ps@pks.im> wrote:
> On Sun, Jan 12, 2025 at 05:51:22PM +0100, Bence Ferdinandy wrote:
>> 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.

Thanks, that's a really good point, not to mention inside set_head gtransport
is used to also access remote, which seems a bit of an oversight.

>
>> 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?
>
>> +	(
>> +		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.
>
>> +	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




-- 
bence.ferdinandy.com

[PATCH v2 1/2] fetch set_head: refactor to use remote directly

[Bence Ferdinandy]

As a preparatory step to use even more properties from the remote
struct, refactor set_head to take the entire struct as a parameter,
instead of the necessary bits. This also allows consolidating the use of
gtransport->remote in set_head, making the access of the remote's
properties consistent in the function.

Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com>
---

Notes:
    v2: - new patch

 builtin/fetch.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index fe2b26c74a..3167b055d1 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1617,13 +1617,13 @@ static void report_set_head(const char *remote, const char *head_name,
 	strbuf_release(&buf_prefix);
 }
 
-static int set_head(const struct ref *remote_refs, int follow_remote_head,
-		const char *no_warn_branch)
+static int set_head(const struct ref *remote_refs, struct remote *remote)
 {
 	int result = 0, create_only, is_bare, was_detached;
 	struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT,
 		      b_local_head = STRBUF_INIT;
-	const char *remote = gtransport->remote->name;
+	int follow_remote_head = remote->follow_remote_head;
+	const char *no_warn_branch = remote->no_warn_branch;
 	char *head_name = NULL;
 	struct ref *ref, *matches;
 	struct ref *fetch_map = NULL, **fetch_map_tail = &fetch_map;
@@ -1661,8 +1661,8 @@ static int set_head(const struct ref *remote_refs, int follow_remote_head,
 		strbuf_addstr(&b_head, "HEAD");
 		strbuf_addf(&b_remote_head, "refs/heads/%s", head_name);
 	} else {
-		strbuf_addf(&b_head, "refs/remotes/%s/HEAD", remote);
-		strbuf_addf(&b_remote_head, "refs/remotes/%s/%s", remote, head_name);
+		strbuf_addf(&b_head, "refs/remotes/%s/HEAD", remote->name);
+		strbuf_addf(&b_remote_head, "refs/remotes/%s/%s", remote->name, head_name);
 	}
 		/* make sure it's valid */
 	if (!is_bare && !refs_ref_exists(refs, b_remote_head.buf)) {
@@ -1678,7 +1678,7 @@ static int set_head(const struct ref *remote_refs, int follow_remote_head,
 	if (verbosity >= 0 &&
 		follow_remote_head == FOLLOW_REMOTE_WARN &&
 		(!no_warn_branch || strcmp(no_warn_branch, head_name)))
-		report_set_head(remote, head_name, &b_local_head, was_detached);
+		report_set_head(remote->name, head_name, &b_local_head, was_detached);
 
 cleanup:
 	free(head_name);
@@ -1924,8 +1924,7 @@ static int do_fetch(struct transport *transport,
 				  "you need to specify exactly one branch with the --set-upstream option"));
 		}
 	}
-	if (set_head(remote_refs, transport->remote->follow_remote_head,
-		transport->remote->no_warn_branch))
+	if (set_head(remote_refs, transport->remote))
 		;
 		/*
 		 * Way too many cases where this can go wrong
-- 
2.48.1.93.g276f59c085

[PATCH v2 2/2] fetch set_head: fix non-mirror remotes in bare repositories

[Bence Ferdinandy]

In b1b713f722 (fetch set_head: handle mirrored bare repositories,
2024-11-22) it was implicitly assumed that all remotes will be mirrors
in a bare repository, thus fetching a non-mirrored remote could lead to
HEAD pointing to a non-existent reference. Make sure we only overwrite
HEAD if we are in a bare repository and fetching from a mirror.
Otherwise, proceed as normally, and create
refs/remotes/<nonmirrorremote>/HEAD instead.

Reported-by: Christian Hesse <list@eworm.de>
Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com>
---

Notes:
    v2: - fixed too many parameters to set_head
        - cleaned up the style of tests

 builtin/fetch.c   | 10 +++++-----
 t/t5505-remote.sh | 10 ++++++++++
 t/t5510-fetch.sh  | 17 +++++++++++++++++
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 3167b055d1..1c740d5aac 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1619,7 +1619,7 @@ static void report_set_head(const char *remote, const char *head_name,
 
 static int set_head(const struct ref *remote_refs, struct remote *remote)
 {
-	int result = 0, create_only, is_bare, was_detached;
+	int result = 0, create_only, baremirror, was_detached;
 	struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT,
 		      b_local_head = STRBUF_INIT;
 	int follow_remote_head = remote->follow_remote_head;
@@ -1655,9 +1655,9 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)
 
 	if (!head_name)
 		goto cleanup;
-	is_bare = is_bare_repository();
-	create_only = follow_remote_head == FOLLOW_REMOTE_ALWAYS ? 0 : !is_bare;
-	if (is_bare) {
+	baremirror = is_bare_repository() && remote->mirror;
+	create_only = follow_remote_head == FOLLOW_REMOTE_ALWAYS ? 0 : !baremirror;
+	if (baremirror) {
 		strbuf_addstr(&b_head, "HEAD");
 		strbuf_addf(&b_remote_head, "refs/heads/%s", head_name);
 	} else {
@@ -1665,7 +1665,7 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)
 		strbuf_addf(&b_remote_head, "refs/remotes/%s/%s", remote->name, head_name);
 	}
 		/* make sure it's valid */
-	if (!is_bare && !refs_ref_exists(refs, b_remote_head.buf)) {
+	if (!baremirror && !refs_ref_exists(refs, b_remote_head.buf)) {
 		result = 1;
 		goto cleanup;
 	}
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 519f7973e3..66e373f71d 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' '
+	test_when_finished rm -rf headnotmain &&
+	(
+		git init --bare -b notmain headnotmain &&
+		cd headnotmain &&
+		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..c9d7b46c87 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -84,6 +84,23 @@ 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" '
+	test_when_finished rm -rf barerepo &&
+	(
+		cd "$D" &&
+		git init --bare barerepo &&
+		cd barerepo &&
+		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"
+	)
+'
+
+
 test_expect_success "fetch test remote HEAD change" '
 	cd "$D" &&
 	cd two &&
-- 
2.48.1.93.g276f59c085

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

[Patrick Steinhardt]

On Sun, Jan 26, 2025 at 11:02:11PM +0100, Bence Ferdinandy wrote:
> In b1b713f722 (fetch set_head: handle mirrored bare repositories,
> 2024-11-22) it was implicitly assumed that all remotes will be mirrors
> in a bare repository, thus fetching a non-mirrored remote could lead to
> HEAD pointing to a non-existent reference. Make sure we only overwrite
> HEAD if we are in a bare repository and fetching from a mirror.
> Otherwise, proceed as normally, and create
> refs/remotes/<nonmirrorremote>/HEAD instead.
> 
> Reported-by: Christian Hesse <list@eworm.de>
> Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com>

Thanks, both of these patches look sensible to me.

Patrick

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Sun, Jan 26, 2025 at 11:02:11PM +0100, Bence Ferdinandy wrote:
>> In b1b713f722 (fetch set_head: handle mirrored bare repositories,
>> 2024-11-22) it was implicitly assumed that all remotes will be mirrors
>> in a bare repository, thus fetching a non-mirrored remote could lead to
>> HEAD pointing to a non-existent reference. Make sure we only overwrite
>> HEAD if we are in a bare repository and fetching from a mirror.
>> Otherwise, proceed as normally, and create
>> refs/remotes/<nonmirrorremote>/HEAD instead.
>> 
>> Reported-by: Christian Hesse <list@eworm.de>
>> Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com>
>
> Thanks, both of these patches look sensible to me.

Yeah, they read quite well.  Thanks, all.
Will queue.