Skip to content
Snippets Groups Projects
  1. Sep 23, 2024
  2. Sep 22, 2024
  3. Sep 25, 2024
    • Matt Harbison's avatar
      statichttprepo: stop shadowing the `bytes` builtin · 77a9c7d8a7ba
      Matt Harbison authored
      PyCharm flagged it, but I also misunderstood when looking at the code, because
      the name implied a byte string, not a number.
      77a9c7d8a7ba
    • Matt Harbison's avatar
      statichttprepo: fix `httprangereader.read()` for py3 · e26a08563223
      Matt Harbison authored
      It looks like there were a bunch of problems, not all of them py3 related:
      
          1) The signature of BinaryIO.read() is -1, not None
          2) The `end` variable can't be bytes and interpolate into str with "%s"
          3) The `end` variable can't be an int and interpolate into str with "%s"
          4) The result slicing could be out of bounds if more is requested than
             returned
      
      I guess if somebody would have called `read(-1)` (either directly or because a
      wrapper defaults to that), it wouldn't have been handled correctly.  The fact
      that it is a valid value meaning to read everything requires some additional
      changes later in the method around when it slices the byte string that was read,
      but that seems to have already been broken.
      e26a08563223
    • Matt Harbison's avatar
      statichttprepo: use a context manager to handle a file descriptor · 159854151f0f
      Matt Harbison authored
      I'm not sure if this should be reduced to `vfs.exists()`.  That would seem to be
      equivalent code (since the result of the read is ignored, so we can't tell if
      the file actually has content, which has been the state of things going back to
      98b6c3dde237), but this is at least safer file descriptor handling.
      159854151f0f
  4. Sep 26, 2024
    • Matt Harbison's avatar
      profiling: pass bytes to `_()` and `error.Abort()` · 499b19683c1b
      Matt Harbison authored
      And of course `other_tool_name` is str too, so that needs to be converted.  The
      type hints from PyCharm say `sys.monitoring.get_tool()` can return None, so
      handle that case explicitly before it trips up pytype.
      499b19683c1b
  5. Jul 08, 2024
    • Jörg Sonnenberger's avatar
      exchange: improve computation of relevant markers for large repos · 8583d138f436
      Jörg Sonnenberger authored
      Compute the candidate nodes with relevant markers directly
      from keys of the predecessors/successors/children dictionaries of
      obsstore. This is faster than iterating over all nodes directly.
      This test could be further improved for repositories with relative
      few markers compared to the repository size, but this is no longer
      hot already. With the current loop structure, the obshashrange use
      works as well as before as it passes lists with a single node.
      
      Adjust the interface by allowing revision lists as well as node lists.
      This helps cases that computes ancestors as it reduces the
      materialisation cost. Use this in _pushdiscoveryobsmarker and
      _getbundleobsmarkerpart. Improve the latter further by directly using
      ancestors().
      
      
      Performance benchmarks show notable and welcome improvement to no-op push and
      pull (that would also apply to other push/pull). This apply to push and pull
      done without evolve.
      
      ### push/pull Benchmark parameter
        # bin-env-vars.hg.flavor          = default
        # benchmark.variants.explicit-rev = none
        # benchmark.variants.protocol     = ssh
        # benchmark.variants.revs         = none
      
       ## benchmark.name                  = hg.command.pull
        # data-env-vars.name              = mercurial-devel-2024-03-22-zstd-sparse-revlog
      before: 5.968537 seconds
      after:  5.668507 seconds (-5.03%, -0.30)
        # data-env-vars.name              = tryton-devel-2024-03-22-zstd-sparse-revlog
      before: 1.446232 seconds
      after:  0.835553 seconds (-42.23%, -0.61)
        # data-env-vars.name              = netbsd-src-draft-2024-09-19-zstd-sparse-revlog
      before: 5.777412 seconds
      after:  2.523454 seconds (-56.32%, -3.25)
      
       ## benchmark.name                  = hg.command.push
        # data-env-vars.name              = mercurial-devel-2024-03-22-zstd-sparse-revlog
      before: 6.155501 seconds
      after:  5.885072 seconds (-4.39%, -0.27)
      
        # data-env-vars.name              = tryton-devel-2024-03-22-zstd-sparse-revlog
      before: 1.491054 seconds
      after:  0.934882 seconds (-37.30%, -0.56)
      
        # data-env-vars.name              = netbsd-src-draft-2024-09-19-zstd-sparse-revlog
      before: 5.902494 seconds
      after:  2.957644 seconds (-49.89%, -2.94)
      
      There is not notable different in these result using the "rust" flavor instead
      of the "default".  The performance impact on the same operation when using
      evolve were also tested and no impact was noted.
      8583d138f436
  6. Sep 21, 2024
    • Matt Harbison's avatar
      typing: make the localrepo classes known to pytype · ee7e106b372b
      Matt Harbison authored
      9d4ad05bc91c and 1b17309cdaab both mentioned making `bundlerepository` and
      `unionrepository` subclass `localrepository` during the type checking phase, but
      that didn't apply to pytype in practice.  See bcaa5d408657 and friends for how
      the zope interfaces confuse pytype, and end up converting the classes they
      decorate into `Any`.
      
      This commit is slightly more complex though, because `localrepository` has mixin
      classes applied to it when it is instantiated.  Specifically, `RevlogFileStorage`
      is added, which adds `def file(f)` (which isn't defined on `localrepository`).
      Therefore a list of `localrepository` superclasses is provided during type
      checking to account for the mixins.  Without this, the `bundlerepository` class
      gets flagged when it attempts to call its superclass implementation of `file()`.
      Note that pytype doesn't understand these mixin superclasses (it marks the
      superclass of `localrepository` as `Any`, because they are zope interfaces it
      doesn't understand), but that's enough to get it to not flag `bundlerepository`.
      PyCharm also stops flagging it as a missing function, though it seems like it is
      able to handle the zope interfaces.
      ee7e106b372b
  7. Sep 23, 2024
    • Matt Harbison's avatar
      typing: add a handful more annotations to `mercurial/vfs.py` · 992fcf6b2473
      Matt Harbison authored
      These came out of refactoring into a protocol class, but they can stand on their
      own.
      
      The `audit` callback is kinda screwy because the internal lambda and the callable
      for `pathutil.pathauditor` have different args and a different return type.  It's
      conditionalized where it is called, and can be cleaned up later if desired.
      992fcf6b2473
  8. Sep 21, 2024
  9. Sep 20, 2024
    • Matt Harbison's avatar
      typing: correct pytype mistakes in `mercurial/vfs.py` · ad83e4f9b40e
      Matt Harbison authored
      With the previous changes in this series (prior to merging the *.pyi file), this
      wasn't too bad- the only definitively wrong things were the `data` argument to
      `writelines()`, and the return type on `backgroundclosing()` (both of these
      errors were dropped in the previous commit; for some reason pytype doesn't like
      `contextlib._GeneratorContextManager`, even though that's what it determined it
      is):
      
          File "/mnt/c/Users/Matt/hg/mercurial/vfs.py", line 411, in abstractvfs:
               Bad return type 'contextlib._GeneratorContextManager' for generator function abstractvfs.backgroundclosing [bad-yield-annotation]
            Expected Generator, Iterable or Iterator
      
      PyCharm thinks this is `Generator[backgroundfilecloser], Any, None]`, which can
      be reduced to `Iterator[backgroundfilecloser]`, but pytype flagged the line that
      calls `yield` without an argument unless it's also `Optional`.  PyCharm is happy
      either way.  For some reason, `Iterable` didn't work for pytype:
      
          File "/mnt/c/Users/Matt/hg/mercurial/vfs.py", line 390, in abstractvfs:
              Function contextlib.contextmanager was called with the wrong arguments [wrong-arg-types]
                   Expected: (func: Callable[[Any], Iterator])
            Actually passed: (func: Callable[[Any, Any, Any], Iterable[Optional[Any]]])
            Attributes of protocol Iterator[_T_co] are not implemented on Iterable[Optional[Any]]: __next__
      ad83e4f9b40e
    • Matt Harbison's avatar
      typing: run `merge-pyi` on `mercurial/vfs.py` · 38720073aa34
      Matt Harbison authored
      The *.pyi file was generated with pytype 2023.11.21.  There were a few things
      here that were wrong (e.g. `writelines()` takes an `Iterable[bytes]`, not
      `bytes`, or inexplicable errors like importing several of the vfs classes from
      this very module), and those changes have been dropped manually here.
      38720073aa34
    • Matt Harbison's avatar
      typing: add type annotations to `mercurial.util.makelock()` · cbd01bf33802
      Matt Harbison authored
      This bubbles up into the `vfs` classes, so get this out of the way.
      cbd01bf33802
    • Matt Harbison's avatar
    • Matt Harbison's avatar
      typing: add type annotations to the `mercurial.util.filestat` class · 1d95a87813ad
      Matt Harbison authored
      It's referenced in the `vfs` classes, so get this out of the way to help there.
      The `TypeVar` definition and its usage was copied from the existing `util.pyi`
      file.
      1d95a87813ad
    • Matt Harbison's avatar
      vfs: do minor copyediting on comments and doc strings · e59e1d8d29d2
      Matt Harbison authored
      These were flagged by PyCharm, so clear them from the gutter.
      e59e1d8d29d2
    • Matt Harbison's avatar
      vfs: simplify the `abstractvfs.rename()` implementation · adbb183c2f27
      Matt Harbison authored
      PyCharm was yapping about `util.rename()` not returning anything, because it is
      typed to return `None`, but the value was captured and returned after calling
      `_avoidambig()`.  Instead, drop all of that, unconditionally rename, and then
      call `_avoidambig()` if appropriate.
      
      While we're here, convert the ersatz ternary operator into a modern one to help
      pytype.  When a variable is initialized the old way, pytype tends to assign the
      type of the LHS of the `and`.  In this case, that's a bool, and it will get
      confused that bool doesn't have a `stat` attribute once this method gets more
      type annotations.  (Currently it thinks the `checkambig` arg is `Any`, so it
      doesn't care.)
      adbb183c2f27
    • Matt Harbison's avatar
      vfs: use @abstractmethod instead of homebrewing abstract methods · f79f98733a5b
      Matt Harbison authored
      The latter confuses PyCharm after adding more type annotations when, for
      example, `abstractvfs.rename()` calls `_auditpath()`- the latter unconditionally
      raised an error, so PyCharm thought the code that came after is unreachable.  It
      also tricked pytype into marking the return type as `Never`, which isn't
      available until Python 3.11 (outside of `typing_extensions`).
      
      This also avoid PyCharm warnings that the call to the superclass constructor was
      missed (it couldn't be called because it raised an error to prevent
      instantiation).
      
      The statichttprepo module needed to be given an override for one of the abstract
      methods, so that it can be instantiated.  In `abstractvfs`, this method is only
      called by `rename()`, so I think we can leave this empty.  We raise an error in
      case somebody accidentally calls it in the future- it would have raised this
      same error prior to this change.
      
      I couldn't wrangle `import-checker.py` into accepting importing `ABC` and
      `abstractmethod`- for each subsequent import, it reports something like:
      
          stdlib import "contextlib" follows local import: abc
      
      I suspect the problem is that near the `if fullname != '__future__'` check, if
      the module doesn't fall into the error case, `seenlocal` gets set to the module
      name.  That causes it to be treated like a local module on the next iteration,
      even though it is in `stdlib_modules`.
      f79f98733a5b
    • Matt Harbison's avatar
      vfs: modernize the detection of the main thread · 1edac12af730
      Matt Harbison authored
      There weren't a lot of good choices when py27 was supported, but starting with
      py34, `threading.main_thread()` is available.  This gets us away from an
      undocumented, internal symbol, and drops a pytype suppression statement.  It is
      also apparently no longer reliable after a process fork.[1][2]
      
      [1] https://stackoverflow.com/a/23207116
      [2] https://github.com/python/cpython/blob/v3.6.3/Lib/threading.py#L1334
      1edac12af730
  10. Sep 22, 2024
    • Matt Harbison's avatar
      store: fix a signature mismatch for a vfs subclass · 2391a5fa111e
      Matt Harbison authored
      This was flagged by PyCharm.  I'm not sure why pytype doesn't catch this- it's
      not excluded from the modules that are currently checked.
      2391a5fa111e
    • Matt Harbison's avatar
      lfs: fix various signature mismatches for vfs subclasses · d62887764687
      Matt Harbison authored
      These were flagged by PyCharm.  I'm not sure why pytype doesn't catch these-
      only `hgext/lfs/__init__.py` in the lfs extension is excluded from being
      checked.
      
      I'm not sure if the `*insidef` arg to `join()` was meant as an internal
      convencience, because I see another class that gets flagged for the same
      signature problem (to be fixed next).  But I don't feel bold enough to make this
      an internal function, and provide a simplified public `join()` on the `vfs`
      classes.  That can still be done later, if desired.  For now, process the
      additional args and pass them along, even though there don't appear to be any
      current callers that provide extra args to these classes.  We need all of the
      subclasses to agree on the signature, or they won't be considered to implement
      the `Vfs` protocol being developed.
      
      While we're copy/pasting from the base class, bring the type annotations along
      for the ride.
      d62887764687
    • Matt Harbison's avatar
      util: add a comment to suppress a PyCharm warning about a PEP 8 violation · 8f3cbea2547c
      Matt Harbison authored
      Slowly trying to get rid of silly warnings, so that real problems aren't hidden.
      8f3cbea2547c
    • Matt Harbison's avatar
      keepalive: fix a signature mismatch for a http.client.HTTPResponse subclass · 1a640aa20e48
      Matt Harbison authored
      Also flagged by PyCharm.  This is checked by pytype too, so I'm not sure why it
      misses this.  I verified in py36 that this argument is documented for the
      function, so maybe this is py2 legacy.
      1a640aa20e48
    • Matt Harbison's avatar
      cbor: drop a duplicate dictionary initialization entry · 6a0afc73472e
      Matt Harbison authored
      Flagged by PyCharm.
      6a0afc73472e
  11. Sep 04, 2024
  12. Sep 19, 2024
    • Matt Harbison's avatar
      51235f6aa067
    • Matt Harbison's avatar
      unionrepo: fix mismatches with revlog classes · 8315175f678d
      Matt Harbison authored
      This is a subset of cfd30df0f8e4, applied to `unionrepository`.  There are none
      of the `write()` method overrides here, like `bundlerepository`.
      
      With these changes, pytype flags the `unionrevlog` constructor:
      
          File "/mnt/c/Users/Matt/hg/mercurial/unionrepo.py", line 55, in __init__:
              No attribute '_revlog' on mercurial.changelog.changelog [attribute-error]
          Called from (traceback):
            line 207, in __init__
          File "/mnt/c/Users/Matt/hg/mercurial/unionrepo.py", line 55, in __init__:
              No attribute '_revlog' on mercurial.revlog.revlog [attribute-error]
          Called from (traceback):
            line 232, in __init__
      
      But it turns out that both `changelog.changelog` and `revlog.revlog` do have a
      `target` attribute, so they wouldn't trip over this.  It seems weird that the
      second caller to be flagged is passing the private `_revlog`, but maybe that's
      how it needs to be.
      8315175f678d
    • Matt Harbison's avatar
      typing: make `unionrepository` subclass `localrepository` while type checking · 1b17309cdaab
      Matt Harbison authored
      This is the same change as 9d4ad05bc91c made for `bundlerepository`, for the
      same reasons.
      
      Also, add a comment here to suppress the PyCharm warning that the superclass
      constructor is not called, that is new now that there's a simulated superclass.
      That lack of a call is by design- `makeunionrepository()` does magic that
      PyCharm isn't aware of.  But PyCharm has been better at catching problems than
      pytype in a lot of cases, so I'd like to reduce the bogus things it flags, to
      make the real issues stand out.
      1b17309cdaab
    • Matt Harbison's avatar
      0afd58c72175
  13. Sep 18, 2024
    • Matt Harbison's avatar
      revlog: make `clearcaches()` signature consistent with ManifestRevlog · 5e79783d4bc7
      Matt Harbison authored
      I'm not sure if this a newly added bug, because of using a different version of
      pytype, or if the recent work around avoiding the zope interface types in the
      type checking phase (see 5eb98ea78fd7 and friends)... but pytype 2023.11.21
      started flagging this series since it was last pushed ~6 weeks ago:
      
          File "/mnt/c/Users/Matt/hg/mercurial/bundlerepo.py", line 204, in <module>:
              Overriding method signature mismatch [signature-mismatch]
            Base signature: 'def mercurial.manifest.ManifestRevlog.clearcaches(self, clear_persisted_data: Any = ...) -> None'.
            Subclass signature: 'def mercurial.revlog.revlog.clearcaches(self) -> None'.
            Not enough positional parameters in overriding method.
      
      Maybe the multiple inheritance in `bundlerepo.bundlemanifest` is bad, but it
      seems like a `ManifestRevlog` is-a `revlog`, even though the class hierarchy
      isn't coded that way.  Additionally, it looks like `revlog.clearcaches()` is
      dealing with some persistent data, so maybe this is useful to have there anyway.
      
      Also sprinkle some trivial type hints on the method, because there are other
      `clearcaches()` definitions in the codebase with these hints, and I don't feel
      like waiting for another pytype run to see if it cares that specifically about
      the signature matching.
      5e79783d4bc7
  14. Aug 03, 2024
    • Matt Harbison's avatar
      bundlerepo: fix mismatches with repository and revlog classes · cfd30df0f8e4
      Matt Harbison authored
      Both pytype and PyCharm complained that `write()` and `_write()` in the
      bundlephasecache class aren't proper overrides- indeed they seem to be missing
      an argument that the base class has.
      
      PyCharm and pytype also complained that the `revlog.revlog` class doesn't have a
      `_chunk()` method.  That looks like it was moved from revlog to `_InnerRevlog`
      back in e8ad6d8de8b8, and wasn't caught because this module wasn't type checked.
      However, I couldn't figure out a syntax with `revlog.revlog._inner._chunk(self, rev)`,
      as it complained about passing too many args.  `bundlerevlog._rawtext()` uses
      this `super(...)` style to call the super class, so hopefully that works, even
      with the wonky dynamic subclassing.  The revlog class needed the `_InnerRevlog`
      field typed because it isn't set in the constructor.
      
      Finally, the vfs type hints look broken.  This initially failed with:
      
          File "/mnt/c/Users/Matt/hg/mercurial/bundlerepo.py", line 65, in __init__: Function readonlyvfs.__init__ was called with the wrong arguments [wrong-arg-types]
                   Expected: (self, vfs: mercurial.vfs.vfs)
            Actually passed: (self, vfs: Callable)
          Called from (traceback):
            line 232, in dirlog
            line 214, in __init__
      
      I don't see a raw Callable, but I tried changing some of the vfs args to be typed
      as `vfsmod.abstractvfs`, but that class doesn't have `options`, so it failed
      elsewhere.  `readonlyvfs` isn't a subclass of `vfs` (it's a subclass of
      `abstractvfs`), so I'm not sure how to handle that.  It would be a shame to have
      to make a union of vfs subclasses (but not all of them have `options` either).
      cfd30df0f8e4
  15. Sep 18, 2024
    • Matt Harbison's avatar
      typing: make `bundlerepository` subclass `localrepository` while type checking · 9d4ad05bc91c
      Matt Harbison authored
      Currently, `mercurial/bundlerepo.py` is excluded from pytype, mostly because it
      complains that various `ui` and `vfs` fields in `localrepository` are missing.
      (`bundlerepository` dynamically subclasses `localrepository` when it is
      instantiated, so it works at runtime.)  This makes that class hierarchy known to
      pytype.
      
      Having a protocol for `Repository` is probably the right thing to do, but that
      will be a lot of work and this still reflects the class at runtime.  Subclassing
      also has the benefit of making sure any method overrides have a matching
      signature, so maybe this is a situation where we do both of these things.  (I'm
      not sure how clear the diagnostics are if a class *almost* implements a
      protocol, but is missing a method argument or similar.)  The subclassing is not
      done outside of type checking runs to avoid any side effects on already complex
      code.
      9d4ad05bc91c
  16. Jun 19, 2024
  17. Sep 17, 2024
  18. Jun 19, 2024
  19. Sep 12, 2024
    • Matt Harbison's avatar
      typing: add `from __future__ import annotations` to remaining source files · 1c5810ce737e
      Matt Harbison authored
      Most of these look newer than when the original imports referenced in the
      previous commit were dropped, so these weren't covered by the backout.  These
      were found with:
      
          hg files mercurial hgext hgext3rd -I '**.py' -X '**/thirdparty' \
              | xargs grep -L 'from __future__ import annotations'
      
      All of the `__init__.py` files that finds are empty, so those were ignored and
      the rest manually edited.
      1c5810ce737e
  20. Sep 16, 2024
    • Matt Harbison's avatar
      typing: add `from __future__ import annotations` to most files · f4733654f144
      Matt Harbison authored
      Now that py36 is no longer supported, we can postpone annotation evaluation.
      This means that the quoting is usually optional (for things imported under the
      guard of `if typing.TYPE_CHECKING:` to avoid circular imports), and there's less
      overhead on startup[1].
      
      There may be some missing here.  I backed out 6000f5b25c9b (which removed the
      `from __future__ import ...` that was supporting py2), reverted the changes in
      `contrib/`, `doc/`, and `tests/`, and then ran:
      
          $ hg status -n --change . | \
              xargs sed -i -e 's/from __future__ import .*$/from __future__ import annotations/'
      
      There were some minor tweaks needed when reviewing (mostly making the spacing
      around the import consistent, and `mercurial/testing/__init__.py` had a
      multiline import that wasn't fully rewritten.
      
      [1] https://docs.python.org/3/whatsnew/3.7.html#pep-563-postponed-evaluation-of-annotations
      f4733654f144
Loading