[Commented On] D8633: share: introduce config option to store requires in .hg/store

Fri Aug 7 08:40:04 UTC 2020

marmoute added inline comments.

INLINE COMMENTS

> indygreg wrote in 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.

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

> indygreg wrote in localrepo.py: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.

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

> indygreg wrote in localrepo.py: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.

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

> indygreg wrote in localrepo.py:3342
> It's really weird that this requirement has `share` in its name but it isn't dependent on being a shared repo.

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.

> indygreg wrote in localrepo.py:3518
> 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.

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

> indygreg wrote in scmutil.py:1479
> Writing of the requirements file should be extremely rare. I don't think we need this lock here.

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.

> indygreg wrote in 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.

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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8633/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8633

To: pulkit, #hg-reviewers, durin42, marmoute
Cc: indygreg, marmoute, mercurial-patches
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-patches/attachments/20200807/e1c6e30f/attachment-0002.html>