| Message ID | 20250219-pks-update-ref-optimization-v2-2-e696e7220b22@pks.im (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | refs: batch refname availability checks | expand |
On Wed, Feb 19, 2025 at 02:23:29PM +0100, Patrick Steinhardt wrote: > When reading an object ID via `get_oid_basic()` or any of its related > functions we perform a check whether the object ID is ambiguous, which > can be the case when a reference with the same name exists. While the > check is generally helpful, there are cases where it only adds to the > runtime overhead without providing much of a benefit. > > Add a new flag that allows us to disable the check. The flag will be > used in a subsequent commit. If we are going to switch to this and get rid of the global warn_on_object_refname_ambiguity flag, I could see it being worth it. But when I looked into doing that, it did not make much sense (there are too many code paths that share the same get_oid calls, and you'd have to plumb the flags through the stack). So if we are going to leave the global flag anyway, and if your patch 3 is just changing all of update-ref to pass the per-call flag in every call, why don't we just skip this new mechanism and have update-ref unset the warn_on_object_refname_ambiguity flag? That makes patch 3 a one-liner, and patches 1 and 2 can go away. -Peff PS Sorry, I haven't looked carefully at the rest of the series. I've been moving houses and am way back-logged on Git stuff, so don't count on me reviewing it anytime soon.
On Fri, Feb 21, 2025 at 03:00:03AM -0500, Jeff King wrote: > On Wed, Feb 19, 2025 at 02:23:29PM +0100, Patrick Steinhardt wrote: > > > When reading an object ID via `get_oid_basic()` or any of its related > > functions we perform a check whether the object ID is ambiguous, which > > can be the case when a reference with the same name exists. While the > > check is generally helpful, there are cases where it only adds to the > > runtime overhead without providing much of a benefit. > > > > Add a new flag that allows us to disable the check. The flag will be > > used in a subsequent commit. > > If we are going to switch to this and get rid of the global > warn_on_object_refname_ambiguity flag, I could see it being worth it. > > But when I looked into doing that, it did not make much sense (there are > too many code paths that share the same get_oid calls, and you'd have to > plumb the flags through the stack). > > So if we are going to leave the global flag anyway, and if your patch 3 > is just changing all of update-ref to pass the per-call flag in every > call, why don't we just skip this new mechanism and have update-ref > unset the warn_on_object_refname_ambiguity flag? > > That makes patch 3 a one-liner, and patches 1 and 2 can go away. > > -Peff > > PS Sorry, I haven't looked carefully at the rest of the series. I've > been moving houses and am way back-logged on Git stuff, so don't > count on me reviewing it anytime soon. Spoiler alert: I do have a patch series locally that gets rid of the global variable completely, and that series builds on top of the new flag I'm introducing here. So I'd prefer to keep it so that we can eventually have less callsites that rely on global state. Patrick
On Fri, Feb 21, 2025 at 09:36:04AM +0100, Patrick Steinhardt wrote: > Spoiler alert: I do have a patch series locally that gets rid of the > global variable completely, and that series builds on top of the new > flag I'm introducing here. So I'd prefer to keep it so that we can > eventually have less callsites that rely on global state. OK. I do agree that getting rid of the global would be nice. I just wasn't sure how ugly it would be to pass the flags through the stack into handle_revision(). -Peff
diff --git a/hash.h b/hash.h index 4367acfec50..79419016513 100644 --- a/hash.h +++ b/hash.h @@ -204,6 +204,7 @@ struct object_id { #define GET_OID_ONLY_TO_DIE 04000 #define GET_OID_REQUIRE_PATH 010000 #define GET_OID_HASH_ANY 020000 +#define GET_OID_SKIP_AMBIGUITY_CHECK 040000 #define GET_OID_DISAMBIGUATORS \ (GET_OID_COMMIT | GET_OID_COMMITTISH | \ diff --git a/object-name.c b/object-name.c index bc0265ad2a1..3e0b7edea11 100644 --- a/object-name.c +++ b/object-name.c @@ -961,7 +961,9 @@ static int get_oid_basic(struct repository *r, const char *str, int len, int fatal = !(flags & GET_OID_QUIETLY); if (len == r->hash_algo->hexsz && !get_oid_hex(str, oid)) { - if (repo_settings_get_warn_ambiguous_refs(r) && warn_on_object_refname_ambiguity) { + if (!(flags & GET_OID_SKIP_AMBIGUITY_CHECK) && + repo_settings_get_warn_ambiguous_refs(r) && + warn_on_object_refname_ambiguity) { refs_found = repo_dwim_ref(r, str, len, &tmp_oid, &real_ref, 0); if (refs_found > 0) { warning(warn_msg, len, str);
When reading an object ID via `get_oid_basic()` or any of its related functions we perform a check whether the object ID is ambiguous, which can be the case when a reference with the same name exists. While the check is generally helpful, there are cases where it only adds to the runtime overhead without providing much of a benefit. Add a new flag that allows us to disable the check. The flag will be used in a subsequent commit. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- hash.h | 1 + object-name.c | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-)