From 5908f9906dc71c0229c2e359fd64b325605b8370 Mon Sep 17 00:00:00 2001 From: Edward Lemur Date: Tue, 12 Sep 2017 23:49:41 +0200 Subject: [PATCH] Fix checkout.py issues when p.patchlevel > 1. When p.patchlevel > 1, p.filename does not correspond to the files that git-apply would modify. See bug for details Bug: 764294 Change-Id: Icdb803056e306edb25238b2d9cdabd3ff175d8ed Reviewed-on: https://chromium-review.googlesource.com/663357 Reviewed-by: Aaron Gable Commit-Queue: Edward Lesmes --- checkout.py | 10 ++++++---- patch.py | 37 ++++++++++++++++++++++++++----------- tests/checkout_test.py | 30 ++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 15 deletions(-) diff --git a/checkout.py b/checkout.py index 4a5900009..c5e61dd9a 100644 --- a/checkout.py +++ b/checkout.py @@ -252,7 +252,8 @@ class GitCheckout(CheckoutBase): for index, p in enumerate(patches): stdout = [] try: - filepath = os.path.join(self.project_path, p.filename) + filepath = os.path.join(self.project_path, + p.filename_after_patchlevel()) if p.is_delete: if (not os.path.exists(filepath) and any(p1.source_filename == p.filename for p1 in patches[0:index])): @@ -260,11 +261,12 @@ class GitCheckout(CheckoutBase): # was already processed because 'git apply' did it for us. pass else: - stdout.append(self._check_output_git(['rm', p.filename])) + stdout.append(self._check_output_git( + ['rm', p.filename_after_patchlevel()])) assert(not os.path.exists(filepath)) stdout.append('Deleted.') else: - dirname = os.path.dirname(p.filename) + dirname = os.path.dirname(p.filename_after_patchlevel()) full_dir = os.path.join(self.project_path, dirname) if dirname and not os.path.isdir(full_dir): os.makedirs(full_dir) @@ -274,7 +276,7 @@ class GitCheckout(CheckoutBase): with open(filepath, 'wb') as f: f.write(content) stdout.append('Added binary file %d bytes' % len(content)) - cmd = ['add', p.filename] + cmd = ['add', p.filename_after_patchlevel()] if verbose: cmd.append('--verbose') stdout.append(self._check_output_git(cmd)) diff --git a/patch.py b/patch.py index 1bc608c9f..00a164601 100644 --- a/patch.py +++ b/patch.py @@ -34,6 +34,7 @@ class FilePatchBase(object): def __init__(self, filename): assert self.__class__ is not FilePatchBase self.filename = self._process_filename(filename) + self.patchlevel = 0 # Set when the file is copied or moved. self.source_filename = None @@ -65,6 +66,25 @@ class FilePatchBase(object): filename, 'Filename can\'t be \'%s\'.' % filename) return filename + def filename_after_patchlevel(self): + """Applies patchlevel to self.filename. + + Applies patchlevel to self.filename so the resulting filename is the same as + the one git-apply would have used. + """ + # We use self.patchlevel-1 since git-apply considers the "a/" in the diff + # as part of the file path. + return self._apply_patchlevel(self.filename, self.patchlevel-1) + + def _apply_patchlevel(self, string, patchlevel=None): + """Apply patchlevel to a file path. + + This function replaces backslashes with slashes and removes the first + patchlevel elements of string. patchlevel is self.patchlevel by default. + """ + patchlevel = patchlevel or self.patchlevel + return '/'.join(string.replace('\\', '/').split('/')[patchlevel:]) + def set_relpath(self, relpath): if not relpath: return @@ -163,7 +183,6 @@ class FilePatchDiff(FilePatchBase): self.diff_header, self.diff_hunks = self._split_header(diff) self.svn_properties = svn_properties or [] self.is_git_diff = self._is_git_diff_header(self.diff_header) - self.patchlevel = 0 if self.is_git_diff: self._verify_git_header() else: @@ -314,10 +333,6 @@ class FilePatchDiff(FilePatchBase): hunks[0].start_src -= 1 return hunks - def mangle(self, string): - """Mangle a file path.""" - return '/'.join(string.replace('\\', '/').split('/')[self.patchlevel:]) - def _verify_git_header(self): """Sanity checks the header. @@ -346,8 +361,8 @@ class FilePatchDiff(FilePatchBase): continue if match.group(1).startswith('a/') and match.group(2).startswith('b/'): self.patchlevel = 1 - old = self.mangle(match.group(1)) - new = self.mangle(match.group(2)) + old = self._apply_patchlevel(match.group(1)) + new = self._apply_patchlevel(match.group(2)) # The rename is about the new file so the old file can be anything. if new not in (self.filename_utf8, 'dev/null'): @@ -430,7 +445,7 @@ class FilePatchDiff(FilePatchBase): self._fail('--- and +++ are reversed') if match.group(1) == '/dev/null': self.is_new = True - elif self.mangle(match.group(1)) != old: + elif self._apply_patchlevel(match.group(1)) != old: # git patches are always well formatted, do not allow random filenames. self._fail('Unexpected git diff: %s != %s.' % (old, match.group(1))) if not lines or not lines[0].startswith('+++'): @@ -443,7 +458,7 @@ class FilePatchDiff(FilePatchBase): self._fail('Unexpected git diff: --- not following +++.') if '/dev/null' == match.group(1): self.is_delete = True - elif self.filename_utf8 != self.mangle(match.group(1)): + elif self.filename_utf8 != self._apply_patchlevel(match.group(1)): self._fail( 'Unexpected git diff: %s != %s.' % (self.filename, match.group(1))) if lines: @@ -482,7 +497,7 @@ class FilePatchDiff(FilePatchBase): self._fail('--- and +++ are reversed') if match.group(1) == '/dev/null': self.is_new = True - elif self.mangle(match.group(1)) != self.filename_utf8: + elif self._apply_patchlevel(match.group(1)) != self.filename_utf8: # guess the source filename. self.source_filename = match.group(1).decode('utf-8') self.is_new = True @@ -496,7 +511,7 @@ class FilePatchDiff(FilePatchBase): self._fail('Unexpected diff: --- not following +++.') if match.group(1) == '/dev/null': self.is_delete = True - elif self.mangle(match.group(1)) != self.filename_utf8: + elif self._apply_patchlevel(match.group(1)) != self.filename_utf8: self._fail('Unexpected diff: %s.' % match.group(1)) if lines: self._fail('Crap after +++') diff --git a/tests/checkout_test.py b/tests/checkout_test.py index 7696b5e17..95ab4c8f3 100755 --- a/tests/checkout_test.py +++ b/tests/checkout_test.py @@ -180,6 +180,23 @@ class BaseTest(fake_repos.FakeReposTestBase): #print fake_repos.read_tree(root) self.assertTree(tree, root) + def _check_delete_patchlevel(self, co): + """Makes sure file moves are handled correctly.""" + co.prepare(None) + patchset = patch.PatchSet([ + patch.FilePatchDelete( + 'something/chromeos/views/DOMui_menu_widget.h', False), + ]) + for p in patchset: + p.patchlevel = 2 + co.apply_patch(patchset) + # Make sure chromeos/views/DOMui_menu_widget.h is deleted and + # chromeos/views/webui_menu_widget.h is correctly created. + root = os.path.join(self.root_dir, self.name) + tree = self.get_trunk(False) + del tree['chromeos/views/DOMui_menu_widget.h'] + self.assertTree(tree, root) + class GitBaseTest(BaseTest): def setUp(self): @@ -321,6 +338,19 @@ class GitCheckout(GitBaseTest): ]) self.assertEquals(expected, out) + def testDeletePatchlevel(self): + co = self._get_co(None) + self._check_delete_patchlevel(co) + out = subprocess2.check_output( + ['git', 'diff', '--staged', '--name-status', '--no-renames'], + cwd=co.project_path) + out = sorted(out.splitlines()) + expected = sorted( + [ + 'D\tchromeos/views/DOMui_menu_widget.h', + ]) + self.assertEquals(expected, out) + if __name__ == '__main__': if '-v' in sys.argv: