This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8771

localrepo: only use 'bookmarksinstore' requirement if we have 'store'
ClosedPublic

Authored by pulkit on Jul 21 2020, 4:31 AM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHGdc457177dbc1: localrepo: only use 'bookmarksinstore' requirement if we have 'store'
Summary

This adds check that whether we have the 'store' requirement or not. If we don't
have that, we skip adding the 'bookmarksinstore' requirement and warn user about
it.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

pulkit created this revision.Jul 21 2020, 4:31 AM
pulkit retitled this revision from localrepo: only use BOOKMARKS_IN_STORE_REQUIRMENT is we have a store to localrepo: only use BOOKMARKS_IN_STORE_REQUIRMENT if we have a store.Jul 21 2020, 4:37 AM
pulkit added a comment.Jul 21 2020, 4:39 AM

Another way we can do is to have a checkrequirementscompat() which will check whether we have BOOKMARKS_IN_STORE_REQUIREMENT but not store one and raise error. This one simply drops without any warning. We seems to have following options:

  • Drop without any warning
  • Drop with a warning
  • Error
  • Add store requirement (which means BOOKMARKS_IN_STORE_REQUIREMENT implies store requirement)
marmoute added a subscriber: marmoute.Jul 21 2020, 6:03 AM

The tricky part is that all the requirements has default value. If requirement B depends on requirement A, and both are enabled by default, we cannot simply automatically enable A if B is true, because B is true by default. Warning would be good, but might end up a bit verbose for the same reason.

However if the user explicitly request A=no and B=yes, it seems bad to silently drop B since it was explicitly requested.

So, I don't see a good solution until we detect explicit activation vs default value (which is a scope bloat).

So for now, maybe using a verbose level message when some requirement are dropped ?

pulkit added a comment.Jul 21 2020, 6:07 AM
In D8771#131190, @marmoute wrote:

The tricky part is that all the requirements has default value. If requirement B depends on requirement A, and both are enabled by default, we cannot simply automatically enable A if B is true, because B is true by default. Warning would be good, but might end up a bit verbose for the same reason.
However if the user explicitly request A=no and B=yes, it seems bad to silently drop B since it was explicitly requested.
So, I don't see a good solution until we detect explicit activation vs default value (which is a scope bloat).

Very much agreed. Since we know that store is enabled by default and bookmarks in store is not enabled, we can be aware whether user is explicitly requesting something.

So for now, maybe using a verbose level message when some requirement are dropped ?

Yeah that sounds like a good and important start is this direction.

marmoute added a comment.Jul 21 2020, 6:10 AM
In D8771#131191, @pulkit wrote:
In D8771#131190, @marmoute wrote:

The tricky part is that all the requirements has default value. If requirement B depends on requirement A, and both are enabled by default, we cannot simply automatically enable A if B is true, because B is true by default. Warning would be good, but might end up a bit verbose for the same reason.
However if the user explicitly request A=no and B=yes, it seems bad to silently drop B since it was explicitly requested.
So, I don't see a good solution until we detect explicit activation vs default value (which is a scope bloat).

Very much agreed. Since we know that store is enabled by default and bookmarks in store is not enabled, we can be aware whether user is explicitly requesting something.

However, we will have other cases where wher both are enabled by default.

pulkit retitled this revision from localrepo: only use BOOKMARKS_IN_STORE_REQUIRMENT if we have a store to localrepo: only use BOOKMARKS_IN_STORE_REQUIRMENT is we have a store.Jul 21 2020, 9:41 AM
pulkit edited the summary of this revision. (Show Details)
pulkit added a child revision: D8772: localrepo: abort creating a shared repo if the source does not have store.
pulkit updated this revision to Diff 21997.
pulkit updated this revision to Diff 22066.Jul 23 2020, 10:50 AM
martinvonz added a subscriber: martinvonz.Jul 31 2020, 11:36 PM

The commit message was hard to read.

localrepo: only use BOOKMARKS_IN_STORE_REQUIRMENT is we have a store

My suggestion: localrepo: only use 'bookmarksinstore' requirement if we have 'store'

This adds check that if we are using store and if we are not, we skip adding the
BOOKMARKS_IN_STORE_REQUIREMENT.

My suggestion: This adds a check that we are have the 'store' requirement and if we are not, we skip adding the 'bookmarksinstore'.

Regarding the patch itself, when will it be useful? Is the point to detect cases where we have created such repos? Or to prevent the user from creating new repos? Why don't we update the read path to ignore 'bookmarksinstore' if 'store' is not enabled? Can we have a test case or two (just manually add 'bookmarksinstore' to .hg/requires)?

martinvonz added inline comments.Jul 31 2020, 11:42 PM
mercurial/localrepo.py
3658–3663

This should probably be rewritten in a language that makes sense to the user (or do we tell them what repo requirements are somewhere?). Perhaps something like "ignoring enabled format.bookmarks-in-store config because it incompatible with the disabled format.usestore config".

pulkit added a comment.Aug 1 2020, 10:28 AM
In D8771#132274, @martinvonz wrote:

The commit message was hard to read.

localrepo: only use BOOKMARKS_IN_STORE_REQUIRMENT is we have a store

My suggestion: localrepo: only use 'bookmarksinstore' requirement if we have 'store'

Thank you!

This adds check that if we are using store and if we are not, we skip adding the
BOOKMARKS_IN_STORE_REQUIREMENT.

My suggestion: This adds a check that we are have the 'store' requirement and if we are not, we skip adding the 'bookmarksinstore'.

Thank you!

Regarding the patch itself, when will it be useful? Is the point to detect cases where we have created such repos? Or to prevent the user from creating new repos? Why don't we update the read path to ignore 'bookmarksinstore' if 'store' is not enabled? Can we have a test case or two (just manually add 'bookmarksinstore' to .hg/requires)?

We prevent user from creating new repos. Added a test for that.

pulkit marked an inline comment as done.Aug 1 2020, 10:30 AM
pulkit retitled this revision from localrepo: only use BOOKMARKS_IN_STORE_REQUIRMENT is we have a store to localrepo: only use 'bookmarksinstore' requirement if we have 'store'.Aug 1 2020, 10:32 AM
pulkit edited the summary of this revision. (Show Details)
pulkit updated this revision to Diff 22169.
indygreg accepted this revision.Aug 2 2020, 3:48 PM
This revision is now accepted and ready to land.Aug 2 2020, 3:48 PM

Revision Contents
Changeset List

PathPackages
Mmercurial/localrepo.py (22 lines)

Diff 21997

mercurial/localrepo.py

requirements.add(bookmarks.BOOKMARKS_IN_STORE_REQUIREMENT) requirements.add(bookmarks.BOOKMARKS_IN_STORE_REQUIREMENT)
if ui.configbool(b'format', b'use-persistent-nodemap'): if ui.configbool(b'format', b'use-persistent-nodemap'):
requirements.add(NODEMAP_REQUIREMENT) requirements.add(NODEMAP_REQUIREMENT)
return requirements return requirements
def checkrequirementscompat(ui, requirements):
""" Checks compatibility of repository requirements enabled and disabled.
Returns a set of requirements which needs to be dropped because dependend
requirements are not enabled. Also warns users about it """
dropped = set()
if b'store' not in requirements:
if bookmarks.BOOKMARKS_IN_STORE_REQUIREMENT in requirements:
ui.warn(
_(
b'dropping %s requirement as store is not enabled'
% bookmarks.BOOKMARKS_IN_STORE_REQUIREMENT
)
)
martinvonzUnsubmitted
Done

This should probably be rewritten in a language that makes sense to the user (or do we tell them what repo requirements are somewhere?). Perhaps something like "ignoring enabled format.bookmarks-in-store config because it incompatible with the disabled format.usestore config".

martinvonz: This should probably be rewritten in a language that makes sense to the user (or do we tell…
dropped.add(bookmarks.BOOKMARKS_IN_STORE_REQUIREMENT)
return dropped
def filterknowncreateopts(ui, createopts): def filterknowncreateopts(ui, createopts):
"""Filters a dict of repo creation options against options that are known. """Filters a dict of repo creation options against options that are known.
Receives a dict of repo creation options and returns a dict of those Receives a dict of repo creation options and returns a dict of those
options that we don't know how to handle. options that we don't know how to handle.
This function is called as part of repository creation. If the This function is called as part of repository creation. If the
returned dict contains any items, repository creation will not returned dict contains any items, repository creation will not
b'unable to create repository because of unknown ' b'unable to create repository because of unknown '
b'creation option: %s' b'creation option: %s'
) )
% b', '.join(sorted(unknownopts)), % b', '.join(sorted(unknownopts)),
hint=_(b'is a required extension not loaded?'), hint=_(b'is a required extension not loaded?'),
) )
requirements = newreporequirements(ui, createopts=createopts) requirements = newreporequirements(ui, createopts=createopts)
requirements -= checkrequirementscompat(ui, requirements)
wdirvfs = vfsmod.vfs(path, expandpath=True, realpath=True) wdirvfs = vfsmod.vfs(path, expandpath=True, realpath=True)
hgvfs = vfsmod.vfs(wdirvfs.join(b'.hg')) hgvfs = vfsmod.vfs(wdirvfs.join(b'.hg'))
if hgvfs.exists(): if hgvfs.exists():
raise error.RepoError(_(b'repository %s already exists') % path) raise error.RepoError(_(b'repository %s already exists') % path)
if b'sharedrepo' in createopts: if b'sharedrepo' in createopts: