Edgewall Software
Modify

Opened 8 years ago

Closed 8 years ago

#12510 closed defect (fixed)

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

Reported by: Ryan J Ollos Owned by: walty <walty8@…>
Priority: normal Milestone: 1.0.12
Component: admin/console Version:
Severity: normal Keywords:
Cc: walty8@… Branch:
Release Notes:

Fix TypeError if repository or changeset omitted in trac-admin changeset added or changset modified commands.

API Changes:
Internal Changes:

Description (last modified by 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

Attachments (0)

Change History (23)

comment:1 by Ryan J Ollos, 8 years ago

Description: modified (diff)

comment:2 by Ryan J Ollos, 8 years ago

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.

comment:3 by walty <walty8@…>, 8 years ago

Cc: walty8@… added

comment:4 by walty <walty8@…>, 8 years ago

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.

in reply to:  4 ; comment:5 by Ryan J Ollos, 8 years ago

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?

Last edited 8 years ago by Ryan J Ollos (previous) (diff)

in reply to:  5 comment:6 by walty <walty8@…>, 8 years ago

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.

in reply to:  description ; comment:7 by Jun Omae, 8 years ago

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

    diff --git a/trac/versioncontrol/api.py b/trac/versioncontrol/api.py
    index 421642430..fa161ec80 100644
    a b class RepositoryManager(Component):  
    716716        if not repositories:
    717717            self.log.warn("Found no repositories matching '%s' base.",
    718718                          base or reponame)
    719             return
     719            return [_("Repository '%(repo)s' not found",
     720                      repo=reponame or _("(default)"))]
    720721
    721722        errors = []
    722723        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).

Last edited 8 years ago by Jun Omae (previous) (diff)

in reply to:  7 ; comment:8 by walty <walty8@…>, 8 years ago

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 
Last edited 8 years ago by Ryan J Ollos (previous) (diff)

in reply to:  8 comment:9 by walty <walty8@…>, 8 years ago

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?

Last edited 8 years ago by Ryan J Ollos (previous) (diff)

in reply to:  8 ; comment:10 by Jun Omae, 8 years ago

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

in reply to:  10 comment:11 by walty <walty8@…>, 8 years ago

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.

comment:12 by walty <walty8@…>, 8 years ago

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.

comment:13 by Jun Omae, 8 years ago

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

    diff --git a/trac/admin/tests/console.py b/trac/admin/tests/console.py
    index fe95385bd..bc303b4f5 100644
    a b class TracAdminTestCaseBase(unittest.TestCase):  
    160160            expected_lines = ['%s\n' % x for x in expected_results.split('\n')]
    161161            return ''.join(difflib.unified_diff(expected_lines, output_lines,
    162162                                                'expected', 'actual'))
    163         if '[...]' in expected_results and 'changeset' not in expected_results:
    164             m = re.match(expected_results.replace('[...]', '.*'), output,
    165                          re.MULTILINE)
     163        if '[...]' in expected_results:
     164            m = re.match('.*'.join(map(re.escape,
     165                                       expected_results.split('[...]'))) +
     166                         '\Z',
     167                         output, re.MULTILINE)
    166168            unittest.TestCase.assertTrue(self, m,
    167169                                         "%r != %r\n%s" % (expected_results,
    168170                                                           output, diff()))

comment:14 by Jun Omae, 8 years ago

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.

in reply to:  description comment:15 by Jun Omae, 8 years ago

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

    diff --git a/trac/admin/console.py b/trac/admin/console.py
    index f7efdc00f..8929dacbb 100755
    a b class TracAdmin(cmd.Cmd):  
    125125            printerr(exception_to_unicode(e))
    126126            rv = 2
    127127            if self.env_check():
    128                 self.env.log.error("Exception in trac-admin command: %s",
     128                self.env.log.error("Exception in trac-admin command: %r%s",
     129                                   line,
    129130                                   exception_to_unicode(e, traceback=True))
    130131        if not self.interactive:
    131132            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

in reply to:  13 comment:16 by walty <walty8@…>, 8 years ago

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

comment:17 by Ryan J Ollos, 8 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

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

comment:18 by Ryan J Ollos, 8 years ago

Owner: changed from Ryan J Ollos to walty <walty8@…>

comment:19 by Ryan J Ollos, 8 years ago

Resolution: fixed
Status: closedreopened
Last edited 8 years ago by Ryan J Ollos (previous) (diff)

comment:20 by Ryan J Ollos, 8 years ago

Tests pass after this change:

  • trac/admin/tests/console.py

    diff --git a/trac/admin/tests/console.py b/trac/admin/tests/console.py
    index 768607c..28203ce 100644
    a b class TracadminTestCase(unittest.TestCase):  
    182182
    183183        if '[...]' in expected_results:
    184184            m = re.match('.*'.join(map(re.escape,
    185                                        expected_results.split('[...]'))) +
    186                          '\Z',
     185                                       expected_results.split('[...]'))),
    187186                         output, re.MULTILINE)
    188187            unittest.TestCase.assertTrue(self, m,
    189188                                         "%r != %r\n%s" % (expected_results,

comment:21 by Ryan J Ollos, 8 years ago

Release Notes: modified (diff)

in reply to:  19 comment:22 by Jun Omae, 8 years ago

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

    diff --git a/trac/admin/tests/console.py b/trac/admin/tests/console.py
    index 768607c85..e97efe5bb 100644
    a b class TracadminTestCase(unittest.TestCase):  
    184184            m = re.match('.*'.join(map(re.escape,
    185185                                       expected_results.split('[...]'))) +
    186186                         '\Z',
    187                          output, re.MULTILINE)
     187                         output, re.DOTALL)
    188188            unittest.TestCase.assertTrue(self, m,
    189189                                         "%r != %r\n%s" % (expected_results,
    190190                                                           output, diff()))
Last edited 8 years ago by Jun Omae (previous) (diff)

comment:23 by Ryan J Ollos, 8 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: reopenedclosed

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain walty <walty8@…>.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from walty <walty8@…> 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.