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

Allow configuting the Repository object created by PrepareFetch #1141

Merged
merged 2 commits into from
Dec 8, 2023

Conversation

bittrance
Copy link
Contributor

In the process of implementing terminal prompt configuration (see #1093) I cannot find a way to apply this configuration when using gix::prepare_clone(), that is, before calling .fetch_only(). There seems to be two alternatives to this:

  • Add a Remote::repo_mut() method so that you can do remote.repo().config_snapshot_mut() in PrepareFetch::configure_remote()
  • Add a PrepareFetch::configure() method which gives access to a &mut SnapshotMut<'_> object

I don't like the first solution. You are supposed to have recevied the Remote from a Repository and you should be able to do configuration from there. To my understanding the configure_{remote,transport} methods exist because you can do advanced stuff with the respective object that is not directly coverend by cofngiration.

I'm not too fond of the second option either (not least because there would be three different configuration methods on PrepareFetch), but I created this pull request to point out that there is currently no way to perform such configuration.
(You may of course claim that gix::prepare_clone is a convenience method and that if you need to configure your repo, you should init an empty repo and fetch into it.)

@Byron
Copy link
Member

Byron commented Nov 30, 2023

Thanks for bringing this up and for the initial analysis!

It's great to go straight to PR with the preferred option as it's much more visual that way. Something I currently find cumbersome is error handling in closures, as one is forced to go down to Box<dyn Error> , and I'd love to avoid that (until there are other options at least, clearly a use-case for therror, CC @NobodyXu).

However, I think a current solution could be to ask for an iterator of command-overrides to pass to append_config() instead.

Would that be enough for your use-case?

@Byron
Copy link
Member

Byron commented Dec 7, 2023

Maybe it's a bit much to have to deal with two work-streams at the same time. If so, please feel free to close this PR and reopen once there is more time.

@bittrance bittrance force-pushed the configure-prepare-fetch branch from e895fef to 9833b45 Compare December 8, 2023 09:46
@bittrance
Copy link
Contributor Author

Not at all. Just had to work around some limitations in the dependent app. I used the name config_overrides because it is what open uses, but I would have preferred with_config_overrides in both places. Similarly, this implementation imitates open in replacing previous overrides when called multiple times.

Any way, in the dependent app, it looks like this:

let repo = gix::prepare_clone(url, target)
    .unwrap()
    .config_overrides(vec!["gitoxide.credentials.terminalPrompt=false"])
    .fetch_only(Discard, &watchdog)
    .map(|(r, _)| r)?;

Config parse errors are not emitted until you fetch, which is a bit of a wart, but probably a minor one since in 9 cases out of 10, you will do both at the same time like the above example.

@Byron
Copy link
Member

Byron commented Dec 8, 2023

Config parse errors are not emitted until you fetch, which is a bit of a wart, but probably a minor one since in 9 cases out of 10, you will do both at the same time like the above example.

That's true, but it's supposed to be used like like here in cargo. Going through Keys makes it safe as it is checked at the call-site.

With it one can affect the repository configuration right before fetching.
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! This looks just like I imagined it :)!

And yes, a lot has to happen to make builders consistent, and further to change the API surface so that it can be stabilized. Right now, there are many open options structs that will break the API with each new addition. Further, each error enum technically breaks the API each time a new variant is added.

In any case, I have made minor adjustments, added a test, and will merge when CI is green.

@bittrance
Copy link
Contributor Author

Nice 👍 Sorry about missing test cases; for some reason I looked for tests only inside gix/src/clone for existing test cases.

@Byron Byron merged commit 281fda0 into GitoxideLabs:main Dec 8, 2023
19 checks passed
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