threads / discuss / 62790

fatal: Not a valid object name HEAD

Subject: fatal: Not a valid object name HEAD

## tl;dr

18 messages between Jan 11, 2025 and Jan 27, 2025.

replies: 17people: 5as markdown or json

Christian Hesse· Jan 11, 2025, 19:26 UTC · lore
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);}
Bence Ferdinandy· Jan 12, 2025, 14:17 UTC · re: Christian Hesse · lore

Re: fatal: Not a valid object name HEAD

On Sat Jan 11, 2025 at 20:26, Christian Hesse <list@eworm.de> wrote:
Show 35 quoted lines
> 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

Junio C Hamano· Jan 13, 2025, 16:30 UTC · re: Bence Ferdinandy · lore

Re: fatal: Not a valid object name HEAD

"Bence Ferdinandy" <bence@ferdinandy.com> writes:
Show 5 quoted lines
> 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.
Bence Ferdinandy· Jan 12, 2025, 16:51 UTC · re: Christian Hesse · lore

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

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
Junio C Hamano· Jan 23, 2025, 21:00 UTC · re: Bence Ferdinandy · lore

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

Bence Ferdinandy <bence@ferdinandy.com> writes:
Show 10 quoted lines
> 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.

Show 5 quoted lines
> ---
>  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.
Bence Ferdinandy· Jan 23, 2025, 21:42 UTC · re: Junio C Hamano · lore

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

On Thu Jan 23, 2025 at 22:00, Junio C Hamano <gitster@pobox.com> wrote:
Show 16 quoted lines
> 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.
Show 8 quoted lines
>
>> ---
>>  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

Eric Sunshine· Jan 23, 2025, 22:00 UTC · re: Bence Ferdinandy · lore

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

On Thu, Jan 23, 2025 at 4:48 PM Bence Ferdinandy <bence@ferdinandy.com> wrote:
Show 10 quoted lines
> 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.)

Christian Hesse· Jan 24, 2025, 14:07 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> on Thu, 2025/01/23 13:00:
Show 24 quoted lines
> 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);}
Junio C Hamano· Jan 24, 2025, 17:17 UTC · re: Christian Hesse · lore

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

Christian Hesse <list@eworm.de> writes:
Show 12 quoted lines
>> > ---
>> >  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.
Patrick Steinhardt· Jan 24, 2025, 05:56 UTC · re: Bence Ferdinandy · lore

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

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
Eric Sunshine· Jan 24, 2025, 10:30 UTC · re: Patrick Steinhardt · lore

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

On Fri, Jan 24, 2025 at 12:56 AM Patrick Steinhardt <ps@pks.im> wrote:
Show 9 quoted lines
> 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/
Junio C Hamano· Jan 24, 2025, 16:07 UTC · re: Patrick Steinhardt · lore

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

Patrick Steinhardt <ps@pks.im> writes:
Show 16 quoted lines
>> 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.
Bence Ferdinandy· Jan 26, 2025, 22:01 UTC · re: Junio C Hamano · lore

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

On Fri Jan 24, 2025 at 17:07, Junio C Hamano <gitster@pobox.com> wrote:
Show 30 quoted lines
> 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
Bence Ferdinandy· Jan 26, 2025, 22:01 UTC · re: Patrick Steinhardt · lore

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

On Fri Jan 24, 2025 at 06:56, Patrick Steinhardt <ps@pks.im> wrote:
Show 19 quoted lines
> 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.

Show 61 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?
>
>> +	(
>> +		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
Bence Ferdinandy· Jan 26, 2025, 22:02 UTC · re: Patrick Steinhardt · lore

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

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
Bence Ferdinandy· Jan 26, 2025, 22:02 UTC · re: Bence Ferdinandy · lore

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

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
Patrick Steinhardt· Jan 27, 2025, 07:30 UTC · re: Bence Ferdinandy · lore

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

On Sun, Jan 26, 2025 at 11:02:11PM +0100, Bence Ferdinandy wrote:
Show 10 quoted lines
> 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
Junio C Hamano· Jan 27, 2025, 20:18 UTC · re: Patrick Steinhardt · lore

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

Patrick Steinhardt <ps@pks.im> writes:
Show 13 quoted lines
> 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.

← back to recent threads