Show images in Rietveld

static/upload.py

 #!/usr/bin/env python
 #
 # Copyright 2007 Google Inc.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
 # You may obtain a copy of the License at
 #
 #     http://www.apache.org/licenses/LICENSE-2.0
 #
@@ skipping 139 matching lines @@
       The authentication token returned by ClientLogin.
     """
     req = self._CreateRequest(
         url="https://www.google.com/accounts/ClientLogin",
         data=urllib.urlencode({
             "Email": email,
             "Passwd": password,
             "service": "ah",
             "source": "rietveld-codereview-upload",
             "accountType": "GOOGLE",
-        })
+        }),
     )
     try:
       response = self.opener.open(req)
       response_body = response.read()
       response_dict = dict(x.split("=")
                            for x in response_body.split("\n") if x)
       return response_dict["Auth"]
     except urllib2.HTTPError, e:
       if e.code == 403:
         body = e.read()
@@ skipping 227 matching lines @@
                  help="Add CC (comma separated email addresses).")
 # Upload options
 group = parser.add_option_group("Patch options")
 group.add_option("-m", "--message", action="store", dest="message",
                  metavar="MESSAGE", default=None,
                  help="A message to identify the patch. "
                       "Will prompt if omitted.")
 group.add_option("-i", "--issue", type="int", action="store",
                  metavar="ISSUE", default=None,
                  help="Issue number to which to add. Defaults to new issue.")
-group.add_option("-l", "--local_base", action="store_true",
-                 dest="local_base", default=False,
-                 help="Base files will be uploaded.")
+group.add_option("--download_base", action="store_true",
+                 dest="download_base", default=False,
+                 help="Base files will be downloaded by the server "
+                 "(side-by-side diffs may not work on files with CRs).")
 group.add_option("--send_mail", action="store_true",
                  dest="send_mail", default=False,
                  help="Send notification email to reviewers.")
 
 
 def GetRpcServer(options):
   """Returns an instance of an AbstractRpcServer.
 
   Returns:
     A new AbstractRpcServer, on which RPC calls can be made.
   """
 
   rpc_server_class = HttpRpcServer
 
   def GetUserCredentials():
     """Prompts the user for a username and password."""
     email = options.email
     if email is None:
-      email = raw_input("Email: ").strip()
+      prompt = "Email (login for uploading to %s): " % options.server
+      email = raw_input(prompt).strip()
     password = getpass.getpass("Password for %s: " % email)
     return (email, password)
 
   # If this is the dev_appserver, use fake authentication.
   host = (options.host or options.server).lower()
   if host == "localhost" or host.startswith("localhost:"):
     email = options.email
     if email is None:
       email = "test@example.com"
       logging.info("Using debug user %s.  Override with --email" % email)
@@ skipping 50 matching lines @@
 
 def GetContentType(filename):
   """Helper to guess the content-type from the filename."""
   return mimetypes.guess_type(filename)[0] or 'application/octet-stream'
 
 
 # Use a shell for subcommands on Windows to get a PATH search.
 use_shell = sys.platform.startswith("win")
 
 
-def RunShell(command, silent_ok=False, universal_newlines=False):
+def RunShell(command, silent_ok=False, universal_newlines=True):
   logging.info("Running %s", command)
   p = subprocess.Popen(command, stdout=subprocess.PIPE,
                        stderr=subprocess.STDOUT, shell=use_shell,
                        universal_newlines=universal_newlines)
   data = p.stdout.read()
   p.wait()
   p.stdout.close()
   if p.returncode:
     ErrorExit("Got error status from %s" % command)
   if not silent_ok and not data:
@@ skipping 29 matching lines @@
       answer = raw_input(prompt).strip()
       if answer != "y":
         ErrorExit("User aborted")
 
   def GetBaseFile(self, filename):
     """Get the content of the upstream version of a file.
 
     Returns:
       A tuple (base_content, new_content, is_binary, status)
         base_content: The contents of the base file.
-        new_content: For text files, this is empty.  For image files, this is

[GvR, 2008/09/07 00:04:18]

image -> binary ?

[JohnA, 2008/09/09 04:06:07]

Done (although this is really the same thing, since the only binary files we
upload are images).

+        new_content: For text files, this is empty.  For binary files, this is
           the contents of the new file, since the diff output won't contain
           information to reconstruct the current file.
         is_binary: True iff the file is binary.
         status: The status of the file.
     """
 
     raise NotImplementedError(
         "abstract method -- subclass %s must override" % self.__class__)
 
-  def UploadBaseFile(self, issue, rpc_server, patch_list, patchset, options,
-                     filename, fileno, content, is_binary, status, is_base):

[GvR, 2008/09/07 00:04:18]

Docstring required.

Also, isn't there some kind of intermediate object that could be passed instead
of nearly a dozen arguments?!?!  Or maybe this ought to be a (rare) nested
function inside UploadBaseFiles()...

If this is not meant to be overridden for specific RCSes, perhaps the name
should start with an underscore?  (Also, the name is awfully close to
UploadBaseFiles()...)

Finally, I just realize this is also used for uploading a *new* (i.e. non-base)
file.  So the name is also wrong...

[JohnA, 2008/09/09 04:06:07]

Added docstring.  Renamed to UploadFile.  Made it a nested function (good idea,
I hadn't realized that the outer function's parameters are available to nested
functions).

-    file_too_large = False
-    if is_base:
-      type = "base"
-    else:
-      type = "current"
-    if len(content) > MAX_UPLOAD_SIZE:
-      print ("Not uploading the %s file for %s because it's too large." %
-             (type, filename))
-      file_too_large = True
-      content = ""
-    checksum = md5.new(content).hexdigest()
-    if options.verbose > 0:
-      print "Uploading %s file for %s" % (type, filename)

[GvR, 2008/09/07 00:04:18]

Isn't this misleading when it's too large?

[JohnA, 2008/09/09 04:06:07]

Done.

-    url = "/%d/upload_content/%d/%d" % (int(issue), int(patchset), fileno)
-    form_fields = [("filename", filename),
-                   ("status", status),
-                   ("checksum", checksum),
-                   ("is_binary", str(is_binary)),
-                   ("is_base", str(is_base)),]

[GvR, 2008/09/07 00:04:18]

Move the ']' to a new line.

[JohnA, 2008/09/09 04:06:07]

Done.

-    if file_too_large:
-      form_fields.append(("file_too_large", "1"))
-    if options.email:

[GvR, 2008/09/07 00:04:18]

I'm curious -- does this mean you can upload anonymously?

[JohnA, 2008/09/09 04:06:07]

No, GetUserCredentials will get called after since the server will return a 401.
 I haven't changed that code, just moved it around.

-      form_fields.append(("user", options.email))
-    ctype, body = EncodeMultipartFormData(form_fields,
-                                          [("data", filename, content)])
-    response_body = rpc_server.Send(url, body,
-                                    content_type=ctype)
-    if not response_body.startswith("OK"):
-      StatusUpdate("  --> %s" % response_body)
-      sys.exit(False)

[GvR, 2008/09/07 00:04:18]

False -> 1.  sys.exit() uses 0 for success, nonzero for failure.

[JohnA, 2008/09/09 04:06:07]

Changed all places where this occurred in the code (I had just moved this
around).

-
   def UploadBaseFiles(self, issue, rpc_server, patch_list, patchset, options):
-    """Uploads the base files."""
+    """Uploads the base files (and if necessary, the current ones as well)."""
+
+    def UploadFile(filename, file_id, content, is_binary, status, is_base):
+      """Uploads a file to the server."""
+      file_too_large = False
+      if is_base:
+        type = "base"
+      else:
+        type = "current"
+      if len(content) > MAX_UPLOAD_SIZE:
+        print ("Not uploading the %s file for %s because it's too large." %
+               (type, filename))
+        file_too_large = True
+        content = ""
+      checksum = md5.new(content).hexdigest()
+      if options.verbose > 0 and not file_too_large:
+        print "Uploading %s file for %s" % (type, filename)
+      url = "/%d/upload_content/%d/%d" % (int(issue), int(patchset), file_id)
+      form_fields = [("filename", filename),
+                     ("status", status),
+                     ("checksum", checksum),
+                     ("is_binary", str(is_binary)),
+                     ("is_current", str(not is_base)),
+                    ]
+      if file_too_large:
+        form_fields.append(("file_too_large", "1"))
+      if options.email:
+        form_fields.append(("user", options.email))
+      ctype, body = EncodeMultipartFormData(form_fields,
+                                            [("data", filename, content)])
+      response_body = rpc_server.Send(url, body,
+                                      content_type=ctype)
+      if not response_body.startswith("OK"):
+        StatusUpdate("  --> %s" % response_body)
+        sys.exit(1)
+
     patches = dict()
     [patches.setdefault(v, k) for k, v in patch_list]
     for filename in patches.keys():
-      fileno = int(patches.get(filename))

[GvR, 2008/09/07 00:04:18]

'fileno' is a terrible name... It makes me thing of Unix file descriptors. :-(

[JohnA, 2008/09/09 04:06:07]

Done.

+      file_id = int(patches.get(filename))
       base_content, new_content, is_binary, status = self.GetBaseFile(filename)
       if base_content:
-        self.UploadBaseFile(issue, rpc_server, patch_list, patchset, options,
-                            filename, fileno, base_content, is_binary, status,
-                            True)
+        UploadFile(filename, file_id, base_content, is_binary, status, True)
       if new_content:
-        self.UploadBaseFile(issue, rpc_server, patch_list, patchset, options,
-                            filename, fileno, new_content, is_binary, status,
-                            False)
+        UploadFile(filename, file_id, new_content, is_binary, status, False)
 
 
 class SubversionVCS(VersionControlSystem):
   """Implementation of the VersionControlSystem interface for Subversion."""
 
   def GuessBase(self, required):
     """Returns the SVN base URL.
 
     Args:
       required: If true, exits if the url can't be guessed, otherwise None is
@@ skipping 65 matching lines @@
       'Author':              ['Author', 'LastChangedBy'],
       'HeadURL':             ['HeadURL', 'URL'],
       'Id':                  ['Id'],
 
       # Aliases
       'LastChangedDate':     ['LastChangedDate', 'Date'],
       'LastChangedRevision': ['LastChangedRevision', 'Rev', 'Revision'],
       'LastChangedBy':       ['LastChangedBy', 'Author'],
       'URL':                 ['URL', 'HeadURL'],
     }
+
     def repl(m):
        if m.group(2):
          return "$%s::%s$" % (m.group(1), " " * len(m.group(3)))
        return "$%s$" % m.group(1)
     keywords = [keyword
                 for name in keyword_str.split(" ")
                 for keyword in svn_keywords.get(name, [])]
     return re.sub(r"\$(%s):(:?)([^\$]+)\$" % '|'.join(keywords), repl, content)
 
   def GetUnknownFiles(self):
     status = RunShell(["svn", "status", "--ignore-externals"], silent_ok=True)
     unknown_files = []
     for line in status.split("\n"):
       if line and line[0] == "?":
         unknown_files.append(line)
     return unknown_files
 
   def IsImage(self, filename):

[GvR, 2008/09/07 00:04:18]

Docstring.

-    unused, ext = os.path.splitext(filename)
-    if not ext:
+    """Returns true if the filename has an image extension."""
+    mimetype =  mimetypes.guess_type(filename)[0]
+    if not mimetype:
       return False
-    ext = ext[1:].lower()
-    return ext in ("bmp", "gif", "ico", "jpeg", "jpg", "png", "tif", "tiff")

[GvR, 2008/09/07 00:04:18]

You should be using the mimetypes module instead of hardcoding a set of
extensions.  (image/* means it's an image file.)

Also, can't you get the mime type directly from svn?

[JohnA, 2008/09/09 04:06:07]

Done (thanks, didn't know about mimetypes).
Well, most of the time svn will just record "application/octet-stream"

+    return mimetype.startswith("image/")
 
   def ReadFile(self, filename):
     """Returns the contents of a file."""
     file = open(filename, 'rb')
-    result = file.read()
-    file.close()

[GvR, 2008/09/07 00:04:18]

Use this pattern:

file = open(...)
try:
  result = file.read()
finally:
  file.close()

[JohnA, 2008/09/09 04:06:07]

Done.

+    result = ""
+    try:
+      result = file.read()
+    finally:
+      file.close()
     return result
 
   def GetBaseFile(self, filename):
     status = RunShell(["svn", "status", "--ignore-externals", filename])
     if not status:
       StatusUpdate("svn status returned no output for %s" % filename)
-      sys.exit(False)
+      sys.exit(1)
     status_lines = status.splitlines()
     # If file is in a cl, the output will begin with
     # "\n--- Changelist 'cl_name':\n".  See
     # http://svn.collab.net/repos/svn/trunk/notes/changelist-design.txt
     if (len(status_lines) == 3 and
         not status_lines[0] and
         status_lines[1].startswith("--- Changelist")):
       status = status_lines[2]
     else:
       status = status_lines[0]
     base_content = ""
     new_content = ""
 
     # If a file is copied its status will be "A  +", which signifies
     # "addition-with-history".  See "svn st" for more information.  We need to
     # upload the original file or else diff parsing will fail if the file was
     # edited.
-    if (status[0] == "A" and status[3] != "+"):

[GvR, 2008/09/07 00:04:18]

No need to have () around the tests.

[JohnA, 2008/09/09 04:06:07]

Done.

+    if status[0] == "A" and status[3] != "+":
       # We'll need to upload the new content if we're adding a binary file
       # since diff's output won't contain it.
       mimetype = RunShell(["svn", "propget", "svn:mime-type", filename],
                           silent_ok=True)
-      is_binary = mimetype.startswith("application/octet-stream")

[GvR, 2008/09/07 00:04:18]

That seems somewhat restrictive. What if someone went in and actually set the
mime type to image/gif?  Maybe it should be considered binary for all non-empty
mime types except for text/*.

[JohnA, 2008/09/09 04:06:07]

Good point, done.

+      is_binary = mimetype and not mimetype.startswith("text/")
       if is_binary and self.IsImage(filename):
         new_content = self.ReadFile(filename)
     elif status[0] == " " and status[1] == "M":
       # Property changed, don't need to do anything.
       pass
     elif (status[0] in ("M", "D", "R") or
           (status[0] == "A" and status[3] == "+")):
       mimetype = RunShell(["svn", "-rBASE", "propget", "svn:mime-type",
                            filename],
                           silent_ok=True)
-      is_binary = mimetype.startswith("application/octet-stream")
+      is_binary = mimetype and not mimetype.startswith("text/")
       get_base = False
       if not is_binary:
         get_base = True
       elif self.IsImage(filename) and status[0] == "M":··········
         new_content = self.ReadFile(filename)
         get_base = True
 
       if get_base:
-        # On Windows svn cat gives \r\n, and calling subprocess.Popen turns
-        # them into \r\r\n, so use universal newlines to avoid the extra \r.
-        if sys.platform.startswith("win") and not is_binary:
-          nl = True
+        if is_binary:
+          universal_newlines = False
         else:
-          nl = False
-        base_content = RunShell(["svn", "cat", filename], universal_newlines=nl)
+          universal_newlines = True
+        base_content = RunShell(["svn", "cat", filename],
+                                universal_newlines=universal_newlines)
         if not is_binary:
           keywords = RunShell(["svn", "-rBASE", "propget", "svn:keywords",
                                filename],
                               silent_ok=True)
           if keywords:
             base_content = self._CollapseKeywords(base_content, keywords)
     else:
       StatusUpdate("svn status returned unexpected output: %s" % status)
-      sys.exit(False)

[GvR, 2008/09/07 00:04:18]

Again, sys.exit(1).  I guess I missed this on an earlier review...

[JohnA, 2008/09/09 04:06:07]

Done.

+      sys.exit(1)
     return base_content, new_content, is_binary, status[0:5]
 
 
 class GitVCS(VersionControlSystem):
   """Implementation of the VersionControlSystem interface for Git."""
 
   def __init__(self):
     # Map of filename -> hash of base file.
     self.base_hashes = {}
 
@@ skipping 17 matching lines @@
         # We want to save the left hash, as that identifies the base file.
         match = re.match(r"index (\w+)\.\.", line)
         if match:
           self.base_hashes[filename] = match.group(1)
       svndiff.append(line + "\n")
     if not filecount:
       ErrorExit("No valid patches found in output from git diff")
     return "".join(svndiff)
 
   def GetUnknownFiles(self):
-    status = RunShell(["git", "ls-files", "--others"], silent_ok=True)
+    status = RunShell(["git", "ls-files", "--exclude-standard", "--others"],
+                      silent_ok=True)
     return status.splitlines()
 
   def GetBaseFile(self, filename):
     hash = self.base_hashes[filename]
+    base_content = ""
+    new_content = ""
+    is_binary = False
     if hash == "0" * 40:  # All-zero hash indicates no base file.
-      return ("", "", False, "A")
+      status = "A"
     else:
-      return (RunShell(["git", "show", hash]), "", False, "M")

[GvR, 2008/09/07 00:04:18]

Mind putting the RunShell() return value into a temp variable here? It's a bit
hard to follow at first what this does.  It might even be possible to set two
variables to what the first and last value of the return tuple should be, and
then have a single return outside the if/else blocks.

[JohnA, 2008/09/09 04:06:07]

Done.

-
-
-# NOTE: this function is duplicated in engine.py, keep them in sync.
+      status = "M"
+      base_content = RunShell(["git", "show", hash])
+    return (base_content, new_content, is_binary, status)
+
+
+# NOTE: The SplitPatch function is duplicated in engine.py, keep them in sync.
 def SplitPatch(data):
   """Splits a patch into separate pieces for each file.
 
   Args:
     data: A string containing the output of svn diff.
 
   Returns:
     A list of 2-tuple (filename, text) where text is the svn diff output
       pertaining to filename.
   """
@@ skipping 33 matching lines @@
   Returns a list of [patch_key, filename] for each file.
   """
   patches = SplitPatch(data)
   rv = []
   for patch in patches:
     if len(patch[1]) > MAX_UPLOAD_SIZE:
       print ("Not uploading the patch for " + patch[0] +
              " because the file is too large.")
       continue
     form_fields = [("filename", patch[0])]
-    if options.local_base:
+    if not options.download_base:
       form_fields.append(("content_upload", "1"))
     files = [("data", "data.diff", patch[1])]
     ctype, body = EncodeMultipartFormData(form_fields, files)
     url = "/%d/upload_patch/%d" % (int(issue), int(patchset))
     print "Uploading patch for " + patch[0]
     response_body = rpc_server.Send(url, body, content_type=ctype)
     lines = response_body.splitlines()
     if not lines or lines[0] != "OK":
       StatusUpdate("  --> %s" % response_body)
-      sys.exit(False)
+      sys.exit(1)
     rv.append([lines[1], patch[0]])
   return rv
 
 
 def GuessVCS():
   """Helper to guess the version control system.
 
   This examines the current directory, guesses which VersionControlSystem
   we're using, and returns an instance of the appropriate class.  Exit with an
   error if we can't figure it out.
@@ skipping 29 matching lines @@
   global verbosity
   verbosity = options.verbose
   if verbosity >= 3:
     logging.getLogger().setLevel(logging.DEBUG)
   elif verbosity >= 2:
     logging.getLogger().setLevel(logging.INFO)
   vcs = GuessVCS()
   if isinstance(vcs, SubversionVCS):
     # base field is only allowed for Subversion.
     # Note: Fetching base files may become deprecated in future releases.
-    base = vcs.GuessBase(not options.local_base)
+    base = vcs.GuessBase(options.download_base)
   else:
     base = None
-  if not base and not options.local_base:
-    options.local_base = True
+  if not base and options.download_base:
+    options.download_base = True
     logging.info("Enabled upload of base file")
   if not options.assume_yes:
     vcs.CheckForUnknownFiles()
   if data is None:
     data = vcs.GenerateDiff(args)
   if verbosity >= 1:
     print "Upload server:", options.server, "(change with -s/--server)"
   if options.issue:
     prompt = "Message describing this patch set: "
   else:
@@ skipping 23 matching lines @@
   if options.description_file:
     if options.description:
       ErrorExit("Can't specify description and description_file")
     file = open(options.description_file, 'r')
     description = file.read()
     file.close()
   if description:
     form_fields.append(("description", description))
   # If we're uploading base files, don't send the email before the uploads, so
   # that it contains the file status.
-  if options.send_mail and not options.local_base:
+  if options.send_mail and options.download_base:
     form_fields.append(("send_mail", "1"))
-  if options.local_base:
+  if not options.download_base:
     form_fields.append(("content_upload", "1"))
   if len(data) > MAX_UPLOAD_SIZE:
     print "Patch is large, so uploading file patches separately."
     files = []
     form_fields.append(("separate_patches", "1"))
   else:
     files = [("data", "data.diff", data)]
   ctype, body = EncodeMultipartFormData(form_fields, files)
   response_body = rpc_server.Send("/upload", body, content_type=ctype)
-  if options.local_base or not files:
+  if not options.download_base or not files:
     lines = response_body.splitlines()
     if len(lines) >= 2:
       msg = lines[0]
       patchset = lines[1].strip()
       patches = [x.split(" ", 1) for x in lines[2:]]
     else:
       msg = response_body
   else:
     msg = response_body
   StatusUpdate(msg)
   if not response_body.startswith("Issue created.") and \
   not response_body.startswith("Issue updated."):
     sys.exit(0)
   issue = msg[msg.rfind("/")+1:]
 
   if not files:
     result = UploadSeparatePatches(issue, rpc_server, patchset, data, options)
-    if options.local_base:
+    if not options.download_base:
       patches = result
 
-  if options.local_base:
+  if not options.download_base:
     vcs.UploadBaseFiles(issue, rpc_server, patches, patchset, options)
     if options.send_mail:
-      rpc_server.Send("/" + issue + "/mail")
+      rpc_server.Send("/" + issue + "/mail", payload="")
   return issue
 
 
 def main():
   try:
     RealMain(sys.argv)
   except KeyboardInterrupt:
     print
     StatusUpdate("Interrupted.")
     sys.exit(1)
 
 
 if __name__ == "__main__":
   main()