Re: [PATCH v3 0/8] hash: introduce unsafe_hash_algo(), drop unsafe_ variants

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 22, 2025, 21:31 UTC
Message-ID: <xmqqwmemv8ky.fsf@gitster.g>
In-Reply-To: <20250118124343.GA3828177@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> .... But maybe
> returning UNKNOWN (as above) is a safer bet than losing the inverse
> nature of the by_ptr() and by_id() functions, which could otherwise
> cause hard-to-find interactions?

Yeah, that sounds sensible.

Show 15 quoted lines

>      I think it "works" because the backtrace is:
>
>       [...]
>       #5  0x0000556d436f6b71 in BUG_fl (file=0x556d43790e8b "hash.h", line=324,
>           fmt=0x556d43790e73 "null hash, return is %d") at usage.c:335
>       #6  0x0000556d4357c2f9 in hash_algo_by_ptr (p=0x0) at /home/peff/compile/git/hash.h:324
>       #7  0x0000556d4357c437 in oidclr (oid=0x7ffdf5810ea4, algop=0x0) at /home/peff/compile/git/hash.h:392
>       #8  0x0000556d435801c7 in prep_exclude (dir=0x7ffdf5811190, istate=0x556d608959c0, base=0x556d6089da50 "nums",
>           baselen=0) at dir.c:1699
>       [...]
>       #16 0x0000556d4344dd4a in cmd_grep (argc=0, argv=0x556d60895ee8, prefix=0x0, repo=0x0) at builtin/grep.c:1257
>
>      So we probably write a totally garbage algo field into the
>      object_id, but nobody ever ends up looking at it. Probably
>      something we should clean up, but way out of scope for your series.

Yeah, that looks bad, but I think it is fine to leave it outside the series to fix that.

>      But I do think it reinforces that returning UNKNOWN is an
>      improvement; it avoids undefined behavior and anybody who tried to
>      use it should get a BUG() from calling git_hash_unknown_*()
>      functions.

Sounds like a sensitive direction to go in.

Thanks.