D1849: lfs: remove internal url in test

[quark (Jun Wu)]

quark created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  `test-lfs-test-server.t` refers to a FB internal domain and requires certain
  implementation (ex. set error code to 404) at that endpoint. Without any
  workaround, It should in theory error out like "Domain cannot be resolved".
  I don't know how Matt Harbison ran the test.
  
  This patch changes the test to only depend on `lfs-test-server`.
  Unfortunately the logic has to be changed since `lfs-test-server` does not
  set error code to 404 but just removes "download" from "actions".

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1849

AFFECTED FILES
  hgext/lfs/blobstore.py
  tests/test-lfs-test-server.t

CHANGE DETAILS

diff --git a/tests/test-lfs-test-server.t b/tests/test-lfs-test-server.t
--- a/tests/test-lfs-test-server.t
+++ b/tests/test-lfs-test-server.t
@@ -160,12 +160,12 @@
   $ rm -rf .hg/store/lfs
   $ rm -rf `hg config lfs.usercache`
   $ hg update -C '.^'
-  abort: LFS server claims required objects do not exist:
-  8e6ea5f6c066b44a0efa43bcce86aea73f17e6e23f0663df0251e7524e140a13!
+  abort: LFS server error. Remote object for "b" not found:(.*)! (re)
   [255]
 
 Check error message when object does not exist:
 
+  $ cd $TESTTMP
   $ hg init test && cd test
   $ echo "[extensions]" >> .hg/hgrc
   $ echo "lfs=" >> .hg/hgrc
@@ -183,7 +183,22 @@
   x-is-binary 0
   $ cd ..
   $ rm -rf `hg config lfs.usercache`
-  $ hg --config 'lfs.url=https://dewey-lfs.vip.facebook.com/lfs' clone test test2
+
+(Restart the server in a different location so it no longer has the content)
+
+  $ $PYTHON $RUNTESTDIR/killdaemons.py $DAEMON_PIDS
+  $ rm $DAEMON_PIDS
+  $ mkdir $TESTTMP/lfs-server2
+  $ cd $TESTTMP/lfs-server2
+#if no-windows
+  $ lfs-test-server &> lfs-server.log &
+  $ echo $! >> $DAEMON_PIDS
+#else
+  $ $PYTHON $TESTTMP/spawn.py >> $DAEMON_PIDS
+#endif
+
+  $ cd $TESTTMP
+  $ hg clone test test2
   updating to branch default
   abort: LFS server error. Remote object for "a" not found:(.*)! (re)
   [255]
diff --git a/hgext/lfs/blobstore.py b/hgext/lfs/blobstore.py
--- a/hgext/lfs/blobstore.py
+++ b/hgext/lfs/blobstore.py
@@ -227,20 +227,26 @@
                                  % rawjson)
         return response
 
-    def _checkforservererror(self, pointers, responses):
+    def _checkforservererror(self, pointers, responses, action):
         """Scans errors from objects
 
         Returns LfsRemoteError if any objects has an error"""
         for response in responses:
-            error = response.get('error')
-            if error:
+            # The server should return 404 when objects cannot be found. Some
+            # server implementation (ex. lfs-test-server)  does not set "error"
+            # but just removes "download" from "actions". Treat that case
+            # as the same as 404 error.
+            notfound = (response.get('error', {}).get('code') == 404
+                        or (action == 'download'
+                            and action not in response.get('actions', [])))
+            if notfound:
                 ptrmap = {p.oid(): p for p in pointers}
                 p = ptrmap.get(response['oid'], None)
-                if error['code'] == 404 and p:
-                    filename = getattr(p, 'filename', 'unknown')
-                    raise LfsRemoteError(
-                        _(('LFS server error. Remote object '
-                          'for "%s" not found: %r')) % (filename, response))
+                filename = getattr(p, 'filename', 'unknown')
+                raise LfsRemoteError(
+                    _(('LFS server error. Remote object '
+                      'for "%s" not found: %r')) % (filename, response))
+            if 'error' in response:
                 raise LfsRemoteError(_('LFS server error: %r') % response)
 
     def _extractobjects(self, response, pointers, action):
@@ -252,21 +258,11 @@
         """
         # Scan errors from objects - fail early
         objects = response.get('objects', [])
-        self._checkforservererror(pointers, objects)
+        self._checkforservererror(pointers, objects, action)
 
         # Filter objects with given action. Practically, this skips uploading
         # objects which exist in the server.
         filteredobjects = [o for o in objects if action in o.get('actions', [])]
-        # But for downloading, we want all objects. Therefore missing objects
-        # should be considered an error.
-        if action == 'download':
-            if len(filteredobjects) < len(objects):
-                missing = [o.get('oid', '?')
-                           for o in objects
-                           if action not in o.get('actions', [])]
-                raise LfsRemoteError(
-                    _('LFS server claims required objects do not exist:\n%s')
-                    % '\n'.join(missing))
 
         return filteredobjects
 



To: quark, #hg-reviewers
Cc: mercurial-devel