Skip to content

Add git.ssh-key-file setting #5458

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
+25 −2
Closed

Add git.ssh-key-file setting #5458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions cli/src/config-schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,11 @@
"type": "string",
"description": "Path to the git executable",
"default": "git"
},
"ssh-key-file": {
"type": "string",
"description": "Path to SSH key file to use; by default, uses the following files in ~/.ssh: id_ed25519_sk, id_ed25519, id_rsa",
"default": ""
}
}
},
Expand Down
9 changes: 7 additions & 2 deletions cli/src/git_util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,8 +311,13 @@ pub fn with_remote_git_callbacks<T>(
.map(|x| x as &mut dyn FnMut(&git::Progress));
callbacks.sideband_progress =
sideband_progress_callback.map(|x| x as &mut dyn FnMut(&[u8]));
let mut get_ssh_keys = get_ssh_keys; // Coerce to unit fn type
callbacks.get_ssh_keys = Some(&mut get_ssh_keys);
let get_ssh_keys: &mut dyn FnMut(&_) -> _ = &mut |username| {
git_settings
.ssh_key_file
.as_ref()
.map_or_else(|| get_ssh_keys(username), |key_file| vec![key_file.clone()])
};
callbacks.get_ssh_keys = Some(get_ssh_keys);
let mut get_pw =
|url: &str, _username: &str| pinentry_get_pw(url).or_else(|| terminal_get_pw(ui, url));
callbacks.get_password = Some(&mut get_pw);
Expand Down
13 changes: 13 additions & 0 deletions lib/src/settings.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ pub struct GitSettings {
pub abandon_unreachable_commits: bool,
pub subprocess: bool,
pub executable_path: PathBuf,
pub ssh_key_file: Option<PathBuf>,
}

impl GitSettings {
Expand All @@ -70,6 +71,17 @@ impl GitSettings {
abandon_unreachable_commits: settings.get_bool("git.abandon-unreachable-commits")?,
subprocess: settings.get_bool("git.subprocess")?,
executable_path: settings.get("git.executable-path")?,
ssh_key_file: match settings.get_string("git.ssh-key-file") {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we add config knob, I would expect it would be a list of paths which defaults to ["~/.ssh/id_..", ..].

And it's probably better to move config handling to CLI. jj_lib::expand_home_path() is half-baked. Ideally, library code shouldn't depend on environment.

Copy link
Author

Choose a reason for hiding this comment

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

Duly noted. If nobody gets to it I'll take a look on the week of Feb 03 when I return from vacation.

Copy link
Author

Choose a reason for hiding this comment

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

Update: I've looked into modifying the new ssh_key_file configuration option to support a list of values (rather than a single string value); however, I got lost in the complexities of rust (I am a total rust noob). It looks like a list-value configuration option would be breaking some new ground.

What if we keep this new config option as a single string value for now. In the future (or in a follow-up PR) it can be expanded to support a list of string values. This updated implementation could treat a string value in the same manner that it would treat a list of one string element, to maintain backwards compatibility.

Copy link
Member

Choose a reason for hiding this comment

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

#5228 has been merged and will be part of the release that goes out today. I suspect we'll make subprocessing the default from the release after.

Copy link
Author

Choose a reason for hiding this comment

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

Ok I'll close this PR and revisit it if git.subprocess doesn't address my specific issue. Thank you for your engagement.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like a list-value configuration option would be breaking some new ground.

Because StackedConfig::get returns any T such that T: Deserialize, you can just config.get::<Vec<String>>("some.key") to get a Result<Vec<String>, ConfigGetError>.

Ok(ssh_key_file) => {
let trimmed_ssh_key_file = ssh_key_file.trim();
if trimmed_ssh_key_file.is_empty() {
None
} else {
Some(crate::file_util::expand_home_path(trimmed_ssh_key_file))
}
}
Err(_) => None,
},
})
}
}
Expand All @@ -81,6 +93,7 @@ impl Default for GitSettings {
abandon_unreachable_commits: true,
subprocess: false,
executable_path: PathBuf::from("git"),
ssh_key_file: None,
}
}
}
Expand Down