Reland "Use core.quotePath=false when git is listing files"

This is a reland of 9219d356

The original was reverted due to a typo (core,quotePath instead
of core.quotePath). This version is fixed.

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
>
> Change-Id: I3b6a6b36c4720116de811b40177b59aa25c263db
> Reviewed-on: https://chromium-review.googlesource.com/815454
> Commit-Queue: Aaron Gable <agable@chromium.org>
> Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>

Bug: 792302
Recipe-Nontrivial-Roll: build
Recipe-Nontrivial-Roll: build_limited_scripts_slave
Change-Id: I28d2260948aaf63bd865888c2f60e4cdee9aea48
Reviewed-on: https://chromium-review.googlesource.com/822990
Commit-Queue: Aaron Gable <agable@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>

gclient_scm.py

@@ -305,7 +305,8 @@ 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(['status', '--porcelain'], strip=False)
+      output = self._Capture([
+          '-c', 'core.quotePath=false', 'status', '--porcelain'], strip=False)
       for line in output.splitlines():
         # --porcelain (v1) looks like:
         # XY filename
@@ -324,7 +325,8 @@ class GitWrapper(SCMWrapper):
     self._Fetch(options, prune=True, quiet=options.verbose)
     self._Scrub(revision, options)
     if file_list is not None:
-      files = self._Capture(['ls-files']).splitlines()
+      files = self._Capture(
+          ['-c', 'core.quotePath=false', 'ls-files']).splitlines()
       file_list.extend([os.path.join(self.checkout_path, f) for f in files])
 
   def _DisableHooks(self):
@@ -440,7 +442,8 @@ class GitWrapper(SCMWrapper):
         self._DeleteOrMove(options.force)
         self._Clone(revision, url, options)
       if file_list is not None:
-        files = self._Capture(['ls-files']).splitlines()
+        files = self._Capture(
+          ['-c', 'core.quotePath=false', '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
@@ -719,7 +722,8 @@ 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(
-          ['ls-files', '--directory', '--others', '--exclude-standard'],
+          ['-c', 'core.quotePath=false', '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)

git_cl.py

@@ -5363,7 +5363,8 @@ 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(['diff', '--name-status', '--diff-filter=U']).strip())
+      print(RunGit(['-c', 'core.quotePath=false', 'diff',
+                    '--name-status', '--diff-filter=U']).strip())
       print('Please rebase your patch and try again.')
       RunGitWithCode(['cherry-pick', '--abort'])
       break

git_drover.py

@@ -262,7 +262,8 @@ 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(['status', '--porcelain']).splitlines()
+    repo_status = self._run_git_command(
+        ['-c', 'core.quotePath=false', '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(

presubmit_support.py

@@ -1038,7 +1038,8 @@ class GitChange(Change):
     """List all files under source control in the repo."""
     root = root or self.RepositoryRoot()
     return subprocess.check_output(
-        ['git', 'ls-files', '--', '.'], cwd=root).splitlines()
+        ['git', '-c', 'core.quotePath=false', 'ls-files', '--', '.'],
+        cwd=root).splitlines()
 
 
 def ListRelevantPresubmitFiles(files, root):

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#146)(self, reason):**
+— **def [add\_failure\_reason](/recipes/recipe_modules/tryserver/api.py#148)(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#209)(self, tag, patch_text=None):**
+— **def [get\_footer](/recipes/recipe_modules/tryserver/api.py#211)(self, tag, patch_text=None):**
 
 Gets a specific tag from a CL description
 
-— **def [get\_footers](/recipes/recipe_modules/tryserver/api.py#182)(self, patch_text=None):**
+— **def [get\_footers](/recipes/recipe_modules/tryserver/api.py#184)(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#213)(self, footer):**
+— **def [normalize\_footer\_name](/recipes/recipe_modules/tryserver/api.py#215)(self, footer):**
 
-— **def [set\_compile\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#125)(self):**
+— **def [set\_compile\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#127)(self):**
 
 Mark the tryjob result as a compile failure.
 
-&emsp; **@contextlib.contextmanager**<br>&mdash; **def [set\_failure\_hash](/recipes/recipe_modules/tryserver/api.py#155)(self):**
+&emsp; **@contextlib.contextmanager**<br>&mdash; **def [set\_failure\_hash](/recipes/recipe_modules/tryserver/api.py#157)(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.
 
-&mdash; **def [set\_invalid\_test\_results\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#137)(self):**
+&mdash; **def [set\_invalid\_test\_results\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#139)(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).
 
-&mdash; **def [set\_patch\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#121)(self):**
+&mdash; **def [set\_patch\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#123)(self):**
 
 Mark the tryjob result as failure to apply the patch.
 
-&mdash; **def [set\_subproject\_tag](/recipes/recipe_modules/tryserver/api.py#103)(self, subproject_tag):**
+&mdash; **def [set\_subproject\_tag](/recipes/recipe_modules/tryserver/api.py#105)(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.
 
-&mdash; **def [set\_test\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#129)(self):**
+&mdash; **def [set\_test\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#131)(self):**
 
 Mark the tryjob result as a test failure.
 

recipes/recipe_modules/tryserver/api.py

@@ -63,12 +63,13 @@ 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('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(
+          '-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)
     paths = [self.m.path.join(patch_root, p) for p in
              step_result.stdout.split()]
     if self.m.platform.is_win:
@@ -84,11 +85,12 @@ 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('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(
+          '-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'))
     paths = step_result.stdout.split()
     if issue_root:
       paths = [self.m.path.join(issue_root, path) for path in paths]

recipes/recipe_modules/tryserver/examples/full.expected/with_git_patch.json

@@ -2,6 +2,8 @@
   {
     "cmd": [
       "git",
+      "-c",
+      "core.quotePath=false",
       "diff",
       "--cached",
       "--name-only"

recipes/recipe_modules/tryserver/examples/full.expected/with_git_patch_luci.json

@@ -2,6 +2,8 @@
   {
     "cmd": [
       "git",
+      "-c",
+      "core.quotePath=false",
       "diff",
       "--cached",
       "--name-only"

recipes/recipe_modules/tryserver/examples/full.expected/with_rietveld_patch.json

@@ -29,6 +29,8 @@
   {
     "cmd": [
       "git",
+      "-c",
+      "core.quotePath=false",
       "diff",
       "--cached",
       "--name-only"

recipes/recipe_modules/tryserver/examples/full.expected/with_rietveld_patch_new.json

@@ -29,6 +29,8 @@
   {
     "cmd": [
       "git",
+      "-c",
+      "core.quotePath=false",
       "diff",
       "--cached",
       "--name-only"

recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch.json

@@ -2,6 +2,8 @@
   {
     "cmd": [
       "git",
+      "-c",
+      "core.quotePath=false",
       "diff",
       "--cached",
       "--name-only"

recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch_new.json

@@ -2,6 +2,8 @@
   {
     "cmd": [
       "git",
+      "-c",
+      "core.quotePath=false",
       "diff",
       "--cached",
       "--name-only"

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 = ['diff', '--name-status', '--no-renames',
-               '-r', '%s...' % upstream_branch]
+    command = ['-c', 'core.quotePath=false', 'diff',
+               '--name-status', '--no-renames', '-r', '%s...' % upstream_branch]
     if not files:
       pass
     elif isinstance(files, basestring):

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', 'ls-files'], cwd=self.base_path,
+        ['git', '-c', 'core.quotePath=false', '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', 'ls-files'], cwd=self.base_path,
+        ['git', '-c', 'core.quotePath=false', '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'],

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', 'diff', '--name-status', '--no-renames', '-r',
-         'fake_ancestor_sha...', '.'],),
+      ((['git', '-c', 'core.quotePath=false', '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', 'diff', '--name-status', '--no-renames', '-r',
-         'fake_ancestor_sha...', '.'],),
+      ((['git', '-c', 'core.quotePath=false', 'diff',
+         '--name-status', '--no-renames', '-r', 'fake_ancestor_sha...', '.'],),
        'M\tfile1.cpp'),
       ((['git', 'config', 'branch.feature.rietveldpatchset'],), '20001'),
       ((['git', 'config', 'branch.feature.rietveldserver'],),
@@ -1228,7 +1228,8 @@ class TestGitCl(TestCase):
         fail_cherry_pick=True,
         debug=False)
     self.calls += [
-      ((['git', 'diff', '--name-status', '--diff-filter=U'],),
+      ((['git', '-c', 'core.quotePath=false', 'diff',
+         '--name-status', '--diff-filter=U'],),
        'U       path/file1\n'
        'U       file2.cpp\n'),
       ((['git', 'cherry-pick', '--abort'],), ''),
@@ -1445,8 +1446,8 @@ class TestGitCl(TestCase):
       ((['git', 'rev-parse', '--show-cdup'],), ''),
       ((['git', 'rev-parse', 'HEAD'],), '12345'),
 
-      ((['git', 'diff', '--name-status', '--no-renames', '-r',
-         ancestor_revision + '...', '.'],),
+      ((['git', '-c', 'core.quotePath=false', 'diff', '--name-status',
+         '--no-renames', '-r', ancestor_revision + '...', '.'],),
        'M\t.gitignore\n'),
       ((['git', 'config', 'branch.master.gerritpatchset'],), CERR1),
     ]

tests/git_drover_test.py

@@ -79,7 +79,8 @@ class GitDroverTest(auto_stub.TestCase):
           (['git', 'branch', '-D', 'drover_branch_123'], self._parent_repo),
       ]
     self.MANUAL_RESOLVE_PREPARATION_COMMANDS = [
-        (['git', 'status', '--porcelain'], self._target_repo),
+        (['git', '-c', 'core.quotePath=false', 'status', '--porcelain'],
+         self._target_repo),
         (['git', 'update-index', '--skip-worktree', '--stdin'],
          self._target_repo),
     ]
@@ -114,7 +115,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', 'status', '--porcelain']:
+    if args == ['git', '-c', 'core.quotePath=false', 'status', '--porcelain']:
       return ' D foo\nUU baz\n D bar\n'
     if args == ['git', 'log', '-1', '--format=%ae']:
       return 'author@domain.org'