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

[pulkit (Pulkit Goyal)]

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

INLINE COMMENTS

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

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

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

Seems like copy-paste leftover, removed it.

> indygreg wrote in localrepo.py:3386
> I'm on the fence about whether we should drop or abort here. Either way is probably fine.

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.

> indygreg wrote in localrepo.py:3520
> `sharedrepo` wants to be converted to a constant.

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

REPOSITORY
  rHG Mercurial

BRANCH
  default

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, indygreg
Cc: indygreg, marmoute, mercurial-patches
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-patches/attachments/20200826/89841310/attachment-0002.html>