This is an archive of the discontinued Mercurial Phabricator instance.

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

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

Details

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

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)

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 ?

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.

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 updated this revision to Diff 21997.
pulkit updated this revision to Diff 22066.Jul 23 2020, 10:50 AM

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
3660–3665

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".

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