From f8792079d2f78d401aebc7df16e9a251454b28a1 Mon Sep 17 00:00:00 2001 From: Edward Lesmes Date: Wed, 13 Sep 2017 08:05:12 +0000 Subject: [PATCH] Revert "Fix checkout.py issues when p.patchlevel > 1." This reverts commit 5908f9906dc71c0229c2e359fd64b325605b8370. Reason for revert: Introduces bugs when deleting files. The reason is that patchlevel = patchlevel or self.patchlevel will evaluate to self.patchlevel also when patchlevel is 0, which is wrong. Original change's description: > 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 TBR=kjellander@chromium.org,agable@chromium.org,ehmaldonado@chromium.org Change-Id: Ifa1f94602a023228cb32e5fe3fa07586b466981a No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 764294 Reviewed-on: https://chromium-review.googlesource.com/663266 Reviewed-by: Edward Lesmes Commit-Queue: Edward Lesmes --- checkout.py | 10 ++++------ patch.py | 37 +++++++++++-------------------------- tests/checkout_test.py | 30 ------------------------------ 3 files changed, 15 insertions(+), 62 deletions(-) diff --git a/checkout.py b/checkout.py index c5e61dd9a..4a5900009 100644 --- a/checkout.py +++ b/checkout.py @@ -252,8 +252,7 @@ class GitCheckout(CheckoutBase): for index, p in enumerate(patches): stdout = [] try: - filepath = os.path.join(self.project_path, - p.filename_after_patchlevel()) + filepath = os.path.join(self.project_path, p.filename) if p.is_delete: if (not os.path.exists(filepath) and any(p1.source_filename == p.filename for p1 in patches[0:index])): @@ -261,12 +260,11 @@ class GitCheckout(CheckoutBase): # was already processed because 'git apply' did it for us. pass else: - stdout.append(self._check_output_git( - ['rm', p.filename_after_patchlevel()])) + stdout.append(self._check_output_git(['rm', p.filename])) assert(not os.path.exists(filepath)) stdout.append('Deleted.') else: - dirname = os.path.dirname(p.filename_after_patchlevel()) + dirname = os.path.dirname(p.filename) full_dir = os.path.join(self.project_path, dirname) if dirname and not os.path.isdir(full_dir): os.makedirs(full_dir) @@ -276,7 +274,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_after_patchlevel()] + cmd = ['add', p.filename] if verbose: cmd.append('--verbose') stdout.append(self._check_output_git(cmd)) diff --git a/patch.py b/patch.py index 00a164601..1bc608c9f 100644 --- a/patch.py +++ b/patch.py @@ -34,7 +34,6 @@ 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 @@ -66,25 +65,6 @@ 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 @@ -183,6 +163,7 @@ 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: @@ -333,6 +314,10 @@ 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. @@ -361,8 +346,8 @@ class FilePatchDiff(FilePatchBase): continue if match.group(1).startswith('a/') and match.group(2).startswith('b/'): self.patchlevel = 1 - old = self._apply_patchlevel(match.group(1)) - new = self._apply_patchlevel(match.group(2)) + old = self.mangle(match.group(1)) + new = self.mangle(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'): @@ -445,7 +430,7 @@ class FilePatchDiff(FilePatchBase): self._fail('--- and +++ are reversed') if match.group(1) == '/dev/null': self.is_new = True - elif self._apply_patchlevel(match.group(1)) != old: + elif self.mangle(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('+++'): @@ -458,7 +443,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._apply_patchlevel(match.group(1)): + elif self.filename_utf8 != self.mangle(match.group(1)): self._fail( 'Unexpected git diff: %s != %s.' % (self.filename, match.group(1))) if lines: @@ -497,7 +482,7 @@ class FilePatchDiff(FilePatchBase): self._fail('--- and +++ are reversed') if match.group(1) == '/dev/null': self.is_new = True - elif self._apply_patchlevel(match.group(1)) != self.filename_utf8: + elif self.mangle(match.group(1)) != self.filename_utf8: # guess the source filename. self.source_filename = match.group(1).decode('utf-8') self.is_new = True @@ -511,7 +496,7 @@ class FilePatchDiff(FilePatchBase): self._fail('Unexpected diff: --- not following +++.') if match.group(1) == '/dev/null': self.is_delete = True - elif self._apply_patchlevel(match.group(1)) != self.filename_utf8: + elif self.mangle(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 95ab4c8f3..7696b5e17 100755 --- a/tests/checkout_test.py +++ b/tests/checkout_test.py @@ -180,23 +180,6 @@ 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): @@ -338,19 +321,6 @@ 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: