Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: add a command for listing git remotes and their URLs #244

Merged
merged 1 commit into from
Apr 29, 2022

Conversation

martinvonz
Copy link
Owner

@martinvonz martinvonz commented Apr 26, 2022

As requested by @talpr. I added this is a separate new command jj git remote list. One could also imagine showing the listing when there is
no sub-command specified to jj git remote, but we don't have other
commands that behave that way yet.

Closes #243

Checklist

  • I have made relevant updates to CHANGELOG.md

Copy link
Collaborator

@talpr talpr left a comment

Choose a reason for hiding this comment

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

That was quick, thanks!

let workspace_command = command.workspace_helper(ui)?;
let repo = workspace_command.repo();
let git_repo = get_git_repo(repo.store())?;
for remote_name in git_repo.remotes()?.iter().flatten() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is flatten() needed here?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Because the iterator before it yields Option<&str>, so this flatten() skips the empty values. I had actually initially written it as an if let Some(...) but then this Clippy lint told me to change to flatten(). I also find it unclear, but maybe we'll prefer it after we've been exposed to it a bit? Or we can disable that Clippy lint if you prefer (it would be the first one we disable).

pub mod common;

#[test]
fn test_git_remotes() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Test a <no URL> case?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think that can only happen if the URl is not valid UTF-8 and the libgit2 API doesn't seem to let us set such a URL (it takes a &str). I even tried to make this test write directly to .jj/repo/store/git/config but libgit2 apparently can't even open the repo if the config file has (or at least the URL?) has invalid UTF-8. So I guess we'll have to do without the test for now.

As requested by @talpr. I added this is a separate new command `jj git
remote list`. One could also imagine showing the listing when there is
no sub-command specified to `jj git remote`, but we don't have other
commands that behave that way yet.

Closes #243
@martinvonz martinvonz merged commit 0719dae into main Apr 29, 2022
@martinvonz martinvonz deleted the git_remotes branch April 29, 2022 21:08
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 a jj git remote list command
3 participants