Skip to content

Avoid duplicate hash computation during object creation#2549

Draft
Sebastian Thiel (Byron) wants to merge 1 commit intomainfrom
no-dupl-compute
Draft

Avoid duplicate hash computation during object creation#2549
Sebastian Thiel (Byron) wants to merge 1 commit intomainfrom
no-dupl-compute

Conversation

@Byron
Copy link
Copy Markdown
Member

Fixes #2516

Tasks

  • refackiew
  • commits with decent changelog entries

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks good to me, just a couple questions about the Write trait changes (TBH I'm really just comparing with the non-known_id variants)

Comment on lines +28 to +29
let _ = id;
self.write_buf(object, from)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this mirror how write_buf calls write_stream? i.e.:

Suggested change
let _ = id;
self.write_buf(object, from)
self.write_stream_with_known_id(id, object, from.len() as u64, &mut from)

It looks like places like cache.rs omit write_buf, so this kind of call might let them omit write_buf_with_known_id similarly (though looking at the loose/write.rs changes which do provide write_buf, perhaps cache.rs should actually be forwarding write_buf?)

size: u64,
from: &mut dyn io::Read,
) -> Result<gix_hash::ObjectId, crate::write::Error> {
let _ = id;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it right to be discarding the id here (versus not providing a default implementation)? Maybe this is appropriate for backwards compatibility; if that's the intent, maybe worth a comment?

Suggested change
let _ = id;
// Provide a default implementation which discards the id.
let _ = id;

@Byron
Copy link
Copy Markdown
Member Author

Jon Ross-Perkins (@jonmeow) Thanks so much for taking a look!

I haven't looked at the code at all yet, but when I do I will particularly look at the places you are highlighting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add API so that Repository write functions only compute hash once

3 participants