Skip to content

Conversation

cruessler
Copy link
Contributor

This PR adds the suffix in_place so some methods that mutate a Reference in place, in order to make that effect more obvious. It is based on this suggestion: #2142 (comment).

This PR intentionally is very small, for two reasons: 1) in order to make it easier to review, 2) because I’m not sure how many methods to rename and where to stop. There is a number of methods that probably also should be renamed, such as peel_to_kind, peel_to_kind_packed, and a couple of follow_to_x methods. I haven’t done that yet because they sit at a lower level and we probably would have to change calls in higher-level methods to use the renamed methods. So, for example, peel_to_blob_in_place currently can still call peel_to_kind, but would have to call peel_to_kind_in_place if we wanted to be thorough.

What do you think?

This commit adds the suffix `in_place` so some methods that mutate a
`Reference` in place, in order to make that effect more obvious.
@Byron
Copy link
Member

Byron commented Sep 1, 2025

Thanks for kicking this off!

When looking at this I noticed that…

  • *_in_place() are the outliers
  • …the _in_place() suffix makes them more unwieldy and cumbersome
  • …I like functions without suffix more

What we are really trying to improve here is what happens in the relatively rare case when HEAD and refs/remotes/origin/HEAD (and the likes) are dealt with. By default, everything starts out immutable, so when someone is trying to use this method, a compiler error follows and people have to sit back and think.

When looking at the method descriptions, often they don't explicitly point out what it means that they are &mut, and maybe that's where I'd start, in addition to deprecating the existing *_in_place() methods for their suffix-less counterparts.

Does that sounds sane to you?


I also thought about internally copying the ref each time, but that would be going against gitoxide principles of avoiding unnecessary clones.

I also also thought about having a sibling method that does the copy internally, but then I thought one could rather my_ref.clone().peel_to_kind(). Because it does make sense that something mutate when you peel it.

Last but not least, maybe there is another verb, i.e. not peel, that would justify working on a copy? Then there could be a sibling method.

And last but not least, having these functions mutable is a good thing as a fully peeled ref won't incur more work when other versions of these methods are called on them, so I think it's good.

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.

2 participants