Edgewall Software
Modify

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#10819 closed defect (fixed)

Incorrect git repo path makes BrowserModule fail to load

Reported by: Ethan Jucovy <ethan.jucovy@…> Owned by: Peter Suter
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.

Attachments (2)

git_error.diff (1.0 KB ) - added by Ethan Jucovy <ethan.jucovy@…> 8 years ago.
T10819_GitError.diff (1.3 KB ) - added by Peter Suter 8 years ago.

Download all attachments as: .zip

Change History (9)

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

Attachment: git_error.diff added

comment:1 by Ethan Jucovy <ethan.jucovy@…>, 8 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, 8 years ago

Attachment: T10819_GitError.diff added

in reply to:  1 comment:2 by Peter Suter, 8 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, 8 years ago

Related to #10809.

comment:4 by Christian Boos, 8 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, 8 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.

comment:6 by Christian Boos, 8 years ago

Resolution: fixed
Status: newclosed

comment:7 by Christian Boos, 8 years ago

Owner: set to Peter Suter

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Peter Suter.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Peter Suter to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.