Reference Name Joining

[FintanH]

Hey 👋

I've been perusing the git-ref family of names, i.e. FullName, FullNameRef, and PartialNameRef. I want to see if I'm using it right and also suggest a couple of additions that might be helpful for consumers of the API.

My use case is the following. I want to end up using try_find_reference, which takes one parameter that is impl TryInto<PartialNameRef>. I have a structural representation of the reference which looks like:

pub enum Remote {
    Default,
    Peer(PeerId),
}

pub struct Reference {
    pub remote: Remote,
    pub urn: Urn,
}

Now, I can't directly write a TryFrom for PartialNameRef as far as I can tell because of the associated lifetimes. But I can write a TryFrom for FullName and use to_partial. I'm wondering if that's the correct and intended way of using the API?

Regarding my suggestions, when I wrote a TryFrom for Remote, I wasn't able to reuse this when writing TryFrom for Reference, since there's no way to join two FullNames. I ended up having to use format! and use the TryFrom instance on &String. It would be useful to be able to join two names to get another valid name.

The other (possibly) useful suggestion would be to have a macro for writing static names, that are known to be valid at compile time. We used this technique in radicle-link[0] and it was extremely nice compare to using try_from everywhere :)

Let me know what you think, and perhaps I could help contribute with these additions ✌️

[Byron]

Thanks for posting, I remember that in order to make it convenient to use, I kept adding TryFrom implementations to cover the inputs encountered during testing. It's likely that many more are missing and I hope it's possible to add them when discovered.

Now, I can't directly write a TryFrom for PartialNameRef as far as I can tell because of the associated lifetimes. But I can write a TryFrom for FullName and use to_partial. I'm wondering if that's the correct and intended way of using the API?
Regarding my suggestions, when I wrote a TryFrom for Remote, I wasn't able to reuse this when writing TryFrom for Reference, since there's no way to join two FullNames. I ended up having to use format! and use the TryFrom instance on &String. It would be useful to be able to join two names to get another valid name.

I have added a test to validate currently possible inputs and to reproduce something akin to what you are describing or so I hope.

https://github.com/Byron/gitoxide/blob/06893cf49f98b0da4878c8d808544b6ec309f24e/git-repository/tests/easy/reference.rs#L13-L52

The other (possibly) useful suggestion would be to have a macro for writing static names, that are known to be valid at compile time. We used this technique in radicle-link[0] and it was extremely nice compare to using try_from everywhere :)

I love this macro and can imagine something like it to become a feature-gated part of git-ref for those who don't want to pay for safety checks at runtime. It's interesting to see how mem::transmute is used to create a newtype that doesn't publicly expose its fields.

What I don't understand is how a macro could help with the issue as I see it as it doesn't work at runtime, and if the name of the partial name is known at compile time, then try_find_reference("origin/foo") would work here as well. When a name is only known at runtime, I see no other way than to create a temporary String, which always needs to be held at the call site, unfortunately, due to the required borrow in PartialNameRef<'_>.

I think it would be possible to turn something like repo.find_reference(&name.to_partial_name()) into repo.find_reference(name.to_partial_name()), hence allowing to pass owned Strings and hold them in PartialNameRef (which then might as well be called PartialName). Using the Borrow might make this possible.

Let me know what you think, and perhaps I could help contribute with these additions ✌️

Based on my recent ramblings, what do you think would these additions look like? To me making PartialNameRef accept something owned (maybe even using a Cow) might do the trick.

[Byron]

I did quickly try the Cow solution as first step towards the supposedly possible generic one, making using String or BString as input possible. This should make it much easier to build input at runtime without having to borrow it.

https://github.com/Byron/gitoxide/blob/b7aab9efd42975e8f2dcb5c97e51495996175702/git-repository/tests/easy/reference.rs#L29-L60

The same would be possible for FullNameRef, but it's probably much more rare to encounter it in the API.

Since the only thing that's saved by this is the & in front of the formatted string I do wonder if there is something crucial I am missing.

Edit: Yes, this implementation I was missing:

https://github.com/Byron/gitoxide/blob/a216d89b8ef51a47aa9b19cc0296fbbe984b1066/git-repository/tests/easy/reference.rs#L53-L59

[FintanH]

I can expand a little bit more on what I was trying to get across using the same example as I had:

pub enum Remote {
    Default,
    Peer(PeerId),
}

pub struct Reference {
    pub remote: Remote,
    pub urn: Urn,
}

So the way I would attempt to tackle this is implement TryFrom for each part. So assuming there's a TryFrom<PeerId> for PartialNameRef and TryFrom<Urn> for PartialNameRef, I would start writing an implementation for Remote:

impl TryFrom<Remote> for PartialNameRef {
  type Error = refs::name::Error;

  fn try_from(remote: Remote) -> Result<Self, Self::Error> {
    match remote {
      Remote::Default => partial_name!("default"), // macro can do the validation at compile time
      Remote::Peer(peer) => ParitalNameRef::try_from(peer),
    }
  }
}

Aside

Admittedly, the macro doesn't shine here, but it is slightly nicer than using try_from. The macro does shine when you're combining parsed components with static components, e.g. from radicle-link:

namespace_prefix.join(reflike!("refs/remotes")).join(peer).join(reflike!("refs"))

I would then go on to build on top of the TryFrom<Remote> to write TryFrom<Reference>. Now I'm going to suppose there was a way to join reference names:

impl TryFrom<Reference> for PartialNameRef {
  type Error = refs::name::Error;

  fn try_from(r: Reference) -> Result<Self, Self::Error> {
    let remote = PartialNameRef::try_from(r.remote)?;
    let namespace = PartialNameRef::try_from(r.urn)?;
    Ok(partial_ref!("refs/rad/remotes").join(namespace).join(remote))
  }
}

The join allows us to build on top of the logic that already knows how to parse the sub-components. To, hopefully, sell you a bit more: say I had PartialNameRef prefix already and had a function:

fn add_remotes(prefix: PartialNameRef, remotes: &[String]) -> Result<Vec<PartialNameRef>, name::Error>

To implement this today, I would have to format! the prefix along with each String, but I already know it's valid so I should only need to parse the String and join it with the prefix. Or even better, I could make this function error-free by making the caller pass in &[PartialNameRef] and we iterate over the list and use join.

In a nutshell, I find this structural way of building up reference names more natural and avoids having to mess with the formatting of strings (too much :)).

What I don't understand is how a macro could help with the issue as I see it as it doesn't work at runtime, and if the name of the partial name is known at compile time, then try_find_reference("origin/foo") would work here as well.

Ya, that's true. But I would find out at runtime if I got my reference formatting wrong, something I don't enjoy 😄 This is all in the vain of catching my idiotic mistakes as soon as possible :)

Thanks for the response! And hopefully the above makes it more clear what I was thinking -- I didn't explain myself well enough the first time :) The PartialNameRef<'static> stuff looks like what I needed for the time being at least!

[Byron]

Thanks a lot for going the extra mile to make crystal clear how having compile-time validation and join(…) functionality can greatly help usability.

Also I am glad that PartialNameRef<'static> is a step forward, which can also implement fallible and non-fallible versions of join(…). The latter I can imagine putting together once I got a moment.

Edit: Here is how it looks like.

https://github.com/Byron/gitoxide/blob/c0fc4f6f1b42c117275be85e1c2e6893b58007ba/git-ref/tests/file/store/find.rs#L79-L82

The macro I could possibly rip off from how its done in radicle-link but it will be a second step and my very first proc macro :).

[FintanH]

The latter I can imagine putting together once I got a moment.

That's great! Obviously, no rush since the API is usable :) Thanks for taking my suggestions on board!

The macro I could possibly rip off from how its done in radicle-link but it will be a second step and my very first proc macro :).

Hehe fun =]

[Byron]

Thanks for taking my suggestions on board!

Always appreciated :), keep 'em coming!