Merge lp:~cjwatson/brz/fastimport-fix-directory-renames into lp:brz

Proposed by Colin Watson
Status: Needs review
Proposed branch: lp:~cjwatson/brz/fastimport-fix-directory-renames
Merge into: lp:brz
Diff against target: 32 lines (+7/-6)
1 file modified
breezy/plugins/fastimport/exporter.py (+7/-6)
To merge this branch: bzr merge lp:~cjwatson/brz/fastimport-fix-directory-renames
Reviewer Review Type Date Requested Status
Ioana Lasc (community) Approve
Breezy developers Pending
Review via email: mp+410767@code.launchpad.net

Commit message

fastimport: Fix export of directory renames.

Description of the change

Passing `specific_files=[change.path[0]]` to `tree_old.iter_entries_by_dir` yields only the entry for that path, rather than for that path and all directory entries contained by it as the code appeared to expect. Fix this to emit the correct rename commands for all directory entries under the path being renamed, and fix up subsequent delete commands to match.

This fixes the export of https://bazaar.launchpad.net/~lazr-developers/launchpadlib/trunk/revision/36.1.1, which renames the `launchpadlib` directory to `src/launchpadlib` as well as making various other changes. It may be relevant to https://bugs.launchpad.net/brz/+bug/1890216 as well, although I haven't checked and I haven't tackled the rather larger task of writing useful tests.

NOTE: I'm not sure whether this is correct in --no-plain mode: in that case it will output rename commands both for the directory and for its contained paths. Is that correct? If not, I should adjust this further.

To post a comment you must log in.
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

For --no-plain mode, it should indeed just emit a rename for the directory - the importer doesn't need to know about all the unchanged children.

I think that just means that you can change the condition on line 597 from:

 if change.kind == ('directory', 'directory'):

to

  if change.kind == ('directory', 'directory') and self.plain_format:

Iterating over the entire tree works, but is not very efficient :( Tree.list_files() is recursive by default and allows you to pass in a from_dir. Would it be possible to use that instead?

It would also be great to have a test if you can easily reproduce the situation in https://bazaar.launchpad.net/~lazr-developers/launchpadlib/trunk/revision/36.1.1; this code is quite subtle so otherwise there's risk that future changes reintroduce this bug.

Revision history for this message
Ioana Lasc (ilasc) :
review: Approve

Unmerged revisions

7542. By Colin Watson

fastimport: Fix export of directory renames.

Passing `specific_files=[change.path[0]]` to
`tree_old.iter_entries_by_dir` yields only the entry for that path,
rather than for that path and all directory entries contained by it as
the code appeared to expect. Fix this to emit the correct rename
commands for all directory entries under the path being renamed, and fix
up subsequent delete commands to match.

This fixes the export of
https://bazaar.launchpad.net/~lazr-developers/launchpadlib/trunk/revision/36.1.1,
which renames the `launchpadlib` directory to `src/launchpadlib` as well
as making various other changes. It may be relevant to
https://bugs.launchpad.net/brz/+bug/1890216 as well, although I haven't
checked and I haven't tackled the rather larger task of writing useful
tests.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'breezy/plugins/fastimport/exporter.py'
2--- breezy/plugins/fastimport/exporter.py 2020-07-18 23:14:00 +0000
3+++ breezy/plugins/fastimport/exporter.py 2021-10-25 21:09:26 +0000
4@@ -594,12 +594,13 @@
5 # Renaming a directory implies all children must be renamed.
6 # Note: changes_from() doesn't handle this
7 if change.kind == ('directory', 'directory'):
8- for p, e in tree_old.iter_entries_by_dir(specific_files=[change.path[0]]):
9+ for p, e in tree_old.iter_entries_by_dir():
10+ if not p.startswith(change.path[0] + '/'):
11+ continue
12 if e.kind == 'directory' and self.plain_format:
13 continue
14- old_child_path = osutils.pathjoin(change.path[0], p)
15- new_child_path = osutils.pathjoin(change.path[1], p)
16- must_be_renamed[old_child_path] = new_child_path
17+ new_child_path = osutils.pathjoin(change.path[1], p[len(change.path[0]) + 1:])
18+ must_be_renamed[p] = new_child_path
19
20 # Add children not already renamed
21 if must_be_renamed:
22@@ -619,8 +620,8 @@
23 continue
24 if change.kind[0] == 'directory' and self.plain_format:
25 continue
26- #path = self._adjust_path_for_renames(path, renamed, revision_id)
27- file_cmds.append(commands.FileDeleteCommand(change.path[0].encode("utf-8")))
28+ path = self._adjust_path_for_renames(change.path[0], renamed, revision_id)
29+ file_cmds.append(commands.FileDeleteCommand(path.encode("utf-8")))
30 return file_cmds, modifies, renamed
31
32 def _adjust_path_for_renames(self, path, renamed, revision_id):

Subscribers

People subscribed via source and target branches