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

[indygreg (Gregory Szorc)]

indygreg added a comment.
indygreg accepted this revision.


  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!

INLINE COMMENTS

> localrepo.py:555
> +    # there exists a `.hg/store/requires` too and we should read it
> +    if requirementsmod.SHARESAFE_REQUIREMENT in requirements:
> +        if shared:

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.

> localrepo.py:560
> +        else:
> +            storevfs = vfsmod.vfs(hgvfs.join(b'store'), cacheaudited=True)
> +

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.

> localrepo.py:3386
> +            )
> +            dropped.add(requirementsmod.SHARESAFE_REQUIREMENT)
> +

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

> localrepo.py:3520
> +    # requirements as they are already present in store requires
> +    if storereq and b'sharedrepo' not in createopts:
> +        scmutil.writerequires(hgvfs, wcreq)

`sharedrepo` wants to be converted to a constant.

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