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

[pulkit (Pulkit Goyal)]

pulkit added a comment.
pulkit marked 2 inline comments as done.


  In D8633#133201 <https://phab.mercurial-scm.org/D8633#133201>, @pulkit wrote:
  
  > In D8633#133117 <https://phab.mercurial-scm.org/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.

INLINE COMMENTS

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

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.

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

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

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

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

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

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

> indygreg wrote in store.py:450
> Wait - why was `requires` here before?

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

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/20200811/cbe9e359/attachment-0002.html>