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

[indygreg (Gregory Szorc)]

indygreg added a comment.


  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.

INLINE COMMENTS

> localrepo.py:545
> +                sharedpath = hgvfs.join(sharedpath)
> +            storevfs = vfsmod.vfs(vfsmod.vfs(sharedpath).join(b'store'))
> +        else:

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.

> localrepo.py:553
> +            if e.errno != errno.ENOENT:
> +                raise
> +

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.

> localrepo.py:3342
> +    if ui.configbool(b'format', b'exp-share-safe'):
> +        requirements.add(SHARESAFE_REQUIREMENT)
> +

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

> localrepo.py:3518
> +                storevfs, requirements - set([SHARESAFE_REQUIREMENT])
> +            )
> +    else:

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.

> localrepo.py:3518
> +                storevfs, requirements - set([SHARESAFE_REQUIREMENT])
> +            )
> +    else:

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.

> scmutil.py:1478
> +
> +    if b'exp-sharesafe' in repo.requirements:
> +        with repo.lock():

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

> scmutil.py:1479
> +    if b'exp-sharesafe' in repo.requirements:
> +        with repo.lock():
> +            writerequires(repo.svfs, repo.requirements)

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

> store.py:378
>      b'bookmarks narrowspec data meta 00manifest.d 00manifest.i'
> -    b' 00changelog.d 00changelog.i phaseroots obsstore'
> +    b' 00changelog.d 00changelog.i phaseroots obsstore requires'
>  )

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.

> store.py:450
>      def copylist(self):
> -        return [b'requires'] + _data.split()
>  

Wait - why was `requires` here before?

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