diff mbox series

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

Message ID 20250126220403.289742-2-bence@ferdinandy.com (mailing list archive)
State New
Headers show
Series [v2,1/2] fetch set_head: refactor to use remote directly | expand

Commit Message

Bence Ferdinandy Jan. 26, 2025, 10:02 p.m. UTC
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(-)

Comments

Patrick Steinhardt Jan. 27, 2025, 7:30 a.m. UTC | #1
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
Junio C Hamano Jan. 27, 2025, 8:18 p.m. UTC | #2
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.
diff mbox series

Patch

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