[Commented On] D9679: sharesafe: introduce functionality to automatically upgrade shares

[marmoute (Pierre-Yves David)]

marmoute added a comment.


  (I just found out I had unsubmitted comments…)

INLINE COMMENTS

> pulkit wrote in configitems.py:1077-1078
> Sure, that sounds good.

Lets add a comment with the planned final name next to the config declaration then.

> upgrade.py:284-286
> +    loading. It may also fails to upgrade if we fails to get a lock. Hence
> +    we need to update the caller about new set of repository requirements.
> +    Therefore returns the set of new repository requirement.

This part is not very clear. Do you mean we can silently (from the code perspective) fail to upgrade ? And that depending of if this works or not, we use a different set of requirements for the share ?

> upgrade.py:289-290
> +    wlock = None
> +    try:
> +        wlock = lockmod.trylock(ui, hgvfs, b'wlock', 0, 0)
> +        store_requirements = localrepo._readrequires(storevfs, False)

What about we use try/else construct to restrict the try block to one actually trying to grab the lock ?

  try:
              wlock = lockmod.trylock(ui, hgvfs, b'wlock', 0, 0)
  except LockError as e:
      …
  else:
      …
  finally:
      …

REPOSITORY
  rHG Mercurial

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

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

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