Merge lp:~unit193/bzr-fastimport/deletion-fixes into lp:bzr-fastimport

Proposed by Unit 193 on 2015-05-04
Status: Needs review
Proposed branch: lp:~unit193/bzr-fastimport/deletion-fixes
Merge into: lp:bzr-fastimport
Diff against target: 61 lines (+23/-0)
1 file modified
exporter.py (+23/-0)
To merge this branch: bzr merge lp:~unit193/bzr-fastimport/deletion-fixes
Reviewer Review Type Date Requested Status
Richard Wilbur needs test(s) 2015-05-04 Needs Information on 2015-07-28
Bazaar Developers 2015-05-04 Pending
Review via email: mp+258178@code.launchpad.net

Description of the change

This fixes several outstanding export issues, patch taken from bug 430347.

To post a comment you must log in.
Richard Wilbur (richard-wilbur) wrote :

Thanks Unit 193 for the patch. It looks well executed.

How hard do you think it would be to create a test (or tests) to demonstrate the problem(s) (absent the fix) and validate the fix? This is important to make sure no one else accidentally breaks this fix, reintroducing the problem(s), in the future.

review: Needs Information (needs test(s))
Unit 193 (unit193) wrote :

I'd presume it wouldn't be too hard for someone used to the test framework, or writing tests. However, I am not that person.

Richard Wilbur (richard-wilbur) wrote :

I'll take a look at it and see if I can implement some decent tests within the test framework to cover the situations mentioned in all three linked bugs.

Unit 193 (unit193) wrote :

Is there any chance of getting this in? I've been using it ever since, and it's quite helpful to me so should equally be useful to others.

Jelmer Vernooij (jelmer) wrote :

Please add some unit tests for it.

It's the kind of change that's likely to regress as other changes are made, and it is not clear to me that it won't have other fallout (and e.g. make files disappear that should be exported) - tests are a good way to demonstrate that.

Jelmer Vernooij (jelmer) wrote :

There are also linked bugs that e.g. include smaller fixes - without having looked closely at the code, why are these fixes not appropriate?

Balint Reczey (rbalint) wrote :

@richard-wilbur The offered patch is applied in Ubuntu since 0.13.0+bzr361-1ubuntu1. Please consider merging it with or without an additional test.
It testing is really needed adding it to autopkgtests would be super easy.

Unmerged revisions

361. By Unit 193 on 2015-05-04

Fix a case where deleteion of an entry in a renamed directory is not reproduced correctly in fast-export. Thanks to Harry Hirsch, Nuno Araujo, and Andrew Huff for the patch.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'exporter.py'
2--- exporter.py 2014-05-15 09:26:03 +0000
3+++ exporter.py 2015-05-04 13:31:46 +0000
4@@ -514,6 +514,7 @@
5 #
6 # 1) bzr rm a; bzr mv b a; bzr commit
7 # 2) bzr mv x/y z; bzr rm x; commmit
8+ # 3) bzr mv x y; bzr rm y/z; bzr commit
9 #
10 # The first must come out with the delete first like this:
11 #
12@@ -525,6 +526,11 @@
13 # R x/y z
14 # D x
15 #
16+ # The third case must come out with delete first like this:
17+ #
18+ # D x/z
19+ # R x y
20+ #
21 # So outputting all deletes first or all renames first won't work.
22 # Instead, we need to make multiple passes over the various lists to
23 # get the ordering right.
24@@ -532,6 +538,7 @@
25 must_be_renamed = {}
26 old_to_new = {}
27 deleted_paths = set([p for p, _, _ in deletes])
28+ deleted_child_paths = set()
29 for (oldpath, newpath, id_, kind,
30 text_modified, meta_modified) in renames:
31 emit = kind != 'directory' or not self.plain_format
32@@ -543,6 +550,20 @@
33 self.note("Skipping empty dir %s in rev %s" % (oldpath,
34 revision_id))
35 continue
36+
37+ if kind == 'directory':
38+ # handling deleted children in renamed directory (case 3 above)
39+ for p, e in tree_old.inventory.iter_entries_by_dir(from_dir=id_):
40+ if e.kind == 'directory' or not self.plain_format:
41+ continue
42+ old_child_path = osutils.pathjoin(oldpath, p)
43+ new_child_path = osutils.pathjoin(newpath, p)
44+ if old_child_path in deleted_paths:
45+ file_cmds.append(commands.FileDeleteCommand(old_child_path.encode("utf-8")))
46+ deleted_paths.remove(old_child_path)
47+ deleted_child_paths.add(old_child_path)
48+
49+
50 #oldpath = self._adjust_path_for_renames(oldpath, renamed,
51 # revision_id)
52 renamed.append([oldpath, newpath])
53@@ -561,6 +582,8 @@
54 continue
55 old_child_path = osutils.pathjoin(oldpath, p)
56 new_child_path = osutils.pathjoin(newpath, p)
57+ if old_child_path in deleted_child_paths:
58+ continue
59 must_be_renamed[old_child_path] = new_child_path
60
61 # Add children not already renamed

Subscribers

People subscribed via source and target branches