D6876: phabricator: support automatically obsoleting old revisions of pulled commits

marmoute (Pierre-Yves David) phabricator at mercurial-scm.org
Mon Oct 7 20:43:05 EDT 2019


marmoute added a comment.


  The feature seens pretty usful, but is also a potential foot-gun/data-loss engine. I think it is useful to take the feature, but maybe with proper documentaiton warning and turned of by efaut. I made a couple of comment about the implementation.

INLINE COMMENTS

> phabricator.py:187
> +    if m:
> +        return b"D%s" % m.group(r'id')
> +

So, this only checks the diff number, right ? So in theory, we could obsolete a later version of this, silently dropping change (instead of detecting potential phase-divergence)

> phabricator.py:192
> + at eh.wrapcommand(b"pull")
> +def _pull(orig, ui, repo, *args, **opts):
> +    if not (obsolete.isenabled(repo, obsolete.createmarkersopt)

It would probably make sense to wrap the pull exchange logic instead. It would catch more instance and could reuse the same transaction.

> phabricator.py:216
> +    unfiltered = repo.unfiltered()
> +    for rev in unfiltered.revs("draft() - obsolete()"):
> +        n = unfiltered[rev]

This should be `non public()` instead of `draft()` these could be draft.

> phabricator.py:225
> +
> +    with unfiltered.lock(), unfiltered.transaction('phabpullcreatemarkers'):
> +        obsolete.createmarkers(unfiltered, tocreate)

We should do the computation before locking to avoid other process updating the repo under our feet.

REPOSITORY
  rHG Mercurial

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

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

To: mharbison72, #hg-reviewers
Cc: pulkit, sheehan, Kwan, mercurial-devel, marmoute


More information about the Mercurial-devel mailing list