Repository admin panel should indicate when repository path is not valid

[Ryan J Ollos]

The repository admin page should indicate when repository paths are invalid by graying out the entry. A warning can also be added when a repository path is saved that is not valid.

[Ryan J Ollos]

At first I had in mind something simple like this (log:rjollos.git:t11351):

However, we might be able to provide more detailed information. In this example the first 3 grayed-out links are due to (1), whereas the fourth grayed-out link is due to (2) (the error messages are shown as tooltips). If we added unique exceptions for the two cases, then we could gray-out the Type when the version control system is not properly enabled, and gray-out the Directory when it doesn't appear to be a version control repository. The exceptions could be something like: UnsupportedConnectorError and InvalidPathError.

1. Unsupported version control system "svn": Can't find an appropriate component, maybe the corresponding plugin was not enabled?
2. home/user/Workspace/11351 does not appear to be a Git repository

[anonymous]

This would be great enhancement! two remarks, why this would be great:

1. To me, corresponding to finally found documentation, git repositories don't look like listed above, but have to have the '/.git' subdir given, which is not quite clear from the UI and unusual for git users, isn't it?

2. Repositories (Repositorys) UI has some strange effects in displaying resp. copy/paste behavior with hidden/ special characters. Explanation about 2.:

I started with some bad file permissions on git repositories leading to "Trac[PyGIT] ERROR: GIT control files missing" erros resp. "TracError: /home/auser/gitrepos/someproject/.git does not appear to be a Git repository" error message on trac-admin repository resync command.

After fixing this I then used the already given repo listings from UI, copied them, deleted the entries and readded them. Thus including some unvisible blanks, where in one case trailing blank was visible, when path was marked for copy and in one case I finally found it just in trac log file. One other effect, immediately visible after pasting path as copy from old entry has been, slahes got converted to <slash><dot> "/." or "/?" when copy it to some editor. This should have alerted me, but I failed with the blanks.

This leads to the misty situation, that only one of 3 repos worked, although all of them had correct permissions and have been created same way.

Finally let me say, I like trac and hope this comment is usefull to somebody!

My setup: Debian jessie(!), german localization, access via FF 26 using incorporated postrges and git packages

Systeminformationen

Paket	Version
Trac	1.0.1
Babel	0.9.6
Docutils	0.11
Genshi	0.6 (with speedups)
GIT	1.8.4.3
mod_python	3.3.1
psycopg2	2.4.5
Pygments	1.6
Python	2.7.5+ (default, Sep 17 2013, 15:33:59) [GCC 4.8.1]
pytz	2012c
setuptools	0.6
jQuery	1.7.2

Installierte Plugins

TracAccountManager

0.4.3

[Ryan J Ollos]

Replying to anonymous:

1. To me, corresponding to finally found documentation, git repositories don't look like listed above, but have to have the '/.git' subdir given, which is not quite clear from the UI and unusual for git users, isn't it?

This issue was addressed recently in #11297.

2. Repositories (Repositorys) UI has some strange effects in displaying resp. copy/paste behavior with hidden/ special characters.

This will be handled in #11371.

[Ryan J Ollos]

Replying to anonymous:

2. Repositories (Repositorys) UI has some strange effects in displaying resp. copy/paste behavior with hidden/ special characters.

I experienced today, when copying a path from the table on the /admin/versioncontrol/repository page into trac.ini, the result is: /home/<200b>user/<200b>Workspace/<200b>th10834/<200b>teo-rjollos.git.

[Remy Blank]

FTR, these are zero-width spaces that I put there intentionally so that long paths are broken properly. I don't remember the exact details, but removing them either made the table larger than the page, or possibly the breaks were in unfortunate locations.

We already work around the copy-paste problem by removing zero-width spaces from certain input fields on the server side, maybe we should just remove them from all inputs. Or maybe there's a better way to solve the path-breaking problem that doesn't need adding special characters to the text.

[Ryan J Ollos]

Stripping the zero-width spaces from input fields could be done as part of #11371, and then possibly extended to other realms.

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t11351.2.

[Ryan J Ollos]

Replying to rjollos:

However, we might be able to provide more detailed information. In this example the first 3 grayed-out links are due to (1), whereas the fourth grayed-out link is due to (2) (the error messages are shown as tooltips). If we added unique exceptions for the two cases, then we could gray-out the Type when the version control system is not properly enabled, and gray-out the Directory when it doesn't appear to be a version control repository. The exceptions could be something like: UnsupportedConnectorError and InvalidPathError.

1. Unsupported version control system "svn": Can't find an appropriate component, maybe the corresponding plugin was not enabled?
2. home/user/Workspace/11351 does not appear to be a Git repository

Providing detailed information on the repository admin page for the error described in (1) will probably be addressed in #11713.

[Ryan J Ollos]

Committed to 1.0-stable in [13605] and merged to trunk in [13606]. Documentation updated in TracDev/Exceptions@12.

Next I'll prepare a patch for the MercurialPlugin.

[Jun Omae]

Test failures on autobuild after [13605]. See ​https://travis-ci.org/edgewall/trac/jobs/45758513. If Subversion binding is not installed, the tests fail.

$ TMP=/dev/shm/tmp make python=25-1.0 clean Trac.egg-info all test=trac/versioncontrol/tests/functional.py
Python: /home/jun66j5/venv/py25-1.0/bin/python
find -name \*.py[co] -exec rm {} \;
rm -f .figleaf* *.figleaf
rm -fr figleaf
coverage erase
rm -fr build/doc

Python: /home/jun66j5/venv/py25-1.0/bin/python

  Package        Version
  ---------------------------------------------------------------------
  Python       : 2.5.6 (r256:88840, Jan 25 2014, 17:35:34)
  [GCC 4.6.3]
  Setuptools   : 0.6
  Genshi       : 0.7
  Babel        : 0.9.6
  sqlite3      : 2.3.2
  PySqlite     : not installed
  MySQLdb      : 1.2.5
  Psycopg2     : 2.5.2 (dt dec pq3 ext)
  SVN bindings : not installed
  Mercurial    : 3.0.1
  Pygments     : 1.6
  Pytz         : 2014.4
  ConfigObj    : 4.7.2
  Docutils     : 0.11
  Twill        : 0.9
  LXML         : 3.3.5
  coverage     : not installed
  figleaf      : not installed

Variables:
  PATH=/home/jun66j5/venv/py25-1.0/bin::$PATH
  PYTHONPATH=.::$PYTHONPATH
  TRAC_TEST_DB_URI=
  server-options= -p 3000 -a '*,/home/jun66j5/src/trac-htdigest.txt,auth'  -e

External dependencies:
  Git version: git version 1.7.9.5

...
python trac/versioncontrol/tests/functional.py
SKIP: versioncontrol/tests/functional.py (no svn bindings)
FFF
...
----------------------------------------------------------------------
Ran 3 tests in 15.111s

FAILED (failures=3)
make: *** [all] Error 1

[Jun Omae]

If repository type is not supported, a TracError would be raised. I think we should raise an InvalidRepository for also that case.

Another thing, there are raw U+200B characters in literal of test cases. I think we should prevent using such characters in literal.

trac/versioncontrol/api.py

@@ -760 +760 @@
             if prio >= 0: # no error condition
                 return connector
             else:
-                raise TracError(
+                raise InvalidRepository(
                     _('Unsupported version control system "%(name)s"'
                       ': %(error)s', name=rtype,
                       error=to_unicode(connector.error)))
         else:
-            raise TracError(
+            raise InvalidRepository(
                 _('Unsupported version control system "%(name)s": '
                   'Can\'t find an appropriate component, maybe the '
                   'corresponding plugin was not enabled? ', name=rtype))

trac/versioncontrol/tests/functional.py

@@ -35 +35 @@
         tc.formvalue('trac-addrepos', 'name', 'InvalidRepos')
         tc.formvalue('trac-addrepos', 'dir', '/the/invalid/path')
         tc.submit()
-        tc.find('<span class="missing" title="/the/invalid/path does not '
-                'appear to be a Subversion repository.">/the/​invalid/​path'
-                '</span>')
+        tc.find((u'<span class="missing" title="[^"]*">'
+                 u'/the/\u200binvalid/\u200bpath</span>').encode('utf-8'))
 
 
 class TestEmptySvnRepo(FunctionalTwillTestCaseSetup):

[Ryan J Ollos]

Replying to jomae:

If repository type is not supported, a TracError would be raised. I think we should raise an InvalidRepository for also that case.

I described some points to consider with that change in comment:9.

[Ryan J Ollos]

With a failing test I was seeing the following:

======================================================================
ERROR: Repository with an invalid path is rendered with an error
----------------------------------------------------------------------
Traceback (most recent call last):
  File "trac/versioncontrol/tests/functional.py", line 38, in runTest
    tc.find('<span class="missing" title="/the/invalid/path does not '
  File "/home/user/Workspace/t11944/teo-rjollos.git/trac/tests/functional/better_twill.py", line 227, in better_find
    (unicode(e), filename))
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 113: ordinal not in range(128)

After this patch, the traceback prints correctly.

trac/tests/functional/better_twill.py

@@ -27 +27 @@
 except ImportError:
     from StringIO import StringIO
 
+from trac.util.text import to_unicode
+
 # On OSX lxml needs to be imported before twill to avoid Resolver issues
 # somehow caused by the mac specific 'ic' module
 try:
@@ -224 +226 @@
         except twill.errors.TwillAssertionError, e:
             filename = twill_write_html()
             raise twill.errors.TwillAssertionError('%s at %s' %
-                                                   (unicode(e), filename))
+                                                   (to_unicode(e), filename))
     tc.find = better_find
 
     def better_notfind(what, flags='', tcnotfind=tc.notfind):
@@ -233 +235 @@
         except twill.errors.TwillAssertionError, e:
             filename = twill_write_html()
             raise twill.errors.TwillAssertionError('%s at %s' %
-                                                   (unicode(e), filename))
+                                                   (to_unicode(e), filename))
     tc.notfind = better_notfind
 
     # Same for tc.url - no hint about what went wrong!
@@ -243 +245 @@
         except twill.errors.TwillAssertionError, e:
             filename = twill_write_html()
             raise twill.errors.TwillAssertionError('%s at %s' %
-                                                   (unicode(e), filename))
+                                                   (to_unicode(e), filename))
     tc.url = better_url
 else:
     b = tc = None

======================================================================
FAIL: Repository with an invalid path is rendered with an error
----------------------------------------------------------------------
Traceback (most recent call last):
  File "trac/versioncontrol/tests/functional.py", line 39, in runTest
    u'/the/\u200binvalid/\u200bpath</span>').encode('utf-8'))
  File "/home/user/Workspace/t11944/teo-rjollos.git/trac/tests/functional/better_twill.py", line 230, in better_find
    (to_unicode(e), filename))
TwillAssertionError: no match to '<span class="missing" title="[^"]*">/the/\u200binvalid/\u200bpath</span>' at file:///home/user/Workspace/t11944/teo-rjollos.git/testenv/trac/log/TestAdminInvalidRepository.html

Does the change make sense?

[Ryan J Ollos]

Changes committed to 1.0-stable in [13613], merged to trunk in [13614]. I'll prepare additional changes in #11713.

[Ryan J Ollos]

Patch from comment:14 committed to 1.0-stable in [13641], merged to trunk in [13642].

[Ryan J Ollos]

Replying to rjollos:

Next I'll prepare a patch for the MercurialPlugin.

Fixed in [41/mercurial-plugin].