Edgewall Software

Opened 12 years ago

Last modified 12 years ago

#10819 closed defect

Incorrect git repo path makes BrowserModule fail to load — at Version 5

Reported by: Ethan Jucovy <ethan.jucovy@…> Owned by:
Priority: normal Milestone: 1.0
Component: plugin/git Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:

Proper error reporting for git repositories with invalid path.

API Changes:
Internal Changes:

Description

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.

Change History (7)

by Ethan Jucovy <ethan.jucovy@…>, 12 years ago

Attachment: git_error.diff added

comment:1 by Ethan Jucovy <ethan.jucovy@…>, 12 years ago

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.

by Peter Suter, 12 years ago

Attachment: T10819_GitError.diff added

in reply to:  1 comment:2 by Peter Suter, 12 years ago

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.

comment:3 by Jun Omae, 12 years ago

Related to #10809.

comment:4 by Christian Boos, 12 years ago

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.

comment:5 by Christian Boos, 12 years ago

Milestone: 1.0
Release Notes: modified (diff)

(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.

Note: See TracTickets for help on using tickets.