Skip to content

Add gix::open_with_environment_overrides #2106

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cruessler
Copy link
Contributor

Based on this suggestion: gitui-org/gitui#2664 (comment).

@Byron Byron marked this pull request as draft August 8, 2025 03:51
@Byron
Copy link
Member

Byron commented Aug 8, 2025

Thanks for kicking this off.

I turned the PR back into draft mode as it seems like it was generated by an agent which can't run cargo locally.

And thinking about it, I think I completely failed to see that open_with_environment_overrides() already exists, so it should just be used in gitui. There probably is no need for this to be top-level, but I don't object to it either.

Can be closed, can be fixed, it's up to you.

@cruessler cruessler force-pushed the add-open-with-environment-overrides branch from 9a595f3 to 5ac6ba1 Compare August 8, 2025 06:47
@cruessler
Copy link
Contributor Author

Thanks for kicking this off.

I turned the PR back into draft mode as it seems like it was generated by an agent which can't run cargo locally.

Actually, the agent was me, just copying gix::discover without running any checks, not taking into account that parameters and return type needed to be adapted. :-)

And thinking about it, I think I completely failed to see that open_with_environment_overrides() already exists, so it should just be used in gitui. There probably is no need for this to be top-level, but I don't object to it either.

I find the addition of gix::discover_with_environment_overrides as well as gix::open_with_environment_overrides useful as I remember my confusion when there was only gix::discover in the docs, but no obvious way of doing the same with environment overrides. Or am I missing something? If so, I could extend the docs so that it becomes clearer which function to use for which use case.

@Byron
Copy link
Member

Byron commented Aug 8, 2025

Actually, the agent was me, just copying gix::discover without running any checks, not taking into account that parameters and return type needed to be adapted. :-)

I admit I am kind of paranoid these days 😅.

I find the addition of gix::discover_with_environment_overrides as well as gix::open_with_environment_overrides useful as I remember my confusion when there was only gix::discover in the docs,

That's all I need to think these top-level functions really should be there. And it would certainly be nice if they could at least have the gist of why one would want to use them in their own docs before referring to the respective docs in ThreadSafeRepository.

@EliahKagan
Copy link
Member

EliahKagan commented Aug 9, 2025

It looks like the remaining CI failures are not due to code that is new in this PR.

  1. The lint failure is due to an improved dead code diagnostic in Rust 1.89; the error is fixed in Mark ReadDataLineFuture as #[allow(dead_code)] #2108.

  2. The test-fast failure is due to a problem on the runner itself:

    The hosted runner lost communication with the server. Anything in your workflow that terminates the runner process, starves it for CPU/Memory, or blocks its network access can cause this error.

If the feature branch here is rebased onto the current tip of the main branch, then I expect (1) to pass because the fix is integrated into main, and (2) to pass because it looks like a failure with low probability that doesn't require any change to fix.

(I have refrained from making those changes myself and merging this PR, in case there are reasons unrelated to those CI failures that this PR has not yet been marked back from draft.)

@Byron
Copy link
Member

Byron commented Aug 9, 2025

Thanks a lot for your help @EliahKagan , I was a bit slow yesterday.

@cruessler
Copy link
Contributor Author

And it would certainly be nice if they could at least have the gist of why one would want to use them in their own docs before referring to the respective docs in ThreadSafeRepository.

I’ll extend the docs, some time this week probably!

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.

3 participants