[Bug]: Network errors are shown even if "Disable notifications" is on.

stroiman commented 2 weeks ago

Describe the bug

I often work disconnected, and I find it annoying to see a git push error message in that case. Despite I have enabled "suppress notifications"

Relevant errors (if available) from notifications or console (`CTRL+SHIFT+I`)

plugin:obsidian-git:29441 Uncaught (in promise) Error: ssh: connect to host github.com port 22: Undefined error: 0
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

    at Object.action (plugin:obsidian-git:29441:25)
    at PluginStore.exec (plugin:obsidian-git:29466:25)
    at eval (plugin:obsidian-git:26971:43)
    at new Promise (<anonymous>)
    at GitExecutorChain.handleTaskData (plugin:obsidian-git:26969:16)
    at GitExecutorChain.eval (plugin:obsidian-git:26953:44)
    at Generator.next (<anonymous>)
    at fulfilled (plugin:obsidian-git:25798:24)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
plugin:obsidian-git:29441 Uncaught (in promise) Error: ssh: connect to host github.com port 22: Undefined error: 0
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

    at Object.action (plugin:obsidian-git:29441:25)
    at PluginStore.exec (plugin:obsidian-git:29466:25)
    at eval (plugin:obsidian-git:26971:43)
    at new Promise (<anonymous>)
    at GitExecutorChain.handleTaskData (plugin:obsidian-git:26969:16)
    at GitExecutorChain.eval (plugin:obsidian-git:26953:44)
    at Generator.next (<anonymous>)
    at fulfilled (plugin:obsidian-git:25798:24)

Steps to reproduce

Enable auto push
Disconnect from network.
Enable "Disable notifications"
Make a change in a document
Wait

Expected Behavior

No error messages shown, when "Disable notifications" is enabled

Addition context

First of all, THANKS A LOT for this plugin - which I appreciate a lot, despite the small annoyance. If you fix that, and disregard the suggestions here, I would still be grateful :D I already am.

I was wondering, if I should write a feature request, but when writing this, and noticed the "disable notification" setting, I decided to say, this is a but.

But there are some scenarios, where a notification would make a lot of sense.

So the primary issue for me is network issues (e.g. I often work from a ferry, where there is no connection for about half the time - but I rarely connect anyway). My expectation is that eventually network should be restored, and my work offline will be pushed. I still have the granular changes due to local commits.

Scenario no. 1: If pushing has consistently failed for a long duration, say more than a day, show an error message.

Scenario no. 2: Pushing fails, not because of a network error, but because git itself generates an error. This could be an authentication issue, or perhaps I had opened the vault on another computer, and pushed incompatible changes, so a fast-forward push was not possible.

In both of these cases, I would very much like to be notified.

Granted, the latter, I respect that you may not have the best option of detecting why a push failed as you are just calling the building git CLI, so unless it returns different exit codes on the different error scenarios, I wouldn't expect you to look into that (I personally wouldn't if I was maintaining this).

Operating system

macOS

Installation Method

AppImage

Plugin version

2.24.2

Vinzent03 commented 1 week ago

I already have detection to recognize network issues specifically. https://github.com/denolehov/obsidian-git/blob/a6d6175382ab4d4d134153383dee8ec151f0063a/src/gitManager/simpleGit.ts#L841-L857 So that the first offline issue is reported, but further ones are not. This doesn't depend on the disable notifications feature, because these are classified as error messages and not pure information notifications, like how many files got committed. The issue here is that your error message is not recognized as a network error. I'm unsure I should add the Undefined error: 0 suffix as well.

stroiman commented 1 week ago

That would be awesome.

I'm puzzled the regex btw, . matches any character, * matches the previous character or group zero or many times, and ? matches the previous character or group zero or one times. The ? seems unnecessary.

denolehov / obsidian-git