This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8633

share: introduce config option to store requires in .hg/store
ClosedPublic

Authored by pulkit on Jun 15 2020, 11:03 AM.

Details

Reviewers
durin42
marmoute
indygreg
Group Reviewers
hg-reviewers
Commits
rHGd252f51ab032: share: introduce config option to store requires in .hg/store
Summary

This introduces a config option which enabled stores the requirements on a
repository in store instead.

When enabled, .hg/requires will contain the share-safe requirement which
marks that the requirements are present in the store.
This is done so that repository requirements can be shared with shares made
using hg share command.

After this patch, hg share checks whether the source repository has
share-safe requirement, if yes, it does not copy the requirements.

Test for the new functionality is added and a test case in exitsing share tests
is also added.

Diff Detail

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

Event Timeline

pulkit created this revision.Jun 15 2020, 11:03 AM
pulkit edited the summary of this revision. (Show Details)Jun 25 2020, 4:52 AM
pulkit updated this revision to Diff 21702.
pulkit updated this revision to Diff 21740.Jul 1 2020, 6:00 AM
marmoute requested changes to this revision.Jul 15 2020, 1:58 PM
marmoute added a subscriber: marmoute.
marmoute added inline comments.
mercurial/localrepo.py
455

Until the semantic is fully implemented, this should be exp-sharesafe otherwise a non complying client could accessing without respecting the semantic.

537–554

This looks like a lot of duplicated code, am I missing something ? Could we do less duplication?

3797–3810

It looks like share-safe should requires store, can we make them dependent of each other.

This revision now requires changes to proceed.Jul 15 2020, 1:58 PM
pulkit marked 2 inline comments as done.Jul 17 2020, 10:24 AM
pulkit added inline comments.
mercurial/localrepo.py
455

Done.

537–554

Refactored in next patch.

3797–3810

Does not share extension implies that store is present?

marmoute added inline comments.Jul 20 2020, 1:05 PM
mercurial/localrepo.py
537–554

I see this is done in D8655 thanks !

3797–3810

Maybe, can you check ?

In all cases, I would rather make the safe-share requirement consistent from the start. Can we have some code either setup store automatically, or reject with some error (whichever solution you prefer).

pulkit updated this revision to Diff 22068.Jul 23 2020, 10:52 AM
pulkit updated this revision to Diff 22171.Aug 1 2020, 10:36 AM
indygreg added a subscriber: indygreg.Aug 5 2020, 9:08 PM

I need to think about the implications of this change a bit more: the requires file plays an important part in how clients open repositories and there are significant backwards and forwards compatibility concerns.

One immediate request is to document semantics in mercurial/helptext/internals/requirements.txt as part of this change.

FWIW I thought I had typed up an internals document on how repositories are opened. But maybe I never submitted it for review or maybe it never landed.

indygreg added a comment.Aug 6 2020, 9:43 PM

I left a handful of comments.

Changing repo opening semantics and requirements handling is a bit scary.

I'm generally in favor of separating working directory requirements from non-working directory requirements so I think this patch is a step in the right direction. There are a few alternative implementations worth considering. (Although I'm unsure if they are superior.)

  1. Write the new requirements file in the .hg directory as .hg/store-requires (or similar). Conceptually, we're talking about working directory and backend requirements. The "store" is just today's layout for storing backend data [which can be shared between working directories].
  2. Write a new requirements file for working directory specific requirements.
mercurial/localrepo.py
545

I'm reasonably certain this logic is duplicated from somewhere. And opening shared repos is quirky enough that I'd feel better if there was a helper function for resolving the store vfs given a vfs for a .hg directory.

553

The swalloing of ENOENT doesn't sit well with me.

The repo opening semantics are such that .hg/requires is read first and all subsequent repo opening is dictated by what is found inside that file.

I think that a .hg/store/requires file presence should be directly indicated via an entry in .hg/requires - say store-requires - and if that entry is present, the file must be present: a missing file would be an error.

So I think the request here would be to rename the requirement to [exp-]store-requires instead of [exp-]share-safe. If we wanted to add additional functionality governing how shared repos are opened, we should consider a separate requirement for that, independent of the one that indicates if a .hg/store/requires exists.

Put another way, we want repo opening semantics to be strictly defined with little room for interpretation. And we want requires entries to be narrowly scoped so there isn't ambiguity about what they mean.

3672

It's really weird that this requirement has share in its name but it isn't dependent on being a shared repo.

3808

This whole block needs more comments because what it is trying to accomplish is important and nuanced.

I would also consider rewriting the code so that we populate variables indicating the set of requirements to populate in .hg/requires and .hg/store/requires. That way we have at most 2 calls to writerequires(), which I think will improve readability.

3808

What this is doing is blindly moving all requirements except SHARESAFE_REQUIREMENT to .hg/store/requires. The movement of most requirements there makes sense because they govern the "store." However, there are requirements like exp-sparse which I believe only concern the working directory and should not be in the "store."

If we want to go the approach of moving store-specific requirements into .hg/store/requires, I think we need to annotate whether requirements belong to the store or the non-store and write/move requirements accordingly.

mercurial/scmutil.py
1478

The presence of this conditional here feels a bit weird to me: I feel like it might be better if we explicitly passed in 2 sets of requirements to read and this function were "dumber."

1479

Writing of the requirements file should be extremely rare. I don't think we need this lock here.

mercurial/store.py
378

Bonus points if you refactor this to be a proper data structure as part of this series. A space delimited list/set makes no sense to me.

450

Wait - why was requires here before?

marmoute added a comment.Aug 7 2020, 4:24 AM
In D8633#133117, @indygreg wrote:

I left a handful of comments.
Changing repo opening semantics and requirements handling is a bit scary.
I'm generally in favor of separating working directory requirements from non-working directory requirements so I think this patch is a step in the right direction. There are a few alternative implementations worth considering. (Although I'm unsure if they are superior.)

  1. Write the new requirements file in the .hg directory as .hg/store-requires (or similar). Conceptually, we're talking about working directory and backend requirements. The "store" is just today's layout for storing backend data [which can be shared between working directories].
  2. Write a new requirements file for working directory specific requirements.

I would advocate for having a file for working-directory specific requirements. It is safer to assume all requirements are store specific unless explicitly specified otherwise.

marmoute added inline comments.Aug 7 2020, 4:40 AM
mercurial/localrepo.py
545

I had the same comment, and it turns out @pulkit is cleaning the duplication in later patches.

553

The swalloing of ENOENT doesn't sit well with me.

The repo opening semantics are such that .hg/requires is read first and all subsequent repo opening is dictated by what is found inside that file.

I think that a .hg/store/requires file presence should be directly indicated via an entry in .hg/requires - say store-requires - and if that entry is present, the file must be present: a missing file would be an error.

+1 for having some explicit error. If we go the route of using a wc specific files, we should be strict on having it too.

I don't have a strong opinion on having the requires file in the .hg/store/ directory yet. It seems cleaner/clearer to do so.

553

So I think the request here would be to rename the requirement to [exp-]store-requires instead of [exp-]share-safe. If we wanted to add additional functionality governing how shared repos are opened, we should consider a separate requirement for that, independent of the one that indicates if a .hg/store/requires exists.

Having a galaxy on interdependent requirements can become a nightmare, because we either have to start dealing with all the corner case of some being enabled without the other or to add extra logic to make sure they are not being used without the other (and error out on bad configuration). So having simple requirement adding a consistent improvement seems like a better idea.

The share-safe requirement comes with other semantic beside the store one, for example regarding the reading of the configuration. So store-requires would be too narrow.

3672

The requirements is named "share-safe" because after this change, the repository is safe to be shared. A repository do not need to be shared to "share-safe" the same way a watch does not need to be in the submerged to be water-proof.

3808

Defaulting to "requirements is a store requirement" and explicitly flagging requirement that are working copy specific seems the safest.

mercurial/scmutil.py
1479

I agreed that the requirements should be written at repository creation time (or upgrade time) and that is all. Maybe we should update the acquiring of the lock with some code checking we are actually holding a lock while doing this?

The upgrade usecase is a good argument in favor of moving the requirement file inside the store directory.

mercurial/store.py
378

I can add an extra bonus point to the stake :-)

pulkit marked 3 inline comments as done.Aug 7 2020, 8:59 AM
pulkit planned changes to this revision.
In D8633#133117, @indygreg wrote:

I left a handful of comments.
Changing repo opening semantics and requirements handling is a bit scary.
I'm generally in favor of separating working directory requirements from non-working directory requirements so I think this patch is a step in the right direction. There are a few alternative implementations worth considering. (Although I'm unsure if they are superior.)

  1. Write the new requirements file in the .hg directory as .hg/store-requires (or similar). Conceptually, we're talking about working directory and backend requirements. The "store" is just today's layout for storing backend data [which can be shared between working directories].
  2. Write a new requirements file for working directory specific requirements.

Hi, thanks a lot for the detailed review. I think we can have .hg/requires as working directory specific and .hg/store/requires as store requirements.

I have not yet implemented all the fixes you mentioned. Hence marking this as planned changes. However I have added some parents patches which should be good to review.

pulkit updated this revision to Diff 22381.Aug 10 2020, 11:26 AM
pulkit updated this revision to Diff 22397.Aug 11 2020, 5:06 AM
pulkit marked 2 inline comments as done.Aug 11 2020, 5:25 AM
In D8633#133201, @pulkit wrote:
In D8633#133117, @indygreg wrote:

I left a handful of comments.
Changing repo opening semantics and requirements handling is a bit scary.
I'm generally in favor of separating working directory requirements from non-working directory requirements so I think this patch is a step in the right direction. There are a few alternative implementations worth considering. (Although I'm unsure if they are superior.)

  1. Write the new requirements file in the .hg directory as .hg/store-requires (or similar). Conceptually, we're talking about working directory and backend requirements. The "store" is just today's layout for storing backend data [which can be shared between working directories].
  2. Write a new requirements file for working directory specific requirements.

Hi, thanks a lot for the detailed review. I think we can have .hg/requires as working directory specific and .hg/store/requires as store requirements.
I have not yet implemented all the fixes you mentioned. Hence marking this as planned changes. However I have added some parents patches which should be good to review.

@indygreg I think I have taken care of all fixes you suggested except one (the finegrained store-requires one). This patch should be good for an another look now.

mercurial/localrepo.py
553

Fixed the swallowing of ENOENT.

About a fine grained store-requires requirement, I am not sure what's the best way out.

Right now exp-share-safe implies two things, requirements in store and source .hgrc be shared. We definitely can have dedicated store-requires but that introduces a spider of inter-dependent requirements.

Does exp-share-safe implies store-requires or need it? etc.

For now, I have not changed anything on this front in this patch.

3672

What @marmoute said, the requirement is named share-safe because after this change, the repository is safe to be shared.

3808

The whole code block is now much simpler, thanks to filterrequirements introduced in previous patches.

mercurial/scmutil.py
1478

This code is much cleaner now after introducing filterrequirements in previous patches.

mercurial/store.py
450

A line of code dating back to 2008. Removing it does not make any tests fail. Will send a patch for it separately.

pulkit marked 2 inline comments as done.Aug 11 2020, 5:27 AM
In D8633#133021, @indygreg wrote:

I need to think about the implications of this change a bit more: the requires file plays an important part in how clients open repositories and there are significant backwards and forwards compatibility concerns.
One immediate request is to document semantics in mercurial/helptext/internals/requirements.txt as part of this change.

Since this patch does not implement all the things which are covered by this requirement, I appended a patch (D8914) at end of series for this.

FWIW I thought I had typed up an internals document on how repositories are opened. But maybe I never submitted it for review or maybe it never landed.

pulkit updated this revision to Diff 22444.Aug 25 2020, 7:39 AM
indygreg accepted this revision.Aug 25 2020, 9:33 PM

I'm approving this as an experimental quality feature. I think the technical direction is sound and the splitting of requirements corrects some long-standing soundness issues and will lead to a better end-user experience for shared repositories.

Before the experimental label is ripped off, I think we should consider:

  • Whether store requirement belongs in .hg/requires or .hg/store/requires. It's weird for it to be in .hg/store since that requirement essentially says files are in store/.
  • The name of the requirement having share in it still doesn't sit well with me. The thing the requirement is saying is that additional requirements exist in store/requires. A side-effect of that is that repository store sharing is more robust. I'd rather the .hg/requires file contain storerequires. If .hg/requires is using a shared store, it can have sharedstorerequires to indicate that special variant. But we could also use storerequires in this case as well: it all depends how specific we want to be with requires entries! Another way of looking at this is that it is weird an unshared repository has a requirement with share in it. That's a good indicator the naming is off!
mercurial/localrepo.py
535

As written, this code will accept the share safe requirement without the store requirement. The store requirement should be mandatory and the share safe requirement without store is nonsensical.

It would be nice if there were code somewhere that rejected this invalid combination of requirements. But the chances of this happening should be small and it is probably OK to live without it in the repo opening code. However, we should look for it when writing the requirement, since it would be possible to coerce config options to produce such a repository.

540

I'm not sure cacheaudited provides any benefit for a single file read. But it should be harmless since we throw away the vfs instance after a single use.

3685

I'm on the fence about whether we should drop or abort here. Either way is probably fine.

3805

sharedrepo wants to be converted to a constant.

indygreg added a comment.Aug 25 2020, 9:48 PM

I was going to push this but there's a test failure:

--- /home/gps/src/hg-committed/tests/test-remotefilelog-share.t
+++ /home/gps/src/hg-committed/tests/test-remotefilelog-share.t#safe.err
@@ -28,6 +28,7 @@


   $ hgcloneshallow ssh://user@dummy/master source --noupdate -q
+  devel-warn: write with no lock: "requires" at: /home/gps/src/hg-committed/mercurial/scmutil.py:1505 (writerequires)
   $ hg share source dest
   updating working directory
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
marmoute accepted this revision.Aug 26 2020, 3:16 AM
In D8633#133933, @indygreg wrote:

I'm approving this as an experimental quality feature. I think the technical direction is sound and the splitting of requirements corrects some long-standing soundness issues and will lead to a better end-user experience for shared repositories.
Before the experimental label is ripped off, I think we should consider:

  • Whether store requirement belongs in .hg/requires or .hg/store/requires. It's weird for it to be in .hg/store since that requirement essentially says files are in store/.

the store requirements is clearly something that needs to be shared between the source repo and the potential shares, so this argue for having it in .hg/store/requires, however as you point out if we are reading .hg/store/requirements we obviously already aware that we have a store. Maybe sharesafe should imply store? Or maybe that special case is not special enough and we can live with the funny inconsistency?

  • The name of the requirement having share in it still doesn't sit well with me. The thing the requirement is saying is that additional requirements exist in store/requires.

The requirements also change the configuration reading logic so that the configuration from the source repository is also read by the shares. So the sharesafe requirement is really about fixing various issue that were making repository unsafe to share. And use multiple behavior/layout change to achieve this.

Another way of looking at this is that it is weird an unshared repository has a requirement with share in it. That's a good indicator the naming is off!

I really don't understand your reasoning here. as I pointed before they are countless example of such naming in real life. For example, waterproof item does not imply they are (or will) submerged, fireproof item does not imply they are (or will) be on fire and military grade hardware does not requires you to be in the military to use them. So having a repository to be "share safe" does not requires it to be shared to me. Can you elaborate on why you find that naming suspicious ?

This revision is now accepted and ready to land.Aug 26 2020, 3:16 AM
pulkit marked 2 inline comments as done.Aug 26 2020, 4:44 AM
pulkit added inline comments.
mercurial/localrepo.py
535

The SHARESAFE_REQUIREMENT implies that store is present. There are two ways SHARESAFE_REQUIREMENT can be introduced:

  1. Creating new repos with config option enabled
  2. Using upgrade

For 1, we check that store is present. Refer checkrequirementscompat() a bit down in this file.

2 is not yet implemented and we will add the check there too when implemented.

So until the user is not manually editing the requires file, we can be sure that if SHARESAFE_REQUIREMENT is present, store will be present.

However, we should look for it when writing the requirement, since it would be possible to
coerce config options to produce such a repository.

Definitely, this patch has a test case for that in test-share.t.

540

Seems like copy-paste leftover, removed it.

3685

I am bit more inclined on aborting here but realized that this whole needs a separate discussion as unfortunately we didn't have such handling yet.

3805

This is bit messy here. sharedrepo here is createopts key rather than a requirement name.

pulkit marked 2 inline comments as done.Aug 26 2020, 6:05 AM
pulkit updated this revision to Diff 22450.
pulkit added a comment.Aug 26 2020, 6:06 AM
In D8633#133939, @indygreg wrote:

I was going to push this but there's a test failure:

--- /home/gps/src/hg-committed/tests/test-remotefilelog-share.t
+++ /home/gps/src/hg-committed/tests/test-remotefilelog-share.t#safe.err
@@ -28,6 +28,7 @@
   $ hgcloneshallow ssh://user@dummy/master source --noupdate -q
+  devel-warn: write with no lock: "requires" at: /home/gps/src/hg-committed/mercurial/scmutil.py:1505 (writerequires)
   $ hg share source dest
   updating working directory
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved

Oops, turns out shallow-cloning in remotefilelog does not acquire a lock. I added D8952 in start of series to acquire lock while writing requires in shallow-cloning in remotefilelog.

pulkit updated this revision to Diff 22654.Sep 16 2020, 7:56 AM
indygreg accepted this revision.Sep 17 2020, 9:13 PM

Revision Contents
Changeset List

Diff 21952

mercurial/configitems.py

b'exp-use-copies-side-data-changeset', b'exp-use-copies-side-data-changeset',
default=False, default=False,
experimental=True, experimental=True,
) )
coreconfigitem( coreconfigitem(
b'format', b'exp-use-side-data', default=False, experimental=True, b'format', b'exp-use-side-data', default=False, experimental=True,
) )
coreconfigitem( coreconfigitem(
b'format', b'exp-share-safe', default=False, experimental=True,
)
coreconfigitem(
b'format', b'internal-phase', default=False, experimental=True, b'format', b'internal-phase', default=False, experimental=True,
) )
coreconfigitem( coreconfigitem(
b'fsmonitor', b'warn_when_unused', default=True, b'fsmonitor', b'warn_when_unused', default=True,
) )
coreconfigitem( coreconfigitem(
b'fsmonitor', b'warn_update_file_count', default=50000, b'fsmonitor', b'warn_update_file_count', default=50000,
) )

mercurial/localrepo.py

# A repository with the the copies-sidedata-changeset requirement will store # A repository with the the copies-sidedata-changeset requirement will store
# copies related information in changeset's sidedata. # copies related information in changeset's sidedata.
COPIESSDC_REQUIREMENT = b'exp-copies-sidedata-changeset' COPIESSDC_REQUIREMENT = b'exp-copies-sidedata-changeset'
# The repository use persistent nodemap for the changelog and the manifest. # The repository use persistent nodemap for the changelog and the manifest.
NODEMAP_REQUIREMENT = b'persistent-nodemap' NODEMAP_REQUIREMENT = b'persistent-nodemap'
# A repository with share implemented safely. The repository has different
# store and working copy requirements i.e. both `.hg/requires` and
# `.hg/store/requires` are present.
SHARESAFE_REQUIREMENT = b'exp-sharesafe'
marmouteUnsubmitted
Done

Until the semantic is fully implemented, this should be exp-sharesafe otherwise a non complying client could accessing without respecting the semantic.

marmoute: Until the semantic is fully implemented, this should be `exp-sharesafe` otherwise a non…
pulkitAuthorUnsubmitted
Done

Done.

pulkit: Done.
# Functions receiving (ui, features) that extensions can register to impact # Functions receiving (ui, features) that extensions can register to impact
# the ability to load repositories with custom requirements. Only # the ability to load repositories with custom requirements. Only
# functions defined in loaded extensions are called. # functions defined in loaded extensions are called.
# #
# The function receives a set of requirement strings that the repository # The function receives a set of requirement strings that the repository
# is capable of opening. Functions will typically add elements to the # is capable of opening. Functions will typically add elements to the
# set to reflect that the extension knows how to handle that requirements. # set to reflect that the extension knows how to handle that requirements.
featuresetupfuncs = set() featuresetupfuncs = set()
# the repository. This file was introduced in Mercurial 0.9.2, # the repository. This file was introduced in Mercurial 0.9.2,
# which means very old repositories may not have one. We assume # which means very old repositories may not have one. We assume
# a missing file translates to no requirements. # a missing file translates to no requirements.
try: try:
requirements = set(hgvfs.read(b'requires').splitlines()) requirements = set(hgvfs.read(b'requires').splitlines())
except IOError as e: except IOError as e:
if e.errno != errno.ENOENT: if e.errno != errno.ENOENT:
raise raise
requirements = set() requirements = set()
indygregUnsubmitted
Done

As written, this code will accept the share safe requirement without the store requirement. The store requirement should be mandatory and the share safe requirement without store is nonsensical.

It would be nice if there were code somewhere that rejected this invalid combination of requirements. But the chances of this happening should be small and it is probably OK to live without it in the repo opening code. However, we should look for it when writing the requirement, since it would be possible to coerce config options to produce such a repository.

indygreg: As written, this code will accept the share safe requirement without the `store` requirement.
pulkitAuthorUnsubmitted
Done

The SHARESAFE_REQUIREMENT implies that store is present. There are two ways SHARESAFE_REQUIREMENT can be introduced:

  1. Creating new repos with config option enabled
  2. Using upgrade

For 1, we check that store is present. Refer checkrequirementscompat() a bit down in this file.

2 is not yet implemented and we will add the check there too when implemented.

So until the user is not manually editing the requires file, we can be sure that if SHARESAFE_REQUIREMENT is present, store will be present.

However, we should look for it when writing the requirement, since it would be possible to
coerce config options to produce such a repository.

Definitely, this patch has a test case for that in test-share.t.

pulkit: The `SHARESAFE_REQUIREMENT` implies that `store` is present. There are two ways…
# if .hg/requires contains the sharesafe requirement, it means
# there exists a `.hg/store/requires` too and we should read it
if SHARESAFE_REQUIREMENT in requirements:
if hgvfs.exists(b'sharedpath'):
indygregUnsubmitted
Done

I'm not sure cacheaudited provides any benefit for a single file read. But it should be harmless since we throw away the vfs instance after a single use.

indygreg: I'm not sure `cacheaudited` provides any benefit for a single file read. But it should be…
pulkitAuthorUnsubmitted
Done

Seems like copy-paste leftover, removed it.

pulkit: Seems like copy-paste leftover, removed it.
# This is a shared repo
sharedpath = hgvfs.read(b'sharedpath').rstrip(b'\n')
if b'relshared' in requirements:
sharedpath = hgvfs.join(sharedpath)
storevfs = vfsmod.vfs(vfsmod.vfs(sharedpath).join(b'store'))
indygregUnsubmitted
Done

I'm reasonably certain this logic is duplicated from somewhere. And opening shared repos is quirky enough that I'd feel better if there was a helper function for resolving the store vfs given a vfs for a .hg directory.

indygreg: I'm reasonably certain this logic is duplicated from somewhere. And opening shared repos is…
marmouteUnsubmitted
Done

I had the same comment, and it turns out @pulkit is cleaning the duplication in later patches.

marmoute: I had the same comment, and it turns out @pulkit is cleaning the duplication in later patches.
else:
storevfs = vfsmod.vfs(hgvfs.join(b'store'), cacheaudited=True)
try:
store_requirements = set(storevfs.read(b'requires').splitlines())
requirements |= store_requirements
except IOError as e:
if e.errno != errno.ENOENT:
raise
indygregUnsubmitted
Not Done

The swalloing of ENOENT doesn't sit well with me.

The repo opening semantics are such that .hg/requires is read first and all subsequent repo opening is dictated by what is found inside that file.

I think that a .hg/store/requires file presence should be directly indicated via an entry in .hg/requires - say store-requires - and if that entry is present, the file must be present: a missing file would be an error.

So I think the request here would be to rename the requirement to [exp-]store-requires instead of [exp-]share-safe. If we wanted to add additional functionality governing how shared repos are opened, we should consider a separate requirement for that, independent of the one that indicates if a .hg/store/requires exists.

Put another way, we want repo opening semantics to be strictly defined with little room for interpretation. And we want requires entries to be narrowly scoped so there isn't ambiguity about what they mean.

indygreg: The swalloing of `ENOENT` doesn't sit well with me. The repo opening semantics are such that `.
marmouteUnsubmitted
Not Done

The swalloing of ENOENT doesn't sit well with me.

The repo opening semantics are such that .hg/requires is read first and all subsequent repo opening is dictated by what is found inside that file.

I think that a .hg/store/requires file presence should be directly indicated via an entry in .hg/requires - say store-requires - and if that entry is present, the file must be present: a missing file would be an error.

+1 for having some explicit error. If we go the route of using a wc specific files, we should be strict on having it too.

I don't have a strong opinion on having the requires file in the .hg/store/ directory yet. It seems cleaner/clearer to do so.

marmoute: >The swalloing of ENOENT doesn't sit well with me. > The repo opening semantics are such that .
marmouteUnsubmitted
Not Done

So I think the request here would be to rename the requirement to [exp-]store-requires instead of [exp-]share-safe. If we wanted to add additional functionality governing how shared repos are opened, we should consider a separate requirement for that, independent of the one that indicates if a .hg/store/requires exists.

Having a galaxy on interdependent requirements can become a nightmare, because we either have to start dealing with all the corner case of some being enabled without the other or to add extra logic to make sure they are not being used without the other (and error out on bad configuration). So having simple requirement adding a consistent improvement seems like a better idea.

The share-safe requirement comes with other semantic beside the store one, for example regarding the reading of the configuration. So store-requires would be too narrow.

marmoute: > So I think the request here would be to rename the requirement to `[exp-]store-requires`…
pulkitAuthorUnsubmitted
Done

Fixed the swallowing of ENOENT.

About a fine grained store-requires requirement, I am not sure what's the best way out.

Right now exp-share-safe implies two things, requirements in store and source .hgrc be shared. We definitely can have dedicated store-requires but that introduces a spider of inter-dependent requirements.

Does exp-share-safe implies store-requires or need it? etc.

For now, I have not changed anything on this front in this patch.

pulkit: Fixed the swallowing of ENOENT. About a fine grained `store-requires` requirement, I am not…
marmouteUnsubmitted
Done

This looks like a lot of duplicated code, am I missing something ? Could we do less duplication?

marmoute: This looks like a lot of duplicated code, am I missing something ? Could we do less duplication?
marmouteUnsubmitted
Done

I see this is done in D8655 thanks !

marmoute: I see this is done in D8655 thanks !
pulkitAuthorUnsubmitted
Done

Refactored in next patch.

pulkit: Refactored in next patch.
# The .hg/hgrc file may load extensions or contain config options # The .hg/hgrc file may load extensions or contain config options
# that influence repository construction. Attempt to load it and # that influence repository construction. Attempt to load it and
# process any new extensions that it may have pulled in. # process any new extensions that it may have pulled in.
if loadhgrc(ui, wdirvfs, hgvfs, requirements): if loadhgrc(ui, wdirvfs, hgvfs, requirements):
afterhgrcload(ui, wdirvfs, hgvfs, requirements) afterhgrcload(ui, wdirvfs, hgvfs, requirements)
extensions.loadall(ui) extensions.loadall(ui)
extensions.populateui(ui) extensions.populateui(ui)
b'generaldelta', b'generaldelta',
b'treemanifest', b'treemanifest',
COPIESSDC_REQUIREMENT, COPIESSDC_REQUIREMENT,
REVLOGV2_REQUIREMENT, REVLOGV2_REQUIREMENT,
SIDEDATA_REQUIREMENT, SIDEDATA_REQUIREMENT,
SPARSEREVLOG_REQUIREMENT, SPARSEREVLOG_REQUIREMENT,
NODEMAP_REQUIREMENT, NODEMAP_REQUIREMENT,
bookmarks.BOOKMARKS_IN_STORE_REQUIREMENT, bookmarks.BOOKMARKS_IN_STORE_REQUIREMENT,
SHARESAFE_REQUIREMENT,
} }
_basesupported = supportedformats | { _basesupported = supportedformats | {
b'store', b'store',
b'fncache', b'fncache',
b'shared', b'shared',
b'relshared', b'relshared',
b'dotencode', b'dotencode',
b'exp-sparse', b'exp-sparse',
requirements.add(b'lfs') requirements.add(b'lfs')
if ui.configbool(b'format', b'bookmarks-in-store'): if ui.configbool(b'format', b'bookmarks-in-store'):
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)
# if share-safe is enabled, let's create the new repository with the new
# requirement
if ui.configbool(b'format', b'exp-share-safe'):
requirements.add(SHARESAFE_REQUIREMENT)
indygregUnsubmitted
Not Done

It's really weird that this requirement has share in its name but it isn't dependent on being a shared repo.

indygreg: It's really weird that this requirement has `share` in its name but it isn't dependent on being…
marmouteUnsubmitted
Not Done

The requirements is named "share-safe" because after this change, the repository is safe to be shared. A repository do not need to be shared to "share-safe" the same way a watch does not need to be in the submerged to be water-proof.

marmoute: The requirements is named "share-safe" because after this change, the repository is safe to be…
pulkitAuthorUnsubmitted
Done

What @marmoute said, the requirement is named share-safe because after this change, the repository is safe to be shared.

pulkit: What @marmoute said, the requirement is named `share-safe` because after this change, the…
return requirements return requirements
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
be allowed, as it means there was a request to create a repository be allowed, as it means there was a request to create a repository
indygregUnsubmitted
Not Done

I'm on the fence about whether we should drop or abort here. Either way is probably fine.

indygreg: I'm on the fence about whether we should drop or abort here. Either way is probably fine.
pulkitAuthorUnsubmitted
Done

I am bit more inclined on aborting here but realized that this whole needs a separate discussion as unfortunately we didn't have such handling yet.

pulkit: I am bit more inclined on aborting here but realized that this whole needs a separate…
with options not recognized by loaded code. with options not recognized by loaded code.
Extensions can wrap this function to filter out creation options Extensions can wrap this function to filter out creation options
they know how to handle. they know how to handle.
""" """
known = { known = {
b'backend', b'backend',
b'lfs', b'lfs',
# The revlog header has version 2, which won't be recognized by # The revlog header has version 2, which won't be recognized by
# such old clients. # such old clients.
hgvfs.append( hgvfs.append(
b'00changelog.i', b'00changelog.i',
b'\0\0\0\2 dummy changelog to prevent using the old repo ' b'\0\0\0\2 dummy changelog to prevent using the old repo '
b'layout', b'layout',
) )
if SHARESAFE_REQUIREMENT in requirements and b'store' in requirements:
if b'sharedrepo' in createopts:
req = set([b'shared', SHARESAFE_REQUIREMENT])
if b'relshared' in requirements:
req.add(b'relshared')
scmutil.writerequires(hgvfs, req)
else:
scmutil.writerequires(hgvfs, set([SHARESAFE_REQUIREMENT]))
storevfs = vfsmod.vfs(hgvfs.join(b'store'), cacheaudited=True)
indygregUnsubmitted
Not Done

sharedrepo wants to be converted to a constant.

indygreg: `sharedrepo` wants to be converted to a constant.
pulkitAuthorUnsubmitted
Done

This is bit messy here. sharedrepo here is createopts key rather than a requirement name.

pulkit: This is bit messy here. `sharedrepo` here is `createopts` key rather than a requirement name.
scmutil.writerequires(
storevfs, requirements - set([SHARESAFE_REQUIREMENT])
)
indygregUnsubmitted
Not Done

This whole block needs more comments because what it is trying to accomplish is important and nuanced.

I would also consider rewriting the code so that we populate variables indicating the set of requirements to populate in .hg/requires and .hg/store/requires. That way we have at most 2 calls to writerequires(), which I think will improve readability.

indygreg: This whole block needs more comments because what it is trying to accomplish is important and…
indygregUnsubmitted
Not Done

What this is doing is blindly moving all requirements except SHARESAFE_REQUIREMENT to .hg/store/requires. The movement of most requirements there makes sense because they govern the "store." However, there are requirements like exp-sparse which I believe only concern the working directory and should not be in the "store."

If we want to go the approach of moving store-specific requirements into .hg/store/requires, I think we need to annotate whether requirements belong to the store or the non-store and write/move requirements accordingly.

indygreg: What this is doing is blindly moving all requirements except `SHARESAFE_REQUIREMENT` to `.
marmouteUnsubmitted
Not Done

Defaulting to "requirements is a store requirement" and explicitly flagging requirement that are working copy specific seems the safest.

marmoute: Defaulting to "requirements is a store requirement" and explicitly flagging requirement that…
pulkitAuthorUnsubmitted
Done

The whole code block is now much simpler, thanks to filterrequirements introduced in previous patches.

pulkit: The whole code block is now much simpler, thanks to `filterrequirements` introduced in previous…
else:
scmutil.writerequires(hgvfs, requirements) scmutil.writerequires(hgvfs, requirements)
marmouteUnsubmitted
Done

It looks like share-safe should requires store, can we make them dependent of each other.

marmoute: It looks like `share-safe` should requires `store`, can we make them dependent of each other.
pulkitAuthorUnsubmitted
Done

Does not share extension implies that store is present?

pulkit: Does not `share` extension implies that `store` is present?
marmouteUnsubmitted
Done

Maybe, can you check ?

In all cases, I would rather make the safe-share requirement consistent from the start. Can we have some code either setup store automatically, or reject with some error (whichever solution you prefer).

marmoute: Maybe, can you check ? In all cases, I would rather make the safe-share requirement consistent…
# Write out file telling readers where to find the shared store. # Write out file telling readers where to find the shared store.
if b'sharedrepo' in createopts: if b'sharedrepo' in createopts:
hgvfs.write(b'sharedpath', sharedpath) hgvfs.write(b'sharedpath', sharedpath)
if createopts.get(b'shareditems'): if createopts.get(b'shareditems'):
shared = b'\n'.join(sorted(createopts[b'shareditems'])) + b'\n' shared = b'\n'.join(sorted(createopts[b'shareditems'])) + b'\n'
hgvfs.write(b'shared', shared) hgvfs.write(b'shared', shared)

mercurial/scmutil.py

ds.copy(src, dst) ds.copy(src, dst)
repo._quick_access_changeid_invalidate() repo._quick_access_changeid_invalidate()
def writereporequirements(repo, requirements=None): def writereporequirements(repo, requirements=None):
""" writes requirements for the repo to .hg/requires """ """ writes requirements for the repo to .hg/requires """
if requirements: if requirements:
repo.requirements = requirements repo.requirements = requirements
if b'exp-sharesafe' in repo.requirements:
indygregUnsubmitted
Done

The presence of this conditional here feels a bit weird to me: I feel like it might be better if we explicitly passed in 2 sets of requirements to read and this function were "dumber."

indygreg: The presence of this conditional here feels a bit weird to me: I feel like it might be better…
pulkitAuthorUnsubmitted
Done

This code is much cleaner now after introducing filterrequirements in previous patches.

pulkit: This code is much cleaner now after introducing `filterrequirements` in previous patches.
with repo.lock():
indygregUnsubmitted
Not Done

Writing of the requirements file should be extremely rare. I don't think we need this lock here.

indygreg: Writing of the requirements file should be extremely rare. I don't think we need this lock here.
marmouteUnsubmitted
Not Done

I agreed that the requirements should be written at repository creation time (or upgrade time) and that is all. Maybe we should update the acquiring of the lock with some code checking we are actually holding a lock while doing this?

The upgrade usecase is a good argument in favor of moving the requirement file inside the store directory.

marmoute: I agreed that the requirements should be written at repository creation time (or upgrade time)…
writerequires(repo.svfs, repo.requirements)
writerequires(repo.vfs, set([b'exp-sharesafe']))
else:
writerequires(repo.vfs, repo.requirements) writerequires(repo.vfs, repo.requirements)
def writerequires(opener, requirements): def writerequires(opener, requirements):
with opener(b'requires', b'w', atomictemp=True) as fp: with opener(b'requires', b'w', atomictemp=True) as fp:
for r in sorted(requirements): for r in sorted(requirements):
fp.write(b"%s\n" % r) fp.write(b"%s\n" % r)

mercurial/store.py

mode = None mode = None
except OSError: except OSError:
mode = None mode = None
return mode return mode
_data = ( _data = (
b'bookmarks narrowspec data meta 00manifest.d 00manifest.i' b'bookmarks narrowspec data meta 00manifest.d 00manifest.i'
b' 00changelog.d 00changelog.i phaseroots obsstore' b' 00changelog.d 00changelog.i phaseroots obsstore requires'
indygregUnsubmitted
Done

Bonus points if you refactor this to be a proper data structure as part of this series. A space delimited list/set makes no sense to me.

indygreg: Bonus points if you refactor this to be a proper data structure as part of this series. A space…
marmouteUnsubmitted
Done

I can add an extra bonus point to the stake :-)

marmoute: I can add an extra bonus point to the stake :-)
) )
def isrevlog(f, kind, st): def isrevlog(f, kind, st):
return kind == stat.S_IFREG and f[-2:] in (b'.i', b'.d') return kind == stat.S_IFREG and f[-2:] in (b'.i', b'.d')
class basicstore(object): class basicstore(object):
''' '''
# yield data files first # yield data files first
for x in self.datafiles(matcher): for x in self.datafiles(matcher):
yield x yield x
for x in self.topfiles(): for x in self.topfiles():
yield x yield x
def copylist(self): def copylist(self):
return [b'requires'] + _data.split() return _data.split()
indygregUnsubmitted
Done

Wait - why was requires here before?

indygreg: Wait - why was `requires` here before?
pulkitAuthorUnsubmitted
Done

A line of code dating back to 2008. Removing it does not make any tests fail. Will send a patch for it separately.

pulkit: A line of code dating back to 2008. Removing it does not make any tests fail. Will send a patch…
def write(self, tr): def write(self, tr):
pass pass
def invalidatecaches(self): def invalidatecaches(self):
pass pass
def markremoved(self, fn): def markremoved(self, fn):
yield f, ef, self.getsize(ef) yield f, ef, self.getsize(ef)
except OSError as err: except OSError as err:
if err.errno != errno.ENOENT: if err.errno != errno.ENOENT:
raise raise
def copylist(self): def copylist(self):
d = ( d = (
b'bookmarks narrowspec data meta dh fncache phaseroots obsstore' b'bookmarks narrowspec data meta dh fncache phaseroots obsstore'
b' 00manifest.d 00manifest.i 00changelog.d 00changelog.i' b' 00manifest.d 00manifest.i 00changelog.d 00changelog.i requires'
) )
return [b'requires', b'00changelog.i'] + [ return [b'requires', b'00changelog.i'] + [
b'store/' + f for f in d.split() b'store/' + f for f in d.split()
] ]
def write(self, tr): def write(self, tr):
self.fncache.write(tr) self.fncache.write(tr)

tests/test-journal-share.t

#testcases safe normal
#if safe
$ echo "[format]" >> $HGRCPATH
$ echo "exp-share-safe = True" >> $HGRCPATH
#endif
Journal extension test: tests the share extension support Journal extension test: tests the share extension support
$ cat >> testmocks.py << EOF $ cat >> testmocks.py << EOF
> # mock out procutil.getuser() and util.makedate() to supply testable values > # mock out procutil.getuser() and util.makedate() to supply testable values
> import os > import os
> from mercurial import util > from mercurial import util
> from mercurial.utils import procutil > from mercurial.utils import procutil
> def mockgetuser(): > def mockgetuser():

tests/test-narrow-share.t

#testcases flat tree #testcases flat tree
#testcases safe normal
#if safe
$ echo "[format]" >> $HGRCPATH
$ echo "exp-share-safe = True" >> $HGRCPATH
#endif
$ . "$TESTDIR/narrow-library.sh" $ . "$TESTDIR/narrow-library.sh"
#if tree #if tree
$ cat << EOF >> $HGRCPATH $ cat << EOF >> $HGRCPATH
> [experimental] > [experimental]
> treemanifest = 1 > treemanifest = 1
> EOF > EOF

tests/test-remotefilelog-share.t

#require no-windows #require no-windows
#testcases safe normal
#if safe
$ echo "[format]" >> $HGRCPATH
$ echo "exp-share-safe = True" >> $HGRCPATH
#endif
$ . "$TESTDIR/remotefilelog-library.sh" $ . "$TESTDIR/remotefilelog-library.sh"
$ cat >> $HGRCPATH <<EOF $ cat >> $HGRCPATH <<EOF
> [extensions] > [extensions]
> remotefilelog= > remotefilelog=
> share= > share=
> EOF > EOF

tests/test-share-bookmarks.t

#testcases vfs svfs #testcases vfs svfs
#testcases safe normal
#if safe
$ echo "[format]" >> $HGRCPATH
$ echo "exp-share-safe = True" >> $HGRCPATH
#endif
$ echo "[extensions]" >> $HGRCPATH $ echo "[extensions]" >> $HGRCPATH
$ echo "share = " >> $HGRCPATH $ echo "share = " >> $HGRCPATH
#if svfs #if svfs
$ echo "[format]" >> $HGRCPATH $ echo "[format]" >> $HGRCPATH
$ echo "bookmarks-in-store = yes " >> $HGRCPATH $ echo "bookmarks-in-store = yes " >> $HGRCPATH
#endif #endif

tests/test-share-safe.t

  • This file was added.
setup
$ cat >> $HGRCPATH <<EOF
> [extensions]
> share =
> [format]
> exp-share-safe = True
> EOF
prepare source repo
$ hg init source
$ cd source
$ cat .hg/requires
exp-sharesafe
$ cat .hg/store/requires
dotencode
fncache
generaldelta
revlogv1
sparserevlog
store
$ hg debugrequirements
dotencode
exp-sharesafe
fncache
generaldelta
revlogv1
sparserevlog
store
$ echo a > a
$ hg ci -Aqm "added a"
$ echo b > b
$ hg ci -Aqm "added b"
$ cd ..
Create a shared repo and check the requirements are shared and read correctly
$ hg share source shared1
updating working directory
2 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ cd shared1
$ cat .hg/requires
exp-sharesafe
shared
$ hg debugrequirements -R ../source
dotencode
exp-sharesafe
fncache
generaldelta
revlogv1
sparserevlog
store
$ hg debugrequirements
dotencode
exp-sharesafe
fncache
generaldelta
revlogv1
shared
sparserevlog
store
$ echo c > c
$ hg ci -Aqm "added c"
$ hg unshare

tests/test-share.t

#testcases safe normal
#if safe
$ echo "[format]" >> $HGRCPATH
$ echo "exp-share-safe = True" >> $HGRCPATH
#endif
$ echo "[extensions]" >> $HGRCPATH $ echo "[extensions]" >> $HGRCPATH
$ echo "share = " >> $HGRCPATH $ echo "share = " >> $HGRCPATH
prepare repo1 prepare repo1
$ hg init repo1 $ hg init repo1
$ cd repo1 $ cd repo1
$ echo a > a $ echo a > a