Re: [PATCH 05/14] rust: add a hash algorithm abstraction
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 29, 2025, 13:27 UTC
- Message-ID
- <xmqq8qgtbzyi.fsf@gitster.g>
- In-Reply-To
- <CAH=ZcbD80RGeuxqcDiWr2KNaQzFCrd=9fQOGo_+pW9E6+HmtQA@mail.gmail.com>
Ezekiel Newren <ezekielnewren@gmail.com> writes:
Show 20 quoted lines
>> > +impl ObjectID {
>> > + pub fn as_slice(&self) -> &[u8] {
>> > + match HashAlgorithm::from_u32(self.algo) {
>> > + Some(algo) => &self.hash[0..algo.raw_len()],
>> > + None => &self.hash,
>> > + }
>> > + }
>> > +
>> > + pub fn as_mut_slice(&mut self) -> &mut [u8] {
>> > + match HashAlgorithm::from_u32(self.algo) {
>> > + Some(algo) => &mut self.hash[0..algo.raw_len()],
>> > + None => &mut self.hash,
>> > + }
>> > + }
>> > +}
>>
>> These cases for "None" surprised me a bit; I would have expected us
>> to error out when given an algorithm we do not recognise.
>
> I think _Result_ would be more appropriate here.Perhaps. But the Option/Result was not what I was suprised about.
When algo is available, we gave back a slice that is properly sized, but when algo is not, I would have expected it to say "nope", instead of yielding the full area of memory available. That was the part I was surprised about.
Perhaps as_mut_slice() side is justifiable (an uninitialized instance of ObjectID is filled by getting the full self.hash and filling it, plus filling the algo), but the same explanation would not apply on the read-only side.