From 6aaee3065cf218d33d3ea549fdbfdf4972722586 Mon Sep 17 00:00:00 2001 From: Robbie Iannucci Date: Tue, 12 Dec 2017 02:15:11 +0000 Subject: [PATCH] Revert "Use core.quotePath=false when git is listing files" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 9219d356885a93d9cbcaf4a2c7fc3f6e29caad49. Reason for revert: unfortunately this says "core,quotePath" and since it includes recipe changes, we need something that the roller can munch on :( Original change's description: > Use core.quotePath=false when git is listing files > > This prevents git from putting quotes around some file names > (those that have astral-plane characters) and not around others. > > R=​maruel > > Bug: 792302 > Recipe-Nontrivial-Roll: build > Recipe-Nontrivial-Roll: build_limited_scripts_slave > Change-Id: I3b6a6b36c4720116de811b40177b59aa25c263db > Reviewed-on: https://chromium-review.googlesource.com/815454 > Commit-Queue: Aaron Gable > Reviewed-by: Marc-Antoine Ruel TBR=maruel@chromium.org,agable@chromium.org Change-Id: I226388f19024403240a1443eb2b878b9293220e1 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 792302 Reviewed-on: https://chromium-review.googlesource.com/821671 Reviewed-by: Robbie Iannucci Commit-Queue: Robbie Iannucci --- gclient_scm.py | 12 ++++------ git_cl.py | 3 +-- git_drover.py | 3 +-- presubmit_support.py | 3 +-- recipes/README.recipes.md | 20 ++++++++-------- recipes/recipe_modules/tryserver/api.py | 24 +++++++++---------- .../full.expected/with_git_patch.json | 2 -- .../full.expected/with_git_patch_luci.json | 2 -- .../full.expected/with_rietveld_patch.json | 2 -- .../with_rietveld_patch_new.json | 2 -- .../full.expected/with_wrong_patch.json | 2 -- .../full.expected/with_wrong_patch_new.json | 2 -- scm.py | 4 ++-- tests/gclient_scm_test.py | 4 ++-- tests/git_cl_test.py | 15 ++++++------ tests/git_drover_test.py | 5 ++-- 16 files changed, 41 insertions(+), 64 deletions(-) diff --git a/gclient_scm.py b/gclient_scm.py index 20115f61a..3dbd64dba 100644 --- a/gclient_scm.py +++ b/gclient_scm.py @@ -305,8 +305,7 @@ class GitWrapper(SCMWrapper): # actually in a broken state here. The index will have both 'a' and 'A', # but only one of them will exist on the disk. To progress, we delete # everything that status thinks is modified. - output = self._Capture([ - '-c', 'core.quotePath=false', 'status', '--porcelain'], strip=False) + output = self._Capture(['status', '--porcelain'], strip=False) for line in output.splitlines(): # --porcelain (v1) looks like: # XY filename @@ -325,8 +324,7 @@ class GitWrapper(SCMWrapper): self._Fetch(options, prune=True, quiet=options.verbose) self._Scrub(revision, options) if file_list is not None: - files = self._Capture( - ['-c', 'core.quotePath=false', 'ls-files']).splitlines() + files = self._Capture(['ls-files']).splitlines() file_list.extend([os.path.join(self.checkout_path, f) for f in files]) def _DisableHooks(self): @@ -442,8 +440,7 @@ class GitWrapper(SCMWrapper): self._DeleteOrMove(options.force) self._Clone(revision, url, options) if file_list is not None: - files = self._Capture( - ['-c', 'core.quotePath=false', 'ls-files']).splitlines() + files = self._Capture(['ls-files']).splitlines() file_list.extend([os.path.join(self.checkout_path, f) for f in files]) if not verbose: # Make the output a little prettier. It's nice to have some whitespace @@ -722,8 +719,7 @@ class GitWrapper(SCMWrapper): # merge-base by default), so doesn't include untracked files. So we use # 'git ls-files --directory --others --exclude-standard' here directly. paths = scm.GIT.Capture( - ['-c', 'core.quotePath=false', 'ls-files', - '--directory', '--others', '--exclude-standard'], + ['ls-files', '--directory', '--others', '--exclude-standard'], self.checkout_path) for path in (p for p in paths.splitlines() if p.endswith('/')): full_path = os.path.join(self.checkout_path, path) diff --git a/git_cl.py b/git_cl.py index 123852f09..171e2fde8 100755 --- a/git_cl.py +++ b/git_cl.py @@ -5359,8 +5359,7 @@ def PushToGitWithAutoRebase(remote, branch, original_description, print('Your patch doesn\'t apply cleanly to \'%s\' HEAD @ %s, ' 'the following files have merge conflicts:' % (branch, parent_hash)) - print(RunGit(['-c', 'core.quotePath=false', 'diff', - '--name-status', '--diff-filter=U']).strip()) + print(RunGit(['diff', '--name-status', '--diff-filter=U']).strip()) print('Please rebase your patch and try again.') RunGitWithCode(['cherry-pick', '--abort']) break diff --git a/git_drover.py b/git_drover.py index 05bde9208..7d877fb19 100755 --- a/git_drover.py +++ b/git_drover.py @@ -262,8 +262,7 @@ class _Drover(object): # Files that have been deleted between branch and cherry-pick will not have # their skip-worktree bit set so set it manually for those files to avoid # git status incorrectly listing them as unstaged deletes. - repo_status = self._run_git_command( - ['-c', 'core.quotePath=false', 'status', '--porcelain']).splitlines() + repo_status = self._run_git_command(['status', '--porcelain']).splitlines() extra_files = [f[3:] for f in repo_status if f[:2] == ' D'] if extra_files: self._run_git_command_with_stdin( diff --git a/presubmit_support.py b/presubmit_support.py index 59bc4c1f4..18ee1f422 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -1038,8 +1038,7 @@ class GitChange(Change): """List all files under source control in the repo.""" root = root or self.RepositoryRoot() return subprocess.check_output( - ['git', '-c', 'core,quotePath=false', 'ls-files', '--', '.'], - cwd=root).splitlines() + ['git', 'ls-files', '--', '.'], cwd=root).splitlines() def ListRelevantPresubmitFiles(files, root): diff --git a/recipes/README.recipes.md b/recipes/README.recipes.md index 041b13126..66248d8c9 100644 --- a/recipes/README.recipes.md +++ b/recipes/README.recipes.md @@ -663,7 +663,7 @@ Returns: #### **class [TryserverApi](/recipes/recipe_modules/tryserver/api.py#12)([RecipeApi][recipe_engine/wkt/RecipeApi]):** -— **def [add\_failure\_reason](/recipes/recipe_modules/tryserver/api.py#148)(self, reason):** +— **def [add\_failure\_reason](/recipes/recipe_modules/tryserver/api.py#146)(self, reason):** Records a more detailed reason why build is failing. @@ -687,11 +687,11 @@ TODO(tandrii): remove this doc. Unless you use patch_root=None, in which case old behavior is used which returns paths relative to checkout aka solution[0].name. -— **def [get\_footer](/recipes/recipe_modules/tryserver/api.py#211)(self, tag, patch_text=None):** +— **def [get\_footer](/recipes/recipe_modules/tryserver/api.py#209)(self, tag, patch_text=None):** Gets a specific tag from a CL description -— **def [get\_footers](/recipes/recipe_modules/tryserver/api.py#184)(self, patch_text=None):** +— **def [get\_footers](/recipes/recipe_modules/tryserver/api.py#182)(self, patch_text=None):** Retrieves footers from the patch description. @@ -708,13 +708,13 @@ Returns true iff the properties exist to match a Gerrit issue. Returns true iff we can apply_issue or patch. -— **def [normalize\_footer\_name](/recipes/recipe_modules/tryserver/api.py#215)(self, footer):** +— **def [normalize\_footer\_name](/recipes/recipe_modules/tryserver/api.py#213)(self, footer):** -— **def [set\_compile\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#127)(self):** +— **def [set\_compile\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#125)(self):** Mark the tryjob result as a compile failure. -  **@contextlib.contextmanager**
— **def [set\_failure\_hash](/recipes/recipe_modules/tryserver/api.py#157)(self):** +  **@contextlib.contextmanager**
— **def [set\_failure\_hash](/recipes/recipe_modules/tryserver/api.py#155)(self):** Context manager that sets a failure_hash build property on StepFailure. @@ -723,7 +723,7 @@ for the same reason. For example, if a patch is bad (breaks something), we'd expect it to always break in the same way. Different failures for the same patch are usually a sign of flakiness. -— **def [set\_invalid\_test\_results\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#139)(self):** +— **def [set\_invalid\_test\_results\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#137)(self):** Mark the tryjob result as having invalid test results. @@ -731,18 +731,18 @@ This means we run some tests, but the results were not valid (e.g. no list of specific test cases that failed, or too many tests failing, etc). -— **def [set\_patch\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#123)(self):** +— **def [set\_patch\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#121)(self):** Mark the tryjob result as failure to apply the patch. -— **def [set\_subproject\_tag](/recipes/recipe_modules/tryserver/api.py#105)(self, subproject_tag):** +— **def [set\_subproject\_tag](/recipes/recipe_modules/tryserver/api.py#103)(self, subproject_tag):** Adds a subproject tag to the build. This can be used to distinguish between builds that execute different steps depending on what was patched, e.g. blink vs. pure chromium patches. -— **def [set\_test\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#131)(self):** +— **def [set\_test\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#129)(self):** Mark the tryjob result as a test failure. diff --git a/recipes/recipe_modules/tryserver/api.py b/recipes/recipe_modules/tryserver/api.py index e0bd0796d..a78e7a021 100644 --- a/recipes/recipe_modules/tryserver/api.py +++ b/recipes/recipe_modules/tryserver/api.py @@ -63,13 +63,12 @@ class TryserverApi(recipe_api.RecipeApi): cwd = self.m.context.cwd or self.m.path['start_dir'].join(patch_root) with self.m.context(cwd=cwd): - step_result = self.m.git( - '-c', 'core.quotePath=false', 'diff', '--cached', '--name-only', - name='git diff to analyze patch', - stdout=self.m.raw_io.output(), - step_test_data=lambda: - self.m.raw_io.test_api.stream_output('foo.cc'), - **kwargs) + step_result = self.m.git('diff', '--cached', '--name-only', + name='git diff to analyze patch', + stdout=self.m.raw_io.output(), + step_test_data=lambda: + self.m.raw_io.test_api.stream_output('foo.cc'), + **kwargs) paths = [self.m.path.join(patch_root, p) for p in step_result.stdout.split()] if self.m.platform.is_win: @@ -85,12 +84,11 @@ class TryserverApi(recipe_api.RecipeApi): cwd = self.m.path['checkout'].join(issue_root) if issue_root else None with self.m.context(cwd=cwd): - step_result = self.m.git( - '-c', 'core.quotePath=false', 'diff', '--cached', '--name-only', - name='git diff to analyze patch', - stdout=self.m.raw_io.output(), - step_test_data=lambda: - self.m.raw_io.test_api.stream_output('foo.cc')) + step_result = self.m.git('diff', '--cached', '--name-only', + name='git diff to analyze patch', + stdout=self.m.raw_io.output(), + step_test_data=lambda: + self.m.raw_io.test_api.stream_output('foo.cc')) paths = step_result.stdout.split() if issue_root: paths = [self.m.path.join(issue_root, path) for path in paths] diff --git a/recipes/recipe_modules/tryserver/examples/full.expected/with_git_patch.json b/recipes/recipe_modules/tryserver/examples/full.expected/with_git_patch.json index def11d76e..de0706341 100644 --- a/recipes/recipe_modules/tryserver/examples/full.expected/with_git_patch.json +++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_git_patch.json @@ -2,8 +2,6 @@ { "cmd": [ "git", - "-c", - "core.quotePath=false", "diff", "--cached", "--name-only" diff --git a/recipes/recipe_modules/tryserver/examples/full.expected/with_git_patch_luci.json b/recipes/recipe_modules/tryserver/examples/full.expected/with_git_patch_luci.json index def11d76e..de0706341 100644 --- a/recipes/recipe_modules/tryserver/examples/full.expected/with_git_patch_luci.json +++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_git_patch_luci.json @@ -2,8 +2,6 @@ { "cmd": [ "git", - "-c", - "core.quotePath=false", "diff", "--cached", "--name-only" diff --git a/recipes/recipe_modules/tryserver/examples/full.expected/with_rietveld_patch.json b/recipes/recipe_modules/tryserver/examples/full.expected/with_rietveld_patch.json index f4039c304..48457e576 100644 --- a/recipes/recipe_modules/tryserver/examples/full.expected/with_rietveld_patch.json +++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_rietveld_patch.json @@ -29,8 +29,6 @@ { "cmd": [ "git", - "-c", - "core.quotePath=false", "diff", "--cached", "--name-only" diff --git a/recipes/recipe_modules/tryserver/examples/full.expected/with_rietveld_patch_new.json b/recipes/recipe_modules/tryserver/examples/full.expected/with_rietveld_patch_new.json index 3b5ae72bd..950948d88 100644 --- a/recipes/recipe_modules/tryserver/examples/full.expected/with_rietveld_patch_new.json +++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_rietveld_patch_new.json @@ -29,8 +29,6 @@ { "cmd": [ "git", - "-c", - "core.quotePath=false", "diff", "--cached", "--name-only" diff --git a/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch.json b/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch.json index ff8071886..d88dc137f 100644 --- a/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch.json +++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch.json @@ -2,8 +2,6 @@ { "cmd": [ "git", - "-c", - "core.quotePath=false", "diff", "--cached", "--name-only" diff --git a/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch_new.json b/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch_new.json index 36f867ca2..b2f53685d 100644 --- a/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch_new.json +++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch_new.json @@ -2,8 +2,6 @@ { "cmd": [ "git", - "-c", - "core.quotePath=false", "diff", "--cached", "--name-only" diff --git a/scm.py b/scm.py index 232fcb953..8708c19b0 100644 --- a/scm.py +++ b/scm.py @@ -131,8 +131,8 @@ class GIT(object): upstream_branch = GIT.GetUpstreamBranch(cwd) if upstream_branch is None: raise gclient_utils.Error('Cannot determine upstream branch') - command = ['-c', 'core.quotePath=false', 'diff', - '--name-status', '--no-renames', '-r', '%s...' % upstream_branch] + command = ['diff', '--name-status', '--no-renames', + '-r', '%s...' % upstream_branch] if not files: pass elif isinstance(files, basestring): diff --git a/tests/gclient_scm_test.py b/tests/gclient_scm_test.py index 5c6bf4cef..60266a61d 100755 --- a/tests/gclient_scm_test.py +++ b/tests/gclient_scm_test.py @@ -631,7 +631,7 @@ class ManagedGitWrapperTestCaseMox(BaseTestCase): options) self.mox.StubOutWithMock(gclient_scm.subprocess2, 'check_output', True) gclient_scm.subprocess2.check_output( - ['git', '-c', 'core.quotePath=false', 'ls-files'], cwd=self.base_path, + ['git', 'ls-files'], cwd=self.base_path, env=gclient_scm.scm.GIT.ApplyEnvVars({}), stderr=-1,).AndReturn('') gclient_scm.subprocess2.check_output( ['git', 'rev-parse', '--verify', 'HEAD'], @@ -668,7 +668,7 @@ class ManagedGitWrapperTestCaseMox(BaseTestCase): options) self.mox.StubOutWithMock(gclient_scm.subprocess2, 'check_output', True) gclient_scm.subprocess2.check_output( - ['git', '-c', 'core.quotePath=false', 'ls-files'], cwd=self.base_path, + ['git', 'ls-files'], cwd=self.base_path, env=gclient_scm.scm.GIT.ApplyEnvVars({}), stderr=-1,).AndReturn('') gclient_scm.subprocess2.check_output( ['git', 'rev-parse', '--verify', 'HEAD'], diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 482c27a77..7a634dd6c 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -813,8 +813,8 @@ class TestGitCl(TestCase): ] + cls._git_sanity_checks('fake_ancestor_sha', 'master') + [ ((['git', 'rev-parse', '--show-cdup'],), ''), ((['git', 'rev-parse', 'HEAD'],), '12345'), - ((['git', '-c', 'core.quotePath=false', 'diff', - '--name-status', '--no-renames', '-r', 'fake_ancestor_sha...', '.'],), + ((['git', 'diff', '--name-status', '--no-renames', '-r', + 'fake_ancestor_sha...', '.'],), 'M\t.gitignore\n'), ((['git', 'config', 'branch.master.rietveldpatchset'],), CERR1), ((['git', 'log', '--pretty=format:%s%n%n%b', @@ -1110,8 +1110,8 @@ class TestGitCl(TestCase): ] + self._git_sanity_checks('fake_ancestor_sha', 'feature') + [ ((['git', 'rev-parse', '--show-cdup'],), ''), ((['git', 'rev-parse', 'HEAD'],), 'fake_sha'), - ((['git', '-c', 'core.quotePath=false', 'diff', - '--name-status', '--no-renames', '-r', 'fake_ancestor_sha...', '.'],), + ((['git', 'diff', '--name-status', '--no-renames', '-r', + 'fake_ancestor_sha...', '.'],), 'M\tfile1.cpp'), ((['git', 'config', 'branch.feature.rietveldpatchset'],), '20001'), ((['git', 'config', 'branch.feature.rietveldserver'],), @@ -1228,8 +1228,7 @@ class TestGitCl(TestCase): fail_cherry_pick=True, debug=False) self.calls += [ - ((['git', '-c', 'core.quotePath=false', 'diff', - '--name-status', '--diff-filter=U'],), + ((['git', 'diff', '--name-status', '--diff-filter=U'],), 'U path/file1\n' 'U file2.cpp\n'), ((['git', 'cherry-pick', '--abort'],), ''), @@ -1446,8 +1445,8 @@ class TestGitCl(TestCase): ((['git', 'rev-parse', '--show-cdup'],), ''), ((['git', 'rev-parse', 'HEAD'],), '12345'), - ((['git', '-c', 'core.quotePath=false', 'diff', '--name-status', - '--no-renames', '-r', ancestor_revision + '...', '.'],), + ((['git', 'diff', '--name-status', '--no-renames', '-r', + ancestor_revision + '...', '.'],), 'M\t.gitignore\n'), ((['git', 'config', 'branch.master.gerritpatchset'],), CERR1), ] diff --git a/tests/git_drover_test.py b/tests/git_drover_test.py index c5e49441f..7374fb1b0 100755 --- a/tests/git_drover_test.py +++ b/tests/git_drover_test.py @@ -79,8 +79,7 @@ class GitDroverTest(auto_stub.TestCase): (['git', 'branch', '-D', 'drover_branch_123'], self._parent_repo), ] self.MANUAL_RESOLVE_PREPARATION_COMMANDS = [ - (['git', '-c', 'core.quotePath=false', 'status', '--porcelain'], - self._target_repo), + (['git', 'status', '--porcelain'], self._target_repo), (['git', 'update-index', '--skip-worktree', '--stdin'], self._target_repo), ] @@ -115,7 +114,7 @@ class GitDroverTest(auto_stub.TestCase): raise subprocess.CalledProcessError(1, args[0]) if args == ['git', 'rev-parse', '--git-dir']: return os.path.join(self._parent_repo, '.git') - if args == ['git', '-c', 'core.quotePath=false', 'status', '--porcelain']: + if args == ['git', 'status', '--porcelain']: return ' D foo\nUU baz\n D bar\n' if args == ['git', 'log', '-1', '--format=%ae']: return 'author@domain.org'