This is an archive of the discontinued Mercurial Phabricator instance.

Differential D12195

obsolete: don't use os.stat in repo.obsstore.__nonzero__ if it's static HTTP
ClosedPublic

Authored by av6 on Feb 16 2022, 1:22 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHGef50a62eec40: obsolete: don't use os.stat in repo.obsstore.__nonzero__ if it's static HTTP
rHGb844b7d4fc06: obsolete: don't use os.stat in repo.obsstore.__nonzero__ if it's static HTTP
Summary

If a repo is accessed via static HTTP, then we obviously can't use os.stat() to
just peek at the file size. Let's download the entire file to check its size.
Yes, this feels wasteful, but:

  1. If we're cloning or pulling a repo from a static HTTP server, we need the contents of the obsstore anyway.
  1. Implementing statichttpvfs.stat() that uses HEAD will result in one more request to a static-only HTTP server, which is already slow. Also parsing a response to a HEAD request to construct os.stat_result is pretty hacky. There's also a question of the remote server properly supporting HEAD method and reporting at least file size.
  1. Implementing statichttpvfs.stat() that uses GET is pretty much the same thing as we do here, except we can't even cache the response easily, unlike simply accessing obsstore._data, which is @propertycache'd.

Importing statichttprepo locally to avoid circular import.

See also: 4507bc001365 and commit message of f8f2ecdde4b5.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

av6 created this revision.Feb 16 2022, 1:22 AM
av6 edited the summary of this revision. (Show Details)Feb 16 2022, 1:24 AM
Alphare requested changes to this revision.Feb 16 2022, 4:33 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
mercurial/obsolete.py
582

Shouldn't this be len(self._data) > 1?

This revision now requires changes to proceed.Feb 16 2022, 4:33 AM
av6 updated this revision to Diff 32253.Feb 16 2022, 6:28 AM
av6 added a comment.Feb 16 2022, 6:32 AM

Argh, this is like the third time I've added len() here, because I'd somehow managed to lose this change twice before. Yes, should be len(self._data).

Alphare accepted this revision.Feb 17 2022, 4:10 AM
This revision is now accepted and ready to land.Feb 17 2022, 4:10 AM

Revision Contents
Changeset List

PathPackages
Mmercurial/obsolete.py (7 lines)

Diff 32254

mercurial/obsolete.py

def __iter__(self): def __iter__(self):
return iter(self._all) return iter(self._all)
def __len__(self): def __len__(self):
return len(self._all) return len(self._all)
def __nonzero__(self): def __nonzero__(self):
from . import statichttprepo
if isinstance(self.repo, statichttprepo.statichttprepository):
# If repo is accessed via static HTTP, then we can't use os.stat()
# to just peek at the file size.
return len(self._data) > 1
AlphareUnsubmitted
Not Done

Shouldn't this be len(self._data) > 1?

Alphare: Shouldn't this be `len(self._data) > 1`?
if not self._cached('_all'): if not self._cached('_all'):
try: try:
return self.svfs.stat(b'obsstore').st_size > 1 return self.svfs.stat(b'obsstore').st_size > 1
except OSError as inst: except OSError as inst:
if inst.errno not in (errno.ENOENT, errno.EINVAL): if inst.errno != errno.ENOENT:
raise raise
# just build an empty _all list if no obsstore exists, which # just build an empty _all list if no obsstore exists, which
# avoids further stat() syscalls # avoids further stat() syscalls
return bool(self._all) return bool(self._all)
__bool__ = __nonzero__ __bool__ = __nonzero__
@property @property