Skip to content

fix the crashes when starting it from .git directory issue #2734

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

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix the crashes when starting it from .git directory issue #2734

wants to merge 2 commits into from

Conversation

0x61nas
Copy link
Contributor

@0x61nas 0x61nas commented Oct 6, 2025

This Pull Request fixes/closes #2732.

It changes the following:

  • Checks if the current working directory is git
  • and if its we take its parent directory

I followed the checklist:

  • I added unittests (no need)
  • I ran make check without errors
  • I tested the overall application
  • I added an appropriate item to the changelog

@extrawurst extrawurst requested a review from cruessler October 9, 2025 09:01
@cruessler
Copy link
Collaborator

@extrawurst What’s the expected outcome? Currently, I’m getting the following error message:

gitoxide/.git on  main [!?⇣]
❯ gitui

GitUI was closed due to an unexpected panic.
Please file an issue on https://github.com/gitui-org/gitui/issues with the following info:

panicked at asyncgit/src/revlog.rs:186:14:
failed to fetch: Gix(Discover(Open(Io(Custom { kind: Other, error: "'./.git/objects' wasn't a directory" }))))

trace:
   0: gitui::set_panic_handler::{{closure}}
   1: std::panicking::rust_panic_with_hook
   2: std::panicking::begin_panic_handler::{{closure}}
   3: std::sys::backtrace::__rust_end_short_backtrace
   4: __rustc::rust_begin_unwind
   5: core::panicking::panic_fmt
   6: core::result::unwrap_failed
   7: core::result::Result<T,E>::expect
   8: <rayon_core::job::HeapJob<BODY> as rayon_core::job::Job>::execute
   9: rayon_core::registry::WorkerThread::wait_until_cold
  10: std::sys::backtrace::__rust_begin_short_backtrace
  11: core::ops::function::FnOnce::call_once{{vtable.shim}}
  12: std::sys::pal::unix::thread::Thread::new::thread_start
  13: start_thread
             at ./nptl/pthread_create.c:448:8
  14: __GI___clone3
             at ./misc/../sysdeps/unix/sysv/linux/x86_64/clone3.S:78:0

Rayon: detected unexpected panic; aborting
fish: Job 1, 'gitui' terminated by signal SIGABRT (Abort)

In general, I don’t think we should try to replicate the logic for detecting whether we’re in a valid git repository as that is already done by libgit2 or gitoxide. Instead, we should focus on surfacing the error returned by libgit2/gitoxide in a more concise way. What do you think? If you agree, we should probably change this

gitui/asyncgit/src/revlog.rs

Line 186 in 1d22485

.expect("failed to fetch");
in a way that the caller gets access to the error and can react accordingly.

@extrawurst
Copy link
Collaborator

I agree

@0x61nas
Copy link
Contributor Author

0x61nas commented Oct 9, 2025

hmm, that's weird it worked in my testing, i'll try to look again

@0x61nas
Copy link
Contributor Author

0x61nas commented Oct 9, 2025

In general, I don’t think we should try to replicate the logic for detecting whether we’re in a valid git repository as that is already done by libgit2 or gitoxide. Instead, we should focus on surfacing the error returned by libgit2/gitoxide in a more concise way. What do you think? If you agree, we should probably change this

I thought of this first, but being in the .git directory is sort of a special case for us and am not sure if libgit2/gitoxide has a specific error for this we can catch it and try to recover

@cruessler
Copy link
Collaborator

For the special case of opening gitui inside .git, your workaround works. What I was referring to was that gitoxide already gives you a good error message: Gix(Discover(Open(Io(Custom { kind: Other, error: "'./.git/objects' wasn't a directory" })))). What I’m proposing is that we go for a more general solution that would allow us to surface all errors that would currently cause the .expect() I was linking to to panic. Instead of panicking in a worker, we would store the error inside AsyncLog and force consumers to write code to handle the error case. Does that explanation help?

@0x61nas
Copy link
Contributor Author

0x61nas commented Oct 9, 2025 •
edited
Loading

yeah, i do agree with you, but isn't that would be considered a major refactoring? and yes it would improve the error handing system in general but i still can't see a reasonable way to recover from this specific error, you might argue that the error message is kinda enough and we shouldn't add an additional complexity to handle this somewhat rare case, but this PR is meant to address this particular issue (#2732) with no breaking changes

although that i do agree that if we just don't panic and display a nice little error message without scaring our users or do something sneaky like in the PR, i don't see it feasible in this scope (Issue/PR)

so maybe we could consider this as a temporary solution and open another issue for the more correct solution that would require some more changes (but then if we do remove this hack it might considered as a breaking change)

@cruessler
Copy link
Collaborator

Yes, that would be a larger change, but I’d say go for it! 😄 In this particular instance, I think we shouldn’t feel constrained by the initial ticket’s scope, rather we should go for the solution that makes the most sense long-term. Or do you have other concerns that I haven’t addressed yet?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cruessler cruessler Awaiting requested review from cruessler

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Gitui crashes at start

3 participants

@0x61nas @cruessler @extrawurst