Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
slarse
force-pushed
the
GB-1075/dont-use-askpass-broker-for-cli
branch
2 times, most recently
from
2df7010 to
e8a3da1
Compare
slarse
force-pushed
the
GB-1075/dont-use-askpass-broker-for-cli
branch
from
e8a3da1 to
e632dcd
Compare
Byron
left a comment
Byron
left a comment
There was a problem hiding this comment.
I took a first look and like where this is going! Thanks for tackling this.
There are a few actionable items as well.
| async fn handle_git_prompt_clone(prompt: String, url: String) -> Option<String> { | ||
| tracing::info!("received prompt for clone of {url}: {prompt:?}"); | ||
| askpass::get_broker() | ||
| .expect("failed to get askpass broker") |
There was a problem hiding this comment.
With the recent improvements, panics (that this line could introduce) are safer as they are turned into errors.
We still have to be careful about them if they could run outside of GUI triggers, like in the filewatcher, but this isn't the case here.
(no action needed, just sharing some insights)
| but-llm.workspace = true | ||
|
|
||
| # this crate is only required so we can disable the askpass broker | ||
| gitbutler-repo-actions.workspace = true |
There was a problem hiding this comment.
Legacy crates have to be behind the legacy feature toggle, and be optional dependencies.
Should also obey the grouping and move down to the reset of 'em.
| on_prompt: F, | ||
| extra: Extra, | ||
| on_prompt: Option<F>, |
| /// If `on_prompt` is provided, the custom askpass broker machinery is used, which MUST have been | ||
| /// properly initialized. It should return the user's response or `None` if the operation should be |
There was a problem hiding this comment.
How would it be initialised? You could link the initialisation function via [crate::init()] or something like it.
| /// This function is **NOT** thread safe. | ||
| #[expect(static_mut_refs)] | ||
| pub unsafe fn init(submit_prompt: impl Fn(PromptEvent<Context>) + Send + Sync + 'static) { | ||
| unsafe { |
| if let Some(branch_id) = askpass { | ||
| tracing::info!("received prompt for branch push {branch_id:?}: {prompt:?}"); | ||
| askpass::get_broker() | ||
| .expect("failed to get askpass broker") |
There was a problem hiding this comment.
Rather than stating the obvious, it would be better to state what would be expected here. Maybe init() was called. In other words, call out the requirement or expectations as it would help fixing the panic if we'd hit it.
2 participants
🧢 Changes
Removes the askpass feature flag and instead figures out at runtime if the askpass broker is active or not.
This is just turning into more of a mess so I'm trying to figure out if we can simplify it. I really don't like how this turned out ...
☕️ Reasoning
When running
butas a symlink togitbutler-tauri, the feature flag will obviously have been set to include the askpass broker. However, the codepath never initializes it. And it doesn't really matter if it does because there's no good mechanism to respond. The CLI should delegate credentials input to whatever the terminal is configured to use (unless we want to build a full on TUI for everything, but we're not there yet).