Skip to contentSkip to conversation

upload-pack: no longer use hidden-refs as exclude_patterns

PATCH In review SURA via GitGitGadget 3 participants 3 messages Feb 27, 2025

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 upload-pack: no longer use hidden-refs as exclude_patterns

From: SURA <[email protected]>

---
upload-pack: No longer use hidden-refs as exclude_patterns

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1866%2FSURA907%2Fmaster-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1866/SURA907/master-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1866

 upload-pack.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)
upload-pack.c +3 -12
index 728b2477fcc..9ae42a463a3 100644
@@ -609,21 +609,12 @@ static int allow_hidden_refs(enum allow_uor allow_uor)
 static void for_each_namespaced_ref_1(each_ref_fn fn,
 				      struct upload_pack_data *data)
 {
-	const char **excludes = NULL;
 	/*
-	 * If `data->allow_uor` allows fetching hidden refs, we need to
-	 * mark all references (including hidden ones), to check in
-	 * `is_our_ref()` below.
-	 *
-	 * Otherwise, we only care about whether each reference's object
-	 * has the OUR_REF bit set or not, so do not need to visit
-	 * hidden references.
+	 * config transfer.hideRefs of upload-pack is diffient from arg exclude of for-each-ref,
+	 * We should not set exclude_patterns here
 	 */
-	if (allow_hidden_refs(data->allow_uor))
-		excludes = hidden_refs_to_excludes(&data->hidden_refs);
-
 	refs_for_each_namespaced_ref(get_main_ref_store(the_repository),
-				     excludes, fn, data);
+				     NULL, fn, data);
 }
 
 

base-commit: 08bdfd453584e489d5a551aecbdcb77584e1b958

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 30 lines · SURA via GitGitGadget

From: SURA <[email protected]>

---
upload-pack: No longer use hidden-refs as exclude_patterns

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1866%2FSURA907%2Fmaster-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1866/SURA907/master-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1866

 upload-pack.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)
upload-pack.c +0 -8
index 728b2477fcc..9ae42a463a3 100644
@@ -609,21 +609,12 @@ static int allow_hidden_refs(enum allow_uor allow_uor)
 static void for_each_namespaced_ref_1(each_ref_fn fn,
 				      struct upload_pack_data *data)
 {
-	const char **excludes = NULL;
 	/*
-	 * If `data->allow_uor` allows fetching hidden refs, we need to
-	 * mark all references (including hidden ones), to check in
-	 * `is_our_ref()` below.
-	 *
-	 * Otherwise, we only care about whether each reference's object
-	 * has the OUR_REF bit set or not, so do not need to visit
-	 * hidden references.

upload-pack.c static int allow_hidden_refs(enum allow_uor allow_uor) · line 618

Quoted 10 lines
upload-pack.c +3 -4
+	 * config transfer.hideRefs of upload-pack is diffient from arg exclude of for-each-ref,
+	 * We should not set exclude_patterns here
 	 */
-	if (allow_hidden_refs(data->allow_uor))
-		excludes = hidden_refs_to_excludes(&data->hidden_refs);
-
 	refs_for_each_namespaced_ref(get_main_ref_store(the_repository),
-				     excludes, fn, data);
+				     NULL, fn, data);
 }

This message is missing any context _why_ we want to do this. For
background: setting up these exclude patterns for hidden references is quite an important performance optimization in large repositories, so disabling it just like that is not an option without a good reason to do so.

So what is the issue that you see and why is this fix the solution for that issue?

Patrick

#2 lore raw
Quoted 47 lines · Patrick Steinhardt
Quoted 30 lines · SURA via GitGitGadget

From: SURA <[email protected]>

---
upload-pack: No longer use hidden-refs as exclude_patterns

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1866%2FSURA907%2Fmaster-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1866/SURA907/master-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1866

  upload-pack.c | 15 +++------------
  1 file changed, 3 insertions(+), 12 deletions(-)
upload-pack.c +0 -8
index 728b2477fcc..9ae42a463a3 100644
@@ -609,21 +609,12 @@ static int allow_hidden_refs(enum allow_uor allow_uor)
  static void for_each_namespaced_ref_1(each_ref_fn fn,
  				      struct upload_pack_data *data)
  {
-	const char **excludes = NULL;
  	/*
-	 * If `data->allow_uor` allows fetching hidden refs, we need to
-	 * mark all references (including hidden ones), to check in
-	 * `is_our_ref()` below.
-	 *
-	 * Otherwise, we only care about whether each reference's object
-	 * has the OUR_REF bit set or not, so do not need to visit
-	 * hidden references.

upload-pack.c static int allow_hidden_refs(enum allow_uor allow_uor) · line 618

Quoted 10 lines
upload-pack.c +3 -4
+	 * config transfer.hideRefs of upload-pack is diffient from arg exclude of for-each-ref,
+	 * We should not set exclude_patterns here
  	 */
-	if (allow_hidden_refs(data->allow_uor))
-		excludes = hidden_refs_to_excludes(&data->hidden_refs);
-
  	refs_for_each_namespaced_ref(get_main_ref_store(the_repository),
-				     excludes, fn, data);
+				     NULL, fn, data);
  }

This message is missing any context _why_ we want to do this. For
background: setting up these exclude patterns for hidden references is quite an important performance optimization in large repositories, so disabling it just like that is not an option without a good reason to do so.

Quoted 4 lines

So what is the issue that you see and why is this fix the solution for that issue?

Patrick

Oh, so sorry, I should merge mail message

See: https://lore.kernel.org/git/CAD6AYr-ZC32VNfUfMB63H-rQRfTdV=VQfBm67i2mG+6GDCNxkQ@mail.gmail.com/T/#u

Copy message here

---

Hello everyone

OS: Linux Mint 22
git version: v2.48.1

I found that packed refs are excluded by the transfer.hideRefs front match, while loose refs use full match (when transfer.hideRefs ends with '/', it is prefix match, which is normal)

When the server uses git, after setting transfer.hideRefs, the references that the client can see before and after server repo gc are different

It seems that 59c35fa accidentally damaged upload-pack when optimizing git for-each-ref

It seems that there is no simple fix except rolling back this commit?