On Mon, Jul 03, 2023 at 02:26:44AM -0400, Jeff King wrote:
> I guess v2 clients don't hit this code at all (they are handled by
> ls-refs, which comes in the next patch). So that just leaves the case
> that allowAny is set. By itself the optimization should kick in (good).
> With allowTip or allowReachable it would not, but that is the "this is
> stupid" case in which it is OK to fall back to the existing behavior
> (even though we _could_ enable the optimization). OTOH, it would be easy
> to check it, as it's just another bit in allow_uor.
>
> I'm OK going either way.
As am I, though I agree that checking for it is not hard. The return
value of `allow_hidden_refs()` as currently written is confusing, so
here's an inversion of that function with this suggestion on top:
--- >8 ---
diff --git a/upload-pack.c b/upload-pack.c
index ef2ca36feb..5a0fa028c6 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -604,7 +604,9 @@ static int get_common_commits(struct upload_pack_data *data,
static int allow_hidden_refs(enum allow_uor allow_uor)
{
- return allow_uor & (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1);
+ if ((allow_uor & ALLOW_ANY_SHA1) == ALLOW_ANY_SHA1)
+ return 1;
+ return !(allow_uor & (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1));
}
static void for_each_namespaced_ref_1(each_ref_fn fn,
@@ -620,7 +622,7 @@ static void for_each_namespaced_ref_1(each_ref_fn fn,
* has the OUR_REF bit set or not, so do not need to visit
* hidden references.
*/
- if (!allow_hidden_refs(data->allow_uor))
+ if (allow_hidden_refs(data->allow_uor))
excludes = hidden_refs_to_excludes(&data->hidden_refs);
for_each_namespaced_ref(excludes, fn, data);
@@ -629,7 +631,7 @@ static void for_each_namespaced_ref_1(each_ref_fn fn,
static int is_our_ref(struct object *o, enum allow_uor allow_uor)
{
- return o->flags & ((allow_hidden_refs(allow_uor) ? HIDDEN_REF : 0) | OUR_REF);
+ return o->flags & ((allow_hidden_refs(allow_uor) ? 0 : HIDDEN_REF) | OUR_REF);
}
/*
--- 8< ---
Thanks,
Taylor
