( )⚙ D6876 phabricator: support automatically obsoleting old revisions of pulled commits

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6876

phabricator: support automatically obsoleting old revisions of pulled commits
Changes PlannedPublic

Authored by mharbison72 on Sep 25 2019, 1:17 AM.

Details

Reviewers
baymax
Group Reviewers
hg-reviewers
Summary

This is basically an import of the pullcreatemarkers extension[1] from the FB
repo, with minor adjustments to getmatchingdiff() to work with modern hg.
Since this is very phabricator specific, it makes more sense to me to bundle it
into the existing extension. It wasn't very obvious from the old name what
functionality was provided, and it may make sense to do this in other scenarios
besides hg pull.

There are two use cases that I can see- first, ensuring that old revisions are
cleaned up for a contributor (I seem to recall something I submitted recently
needed to be explicitly pruned, though most submissions do clean up
automatically). Second, any hg phabread | hg import - would otherwise need to
be manually cleaned up. The latter is annoying enough that I tend not to grab
the code and try it when reviewing.

It is currently guarded by a config option (off by default), because @marmoute
expressed concerns about duplicate marker creation if the pushing reviewer also
creates a marker. I don't think that's possible here, since the obsolete
revisions are explicitly excluded. But maybe there are other reasons someone
wouldn't want older revisions obsoleted. The config name reflects the fact that
I'm not sure if other things like import should get this too.

I suspect that we could wrap a function deeper in the pull sequence to improve
both the code and the UX. For example, when pulling an obsolete marker, it can
print out a warning that the working directory parent is obsolete, but that
doesn't happen here. (It won't happen with this test. It *should* without the
--bypass option, but doesn't.) It should also be possible to not have to
query the range of new revisions, and maybe it can be added to the existing
transaction.

[1] https://bitbucket.org/facebook/hg-experimental/src/default/hgext3rd/pullcreatemarkers.py

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

mharbison72 created this revision.Sep 25 2019, 1:17 AM
pulkit added subscribers: sheehan, pulkit.Oct 5 2019, 9:35 AM

@sheehan this might be of interest to you.

marmoute added a comment.Oct 7 2019, 8:43 PM

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.

hgext/phabricator.py
187

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)

192

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

216

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

225

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

mharbison72 added a comment.Oct 8 2019, 12:20 AM
In D6876#102925, @marmoute wrote:

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.

I basically agree with the inline comments. This was an import with minimal changes as a baseline, and I didn't want to spend time making a bunch of follow ups until I gauged interest in this.

Alternately, if there's concern over obsolete marker growth, should this use the archived phase instead? Since the use case is mostly around stuff a developer imports locally, I don't see much value in those markers being exchangeable. The only potential hole I see is if someone has pushed this to a non publishing repo before pulling. Then everyone else could pull the (not obsoleted) commit. But then they would archive it on pull too, instead of N developers potentially creating N markers for the same commit.

hgext/phabricator.py
187

It looks like, based on the loop where it's used, it will obsolete all (non public) commits that match. The only way I can think of where that happens is if I phabimport someone else's patches, they make updates, and then I phabimport again. That's why I was wondering aloud about doing this on import, but if the eventual pull cleans it all up, that may be sufficient for the use case I had in mind. I don't see how a developer would do that on their own commits with amend and friends.

Do you have any ideas to detect phase divergence? And can that even happen if we're ignoring public commits?

192

I wonder if it would be better in some txn hook. And then it could (I think) share the existing transaction and lock.

216

Agreed.

I saw mention of the archived phase the other day; how would that (and other special phases) play here?

225

You mean after locking, correct?

marmoute added inline comments.Oct 8 2019, 6:17 AM
hgext/phabricator.py
187

User A phasend a patch

  • User A phasend a changeset A as D1337
  • User B phabimport D1337, rebase (as A') and pushes
  • User A fix critical bug in D1337 and amend it as A''
  • User A pull the old version of D1337 without the fix.

If we obsolete the new version of D1337 (A''), we are silently dropping the fix to the critical bug.

If markers were properly exchange, the evolution machinery would detect the divergence, warn the user and be able to automatically solve it. Preserving the fix to the criticial bug.

216

Thing could be in secret phase too.

225

I do mean after locking (sorry)

mharbison72 added inline comments.Oct 8 2019, 8:12 AM
hgext/phabricator.py
187

I see. So maybe this should only obsolete things that have no precursors, and warn (and ignore) things that do. That means it doesn’t clean up if the reviewer accidentally forgets to import with —obsolete, but that’s easy enough to fix with a copy/paste command if A and A' are identified in the output.

216

Right, and this is probably the typical case. (The phabimport alias I have imports as secret to prevent accidental push.) But secret is visible to the user, and benefits from the cleanup. Archived isn’t visible (IIUC), so it probably doesn’t need to be obsoleted too- especially if we’re worried about marker growth.

(I’m also not sure how well thg handles archived, and that probably needs to be squared away first if we opt to archive instead of obsolete here.)

baymax requested changes to this revision.Jan 24 2020, 12:31 PM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.Jan 24 2020, 12:31 PM
mharbison72 planned changes to this revision.Jul 15 2020, 1:23 PM

Revision Contents
Changeset List

PathPackages
Mhgext/phabricator.py (51 lines)
Mtests/test-phabricator.t (18 lines)

Diff 16604

hgext/phabricator.py

from mercurial import ( from mercurial import (
cmdutil, cmdutil,
context, context,
encoding, encoding,
error, error,
exthelper, exthelper,
httpconnection as httpconnectionmod, httpconnection as httpconnectionmod,
mdiff, mdiff,
obsolete,
obsutil, obsutil,
parser, parser,
patch, patch,
phases, phases,
pycompat, pycompat,
scmutil, scmutil,
smartset, smartset,
tags, tags,
testedwith = 'ships-with-hg-core' testedwith = 'ships-with-hg-core'
eh = exthelper.exthelper() eh = exthelper.exthelper()
cmdtable = eh.cmdtable cmdtable = eh.cmdtable
command = eh.command command = eh.command
configtable = eh.configtable configtable = eh.configtable
templatekeyword = eh.templatekeyword templatekeyword = eh.templatekeyword
uisetup = eh.finaluisetup
eh.configitem(b'experimental', b'phabricator.fabricate-obsmarkers',
default=False,
)
# developer config: phabricator.batchsize # developer config: phabricator.batchsize
eh.configitem(b'phabricator', b'batchsize', eh.configitem(b'phabricator', b'batchsize',
default=12, default=12,
) )
eh.configitem(b'phabricator', b'callsign', eh.configitem(b'phabricator', b'callsign',
default=None, default=None,
) )
eh.configitem(b'phabricator', b'curlcmd', eh.configitem(b'phabricator', b'curlcmd',
return fn(*args, **kwargs) return fn(*args, **kwargs)
return fn(*args, **kwargs) return fn(*args, **kwargs)
inner.__name__ = fn.__name__ inner.__name__ = fn.__name__
inner.__doc__ = fn.__doc__ inner.__doc__ = fn.__doc__
return command(name, fullflags, spec, helpcategory=helpcategory, return command(name, fullflags, spec, helpcategory=helpcategory,
optionalrepo=optionalrepo)(inner) optionalrepo=optionalrepo)(inner)
return decorate return decorate
def getmatchingdiff(ctx):
m = _differentialrevisiondescre.search(ctx.description())
if m:
return b"D%s" % m.group(r'id')
marmouteUnsubmitted
Not Done

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)

marmoute: So, this only checks the diff number, right ? So in theory, we could obsolete a later version…
mharbison72AuthorUnsubmitted
Not Done

It looks like, based on the loop where it's used, it will obsolete all (non public) commits that match. The only way I can think of where that happens is if I phabimport someone else's patches, they make updates, and then I phabimport again. That's why I was wondering aloud about doing this on import, but if the eventual pull cleans it all up, that may be sufficient for the use case I had in mind. I don't see how a developer would do that on their own commits with amend and friends.

Do you have any ideas to detect phase divergence? And can that even happen if we're ignoring public commits?

mharbison72: It looks like, based on the loop where it's used, it will obsolete //all// (non public) commits…
marmouteUnsubmitted
Not Done

User A phasend a patch

  • User A phasend a changeset A as D1337
  • User B phabimport D1337, rebase (as A') and pushes
  • User A fix critical bug in D1337 and amend it as A''
  • User A pull the old version of D1337 without the fix.

If we obsolete the new version of D1337 (A''), we are silently dropping the fix to the critical bug.

If markers were properly exchange, the evolution machinery would detect the divergence, warn the user and be able to automatically solve it. Preserving the fix to the criticial bug.

marmoute: User A phasend a patch - User A phasend a changeset `A` as D1337 - User B phabimport…
mharbison72AuthorUnsubmitted
Not Done

I see. So maybe this should only obsolete things that have no precursors, and warn (and ignore) things that do. That means it doesn’t clean up if the reviewer accidentally forgets to import with —obsolete, but that’s easy enough to fix with a copy/paste command if A and A' are identified in the output.

mharbison72: I see. So maybe this should only obsolete things that have no precursors, and warn (and ignore)…
return None
@eh.wrapcommand(b"pull")
def _pull(orig, ui, repo, *args, **opts):
marmouteUnsubmitted
Not Done

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

marmoute: It would probably make sense to wrap the pull exchange logic instead. It would catch more…
mharbison72AuthorUnsubmitted
Not Done

I wonder if it would be better in some txn hook. And then it could (I think) share the existing transaction and lock.

mharbison72: I wonder if it would be better in some txn hook. And then it could (I think) share the…
if not (obsolete.isenabled(repo, obsolete.createmarkersopt)
and ui.configbool(b'experimental',
b'phabricator.fabricate-obsmarkers')):
return orig(ui, repo, *args, **opts)
maxrevbeforepull = len(repo.changelog)
r = orig(ui, repo, *args, **opts)
maxrevafterpull = len(repo.changelog)
# Collect the diff number of the landed diffs
landeddiffs = {}
for rev in range(maxrevbeforepull, maxrevafterpull):
n = repo[rev]
if n.phase() == phases.public:
diff = getmatchingdiff(n)
if diff is not None:
landeddiffs[diff] = n
if not landeddiffs:
return r
# Try to find match with the drafts
tocreate = []
unfiltered = repo.unfiltered()
for rev in unfiltered.revs("draft() - obsolete()"):
marmouteUnsubmitted
Not Done

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

marmoute: This should be `non public()` instead of `draft()` these could be draft.
mharbison72AuthorUnsubmitted
Not Done

Agreed.

I saw mention of the archived phase the other day; how would that (and other special phases) play here?

mharbison72: Agreed. I saw mention of the archived phase the other day; how would that (and other special…
marmouteUnsubmitted
Not Done

Thing could be in secret phase too.

marmoute: Thing could be in `secret` phase too.
mharbison72AuthorUnsubmitted
Not Done

Right, and this is probably the typical case. (The phabimport alias I have imports as secret to prevent accidental push.) But secret is visible to the user, and benefits from the cleanup. Archived isn’t visible (IIUC), so it probably doesn’t need to be obsoleted too- especially if we’re worried about marker growth.

(I’m also not sure how well thg handles archived, and that probably needs to be squared away first if we opt to archive instead of obsolete here.)

mharbison72: Right, and this is probably the typical case. (The `phabimport` alias I have imports as secret…
n = unfiltered[rev]
diff = getmatchingdiff(n)
if diff in landeddiffs and landeddiffs[diff].rev() != n.rev():
tocreate.append((n, (landeddiffs[diff],)))
if not tocreate:
return r
with unfiltered.lock(), unfiltered.transaction('phabpullcreatemarkers'):
marmouteUnsubmitted
Not Done

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

marmoute: We should do the computation before locking to avoid other process updating the repo under our…
mharbison72AuthorUnsubmitted
Not Done

You mean after locking, correct?

mharbison72: You mean //after// locking, correct?
marmouteUnsubmitted
Not Done

I do mean after locking (sorry)

marmoute: I do mean **after** locking (sorry)
obsolete.createmarkers(unfiltered, tocreate)
return r
def urlencodenested(params): def urlencodenested(params):
"""like urlencode, but works with nested parameters. """like urlencode, but works with nested parameters.
For example, if params is {'a': ['b', 'c'], 'd': {'e': 'f'}}, it will be For example, if params is {'a': ['b', 'c'], 'd': {'e': 'f'}}, it will be
flattened to {'a[0]': 'b', 'a[1]': 'c', 'd[e]': 'f'} and then passed to flattened to {'a[0]': 'b', 'a[1]': 'c', 'd[e]': 'f'} and then passed to
urlencode. Note: the encoding is consistent with PHP's http_build_query. urlencode. Note: the encoding is consistent with PHP's http_build_query.
""" """
flatparams = util.sortdict() flatparams = util.sortdict()

tests/test-phabricator.t

$ hg phabsend -r . -m "Address review comments" --test-vcr "$VCR/phabsend-comment-updated.json" $ hg phabsend -r . -m "Address review comments" --test-vcr "$VCR/phabsend-comment-updated.json"
D1253 - updated - 1acd4b60af38: create comment for phabricator test D1253 - updated - 1acd4b60af38: create comment for phabricator test
Phabsending a skipped commit: Phabsending a skipped commit:
$ hg phabsend --no-amend -r . --test-vcr "$VCR/phabsend-skipped.json" $ hg phabsend --no-amend -r . --test-vcr "$VCR/phabsend-skipped.json"
D1253 - skipped - 1acd4b60af38: create comment for phabricator test D1253 - skipped - 1acd4b60af38: create comment for phabricator test
Phabreading a DREV with a local:commits time as a string: Phabreading a DREV with a local:commits time as a string:
$ hg phabread --test-vcr "$VCR/phabread-str-time.json" D1285 $ hg phabread --test-vcr "$VCR/phabread-str-time.json" D1285 > ../d1285.diff
$ cat ../d1285.diff
# HG changeset patch # HG changeset patch
# User test <test> # User test <test>
# Date 1562019844 0 # Date 1562019844 0
# Branch default # Branch default
# Node ID da5c8c6bf23a36b6e3af011bc3734460692c23ce # Node ID da5c8c6bf23a36b6e3af011bc3734460692c23ce
# Parent 1f634396406d03e565ed645370e5fecd062cf215 # Parent 1f634396406d03e565ed645370e5fecd062cf215
test string time test string time
Differential Revision: https://phab.mercurial-scm.org/D1285 Differential Revision: https://phab.mercurial-scm.org/D1285
diff --git a/test b/test diff --git a/test b/test
new file mode 100644 new file mode 100644
--- /dev/null --- /dev/null
+++ b/test +++ b/test
@@ * @@ (glob) @@ * @@ (glob)
+test +test
$ hg init ../simple
$ hg -R ../simple import -q ../d1285.diff
$ hg import --bypass -q ../d1285.diff
$ hg pull -f ../simple --config experimental.evolution.createmarkers=True \
> --config experimental.phabricator.fabricate-obsmarkers=True
pulling from ../simple
searching for changes
warning: repository is unrelated
requesting all changes
adding changesets
adding manifests
adding file changes
added 1 changesets with 0 changes to 1 files (+1 heads)
new changesets b68c34204469
(run 'hg heads' to see heads, 'hg merge' to merge)
$ cd .. $ cd ..