Incorrect git repo path makes BrowserModule fail to load

[Ethan Jucovy <ethan.jucovy@…>]

To reproduce:

1. Navigate to /admin/versioncontrol/repository/ and add a new repository of type "git" with a directory that does not exist or is not a git repository, like "/tmp/"
2. Save changes

When the page is reloaded, a yellow Trac warning will appear: Warning: Error with navigation contributor "BrowserModule"

Additionally, the "Browse Source" tab in the nav bar will have vanished. If you navigate to /browser/ directly, Trac will spit out an Internal Error: "GitError: GIT control files not found, maybe wrong directory?"

Note that when adding an invalid repository of type "svn", this does not happen. Instead, the Browse Source tab still exists and can be viewed without an internal error. When navigating to that page, the corresponding error will be displayed inline in the browser view: "/tmp/ does not appear to be a Subversion repository."

The Git backend should be made consistent with the SVN backend. As it is, it's hard for an admin to diagnose the user error, and a typo can bring down the whole BrowserModule.

[Ethan Jucovy <ethan.jucovy@…>]

The attached patch makes two changes:

1. The GitError in PyGIT.py inherits from TracError instead of Exception. This way Trac core's BrowserModule can catch the error and handle it elegantly. (That already happens, in source:trunk/trac/versioncontrol/web_ui/browser.py#L517​ — but only for exceptions that inherit from TracError.) This fixes the problem — the Browse Source tab no longer disappears if an invalid Git repository is added, and the behavior is consistent with the SVN versioncontrol backend.
2. I added the git_dir path to this particular error in the git backend. So instead of "GIT control files not found, maybe wrong directory?" it now says "GIT control files not found at /tmp/, maybe wrong directory?" This makes user error easier to diagnose, and is more consistent with the SVN backend error: "/tmp/ does not appear to be a Subversion repository."

I'm not sure if PyGIT.py is supposed to have no dependencies on Trac core, though — if that's the case then a different approach would be needed.

[Peter Suter]

Replying to Ethan Jucovy <ethan.jucovy@…>:

I'm not sure if PyGIT.py is supposed to have no dependencies on Trac core, though — if that's the case then a different approach would be needed.

I'm not sure if the dependency-free PyGIT.py is intentional either (and if keeping it that way is a priority). In this case it wouldn't be that difficult (see attachment:T10819_GitError.diff​).

Comparing with SVN​ highlights another issue with the Git support: No internationalisation.

[Jun Omae]

Related to #10809.

[Christian Boos]

The first patch has (at least) one unwanted side-effect: the code that raise other errors would need to be adapted (raise GitErrorSha → internal error TypeError: __init__() takes at least 2 arguments (1 given)).

Peter's patch seems to be the way to go.

[Christian Boos]

(forgot to assign this to 1.0)

The Timeline will also end up with the internal error in such case, so we really need to fix this.

I went ahead and applied Peter's patch in r11226.