Merge lp://staging/~a-s-usov/bzr-fastimport/renaming-tags into lp://staging/bzr-fastimport
Proposed by
Alex Usov
| Status: | Merged |
|---|---|
| Approved by: | Jelmer Vernooij |
| Approved revision: | no longer in the source branch. |
| Merged at revision: | 337 |
| Proposed branch: | lp://staging/~a-s-usov/bzr-fastimport/renaming-tags |
| Merge into: | lp://staging/bzr-fastimport |
| Diff against target: |
223 lines (+110/-7) 4 files modified
cmds.py (+10/-2) exporter.py (+46/-5) tests/test_commands.py (+19/-0) tests/test_exporter.py (+35/-0) |
| To merge this branch: | bzr merge lp://staging/~a-s-usov/bzr-fastimport/renaming-tags |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Jelmer Vernooij (community) | Approve | ||
|
Review via email:
|
|||
This proposal supersedes a proposal from 2011-10-12.
Description of the change
Implement comments from patch review:
- style fixes
- add integration test for --rewrite-tag-names
- removed rewrite_dict as we can't really guarantee uniqness of tag names. More correct way would be to use mapping file
To post a comment you must log in.
Revision history for this message
| Jelmer Vernooij (jelmer) wrote | # |
Revision history for this message
| Jelmer Vernooij (jelmer) | # |
review:
Needs Fixing
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
Thanks for the patch. This mostly looks ok; some minor issues:
* two empty lines are necessary before "+class CheckRefnameRew
* The --rewrite-git-tags option needs a test
* Please use underscores rather than camelcasing
* rewrite_dict could be a local variable rather than a class variable
I'm not sure if it's all that useful to try to prevent duplicate tag names, as this is pretty much impossible. Even with your changes there are risks of duplicate tags (e.g. with incremental imports). Likewise, your change may cause tag names to change if another tag is removed.