Skip to content
Snippets Groups Projects
  1. Feb 22, 2025
  2. Oct 10, 2024
    • Matt Harbison's avatar
      tests: allow optional output when `test-lfs-server` is sent SIGTERM · 521df44118a8
      Matt Harbison authored
      I guess this has only ever been tested on Windows, but on macOS, the test was
      failing with changes like:
      
          +++ /private/tmp/mercurial-ci/tests/test-lfs-test-server.t#git-server.err
          @@ -858,6 +858,7 @@
           (Restart the server in a different location so it no longer has the content)
      
             $ "$PYTHON" $RUNTESTDIR/killdaemons.py $DAEMON_PIDS
          +  $TESTTMP.sh: line 153: 38639 Terminated: 15          lfs-test-server > lfs-server.log 2>&1
      
           #if hg-server
             $ cat $TESTTMP/access.log $TESTTMP/errors.log
      521df44118a8
  3. Dec 11, 2023
    • Pierre-Yves David's avatar
      changelog: never inline changelog · dcaa2df1f688
      Pierre-Yves David authored
      The test suite mostly use small repositories, that implies that most changelog in the
      tests are inlined. As a result, non-inlined changelog are quite poorly tested.
      Since non-inline changelog are most common case for serious repositories, this
      lack of testing is a significant problem that results in high profile issue like
      the one recently fixed by 66417f55ea33 and 849745d7da89.
      
      Inlining the changelog does not bring much to the table, the number of total
      file saved is negligible, and the changelog will be read by most operation
      anyway.
      
      So this changeset is make it so we never inline the changelog, and de-inline the
      one that are still inlined whenever we touch them.
      
      By doing that, we remove the "dual code path" situation for writing new entry to
      the changelog and move to a "single code path" situation. Having a single
      code path simplify the code and make sure it is covered by test (if test cover
      that situation obviously)
      
      
      This impact all tests that care about the number of file and the exchange size,
      but there is nothing too complicated in them just a lot of churn.
      
      The churn is made "worse" by the fact rust will use the persistent nodemap on
      any changelog now. Which is overall a win as it means testing the persistent
      nodemap more and having less special cases.
      
      In short, having inline changelog is mostly useless and an endless source of
      pain. We get rid of it.
      dcaa2df1f688
  4. Apr 14, 2021
  5. Jan 20, 2021
    • Matt Harbison's avatar
      tests: skip a detailed exit status in test-lfs-test-server · 47b11629a0f2
      Matt Harbison authored
      The mode of failure here differs between `lfs-test-server` and `hg serve`, and
      they each throw a different exception.  The `hg serve` case raises a subclass of
      `StorageError`, which gets a detailed status.  The `lfs-test-server` case raises
      a subclass of `Abort`, which does not.  Since the exit code isn't currently
      conditionizable in the tests, this is the simplest way to avoid the failure.
      
      Differential Revision: https://phab.mercurial-scm.org/D9836
      47b11629a0f2
  6. Dec 10, 2020
  7. Nov 23, 2020
  8. Feb 26, 2020
    • sliquister's avatar
      exchange: turn on option that makes concurrent pushes work better · edc8504bc26b
      sliquister authored
      The motivation is simply to make hg work better out of the box.
      
      This is a slight backwards compatibility break, because client
      extensions could have assumed that the list of heads the client sees
      during discovery will be the list of heads during the entirety of the
      push. It seems unlikely to matter, and not worth mentioning.
      
      There's a fair amount of diff in tests, but this is just due to
      sending a few more bytes on the wire, except for test-acl.t.
      The extra "invalid branch cache" lines in test-acl.t don't seem to
      indicate a problem: the branchcache now get computed during the bundle
      application (because of the check:updated-heads bundle part), but
      doesn't get rolled back when transactions rollback, thus causing a
      message in the next operation computing the branch cache. Before this
      change, I assume the branchcache was only computed on transaction
      commit, so not computed at all when the transactions roll back, thus
      no messages.
      
      Differential Revision: https://phab.mercurial-scm.org/D8202
      edc8504bc26b
  9. Jan 21, 2020
    • Matt Harbison's avatar
      lfs: enable workers by default · 87167caa2989
      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
      87167caa2989
  10. Sep 11, 2019
  11. Sep 08, 2019
    • Pierre-Yves David's avatar
      changegroup: move message about added changes to transaction summary · d7304434390f
      Pierre-Yves David authored
      Before that, applying multiple changegroups in the same transaction issued the
      message multiple time. This result in a confusing output:
      
          adding changesets
          adding manifests
          adding file changes
          added 32768 changesets with 60829 changes to 2668 files
          adding changesets
          adding manifests
          adding file changes
          added 8192 changesets with 16885 changes to 1553 files
          adding changesets
          adding manifests
          adding file changes
          added 1020 changesets with 1799 changes to 536 files
          adding changesets
          adding manifests
          ...
      
      Instead, we now only issue the message once at the end of the transaction,
      summing up all added changesets, changes and files. The line is identical, but
      happens sightly later in the output.
      
      There are other suboptimal behavior around issue multiple changegroup (eg:
      progress bar). We'll cover them later.
      
      This impact of lot of test as one would expect, but a two pass check show they
      are just the order change we expected.
      
      To deal with "under the hood" bundle application by internal code, we had to
      take a slightly hacky move. We could clean that up with a more official way to
      enter "under the hood" section, however I want to keep this series simple to get
      it landed. This kind of change have a very high bit rot rate since it impact a
      lot of test output.
      d7304434390f
  12. Sep 19, 2018
  13. Aug 31, 2018
    • Sushil Khanchi's avatar
      revert: fix the inconsistency of status msgs in --interactive mode · cb70501d8b71
      Sushil Khanchi authored
      Before this patch we were priting every action msg before actually
      performing that action and that was resulting in inconsistencies;
      like in --interactive session if user decided to not revert any
      changes in a file foo, still there will be a msg on console saying
      "reverting foo".
      
      To fix this, I have made some changes to print status msg just
      before the action it is going to perform, no matter if --interactive
      or not.
      
      Changes made in test-revert-interactive.t reflect the changed behavior.
      There are also some changes in test-revert.t because of change in the
      order of messages.
      
      Differential Revision: https://phab.mercurial-scm.org/D4380
      cb70501d8b71
  14. May 15, 2018
  15. May 06, 2018
    • Gregory Szorc's avatar
      tests: remove pid file by default · 89793289c891
      Gregory Szorc authored
      Previously, killdaemons.py would kill PIDs listed in a file then
      leave the file lingering around.
      
      If the PIDs are killed, then there's no point leaving the PID file
      around. In the worst case, a later invocation of killdaemons.py
      (run-tests.py invokes killdaemons.py after running a test) could kill
      a separate process whose PID conflicted with a previously-killed
      process. By removing the PID file, we eliminate this possibility.
      
      Some tests were manually removing the PID file after calling
      killdaemons.py. So we update these tests to not do this.
      
      Differential Revision: https://phab.mercurial-scm.org/D3443
      89793289c891
  16. Apr 15, 2018
    • Matt Harbison's avatar
      lfs: enable the final download count status message · ab04972a33ef
      Matt Harbison authored
      At this point, I think all of the core commands are prefetching, except grep and
      verify.  Verify will need some special handling, in case the revlogs are
      corrupt.
      
      Grep has an issue that still needs to be debugged, but we probably need to give
      the behavior some thought too- it would be a shame to have to download
      everything in order to search.  I think the benefit of having this info for all
      commands outweighs extra printing in a command that is arguably not well
      behaved in this context anyway.
      ab04972a33ef
  17. Apr 14, 2018
    • Matt Harbison's avatar
      scmutil: teach the file prefetch hook to handle multiple commits · 7269b87f817c
      Matt Harbison authored
      The remainder of the commands that need prefetch deal with multiple revisions.
      I initially coded this as a separate hook, but then it needed a list of files
      to handle `diff` and `grep`, so it didn't seem worth keeping them separate.
      
      Not every matcher will emit bad file messages (some are built from a list of
      files that are known to exist).  But it seems better to filter this in one place
      than to push this on either each caller or each hook implementation.
      7269b87f817c
  18. Apr 08, 2018
    • Matt Harbison's avatar
      lfs: infer the blob store URL from an explicit push dest or default-push · 31a4ea773369
      Matt Harbison authored
      Unlike pull, the blobs are uploaded within the exchange.push() window, so simply
      wrap it and swap in a properly configured remote store.  The '_subtoppath' field
      shouldn't be available during this window, but give the passed path priority for
      clarity.
      
      At one point I hit an AttributeError in one of the convert tests when trying to
      save the original remote blobstore when the swap was run unconditionally.  I
      wrapped it in a util.safehasattr(), but then today I wasn't able to reproduce
      it.  But now the whole thing is tucked under the requirement guard because
      without the requirement, there are no blobs in the repo, even if the extension
      is loaded.
      31a4ea773369
    • Matt Harbison's avatar
      lfs: infer the blob store URL from paths.default · 092eff6833a7
      Matt Harbison authored
      If `lfs.url` is specified, it takes precedence.  However, now that we support
      serving blobs via hgweb, we shouldn't *require* this setting.  Less
      configuration is better (things will work out of the box once this is sorted
      out), and git has similar functionality.
      
      This is not a complete solution- it isn't able to infer the blob store from an
      explicitly supplied path, and it should consider `paths.default-push` for push.
      The pull solution for that is a bit hacky, and this alone is an improvement for
      the vast majority of cases.
      
      Even though there are only a handful of references to the saved remote store,
      the location of them makes things complicated.
      
        1) downloading files on demand in the revlog flag processor
        2) copying to readonlyvfs with bundlerepo
        3) downloading in the file prefetch hook
        4) the canupload()/skipdownload() checks
        5) uploading blobs
      
      Since revlog doesn't have a repo or ui reference, we can't avoid creating a
      remote store when the extension is loaded.  While the long term goal is to make
      sure the prefetch hook is invoked early for every command for efficiency, this
      handling in the flag processor is needed as a last ditch fetch.
      
      In order to support the clone command, the remote store needs to be created
      later than when the extension loads, since `paths.default` isn't set until just
      before the files are checked out.  Therefore, this patch changes the prefetch
      hook to ignore the saved reference, and build a new one.
      
      The canupload()/skipdownload() checks simply check if the stored instance is a
      `_nullremote`.  Since this can only be set via `lfs.url` (which is reflected in
      the saved reference), checking only the instance created when the extension
      loaded is fine.
      
      The blob uploading function is called from several places:
      
        1) a prepush hook
        2) when writing a new bundle
        3) from infinitepush
      
      The prepush hook gets an exchange.pushop, so it has a path to where the push is
      going.  The bundle writer and infinitepush don't.  Further, bundle creation for
      things like strip and amend are causing blobs to be uploaded.  This seems wrong,
      but I don't want to side track this sorting that out, so punt on trying to
      handle explicit push paths or `paths.default-push`.
      
      I also think that sending blobs to a remote store when pushing to a local repo
      is wrong.  This functionality predates the usercache, so perhaps that's the
      reason for it.  I've got some patches floating around to stop sending blobs
      remotely in this case, and instead write directly to the other repo's blob
      store.  But the tests for corruption handling weren't happy with this change,
      and I don't have time to rewrite them.  So exclude filesystem based paths from
      this for now.
      
      I don't think there's much of a chance to implement `paths.remote:lfsurl` style
      configs, given how early these are resolved vs how late the remote store is
      created.  But git has it, so I threw a TODO in there, in case anyone has ideas.
      
      I have no idea why this is now doing http auth twice when it wasn't before.  I
      don't think the original blobstore's url is ever being used in these cases.
      092eff6833a7
  19. Apr 03, 2018
  20. Apr 02, 2018
    • Gregory Szorc's avatar
      tests: don't use revlog paths in tests · 435481393198
      Gregory Szorc authored
      Debug commands operating on revlogs don't need the full revlog
      path: they can accept the relative path to a tracked file or use
      -c/-m to specify a changelog or manifest.
      
      Not using the revlog path makes tests more resilient to cases
      where revlogs aren't being used for storage.
      
      Differential Revision: https://phab.mercurial-scm.org/D3025
      435481393198
  21. Feb 26, 2018
    • Matt Harbison's avatar
      lfs: improve the client message when the server signals an object error · 67db84842356
      Matt Harbison authored
      Two things here.  First, the previous message included a snippet of JSON, which
      tends to be long (and in the case of lfs-test-server, has no error message).
      Instead, give a concise message where possible, and leave the JSON to a debug
      output.  Second, the server can signal issues other than a missing individual
      file.  This change shows a corrupt file, but I'm debating letting the corrupt
      file get downloaded, because 1) the error code doesn't really fit, and 2) having
      it locally makes forensics easier.  Maybe need a config knob for that.
      67db84842356
  22. Mar 17, 2018
    • Matt Harbison's avatar
      test-lfs-test-server: add a testcase for `hg serve` · f51c2780db3a
      Matt Harbison authored
      I haven't figured out yet how to make the authentication checks work for a
      specific list of users, so the 'web.allow-push' list is wildcarded.  (It appears
      that the client doesn't react to a 401 by sending authentication data, which may
      be caused in part by not having all of the headers in httpbasicauthhandler's
      http_error_auth_reqed(), compared to a run of test-http.t.  But in any case, we
      should probably have a separate set of tests for various authentication
      scenarios.  As it is, without the wildcard, no push access is granted.)
      
      There are several deviations from the `lfs-test-server` case:
      
       - `hg serve` emits a Server header.  I think Gregory indicated that this isn't
         easily suppressed.
      
       - `hg serve` names the "basic" transfer handler in the Batch API response.  Not
         having to specify it was for backwards compatability, so this seems like the
         right thing to do.  (`lfs-test-server` doesn't name it, whether it was
         explicitly requested by the client or not.)
      
       - PUT status for a newly created file is 201, per RFC-2616 [1].  The Basic
         Transfer API [2] shows an example upload transcript with a 200 response.  It
         doesn't make much sense to re-upload a file (unless it is corrupt) in an
         example, but I wouldn't be surprised if some other implementations also
         expect 200 because of this.  But the RFC says MUST use 201 for creation.
      
       - The Content-Type for the file transfers is "application/octet-stream", like
         the sample transcript (though I don't see it explicitly called out in the
         text elsewhere).  Using "text/plain" seems clearly wrong.
      
       - `lfs-test-server` isn't removing the action property and sending back an
         error code like the spec calls out when a file is missing or corrupt.  Doing
         so on the `hg serve` side reveals a bug in our client code when handling the
         response- it indicates the remote file is missing instead of corrupt around
         line 452.
      
      I'll probably glob over the Content-Length differences once this settles down.
      Prior to the recent hgweb refactoring, the Batch API response was using chunked
      encodings instead.
      
      Back to the RFC, I have no idea if the python framework handles the "MUST NOT
      ignore any Content-* (e.g. Content-Range) headers that it does not understand or
      implement and MUST return a 501" for a PUT request.
      
      [1] https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.6
      [2] https://github.com/git-lfs/git-lfs/blob/master/docs/api/basic-transfers.md#uploads
      f51c2780db3a
  23. Mar 15, 2018
    • Matt Harbison's avatar
      test-lfs: drop trailing ', ' item separators from debug JSON output · c37c47e47a95
      Matt Harbison authored
      The trailing space looks weird when conditionalizing the line.  The commas
      shouldn't be necessary because of the indenting.  The `lfs-test-server` isn't
      sending all of the same items (notably, the "transfer" attribute is missing), so
      having the commas means more lines need to be conditionalized.
      c37c47e47a95
  24. Mar 18, 2018
  25. Mar 13, 2018
    • Matt Harbison's avatar
      lfs: debug print HTTP headers and JSON payload received from the server · 0dcf50dc90b6
      Matt Harbison authored
      This has been extremely valuable to show divergences between `hg serve` and
      `lfs-test-server`.  Once the `hg serve` code lands, there will be a certain
      amount of conditionalizing that needs to be done, because `lfs-test-server`
      doesn't always follow its spec.
      
      The $ISO_8601_DATE_TIME$ pattern masks the fact that `lfs-test-serve` is sending
      out an expires_at value of "0001-01-01T00:00:00Z".  `hg serve` will (probably)
      use current time + 10 minutes or similar.  The $HTTP_DATE$ is the current time.
      0dcf50dc90b6
    • Matt Harbison's avatar
      test-lfs: dial up the debugging on commands that interact with the server · 24e6342db1cb
      Matt Harbison authored
      This will be useful to let the client print out the HTTP headers and JSON in a
      future patch, so we can compare native LFS serving against test-lfs-server
      behavior.  There tends to be a lot of debug stuff that we don't care about here
      in a push, so I was tempted to print this output with a [devel] config.  But
      this will be useful for field debugging too, so just put up with the extra
      output here.
      
      It would have been nice to be able to set ui.debug once, but issue5815 prevents
      that.
      24e6342db1cb
  26. Feb 04, 2018
    • Matt Harbison's avatar
      lfs: deduplicate oids in the transfer · 9b413478f261
      Matt Harbison authored
      Apparently, we can't rely on the server to deduplicate for us.
      
      Sadly, the pointer object isn't hashable, so it can't be reduced by converting
      it to a set.  In order to be hashable, it needs to be immutable.  I had a bunch
      of code to change it to composition and forward the readonly dict methods to a
      member dict.  But the pointer is updated via __setitem__() when creating the
      pointer file.  So it didn't see worth adding all of that code to the class.
      9b413478f261
    • Matt Harbison's avatar
      cat: call the storage prefetch hook · 264b90a060b7
      Matt Harbison authored
      It's not important to call in the case of a single file, but maybe it's better
      to do so for consistency.
      264b90a060b7
    • Matt Harbison's avatar
      archive: call the storage prefetch hook · 533f04d4cb6d
      Matt Harbison authored
      533f04d4cb6d
    • Matt Harbison's avatar
      lfs: prefetch lfs blobs during revert · d857cad588e4
      Matt Harbison authored
      The revert command oddly prints out what it will do before requesting the files
      to be prefetched.  But the 'need to transfer' line indicates the blobs are being
      grouped.
      d857cad588e4
    • Matt Harbison's avatar
      lfs: prefetch lfs blobs when applying merge updates · 0b79f99fd7b0
      Matt Harbison authored
      In addition to merge, this method ultimately gets called by many commands:
      
        - backout
        - bisect
        - clone
        - fetch
        - graft
        - import (without --bypass)
        - pull -u
        - rebase
        - strip
        - share
        - transplant
        - unbundle
        - update
      
      Additionally, it's also called by histedit, shelve, unshelve, and split, but it
      seems that the related blobs should always be available locally for these.
      
      For `hg update`, it happens after the normal argument checking and pre-update
      hook processing, and remote corruption is detected prior to manipulating the
      working directory.  Other commands could use this treatment (archive, cat,
      revert, etc), but this covers so many of the frequently used bulk commands, it
      seems like a good starting point.
      
      Losing the verbose message that prints the file name before a corrupt blob
      aborts the command is a little sad, because there's no easy way to go from oid
      to file name.  I'd like to change that message to list the file name so it looks
      cleaner and less cryptic, but the pointer object is nowhere near where it needs
      to be to do this.  So punt on that for now.
      0b79f99fd7b0
  27. Jan 30, 2018
    • Matt Harbison's avatar
      lfs: emit a status message to indicate how many blobs were uploaded · fa993c3c8462
      Matt Harbison authored
      Previously, there was a progress bar indicating the byte count, but then it
      disappeared once the transfer was done.  Having that value stay on the screen
      seems useful.  Downloads are done one at a time, so hold off on that until they
      can be coalesced, to avoid a series of lines being printed.  (I don't have any
      great ideas on how to do that.  It would be a shame to have to wrap a bunch of
      read commands to be able to do this.)
      
      I'm not sure if the 'lfs:' prefix is the right thing to do here.  The others in
      the test are verbose/debug messages, so in the normal case, this is the only
      line that's prefixed.
      fa993c3c8462
  28. Jan 11, 2018
    • Jun Wu's avatar
      lfs: remove internal url in test · 2c6ebd0c850e
      Jun Wu authored
      `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".
      
      Differential Revision: https://phab.mercurial-scm.org/D1849
      2c6ebd0c850e
  29. Dec 31, 2017
  30. Jan 07, 2018
    • Matt Harbison's avatar
      lfs: improve the error message for a missing remote blob · ebf14075a5c1
      Matt Harbison authored
      It seems better to print the name known to the user, not the internal file.  The
      previous code unconditionally set 'p.filename'.  That potentially made the
      attribute None, and would be printed as such in
      _gitlfsremote._checkforservererror() instead of "unknown".  Normally, files are
      printed relative to CWD, but I don't see a way to get the repo path to make that
      adjustment.
      
      The test modified here apparently only runs within Facebook, but a print
      statement confirmed the name change.  I tried uploading the blob to a different
      remote store (so the git server never saw it), and also killing the git server
      and removing the blob directory, and removing the 'lfs.db' file.  All resulted
      in a message:
      
        abort: LFS server claims required objects do not exist:
        bdc26931acfb734b142a8d675f205becf27560dc461f501822de13274fe6fc8a!
      
      So I have no idea how to make this test generally runnable.
      ebf14075a5c1
  31. Dec 22, 2017
    • Matt Harbison's avatar
      lfs: use the localstore download method to transfer from remote stores · fd610befc37f
      Matt Harbison authored
      Both gitlfsremote and file based remotes benefit from not requiring the whole
      file in memory (though the whole file is still loaded when passing through the
      revlog interface).  With a method specific to downloading from a remote store,
      the misleading 'use hg verify' hint is removed.  The behavior is otherwise
      unchanged, in that a download from both remote store types will yield a copy of
      the blob via util.atomictempfile.
      
      There's no response payload defined for the non 'download' actions, but the
      previous code attempted to read the payload in this case anyway.  This
      refactored code made that more obvious, so any payload is printed as a debug
      message, just in case.
      fd610befc37f
  32. Jan 03, 2018
    • Matt Harbison's avatar
      lfs: use the local store method for opening a blob · e8f80529abeb
      Matt Harbison authored
      I noticed that when I cloned without updating and then turned around and pushed
      that clone to an lfs server, it was only trying to find the blob in the local
      store.
      
      Writes to the dummyremote (file based store) use local.read(), which looks at
      both the usercache and local store.
      e8f80529abeb
Loading