trac-admin "changeset added" command without repository raises TypeError

[Ryan J Ollos]

changeset added without specifying the repository results in a TypeError:

trac-admin trac changeset added abc2a755f97689145de81000534cca138736a539
TypeError: 'NoneType' object is not iterable

2016-06-06 22:02:57,103 Trac[console] ERROR: Exception in trac-admin command: 
Traceback (most recent call last):
  File "/var/www/bugs.jqueryui.com/private/pve/local/lib/python2.7/site-packages/trac/admin/console.py", line 110, in onecmd
    rv = cmd.Cmd.onecmd(self, line) or 0
  File "/usr/lib/python2.7/cmd.py", line 220, in onecmd
    return self.default(line)
  File "/var/www/bugs.jqueryui.com/private/pve/local/lib/python2.7/site-packages/trac/admin/console.py", line 288, in default
    return self.cmd_mgr.execute_command(*args)
  File "/var/www/bugs.jqueryui.com/private/pve/local/lib/python2.7/site-packages/trac/admin/api.py", line 127, in execute_command
    return f(*fargs)
  File "/var/www/bugs.jqueryui.com/private/pve/local/lib/python2.7/site-packages/trac/versioncontrol/admin.py", line 97, in _do_changeset_added
    for error in errors:
TypeError: 'NoneType' object is not iterable

[Ryan J Ollos]

If the revision is also omitted, the output is:

$ trac-admin ../tracenvs/proj-1.0/ changeset added
Error: Invalid arguments

changeset added <repos> <rev> [rev] [...]

    Notify trac about changesets added to a repository

    This command should be called from a post-commit hook. It will trigger a
    cache update and notify components about the addition.

I'd expect the same message to be output when omitting just the repository name.

[walty <walty8@…>]

hi,

I got a tricky problem when working on the unit test part, and I would like to ask for some advice here.

here is what I put into trac/admin/tests/console.py:

1506     def test_changeset_add_no_argument(self):
1507         rv, output = self._execute('changeset added')
1508         self.assertEqual(2, rv, output)
1509         self.assertExpectedResult(output)

and here is what I put into trac/admin/tests/console-tests.txt:

1091 ===== test_changeset_add_no_argument =====
1092 Error: Invalid arguments
1093 
1094 changeset added <repos> <rev> [rev] [...]
1095 
1096     Notify trac about changesets added to a repository
1097 
1098     This command should be called from a post-commit hook. It will trigger a
1099     cache update and notify components about the addition.
1100 

I checked the result should be correct, but the unittest never let me pass with empty difference is shown!

Finally I found the following code in trac/admin/tests/console.py

 163         if '[...]' in expected_results:
 164             m = re.match(expected_results.replace('[...]', '.*'), output,
 165                          re.MULTILINE)
 166             unittest.TestCase.assertTrue(self, m,
 167                                          "%r != %r\n%s" % (expected_results,
 168                                                            output, diff()))

Note that line 163 has a special handling for [...] which happens to appear in the error message, may I know what's that, and should I hardcode something to skip that if-condition please?

Thanks.

[Ryan J Ollos]

Replying to walty <walty8@…>:

Note that line 163 has a special handling for [...] which happens to appear in the error message, may I know what's that, and should I hardcode something to skip that if-condition please?

That is an unfortunate coincidence. [...] performs a wildcard match, and was added in [10180]. It looks like the feature was added to address variation in the Python exception message across Python versions. I recently encountered the issue in #12488.

It's not obvious to me why your test doesn't pass though. Could you post your work-in-progress patch?

[walty <walty8@…>]

Replying to Ryan J Ollos:

It's not obvious to me why your test doesn't pass though. Could you post your work-in-progress patch?

Please see my latest patch in ​de475a.

The patch actually does work now, but line 163 of trac/admin/tests/console.py contains some hardcoded conditional clause. If that line is restored, then unittest could not pass.

[Jun Omae]

Replying to Ryan J Ollos:

changeset added without specifying the repository results in a TypeError:

trac-admin trac changeset added abc2a755f97689145de81000534cca138736a539
TypeError: 'NoneType' object is not iterable

I think RepositoryManager.notify() should return a list of error messages instead of None when the given repository is not found.

trac/versioncontrol/api.py

@@ -716 +716 @@
         if not repositories:
             self.log.warn("Found no repositories matching '%s' base.",
                           base or reponame)
-            return
+            return [_("Repository '%(repo)s' not found",
+                      repo=reponame or _("(default)"))]
 
         errors = []
         for repos in sorted(repositories, key=lambda r: r.reponame):

After the patch:

Trac [/var/trac/1.0-sqlite]> changeset added abc2a755f97689145de81000534cca138736a539
Repository 'abc2a755f97689145de81000534cca138736a539' not found

Or we should raise a TracError (see also tags/trac-1.0.11/trac/versioncontrol/web_ui/changeset.py@:232,234,246-252#L232​).

[walty <walty8@…>]

Replying to Jun Omae:

Replying to Ryan J Ollos:

changeset added without specifying the repository results in a TypeError:

trac-admin trac changeset added abc2a755f97689145de81000534cca138736a539
TypeError: 'NoneType' object is not iterable

I think RepositoryManager.notify() should return a list of error messages instead of None when the given repository is not found.

But I thought it's more consistent to use the same mechanism to show the error message, just as pointed out in comment:2.

In my patch, I changed the function signature, so the same error catching and reporting would work for the following two cases:

trac changeset added abc2a755f97689145de81000534cca138736a539

trac changeset added 

[walty <walty8@…>]

Replying to walty <walty8@…>:

After second thought, maybe I could merge the two patches.

If the repository is not entered, then show the message prompt about how to use. But if the repository is entered but not valid, we would raise out the error message of repository not found?

[Jun Omae]

But I thought it's more consistent to use the same mechanism to show the error message, just as pointed out in comment 2.

No. Your patch wouldn't resolve the original issue.

Trac [/home/jun66j5/var/trac/1.2dev-sqlite]> changeset added invalid-repository 42
TypeError: 'NoneType' object is not iterable

[walty <walty8@…>]

The patch just tries to solve the issue of following:

trac-admin . changeset added 42

i.e. the user forgot to enter the repository at all.

[walty <walty8@…>]

hi,

pls find ​here the new patch, which has integrated the suggested message prompt for invalid repository name. The corresponding unit tests are added as well.

[Jun Omae]

The following in those changes is too ad hoc.

        if '[...]' in expected_results and 'changeset' not in expected_results:

We should escape other texts of the [...] using re.escape() before passing re.match().

trac/admin/tests/console.py

@@ -160 +160 @@
             expected_lines = ['%s\n' % x for x in expected_results.split('\n')]
             return ''.join(difflib.unified_diff(expected_lines, output_lines,
                                                 'expected', 'actual'))
-        if '[...]' in expected_results and 'changeset' not in expected_results:
-            m = re.match(expected_results.replace('[...]', '.*'), output,
-                         re.MULTILINE)
+        if '[...]' in expected_results:
+            m = re.match('.*'.join(map(re.escape,
+                                       expected_results.split('[...]'))) +
+                         '\Z',
+                         output, re.MULTILINE)
             unittest.TestCase.assertTrue(self, m,
                                          "%r != %r\n%s" % (expected_results,
                                                            output, diff()))

[Jun Omae]

Also, I think the naming of test_changeset_add_invalid_project should be test_changeset_add_invalid_repository because changeset added accepts repository name and revisions, not project.

[Jun Omae]

Replying to Ryan J Ollos:

2016-06-06 22:02:57,103 Trac[console] ERROR: Exception in trac-admin command: 
Traceback (most recent call last):
  File "/var/www/bugs.jqueryui.com/private/pve/local/lib/python2.7/site-packages/trac/admin/console.py", line 110, in onecmd
    rv = cmd.Cmd.onecmd(self, line) or 0
...
TypeError: 'NoneType' object is not iterable

The original command line is not logged in that case. I think we could log the given line to help trouble-shooting.

trac/admin/console.py

@@ -125 +125 @@
             printerr(exception_to_unicode(e))
             rv = 2
             if self.env_check():
-                self.env.log.error("Exception in trac-admin command: %s",
+                self.env.log.error("Exception in trac-admin command: %r%s",
+                                   line,
                                    exception_to_unicode(e, traceback=True))
         if not self.interactive:
             return rv

After the patch:

2016-06-17 18:12:57,420 Trac[console] ERROR: Exception in trac-admin command: u"changeset added abc2a755f97689145de81000534cca138736a539"
Traceback (most recent call last):
  File "trac/admin/console.py", line 112, in onecmd
    rv = cmd.Cmd.onecmd(self, line) or 0

[walty <walty8@…>]

Replying to Jun Omae:

The following in those changes is too ad hoc.

        if '[...]' in expected_results and 'changeset' not in expected_results:

We should escape other texts of the [...] using re.escape() before passing re.match().

Yes, that makes a lot more sense!

Please find the latest patch in ​6cb0e11.

Beside the original logic to give better error prompt, this patch also includes

1. new logic for [...] handling in unit test
2. naming switch from project to repository
3. added log for command line

[Ryan J Ollos]

Committed to 1.0-stable in r14855, merged to trunk in r14856.

[Ryan J Ollos]

​Test failures.

[Ryan J Ollos]

Tests pass after this change:

trac/admin/tests/console.py

@@ -182 +182 @@
 
         if '[...]' in expected_results:
             m = re.match('.*'.join(map(re.escape,
-                                       expected_results.split('[...]'))) +
-                         '\Z',
+                                       expected_results.split('[...]'))),
                          output, re.MULTILINE)
             unittest.TestCase.assertTrue(self, m,
                                          "%r != %r\n%s" % (expected_results,

[Jun Omae]

Replying to Ryan J Ollos:

​Test failures.

Sorry. I did confirm only with SQLite.

I think we should use re.DOTALL rather than re.MULTILINE. We expect that [...] matches all characters including newline and use no ^ and $ characters in this case.

(confirmed unit tests passing with all database backends)

trac/admin/tests/console.py

@@ -184 +184 @@
             m = re.match('.*'.join(map(re.escape,
                                        expected_results.split('[...]'))) +
                          '\Z',
-                         output, re.MULTILINE)
+                         output, re.DOTALL)
             unittest.TestCase.assertTrue(self, m,
                                          "%r != %r\n%s" % (expected_results,
                                                            output, diff()))

[Ryan J Ollos]

Thanks, I also should have confirmed before committing. All tests pass with the change. Fixed on 1.0-stable in [14860], merged to trunk in [14861].