Skip to content

Improve error messages #2617

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

Merged
merged 3 commits into from
Apr 21, 2025
Merged

Improve error messages #2617

merged 3 commits into from
Apr 21, 2025

Conversation

acuteenvy
Copy link
Contributor

@acuteenvy acuteenvy commented Apr 16, 2025

This Pull Request fixes/closes #2599.

It changes the following:

  • Improve error messages when the repository cannot be opened.
    • for example, when the repo doesn't exist:
    # previously
    $ gitui
    invalid path
    please run gitui inside of a non-bare git repository
    
    # now
    $ gitui
    Error: could not find repository at '.'; class=Repository (6); code=NotFound (-3)
  • Remove the panic handler from rayon, since panic::set_hook already handles all panics. Setting both panic handlers causes the panic message to be displayed twice.
  • Do both log::error! and eprintln! wherever possible

I followed the checklist:

  • I added unittests
  • I ran make check without errors
    error: Dependencies for . are not sorted
    error: Cargo.toml for . is not formatted
    Checking asyncgit...
    Checking filetreelist...
    Checking git2-hooks...
    Checking git2-testing...
    Checking scopetime...
    error: Dependencies for scopetime are not sorted
    error: Cargo.toml for scopetime is not formatted
    make: *** [Makefile:108: sort] Error 1
    
  • I tested the overall application
  • I added an appropriate item to the changelog

@naseschwarz

This comment was marked as outdated.

@acuteenvy
Copy link
Contributor Author

I just found out why - cargo-sort v1.1.0 is broken and has been yanked from crates.io. I installed the broken version from the Arch repositories.

Installing v1.0.9 fixes this issue.

Copy link
Contributor

@naseschwarz naseschwarz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this. This clearly improves error handling. For a non-repo folder, we get / got:

image

For dubious ownership, the original cause here, we get / got /git says:

image

I'd almost argue that the new message is better than what git says. "Let's just ignore this security feature with this command" maybe is not soooo cool. ;)

I have to pick on one thing, though. The panic handler was there for a reason, as far as I can tell. See my remark below.

@naseschwarz
Copy link
Contributor

Also, would you please be so kind and add a changelog entry?

@naseschwarz naseschwarz self-requested a review April 17, 2025 13:35
Copy link
Contributor

@naseschwarz naseschwarz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the duplicate panic issue. @extrawurst, you added this initially, you probably have an opinion yourself.

@extrawurst
Copy link
Collaborator

Ok I tested the extra error handler for rayon is not needed as far as i can see. removed in on master. @acuteenvy please rebase and then we can merge this

@extrawurst extrawurst merged commit ee5c243 into gitui-org:master Apr 21, 2025
22 checks passed
@acuteenvy acuteenvy deleted the better-errors branch April 21, 2025 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

path ownership rights issue leads to invalid bare repo detection
3 participants