Skip to content

Conversation

metlos
Copy link
Contributor

@metlos metlos commented Sep 15, 2025

The remote has a couple of "builder" methods to change is fields, e.g. push_url for setting the push url.

A builder method for changing the fetch url of a remote was missing. This makes it impossible to fully replicate the functionality of git remote set-url (e.g. look at the lengths I'm trying to workaround this (imperfectly) in a PR to replicate git remote set-url --push) in jj: https://github.com/metlos/jj/blob/8ec7cd1f68a5279e6aff2faea248a4978d46d17a/lib/src/git.rs#L2024)

metlos and others added 2 commits September 15, 2025 21:35
The remote has a couple of "builder" methods to change
is fields, e.g. `push_url` for setting the push url.

A builder method for changing the fetch url of a remote
was missing. This makes it impossible to fully replicate 
the functionality of `git remote set-url`.
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot for contributing!

Having API symmetry is certainly a good trait to have. Probably I avoided having these to avoid confusion with the Repository::remote_at*() methods which do the same, but I added a note that ideally these are used instead.

To me these kinds of API are still unsolved, as in theory one would either want to have free access to the fields (something I prefer), or one has to provide both builders and setters for everything which is cumbersome.
It's certainly possible to find more instances where certain methods are missing even though one would expect them to be there.

@Byron Byron enabled auto-merge September 17, 2025 03:04
@Byron Byron merged commit 51f998f into GitoxideLabs:main Sep 17, 2025
25 checks passed
@metlos
Copy link
Contributor Author

metlos commented Sep 17, 2025

Yeah, there's no silver bullet for the APIs. Thanks for merging this! 🫶

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