[git-llvm] Make `push` work on CRLF files with svn:eol-style=native

[rnk]

Summary

git apply on Windows doesn't work right now for files that SVN checks
out as CRLF. There is no way to force SVN to check everything out with
Unix line endings on Windows. Files with svn:eol-style=native will
always come out with CRLF, breaking git apply, which wants Unix line
endings. My workaround is to list all files with this property set in
the change, and run dos2unix on them. SVN doesn't commit a mass
reformatting, it knows the file is a text file because of the
svn:eol-style property.

[rnk]

I committed rL301245 with this modified script, which touches files with and without svn:eol-style=native.

[zturner]

There are certain files in the repo that absolutely must be CRLF, and are documented as such. So this patch would break those files. I'm not sure if that's a problem though. Can we just wash our hands of this added complexity and globally remove this property? We've been doing it on a file-by-file basis for months, and nobody has noticed or complained

[jlebar]

Well that's exciting.

I don't have a position on what Zach is suggesting, but as far as the change to the script goes, I'm happy with it.

llvm/utils/git-svn/git-llvm

92 ↗

(On Diff #96465)

Do we need to close this at some point?

195 ↗

(On Diff #96465)

Would prefer to call some os.path function if there's one that DTRT for this, but if not, whatever.

202 ↗

(On Diff #96465)

Nit, probably want something to guard against the case where the filename contains ' - '. Probably rsplit(' - ', 1)?

[rnk]

In D32452#735996, @zturner wrote:

There are certain files in the repo that absolutely must be CRLF, and are documented as such. So this patch would break those files. I'm not sure if that's a problem though.

It won't, because those files will not set svn:eol-style to native. The ones I know of in the clangd test directory explicitly set svn:eol-style to CRLF, or don't set it at all.

Can we just wash our hands of this added complexity and globally remove this property? We've been doing it on a file-by-file basis for months, and nobody has noticed or complained

I'd rather do this, and hope it works well enough that we can survive until the git migration.

[rnk]

llvm/utils/git-svn/git-llvm

92 ↗

(On Diff #96465)

We could, but what do you think about opening it once in a global and never closing it?

195 ↗

(On Diff #96465)

git seems to use forward slashes in its output on Windows, so I hope this is safe. I could probably do f.split('\\/', 1)[1], or try to combine os.path.sep and os.path.altsep, but that doesn't seem better.

202 ↗

(On Diff #96465)

Done. I created such a file, added it to svn, gave it a property, listed it, and rsplit does the right thing.

[jlebar]

llvm/utils/git-svn/git-llvm

195 ↗

(On Diff #96465)

Ah, I remember now, I have fought with this before. Path munging is done by msys. Really horrible. But if your Python is not aware of it, then pathsep doesn't work, and...yeah. If this works, is probably good enough for now.

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

for d in [
         'llvm',
         'polly',
     ]
 }
 GIT_TO_SVN_DIR.update({'clang': 'cfe/trunk'})
 
 VERBOSE = False
 QUIET = False
+dev_null_fd = None
 
 
 def eprint(*args, **kwargs):
     print(*args, file=sys.stderr, **kwargs)
 
 
 def log(*args, **kwargs):
     if QUIET:

 def first_dirname(d):
     while True:
         (head, tail) = os.path.split(d)
         if not head or head == '/':
             return tail
         d = head
 
 
-def shell(cmd, strip=True, cwd=None, stdin=None, die_on_failure=True):
+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):
     log_verbose('Running: %s' % ' '.join(cmd))
 
+    err_pipe = subprocess.PIPE
+    if ignore_errors:
+        # Silence errors if requested.
+        err_pipe = get_dev_null()
+
     start = time.time()
-    p = subprocess.Popen(cmd, cwd=cwd, stdout=subprocess.PIPE,
-                         stderr=subprocess.PIPE, stdin=subprocess.PIPE)
+    p = subprocess.Popen(cmd, cwd=cwd, stdout=subprocess.PIPE, stderr=err_pipe,
+                         stdin=subprocess.PIPE)
     stdout, stderr = p.communicate(input=stdin)
     elapsed = time.time() - start
 
     log_verbose('Command took %0.1fs' % elapsed)
 
-    if p.returncode == 0:
-        if stderr:
+    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)
     eprint(err_msg)
     if stderr:
         eprint(stderr.rstrip())
     if die_on_failure:
         sys.exit(2)
     raise RuntimeError(err_msg)
 
 
 def git(*cmd, **kwargs):
     return shell(['git'] + list(cmd), kwargs.get('strip', True))
 
 
 def svn(cwd, *cmd, **kwargs):
     # TODO: Better way to do default arg when we have *cmd?
-    return shell(['svn'] + list(cmd), cwd=cwd, stdin=kwargs.get('stdin', None))
+    return shell(['svn'] + list(cmd), cwd=cwd, stdin=kwargs.get('stdin', None),
+                 ignore_errors=kwargs.get('ignore_errors', None))
 
 
 def get_default_rev_range():
     # Get the branch tracked by the current branch, as set by
     # git branch --set-upstream-to  See http://serverfault.com/a/352236/38694.
     cur_branch = git('rev-parse', '--symbolic-full-name', 'HEAD')
     upstream_branch = git('for-each-ref', '--format=%(upstream:short)',
                           cur_branch)
if not os.path.exists(svn_root):
         svn(svn_root, 'checkout', '--depth=immediates',
             'https://llvm.org/svn/llvm-project/', '.')
         svn(svn_root, 'update', *list(GIT_TO_SVN_DIR.values()))
         log("svn staging area ready in '%s'" % svn_root)
     if not os.path.isdir(svn_root):
         die("Can't initialize svn staging dir (%s)" % svn_root)
 
 
+def fix_eol_style_native(rev, sr, svn_sr_path):
+    """Fix line endings before applying patches with Unix endings
+
+    SVN on Windows will check out files with CRLF for files with the
+    svn:eol-style property set to "native". This breaks `git apply`, which
+    typically works with Unix-line ending patches. Work around the problem here
+    by doing a dos2unix up front for files with svn:eol-style set to "native".
+    SVN will not commit a mass line ending re-doing because it detects the line
+    ending format for files with this property.
+    """
+    files = git('diff-tree', '--no-commit-id', '--name-only', '-r', rev, '--',
+                sr).split('\n')
+    files = [f.split('/', 1)[1] for f in files]
+    # 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).split('\n')
+    crlf_files = []
+    for eol_prop in eol_props:
+        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)
+    # 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)
+
+
 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')
     if status:
         die("Can't push git rev %s because svn status is not empty:\n%s" %
             (rev, status))
 
     for sr in subrepos:
-        diff = git('show', '--binary', rev, '--', sr, strip=False)
         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:
             shell(['git', 'apply', '-p2', '-'], cwd=svn_sr_path, stdin=diff,
                   die_on_failure=False)
         except RuntimeError as e:
             eprint("Patch doesn't apply: maybe you should try `git pull -r` "
                    "first?")