Use --internal-diff only for svn >= 1.7.

scm.py

Index: scm.py
===================================================================
--- scm.py	(revision 195011)
+++ scm.py	(working copy)
@@ -772,40 +772,109 @@
       return ''
 
   @staticmethod
-  def DiffItem(filename, cwd, full_move, revision):
-    """Diffs a single file.
+  def GenerateDiff(filenames, cwd, full_move, revision):
+    """Returns a string containing the diff for the given file list.
 
-    Should be simple, eh? No it isn't.
-    Be sure to be in the appropriate directory before calling to have the
-    expected relative path.
-    full_move means that move or copy operations should completely recreate the
-    files, usually in the prospect to apply the patch for a try job."""
+    The files in the list should either be absolute paths or relative to the
+    given root. If no root directory is provided, the repository root will be
+    used.
+    The diff will always use relative paths.
+    """
+    assert isinstance(filenames, (list, tuple))
     # If the user specified a custom diff command in their svn config file,
     # then it'll be used when we do svn diff, which we don't want to happen
-    # since we want the unified diff.  Using --diff-cmd=diff doesn't always
-    # work, since they can have another diff executable in their path that
-    # gives different line endings.  So we use a bogus temp directory as the
-    # config directory, which gets around these problems.
-    bogus_dir = tempfile.mkdtemp()
-    try:
-      # Use "svn info" output instead of os.path.isdir because the latter fails
-      # when the file is deleted.
-      return SVN._DiffItemInternal(
-          filename,
-          cwd,
-          SVN.CaptureLocalInfo([filename], cwd),
-          bogus_dir,
-          full_move,
-          revision)
-    finally:
-      gclient_utils.RemoveDirectory(bogus_dir)
+    # since we want the unified diff.  On svn >= 1.7, the "--internal-diff" flag

[M-A Ruel, 2013/04/19 16:17:08]

Could you split the comments, move the svn >= 1.7 part inside the condition and
the svn < 1.7 into the else. I think it'd be more readable.

+    # will solve this.  On svn < 1.7, however, this flag doesn't exist.  Using
+    # --diff-cmd=diff doesn't always work, since some users (e.g. Windows cmd
+    # users) may have no diff executable in their paths at all, and even people
+    # who do can have a diff executable in their path that uses unexpected line
+    # endings.  So we use a bogus temp directory as the config directory, which
+    # bypasses any user settings for the diff-cmd.
+    if SVN.AssertVersion("1.7")[0]:
+      return SVN._GenerateDiffInternal(filenames, cwd, full_move, revision,
+                                       ["diff", "--internal-diff"])
+    else:
+      bogus_dir = tempfile.mkdtemp()
+      try:
+        return SVN._GenerateDiffInternal(filenames, cwd, full_move, revision,
+                                         ["diff", "--config_dir", bogus_dir])
+      finally:
+        gclient_utils.RemoveDirectory(bogus_dir)
 
   @staticmethod
-  def _DiffItemInternal(filename, cwd, info, bogus_dir, full_move, revision):
+  def _GenerateDiffInternal(filenames, cwd, full_move, revision, diff_command):
+    root = os.path.normcase(os.path.join(cwd, ''))
+    def RelativePath(path, root):
+      """We must use relative paths."""
+      if os.path.normcase(path).startswith(root):
+        return path[len(root):]
+      return path
+    # Cleanup filenames
+    filenames = [RelativePath(f, root) for f in filenames]
+    # Get information about the modified items (files and directories)
+    data = dict([(f, SVN.CaptureLocalInfo([f], root)) for f in filenames])

[M-A Ruel, 2013/04/19 16:17:08]

data = dict((f, SVN.CaptureLocalInfo([f], root)) for f in filenames)

[] are unnecessary.

+    diffs = []
+    if full_move:
+      # Eliminate modified files inside moved/copied directory.
+      for (filename, info) in data.iteritems():
+        if SVN.IsMovedInfo(info) and info.get("Node Kind") == "directory":
+          # Remove files inside the directory.
+          filenames = [f for f in filenames
+                       if not f.startswith(filename + os.path.sep)]
+      for filename in data.keys():
+        if not filename in filenames:
+          # Remove filtered out items.
+          del data[filename]
+    else:
+      metaheaders = []
+      for (filename, info) in data.iteritems():
+        if SVN.IsMovedInfo(info):
+          # for now, the most common case is a head copy,
+          # so let's just encode that as a straight up cp.
+          srcurl = info.get('Copied From URL')
+          file_root = info.get('Repository Root')
+          rev = int(info.get('Copied From Rev'))
+          assert srcurl.startswith(file_root)
+          src = srcurl[len(file_root)+1:]
+          try:
+            srcinfo = SVN.CaptureRemoteInfo(srcurl)
+          except subprocess2.CalledProcessError, e:
+            if not 'Not a valid URL' in e.stderr:
+              raise
+            # Assume the file was deleted. No idea how to figure out at which
+            # revision the file was deleted.
+            srcinfo = {'Revision': rev}
+          if (srcinfo.get('Revision') != rev and
+              SVN.Capture(diff_command + ['-r', '%d:head' % rev, srcurl], cwd)):
+            metaheaders.append("#$ svn cp -r %d %s %s "
+                               "### WARNING: note non-trunk copy\n" %
+                               (rev, src, filename))
+          else:
+            metaheaders.append("#$ cp %s %s\n" % (src,
+                                                  filename))
+      if metaheaders:
+        diffs.append("### BEGIN SVN COPY METADATA\n")
+        diffs.extend(metaheaders)
+        diffs.append("### END SVN COPY METADATA\n")
+    # Now ready to do the actual diff.
+    for filename in sorted(data.iterkeys()):

[M-A Ruel, 2013/04/19 16:17:08]

for filename in sorted(data):

+      diffs.append(SVN._DiffItemInternal(
+          filename, cwd, data[filename], diff_command, full_move, revision))
+    # Use StringIO since it can be messy when diffing a directory move with
+    # full_move=True.
+    buf = cStringIO.StringIO()
+    for d in filter(None, diffs):
+      buf.write(d)
+    result = buf.getvalue()
+    buf.close()
+    return result
+
+  @staticmethod
+  def _DiffItemInternal(filename, cwd, info, diff_command, full_move, revision):
     """Grabs the diff data."""
-    command = ["diff", "--config-dir", bogus_dir, filename]
+    diff_command.append(filename)
     if revision:
-      command.extend(['--revision', revision])
+      diff_command.extend(['--revision', revision])
     data = None
     if SVN.IsMovedInfo(info):
       if full_move:
@@ -837,7 +906,7 @@
       else:
         if info.get("Node Kind") != "directory":
           # svn diff on a mv/cp'd file outputs nothing if there was no change.
-          data = SVN.Capture(command, cwd)
+          data = SVN.Capture(diff_command, cwd)
           if not data:
             # We put in an empty Index entry so upload.py knows about them.
             data = "Index: %s\n" % filename.replace(os.sep, '/')
@@ -846,7 +915,7 @@
       if info.get("Node Kind") != "directory":
         # Normal simple case.
         try:
-          data = SVN.Capture(command, cwd)
+          data = SVN.Capture(diff_command, cwd)
         except subprocess2.CalledProcessError:
           if revision:
             data = GenFakeDiff(filename)
@@ -856,93 +925,6 @@
     return data
 
   @staticmethod
-  def GenerateDiff(filenames, cwd, full_move, revision):
-    """Returns a string containing the diff for the given file list.
-
-    The files in the list should either be absolute paths or relative to the
-    given root. If no root directory is provided, the repository root will be
-    used.
-    The diff will always use relative paths.
-    """
-    assert isinstance(filenames, (list, tuple))
-    root = os.path.normcase(os.path.join(cwd, ''))
-    def RelativePath(path, root):
-      """We must use relative paths."""
-      if os.path.normcase(path).startswith(root):
-        return path[len(root):]
-      return path
-    # If the user specified a custom diff command in their svn config file,
-    # then it'll be used when we do svn diff, which we don't want to happen
-    # since we want the unified diff.  Using --diff-cmd=diff doesn't always
-    # work, since they can have another diff executable in their path that
-    # gives different line endings.  So we use a bogus temp directory as the
-    # config directory, which gets around these problems.
-    bogus_dir = tempfile.mkdtemp()
-    try:
-      # Cleanup filenames
-      filenames = [RelativePath(f, root) for f in filenames]
-      # Get information about the modified items (files and directories)
-      data = dict([(f, SVN.CaptureLocalInfo([f], root)) for f in filenames])
-      diffs = []
-      if full_move:
-        # Eliminate modified files inside moved/copied directory.
-        for (filename, info) in data.iteritems():
-          if SVN.IsMovedInfo(info) and info.get("Node Kind") == "directory":
-            # Remove files inside the directory.
-            filenames = [f for f in filenames
-                         if not f.startswith(filename + os.path.sep)]
-        for filename in data.keys():
-          if not filename in filenames:
-            # Remove filtered out items.
-            del data[filename]
-      else:
-        metaheaders = []
-        for (filename, info) in data.iteritems():
-          if SVN.IsMovedInfo(info):
-            # for now, the most common case is a head copy,
-            # so let's just encode that as a straight up cp.
-            srcurl = info.get('Copied From URL')
-            file_root = info.get('Repository Root')
-            rev = int(info.get('Copied From Rev'))
-            assert srcurl.startswith(file_root)
-            src = srcurl[len(file_root)+1:]
-            try:
-              srcinfo = SVN.CaptureRemoteInfo(srcurl)
-            except subprocess2.CalledProcessError, e:
-              if not 'Not a valid URL' in e.stderr:
-                raise
-              # Assume the file was deleted. No idea how to figure out at which
-              # revision the file was deleted.
-              srcinfo = {'Revision': rev}
-            if (srcinfo.get('Revision') != rev and
-                SVN.Capture(['diff', '-r', '%d:head' % rev, srcurl], cwd)):
-              metaheaders.append("#$ svn cp -r %d %s %s "
-                                 "### WARNING: note non-trunk copy\n" %
-                                 (rev, src, filename))
-            else:
-              metaheaders.append("#$ cp %s %s\n" % (src,
-                                                    filename))
-
-        if metaheaders:
-          diffs.append("### BEGIN SVN COPY METADATA\n")
-          diffs.extend(metaheaders)
-          diffs.append("### END SVN COPY METADATA\n")
-      # Now ready to do the actual diff.
-      for filename in sorted(data.iterkeys()):
-        diffs.append(SVN._DiffItemInternal(
-            filename, cwd, data[filename], bogus_dir, full_move, revision))
-      # Use StringIO since it can be messy when diffing a directory move with
-      # full_move=True.
-      buf = cStringIO.StringIO()
-      for d in filter(None, diffs):
-        buf.write(d)
-      result = buf.getvalue()
-      buf.close()
-      return result
-    finally:
-      gclient_utils.RemoveDirectory(bogus_dir)
-
-  @staticmethod
   def GetEmail(cwd):
     """Retrieves the svn account which we assume is an email address."""
     try: