[git-llvm] Fix eol conversion on Windows for explicit CRLF files

[zturner]

Summary

We were checking for 'native', but we weren't considering files that are explicitly EOL. Secondly, dos2unix does not have a -q option on Windows (how did that even work for you?), so if the command fails we retry without -q.

[rnk]

llvm/utils/git-svn/git-llvm

242 ↗

(On Diff #163139)

if crlf_files: is more python-y

248 ↗

(On Diff #163139)

-q is for quiet. My goal was to suppress all warning output. Instead, you could probably do:

shell(['dos2unix'] + crlf_files, ignore_errors=True)

I don't remember what warnings I saw, but they were annoying.

[zturner]

I never got around to getting this committed, but incidentally I built a new machine over the weekend and ran into the same problem *plus* an additional problem which is that with Python 3, git apply doesn't like it when universal_newlines is set to true. Without this additional changes, it is impossible to push anything using the following configuration:

Windows 10
Git 2.18.0.windows.1
Python 3.7.0 32-bit
Running git llvm push from a cmd prompt.

[rnk]

lgtm

In D51444#1257498, @zturner wrote:

Python 3.7.0 32-bit

^ There's your problem =P

[zturner]

In D51444#1258115, @rnk wrote:

lgtm

In D51444#1257498, @zturner wrote:

Python 3.7.0 32-bit

^ There's your problem =P

I know, I know. But we claim to support Python 3, and honestly this is going to be the most likely configuration for new users, because people will just go to the download page and hit download, and that's what they'll get.

llvm/trunk/utils/git-svn/git-llvm

def get_dev_null():
     """Lazily create a /dev/null fd for use in shell()"""
     global dev_null_fd
     if dev_null_fd is None:
         dev_null_fd = open(os.devnull, 'w')
     return dev_null_fd
 
 
 def shell(cmd, strip=True, cwd=None, stdin=None, die_on_failure=True,
-          ignore_errors=False):
+          ignore_errors=False, force_binary_stdin=False):
     log_verbose('Running: %s' % ' '.join(cmd))
 
     err_pipe = subprocess.PIPE
     if ignore_errors:
         # Silence errors if requested.
         err_pipe = get_dev_null()
 
+    if force_binary_stdin and stdin:
+        stdin = stdin.encode('utf-8')
+
     start = time.time()
+    text = not force_binary_stdin
     p = subprocess.Popen(cmd, cwd=cwd, stdout=subprocess.PIPE, stderr=err_pipe,
-                         stdin=subprocess.PIPE, universal_newlines=True)
+                         stdin=subprocess.PIPE, universal_newlines=text)
     stdout, stderr = p.communicate(input=stdin)
     elapsed = time.time() - start
 
     log_verbose('Command took %0.1fs' % elapsed)
 
+    if not text:
+        stdout = stdout.decode('utf-8')
+        stderr = stderr.decode('utf-8')
+
     if p.returncode == 0 or ignore_errors:
         if stderr and not ignore_errors:
             eprint('`%s` printed to stderr:' % ' '.join(cmd))
             eprint(stderr.rstrip())
         if strip:
             stdout = stdout.rstrip('\r\n')
         return stdout
     err_msg = '`%s` returned %s' % (' '.join(cmd), p.returncode)
def fix_eol_style_native(rev, sr, svn_sr_path):
     files = [f for f in files if os.path.exists(os.path.join(svn_sr_path, f))]
     # Use ignore_errors because 'svn propget' prints errors if the file doesn't
     # have the named property. There doesn't seem to be a way to suppress that.
     eol_props = svn(svn_sr_path, 'propget', 'svn:eol-style', *files,
                     ignore_errors=True)
     crlf_files = []
     if len(files) == 1:
         # No need to split propget output on ' - ' when we have one file.
-        if eol_props.strip() == 'native':
+        if eol_props.strip() in ['native', 'CRLF']:
             crlf_files = files
     else:
         for eol_prop in eol_props.split('\n'):
             # Remove spare CR.
             eol_prop = eol_prop.strip('\r')
             if not eol_prop:
                 continue
             prop_parts = eol_prop.rsplit(' - ', 1)
             if len(prop_parts) != 2:
                 eprint("unable to parse svn propget line:")
                 eprint(eol_prop)
                 continue
             (f, eol_style) = prop_parts
             if eol_style == 'native':
                 crlf_files.append(f)
+    if crlf_files:
         # Reformat all files with native SVN line endings to Unix format. SVN knows
         # files with native line endings are text files. It will commit just the
         # diff, and not a mass line ending change.
-    shell(['dos2unix', '-q'] + crlf_files, cwd=svn_sr_path)
-
+        shell(['dos2unix'] + crlf_files, ignore_errors=True, cwd=svn_sr_path)
 
 def svn_push_one_rev(svn_repo, rev, dry_run):
     files = git('diff-tree', '--no-commit-id', '--name-only', '-r',
                 rev).split('\n')
     subrepos = {first_dirname(f) for f in files}
     if not subrepos:
         raise RuntimeError('Empty diff for rev %s?' % rev)
 
     status = svn(svn_repo, 'status', '--no-ignore')
     if status:
         die("Can't push git rev %s because svn status is not empty:\n%s" %
             (rev, status))
 
     for sr in subrepos:
         svn_sr_path = os.path.join(svn_repo, GIT_TO_SVN_DIR[sr])
         if os.name == 'nt':
             fix_eol_style_native(rev, sr, svn_sr_path)
         diff = git('show', '--binary', rev, '--', sr, strip=False)
         # git is the only thing that can handle its own patches...
         log_verbose('Apply patch: %s' % diff)
         try:
+            # If we allow python to apply the diff in text mode, it will silently
+            # convert \n to \r\n which git doesn't like.
             shell(['git', 'apply', '-p2', '-'], cwd=svn_sr_path, stdin=diff,
-                  die_on_failure=False)
+                  die_on_failure=False, force_binary_stdin=True)
         except RuntimeError as e:
             eprint("Patch doesn't apply: maybe you should try `git pull -r` "
                    "first?")
             sys.exit(2)
 
     status_lines = svn(svn_repo, 'status', '--no-ignore').split('\n')
 
     for l in (l for l in status_lines if (l.startswith('?') or