on merge during rebase, check to see if source and destination are both draft commits. If so, use full copytrace to support more functionality.
Details
Details
- Reviewers
stash - Group Reviewers
Restricted Project - Commits
- rFBHGXe3f32b63fb60: copytrace: use full version of copytrace for draft branch rebases
In tests/test-copytrace.t, added two tests:
- Change a file name and move containing directory in one branch. Modify contents of original file name in other branch. Rebase modded file into new file name in new directory.
- Move directory in one merge parent. Add new file to original directory in other merge parent. File moved to new directory on rebase.
Diff Detail
Diff Detail
- Repository
- rFBHGX Facebook Mercurial Extensions
- Branch
- default
- Lint
Lint OK - Unit
Unit Tests OK
Event Timeline
Comment Actions
@stash: There are some problems in the unit test test-check-commit-hg.t. I don't understand them and I try to change the commit formatting to please them, but they won't budge. It's an easy fix, I'm sure, but I still wanted to push the meat of the commit. Also, there are 4 tests in test-copytrace.t that cannot be switched to public states because they would break, but full copytrace takes them over otherwise so it's not testing your extension. Should I delete them?
Comment Actions
The title should not literally start with the word "topic" but with the one-word topic of your commit, which in this case is "copytrace". In this case, I'd suggest "copytrace: use full version for draft branch rebases"
Can you share the exact errors you're seeing from check-test-commit please?
Comment Actions
One more thing about test-copytrace-amend.t. Thanks for noticing the problem. I think even if we use original copytracing, we still want to call enableamendcopytrace:
and update the result that was returned from the original copytracing
if _isfullcopytraceable(cdst, base):
configoverrides = {('experimental', 'disablecopytrace'): False}
with repo.ui.configoverride(configoverrides, 'mergecopies'):
result = orig(repo, cdst, csrc, base)
if repo.ui.configbool("copytrace", "enableamendcopytrace", True):
# Look for additional amend-copies.
amend_copies = _getamendcopies(repo, cdst, base.p1())
# update result[0] dict with amend_copies
return resultdoes it make sense?
| tests/test-copytrace.t | ||
|---|---|---|
| 231 | Can you just make 1451231c87572a7d3f92fc210b4b35711c949a98 public? | |
| 317 | Same - can you make 36859b8907c513a3a87ae34ba5b1e7eea8c20944 public? | |
| 414–415 | And 1451231c87572a7d3f92fc210b4b35711c949a98 public? | |
Comment Actions
Can you change the commit message as @rmcelroy suggested?
from "topic: use full version of copytrace for draft branch rebases" to "copytrace: use full version of copytrace for draft branch rebases"? You need to click on the "Edit revision" button in the top right corner
Comment Actions
@goozie001 Lgtm! Please fix three last comments (the most important is about new config option to disable your functionality by default).
@pulkit we'll land it soon, and then you can move it upstream. Thanks!
| hgext3rd/copytrace.py | ||
|---|---|---|
| 354–367 | Can you also add a comment about why are we doing it? "If base, destination and source are draft then let's use full copytracing because it will work fast. " | |
| 479–482 | Let's use ui.configint('copytrace', 'sourcecommitlimit', 100) instead of just magical constant 100. You'll need to change signature of course: def _isfullcopytraceable(ui, cdst, base): And since you are passing ui object here let's add a separate config option to enable/disable your functionality: if not ui.configbool("copytrace", "draftusefullcopytrace", False):
return FalseLet's set default to False just to make sure we don't accidentally break everything :). And then I'll slowly roll it out to the users. You'll also need to update docblock at the beginning of this file. | |
Comment Actions
Disable full copytrace on draft by default. Add variable for draft branch size limit.
Comment Actions
I get this error which is hiding the real exception after all normal unit tests pass
File "/data/users/drichardson/facebook-hg-rpms/hg-crew/mercurial/dispatch.py", line 914, in _runcommand
+ File "/data/users/drichardson/facebook-hg-rpms/hg-crew/mercurial/dispatch.py", line 303, in _runcatchfunc
+ return _dispatch(req)
+ File "/data/users/drichardson/facebook-hg-rpms/hg-crew/mercurial/dispatch.py", line 906, in _dispatch
+ cmdpats, cmdoptions)
+ File "/data/users/drichardson/facebook-hg-rpms/hg-crew/mercurial/extensions.py", line 322, in closure
+ return func(*(args + a), **kw)
+ File "/data/users/drichardson/facebook-hg-rpms/fb-hgext/hgext3rd/fbamend/hiddenoverride.py", line 119, in runcommand
+ result = orig(lui, repo, cmd, fullargs, *args)
+ File "/data/users/drichardson/facebook-hg-rpms/hg-crew/mercurial/extensions.py", line 322, in closure
+ return func(*(args + a), **kw)
+ File "/data/users/drichardson/facebook-hg-rpms/fb-hgext/hgext3rd/copytrace.py", line 135, in _runcommand
+ return orig(lui, repo, cmd, fullargs, ui, *args, **kwargs)
+ File "/data/users/drichardson/facebook-hg-rpms/hg-crew/mercurial/dispatch.py", line 669, in runcommand
+ ret = _runcommand(ui, options, cmd, d)
+ File "/data/users/drichardson/facebook-hg-rpms/hg-crew/mercurial/dispatch.py", line 914, in _runcommand
+ return cmdfunc()
+ File "/data/users/drichardson/facebook-hg-rpms/hg-crew/mercurial/dispatch.py", line 903, in <lambda>
+ d = lambda: util.checksignature(func)(ui, *args, **strcmdopt)
+ File "/data/users/drichardson/facebook-hg-rpms/hg-crew/mercurial/util.py", line 1084, in check
+ return func(*args, **kwargs)
+ File "/data/users/drichardson/facebook-hg-rpms/hg-crew/mercurial/extensions.py", line 322, in closure
+ return func(*(args + a), **kw)
+ File "/data/users/drichardson/facebook-hg-rpms/hg-crew/mercurial/util.py", line 1084, in check
+ return func(*args, **kwargs)
+ File "/data/users/drichardson/facebook-hg-rpms/fb-hgext/hgext3rd/fbamend/__init__.py", line 451, in wraprebase
+ return restack.restack(ui, repo, opts)
+ File "/data/users/drichardson/facebook-hg-rpms/fb-hgext/hgext3rd/fbamend/restack.py", line 40, in restack
+ targets = _findrestacktargets(repo, latest)
+ File "/data/users/drichardson/facebook-hg-rpms/fb-hgext/hgext3rd/fbamend/restack.py", line 70, in _findrestacktargets
+ repo.revs('%d + allpredecessors(%d)', base, base))
+ File "/data/users/drichardson/facebook-hg-rpms/hg-crew/mercurial/localrepo.py", line 757, in revs
+ return m(self)
+ File "/data/users/drichardson/facebook-hg-rpms/hg-crew/mercurial/revset.py", line 2100, in mfunc
+ return getset(repo, subset, tree)
+ File "/data/users/drichardson/facebook-hg-rpms/hg-crew/mercurial/revset.py", line 58, in getset
+ return methods[x[0]](repo, subset, *x[1:])
+ File "/data/users/drichardson/facebook-hg-rpms/hg-crew/mercurial/revset.py", line 149, in orset
+ return _orsetlist(repo, subset, xs)
+ File "/data/users/drichardson/facebook-hg-rpms/hg-crew/mercurial/revset.py", line 140, in _orsetlist
+ b = _orsetlist(repo, subset, xs[p:])
+ File "/data/users/drichardson/facebook-hg-rpms/hg-crew/mercurial/revset.py", line 137, in _orsetlist
+ return getset(repo, subset, xs[0])
+ File "/data/users/drichardson/facebook-hg-rpms/hg-crew/mercurial/revset.py", line 58, in getset
+ return methods[x[0]](repo, subset, *x[1:])
+ File "/data/users/drichardson/facebook-hg-rpms/hg-crew/mercurial/revset.py", line 192, in func
+ return func(repo, subset, b)
+ File "/data/users/drichardson/facebook-hg-rpms/fb-hgext/hgext3rd/fbamend/revsets.py", line 61, in allpredecessors
+ return _calculateset(repo, subset, x, f)
+ File "/data/users/drichardson/facebook-hg-rpms/fb-hgext/hgext3rd/fbamend/revsets.py", line 32, in _calculateset
+ for n in f(tonode(r) for r in revs)
+ File "/data/users/drichardson/facebook-hg-rpms/fb-hgext/hgext3rd/fbamend/revsets.py", line 60, in <lambda>
+ f = lambda nodes: obsutil.allpredecessors(repo.obsstore, nodes)
+ AttributeError: 'module' object has no attribute 'allpredecessors'
[1]
$ cd ..
$ rm -rf repoComment Actions
This is because fb-hgext is not yet compatible with the latest hg because of renames of some functions and attributes.
Comment Actions
If this is the only failure which you are seeing and there is no other problems with the patch. BTW I am not the right to person to ask this ;)
Comment Actions
@goozie001 , if you pull in hg-crew and fb-hgext, rebase your changes and re-run the command, I think the tests should pass.
Anyway, the change is fine to land, just a small comment
| hgext3rd/copytrace.py | ||
|---|---|---|
| 132–134 | We don't need it, please delete | |
Comment Actions
I tried landing it, but it's claiming that I'm attempting to mutate the remote head. I'm not sure why that's happening. Here is the stack trace of the error:
<<< [22] <http> 118,410 us
<<< [21] <conduit> 118,796 us
>>> [32] <event> land.willPushRevision <listeners = 0>
<<< [32] <event> 108 us
<<< [13] <exec> 186,595 us
>>> [14] <exec> $ HGPLAIN=1 hg checkout '3e62ca'
<<< [14] <exec> 1,259,520 us
Switched to branch 3e62ca. Identifying and merging...
>>> [15] <exec> $ HGPLAIN=1 hg log -l 1 --template '{node}' -r 'ancestor('\''@'\'',.)' --
<<< [15] <exec> 157,566 us
>>> [16] <exec> $ HGPLAIN=1 hg log --template '{node}{desc}' --rev '('\''1212fe1b468ab7efd100cc946bada7e4ed52ea3f'\''::. - '\''1212fe1b468ab7efd100cc946bada7e4ed52ea3f'\'')' --branch 'default' --
<<< [16] <exec> 155,517 us
>>> [17] <conduit> differential.query() <bytes = 141>
>>> [18] <http> https://phab.mercurial-scm.org/api/differential.query
<<< [18] <http> 115,960 us
<<< [17] <conduit> 116,253 us
>>> [19] <conduit> differential.getcommitmessage() <bytes = 149>
>>> [20] <http> https://phab.mercurial-scm.org/api/differential.getcommitmessage
<<< [20] <http> 141,311 us
<<< [19] <conduit> 141,558 us
Landing revision 'D481: copytrace: use full version of copytrace for draft branch rebases'...
>>> [21] <conduit> harbormaster.querybuildables() <bytes = 218>
>>> [22] <http> https://phab.mercurial-scm.org/api/harbormaster.querybuildables
<<< [22] <http> 75,484 us
<<< [21] <conduit> 75,782 us
>>> [23] <exec> $ HGPLAIN=1 hg log -l 1 --template '{node}' -r '@' --
<<< [23] <exec> 162,227 us
>>> [24] <exec> $ HGPLAIN=1 hg log -l 1 --template '{node}' -r 'ancestor('\''@'\'', '\''3e62ca'\'')' --
<<< [24] <exec> 159,340 us
>>> [25] <exec> $ HGPLAIN=1 hg log -l 1 --template '{node}' -r '3e62ca' --
<<< [25] <exec> 161,143 us
>>> [26] <exec> $ HGPLAIN=1 hg log -l 1 --template '{node}' -r 'first(('\''@'\''::'\''3e62ca'\'')-'\''@'\'')' --
<<< [26] <exec> 186,381 us
>>> [27] <exec> $ HGPLAIN=1 hg log --template '{node}\n' -r 'roots(descendants('\''3e62ca598e4d2d929c7cf8302c5d0fdc266ba754'\'')-descendants('\''3e62ca'\'')-('\''3e62ca598e4d2d929c7cf8302c5d0fdc266ba754'\''::'\''3e62ca'\''))'
<<< [27] <exec> 175,481 us
>>> [28] <exec> $ HGPLAIN=1 hg rebase --collapse --keep --logfile '/tmp/drichardson/dzrnxj6luqoggcko/4165119-35Uejv' -r '('\''3e62ca598e4d2d929c7cf8302c5d0fdc266ba754'\''::'\''3e62ca'\'')' -d '@'
rebasing 3543:3e62ca598e4d "copytrace: use full version of copytrace for draft branch rebases"
<<< [28] <exec> 264,713 us
>>> [29] <exec> $ HGPLAIN=1 hg bookmarks
<<< [29] <exec> 176,586 us
>>> [30] <exec> $ HGPLAIN=1 hg log -r 'children('\''3e62ca'\'')' --template '{node}\n'
<<< [30] <exec> 177,109 us
>>> [31] <exec> $ HGPLAIN=1 hg checkout '@'
<<< [31] <exec> 1,203,243 us
>>> [32] <event> land.willPushRevision <listeners = 0>
<<< [32] <event> 100 us
Pushing change...
>>> [33] <exec> $ HGPLAIN=1 hg push -r '@'
pushing to ssh://hg.vip.facebook.com//data/scm/fb-hgext
searching for changes
no changes found
<<< [33] <exec> 653,082 us
PUSH FAILED!
>>> [34] <exec> $ HGPLAIN=1 hg --config extensions.mq= strip '@'
<<< [34] <exec> 187,864 us
[2017-08-28 14:44:49] EXCEPTION: (CommandException) Command failed with error #255!
COMMAND
HGPLAIN=1 hg --config extensions.mq= strip '@'
STDOUT
(empty)
STDERR
abort: cannot prune immutable changeset: 1212fe1b468a
(see 'hg help phases' for details)
at [<phutil>/src/future/exec/ExecFuture.php:374]
arcanist(head=master, ref.master=5eda40337bb4), fb-hgext-arcanist(), phutil(head=master, ref.master=276f6d304b69)
#0 ExecFuture::resolvex() called at [<arcanist>/src/repository/api/ArcanistRepositoryAPI.php:406]
#1 ArcanistRepositoryAPI::execxLocal(string, string) called at [<arcanist>/src/workflow/ArcanistLandWorkflow.php:1235]
#2 ArcanistLandWorkflow::executeCleanupAfterFailedPush() called at [<arcanist>/src/workflow/ArcanistLandWorkflow.php:1209]
#3 ArcanistLandWorkflow::push() called at [<arcanist>/src/workflow/ArcanistLandWorkflow.php:278]
#4 ArcanistLandWorkflow::run() called at [<arcanist>/scripts/arcanist.php:394]Comment Actions
@stash This one is pushed, do you have more plans or the work is done for now so that can I move it to core.
We don't need it, please delete