Skip to content
Snippets Groups Projects
  1. Jul 10, 2020
  2. Feb 09, 2020
    • Matt Harbison's avatar
      lfs: use str for the open() mode when opening a blob for py3 · 234001d2
      Matt Harbison authored
      The other fix for this was to leave the mode as bytes, and import
      `pycompat.open()` like a bunch of other modules do.  But I think it's confusing
      to still use bytes at the python boundary, and obviously error prone.  Grepping
      for ` open\(.+, ['"][a-z]+['"]\)` and ` open\(.+, b['"][a-z]+['"]\)` outside of
      `tests`, there are 51 and 87 uses respectively, so it's not like this is a rare
      direct usage.
      
      Differential Revision: https://phab.mercurial-scm.org/D8099
      234001d2
  3. Jan 21, 2020
    • Matt Harbison's avatar
      lfs: enable workers by default · 87167caa
      Matt Harbison authored
      With the stall issue seemingly fixed, there's no reason not to use workers.  The
      setting is left for now to keep the test output deterministic, and in case other
      issues come up.  If none do, this can be converted to a developer setting for
      usage with the tests.
      
      Differential Revision: https://phab.mercurial-scm.org/D7963
      87167caa
    • Matt Harbison's avatar
      lfs: fix the stall and corruption issue when concurrently uploading blobs · 43eea17a
      Matt Harbison authored
      We've avoided the issue up to this point by gating worker usage with an
      experimental config.  See 10e62d5efa73, and the thread linked there for some of
      the initial diagnosis, but essentially some data was being read from the blob
      before an error occurred and `keepalive` retried, but didn't rewind the file
      pointer.  So the leading data was lost from the blob on the server, and the
      connection stalled, trying to send more data than available.
      
      In trying to recreate this, I was unable to do so uploading from Windows to
      CentOS 7.  But it reproduced every time going from CentOS 7 to another CentOS 7
      over https.
      
      I found recent fixes in the FaceBook repo to address this[1][2].  The commit
      message for the first is:
      
          The KeepAlive HTTP implementation is bugged in it's retry logic, it supports
          reading from a file pointer, but doesn't support rewinding of the seek cursor
          when it performs a retry. So it can happen that an upload fails for whatever
          reason and will then 'hang' on the retry event.
      
          The sequence of events that get triggered are:
           - Upload file A, goes OK. Keep-Alive caches connection.
           - Upload file B, fails due to (for example) failing Keep-Alive, but LFS file
             pointer has been consumed for the upload and fd has been closed.
           - Retry for file B starts, sets the Content-Length properly to the expected
             file size, but since file pointer has been consumed no data will be uploaded,
             causing the server to wait for the uploaded data until either client or
             server reaches a timeout, making it seem as our mercurial process hangs.
      
          This is just a stop-gap measure to prevent this behavior from blocking Mercurial
          (LFS has retry logic). A proper solutions need to be build on top of this
          stop-gap measure: for upload from file pointers, we should support fseek() on
          the interface. Since we expect to consume the whole file always anyways, this
          should be safe. This way we can seek back to the beginning on a retry.
      
      I ported those two patches, and it works.  But I see that `url._sendfile()` does
      a rewind on `httpsendfile` objects[3], so maybe it's better to keep this all in
      one place and avoid a second seek.  We may still want the first FaceBook patch
      as extra protection for this problem in general.  The other two uses of
      `httpsendfile` are in the wire protocol to upload bundles, and to upload
      largefiles.  Neither of these appear to use a worker, and I'm not sure why
      workers seem to trigger this, or if this could have happened without a worker.
      
      Since `httpsendfile` already has a `close()` method, that is dropped.  That
      class also explicitly says there's no `__len__` attribute, so that is removed
      too.  The override for `read()` is necessary to avoid the progressbar usage per
      file.
      
      [1] https://github.com/facebookexperimental/eden/commit/c350d6536d90c044c837abdd3675185644481469
      [2] https://github.com/facebookexperimental/eden/commit/77f0d3fd0415e81b63e317e457af9c55c46103ee
      [3] https://www.mercurial-scm.org/repo/hg/file/5.2.2/mercurial/url.py#l176
      
      Differential Revision: https://phab.mercurial-scm.org/D7962
      43eea17a
    • Matt Harbison's avatar
      lfs: add a method to the local blobstore to convert OIDs to file paths · 06de4a67
      Matt Harbison authored
      This is less ugly than passing an open callback to the `httpsendfile`
      constuctor.
      
      Differential Revision: https://phab.mercurial-scm.org/D7961
      06de4a67
    • Matt Harbison's avatar
      lfs: move the initialization of the upload request into the try block · 46c8f15f
      Matt Harbison authored
      This (almost) guarantees that the file is closed in the case of an exception.
      The one hole is if the `seek(SEEK_END)`/`tell()`/`seek(0)` sequence fails.  But
      that's going to go away when subclassing `httpconnection.httpsendfile` to fix
      the worker problem, so I'm not going to worry too much.  (And that class appears
      to have the same problem.)
      
      Differential Revision: https://phab.mercurial-scm.org/D7959
      46c8f15f
    • Matt Harbison's avatar
      lfs: drop an unnecessary r'' prefix · b2408aca
      Matt Harbison authored
      No longer necessary since the source transformer was removed.
      
      # skip-blame for changing string prefixes
      
      Differential Revision: https://phab.mercurial-scm.org/D7958
      b2408aca
    • Matt Harbison's avatar
      lfs: explicitly close the file handle for the blob being uploaded · 2ad4e8ae
      Matt Harbison authored
      The previous code relied on reading the blob fully to close it.  The obvious
      problem is if an error occurs before that point.  But there is also a problem
      when using workers where the data may need to be re-read, which can't happen
      once it is closed.  This eliminates the surprising behavior before attempting to
      fix the worker problem.
      
      Differential Revision: https://phab.mercurial-scm.org/D7957
      2ad4e8ae
    • Matt Harbison's avatar
      lfs: drop the unused progressbar code in the `filewithprogress` class · 5f841daf
      Matt Harbison authored
      This has been unused since f98fac24b757, which added worker based transfers for
      concurrency, shifting the progressbar maintenance to the single thread waiting
      on the worker to complete.  Since the name no longer fits, rename the class.
      
      Differential Revision: https://phab.mercurial-scm.org/D7956
      5f841daf
  4. Jan 15, 2020
  5. Jan 14, 2020
  6. Dec 23, 2019
    • Matt Harbison's avatar
      verify: allow the storage to signal when renames can be tested on `skipread` · b9e174d4
      Matt Harbison authored
      This applies the new marker in the lfs handler to show it in action, and adds
      the test mentioned at the beginning of the series to show that fulltext isn't
      necessary in the LFS case.
      
      The existing `skipread` isn't enough, because it is also set if an error occurs
      reading the revlog data, or the data is censored.  It could probably be cleared,
      but then it technically violates the interface contract.  That wouldn't matter
      for the existing verify algorithm, but it isn't clear how that will change as
      alternate storage support is added.
      
      The flag is probably pretty revlog specific, given the comments in verify.py.
      But there's already filelog specific stuff in there and I'm not sure what future
      storage will bring, so I don't want to over-engineer this.  Likewise, I'm not
      sure that we want the verify method for each storage type to completely drive
      the bus when it comes to detecting renames, so I don't want to go down the
      rabbithole of having verifyintegrity() return metadata hints at this point.
      
      Differential Revision: https://phab.mercurial-scm.org/D7713
      b9e174d4
    • Matt Harbison's avatar
      lfs: don't skip locally available blobs when verifying · 1a6dd50c
      Matt Harbison authored
      The `skipflags` config was introduced in a2ab9ebcd85b, which specifically calls
      out downloading and storing all blobs as potentially too expensive.  But I don't
      see any reason to skip blobs that are already available locally.  Hashing the
      blob is the only way to indirectly verify the rawdata content stored in the
      revlog.
      
      (The note in that commit about skipping renamed is still correct, but the reason
      given about needing fulltext isn't.)
      
      Differential Revision: https://phab.mercurial-scm.org/D7712
      1a6dd50c
  7. Dec 20, 2019
  8. Dec 23, 2019
  9. Nov 08, 2019
    • Augie Fackler's avatar
      cleanup: remove pointless r-prefixes on single-quoted strings · 9f70512a
      Augie Fackler authored
      This is the promised second step on single-quoted strings. These had
      existed because our source transformer didn't turn r'' into b'', so we
      had tagged some strings as r-strings to get "native" strings on both
      Pythons. Now that the transformer is gone, we can dispense with this
      nonsense.
      
      Methodology:
      
      I ran
      
          hg locate 'set:added() or modified() or clean()' | egrep '.*\.py$'  | xargs egrep --color=never -n  -- \[\^b\]\[\^a-z\]r\'\[\^\'\\\\\]\*\'\[\^\'\
      
      in an emacs grep-mode buffer, and then used a keyboard macro to
      iterate over the results and remove the r prefix as needed.
      
      # skip-blame removing unneeded r prefixes left over from Python 3 migration.
      
      Differential Revision: https://phab.mercurial-scm.org/D7306
      9f70512a
  10. Nov 07, 2019
    • Augie Fackler's avatar
      cleanup: remove pointless r-prefixes on double-quoted strings · 313e3a27
      Augie Fackler authored
      This is only double-quoted strings. I'll do single-quoted strings as a
      second step. These had existed because our source transformer didn't
      turn r"" into b"", so we had tagged some strings as r-strings to get
      "native" strings on both Pythons. Now that the transformer is gone, we
      can dispense with this nonsense.
      
      Methodology:
      
      I ran
      
          hg locate 'set:added() or modified() or clean()' | egrep '.*\.py$'  | xargs egrep --color=never -n  -- \[\^a-z\]r\"\[\^\"\\\\\]\*\"\[\^\"\]
      
      in an emacs grep-mode buffer, and then used a keyboard macro to
      iterate over the results and remove the r prefix as needed.
      
      # skip-blame removing unneeded r prefixes left over from Python 3 migration.
      
      Differential Revision: https://phab.mercurial-scm.org/D7305
      313e3a27
  11. Nov 02, 2019
    • Gregory Szorc's avatar
      py3: define and use json.loads polyfill · 579672b3
      Gregory Szorc authored
      Python 3.5's json.loads() requires a str. Only Python 3.6+
      supports passing a bytes or bytearray.
      
      This commit implements a json.loads() polyfill on Python 3.5
      so that we can use bytes. The added function to detect encodings
      comes verbatim from Python 3.7.
      579672b3
  12. Oct 21, 2019
    • Denis Laxalde's avatar
      lfs: dedent documentation section about .hglfs file · 26caf96a
      Denis Laxalde authored
      The leading ".hglfs::" is interpreted as a macro in generated man page
      and, as it is unknown, the whole section does not render. Also, having
      the section marked as preformatted (::) makes it render verbatim in
      HTML, which is not desired as the text contains formatting markers. So
      we just dedent the section and remove the ".hglfs::" line. The example
      file is still indented and rendered preformatted.
      26caf96a
  13. Oct 07, 2019
  14. Oct 06, 2019
  15. Sep 03, 2019
  16. Aug 17, 2019
  17. Aug 07, 2019
  18. Jul 14, 2019
    • sliquister's avatar
      convert: add a config option to help doing identity hg->hg conversion · d98ec36b
      sliquister authored
      I want to change the computation of the list of files modified by a
      commit. In principle, this would simply change a cache. But since this
      information is stored in commits rather than a cache, changing it
      means changing commit hashes (going forward).
      
      Some users rely on the convert extension from hg to hg not changing
      hashes when nothing changes (usually). Allow these users to preserve
      hashes despite changes to the changelog files computation by reusing
      these files lists when the manifest is unchanged (since these files
      list are derived from the manifest).
      
      Differential Revision: https://phab.mercurial-scm.org/D6643
      d98ec36b
  19. Jun 14, 2019
  20. May 04, 2019
    • Matt Harbison's avatar
      lfs: add a TODO file · 1756859a
      Matt Harbison authored
      This is a cleaned up and reorganized list of items I sent out about a year ago.
      But tracking this in the repo (like the narrow extension) gives more visibility
      in case anyone wants to help out.
      1756859a
  21. Feb 07, 2019
    • Matt Harbison's avatar
      lfs: disable all authentication except Basic for HTTP(S) connections · 698667eb
      Matt Harbison authored
      I ran into a problem pushing to an old Apache server- the normal outgoing
      traffic occurred, the Batch API request and response occurred, and then things
      suddenly halted.  5 minutes later, a 500 was returned, and the server log had a
      timeout reading 32K from `self._req.bodyfh` in hgweb.request.sendresponse().
      Watching in WireShark, the Batch API got a 401, retried properly, then proceeded
      to PUT the blob (without authentication headers).  This got a 401, but the
      client never retried with authentication.  Worse, the blob was sent over the
      wire in the failed attempt.
      
      This kills digests for both the Batch API and the Transfer API.  While in theory
      we could have the Batch API provide external URLs to a place that supports Basic
      Authentication, the LFS spec actually calls out using Basic Authentication[1].
      It's not clear to me if they've been able to shoehorn in other methods.  But
      let's keep it simple until somebody needs it.
      
      If we only had to support python2, we could just not add the handler for digest
      authentication.  However in python3, AbstractBasicAuthHandler raises ValueError
      if it sees a scheme other than Basic.  So we need to intercept all other schemes
      before it gets to that point.
      
      # no-check-commit because of urllib2.OpenerDirector foo_bar calling conventions
      
      [1] https://github.com/git-lfs/git-lfs/blob/master/docs/api/authentication.md
      698667eb
  22. Jan 29, 2019
    • Matt Harbison's avatar
      lfs: explicitly add the Content-Length header when uploading blobs, for py3 · 1bc01490
      Matt Harbison authored
      This was the reason for test-lfs-test-server.t#git-server complaining about an
      "invalid byte in chunk length".  For some reason if this isn't explicitly added,
      py3.7.1 is adding `transfer-encoding: chunked` as well as `Content-length: x`.
      Wireshark flagged this as malformed.  However, if this is set, it doesn't bother
      with `transfer-encoding`.
      
      Before this patch with py3:
      
        PUT /objects/31cf46fbc4ecd458a0943c5b4881f1f5a6dd36c53d6167d5b69ac45149b38e5b HTTP/1.1
        Accept-Encoding: identity
        Content-length: 12
        accept: application/vnd.git-lfs
        content-type: application/octet-stream
        host: localhost:20062
        transfer-encoding: chunked
        user-agent: git-lfs/2.3.4 (Mercurial 4.9rc0+149-7eb7637e34bf)
      
      Before this patch with py27:
      
        PUT /objects/31cf46fbc4ecd458a0943c5b4881f1f5a6dd36c53d6167d5b69ac45149b38e5b HTTP/1.1
        Accept-Encoding: identity
        accept: application/vnd.git-lfs
        content-type: application/octet-stream
        content-length: 12
        host: localhost:20062
        user-agent: git-lfs/2.3.4 (Mercurial 4.9rc0+149-7eb7637e34bf+20190128)
      
      With this patch and py3, the content is the same as the py27 example.  RFC2616
      says to ignore `Content-Length` if `Transfer-Encoding` is present, so maybe
      there's nothing to do in the hg-server side (though I'm not sure which it is
      using if presented both).
      
      Maybe chunked encoding is better to do?  If someone knows how to suppress the
      `Content-Length`, we can try that instead.
      1bc01490
  23. Jan 28, 2019
  24. Jan 27, 2019
Loading