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

Fix log crashing in subdirectories #2301

Merged

Conversation

cruessler
Copy link
Contributor

When opened in a sub-directory of a git repository, the log view crashes. Replacing gix::open by gix::discover fixes this issue. gix::open errors when the passed directory is not a git repository, gix::discover searches parent directories for a git repository. The code causing the crash has not been released yet, so no changelog entry is necessary.

Replace `gix::open` by `gix::discover`. `gix::open` errors when the
passed directory is not a git repository.
@extrawurst
Copy link
Owner

@cruessler can you add a teest for that so we don't regress on this again?

@extrawurst
Copy link
Owner

@cruessler we also need to make sure it does respect the env vars, see #2298

@cruessler
Copy link
Contributor Author

@cruessler can you add a teest for that so we don't regress on this again?

@extrawurst I just added a test for running AsyncLog in a sub-directory (the test fails on master, but passes on this branch). The test works, but it feels a bit like testing implementation details. I think we could keep it for now, but might want to come up with a cleaner test in the future. What do you think?

I’ll have a look at environment variables next.

@extrawurst extrawurst merged commit 2b8ef40 into extrawurst:master Sep 17, 2024
21 checks passed
@extrawurst
Copy link
Owner

@cruessler please make a followup with a test for the env, this broke in the past and it would be good to know when it does :)

@cruessler
Copy link
Contributor Author

@cruessler please make a followup with a test for the env, this broke in the past and it would be good to know when it does :)

Will do, I put it on my todo list. :-)

@cruessler
Copy link
Contributor Author

@extrawurst I would really like to write an integration test for env variables, similar to what I’ve tried in #2368. That way, we ideally would have a test that read like “set env variable, start app, expect screenshot showing the loaded app to look like snapshot”. I will also try adding a test to app.rs.

@extrawurst
Copy link
Owner

Sounds great!

@cruessler cruessler deleted the use-gix-discover-instead-of-open branch October 15, 2024 06:50
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