This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4788

narrow: don't do the dirstate dance if ellipses is not enabled
ClosedPublic

Authored by pulkit on Sep 28 2018, 4:58 PM.

Details

Reviewers
durin42
martinvonz
Group Reviewers
hg-reviewers
Commits
rHG693dda764efe: narrow: don't do the dirstate dance if ellipses is not enabled
Summary

I believe we set dirstate parents to nullid before widening pull because in
ellipses cases, the parent might be stripped off with a new changeset. However
the second ds.setparents() call invalidate my assumption. I am not sure why we
do this. So here is a patch.

This patch also adds tests showing we break nothing in non-ellipses cases.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pulkit created this revision.Sep 28 2018, 4:58 PM
pulkit added a subscriber: martinvonz.Sep 28 2018, 5:01 PM

I am not sure about this one. I was unable to think of a reason why we need to do this dirstate dance in non-ellipses cases. @martinvonz @durin42 do you know why we do this?

pulkit edited the summary of this revision. (Show Details)Sep 30 2018, 12:11 PM
pulkit updated this revision to Diff 11502.
martinvonz added a comment.Oct 1 2018, 2:58 AM
In D4788#72444, @pulkit wrote:

I am not sure about this one. I was unable to think of a reason why we need to do this dirstate dance in non-ellipses cases. @martinvonz @durin42 do you know why we do this?

What I usually do when I can't understand why something is needed: remove the code and run tests :) That will often tell you there there was in fact a reason for the code and give you some hints what that reason is. Of course, in some cases it won't tell you that and then you'll have to figure out if it's just untested or actually useless. Do you mean lines 279-280 of hgext/narrow/narrowcommands.py before this patch? If you remove those lines, you will see that test-narrow{,patterns,widen,widen-no-ellipsis}.t fail. You were right about the reason for the first setparent() call, btw. The reason for the code is so we move back to the old commit once we've restored the temporarily stripped commits.

hgext/narrow/narrowcommands.py
254–260

You have add the requirement earlier in the series, so you can just use that here? (I suspect this is a leftover from an earlier version of this patch.)

279–280

I think I'd prefer to not silence developer warnings more than necessary even if that means duplicating the repo.ui.configoverride(overrides, 'widen') (i.e. I'd prefer to keep that together with the wrappedextraprepare block).

pulkit added a comment.Oct 1 2018, 11:03 AM
In D4788#72714, @martinvonz wrote:
In D4788#72444, @pulkit wrote:

I am not sure about this one. I was unable to think of a reason why we need to do this dirstate dance in non-ellipses cases. @martinvonz @durin42 do you know why we do this?

What I usually do when I can't understand why something is needed: remove the code and run tests :) That will often tell you there there was in fact a reason for the code and give you some hints what that reason is. Of course, in some cases it won't tell you that and then you'll have to figure out if it's just untested or actually useless. Do you mean lines 279-280 of hgext/narrow/narrowcommands.py before this patch? If you remove those lines, you will see that test-narrow{,patterns,widen,widen-no-ellipsis}.t fail. You were right about the reason for the first setparent() call, btw. The reason for the code is so we move back to the old commit once we've restored the temporarily stripped commits.

And in non-ellipses cases, we are not stripping commits, so we can easily make this only execute when ellipses is enabled.

hgext/narrow/narrowcommands.py
254–260

I think it's getting confusing for me and suspect bit hard for you too as the series tries to do two quite different yet related things. I will drop the ellipses requirement patch from this series and will first concentrate on the introducing wireprotocol command and the ellipses requirement thing as a seperate series when the wireprotocol one is reviewed and pushed.

pulkit updated this revision to Diff 11517.Oct 1 2018, 12:01 PM
pulkit edited the summary of this revision. (Show Details)Oct 2 2018, 10:19 AM
martinvonz accepted this revision.Oct 2 2018, 12:17 PM
martinvonz added inline comments.
hgext/narrow/narrowcommands.py
286–287

No need to change this, but FYI, I think the config override is one of the context managers that do their setup in __enter__ (not __init__), so it should be fine to assign repo.ui.configoverride(overrides, 'widen') to a variable if wanted to fit this on one line

This revision is now accepted and ready to land.Oct 2 2018, 12:17 PM
Closed by commit rHG693dda764efe: narrow: don't do the dirstate dance if ellipses is not enabled (authored by pulkit). · Explain WhyOct 2 2018, 12:55 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 11568

hgext/narrow/narrowcommands.py

newexcludes, oldmatch, newmatch) newexcludes, oldmatch, newmatch)
repo.setnarrowpats(newincludes, newexcludes) repo.setnarrowpats(newincludes, newexcludes)
repo.destroyed() repo.destroyed()
def _widen(ui, repo, remote, commoninc, newincludes, newexcludes): def _widen(ui, repo, remote, commoninc, newincludes, newexcludes):
newmatch = narrowspec.match(repo.root, newincludes, newexcludes) newmatch = narrowspec.match(repo.root, newincludes, newexcludes)
# for now we assume that if a server has ellipses enabled, we will be
# exchanging ellipses nodes. In future we should add ellipses as a client
# side requirement (maybe) to distinguish a client is shallow or not and
# then send that information to server whether we want ellipses or not.
# Theoretically a non-ellipses repo should be able to use narrow
# functionality from an ellipses enabled server
ellipsesremote = wireprotoserver.ELLIPSESCAP in remote.capabilities()
martinvonzUnsubmitted
Not Done

You have add the requirement earlier in the series, so you can just use that here? (I suspect this is a leftover from an earlier version of this patch.)

martinvonz: You have add the requirement earlier in the series, so you can just use that here? (I suspect…
pulkitAuthorUnsubmitted
Not Done

I think it's getting confusing for me and suspect bit hard for you too as the series tries to do two quite different yet related things. I will drop the ellipses requirement patch from this series and will first concentrate on the introducing wireprotocol command and the ellipses requirement thing as a seperate series when the wireprotocol one is reviewed and pushed.

pulkit: I think it's getting confusing for me and suspect bit hard for you too as the series tries to…
def pullbundle2extraprepare_widen(orig, pullop, kwargs): def pullbundle2extraprepare_widen(orig, pullop, kwargs):
orig(pullop, kwargs) orig(pullop, kwargs)
# The old{in,ex}cludepats have already been set by orig() # The old{in,ex}cludepats have already been set by orig()
kwargs['includepats'] = newincludes kwargs['includepats'] = newincludes
kwargs['excludepats'] = newexcludes kwargs['excludepats'] = newexcludes
kwargs['widen'] = True kwargs['widen'] = True
wrappedextraprepare = extensions.wrappedfunction(exchange, wrappedextraprepare = extensions.wrappedfunction(exchange,
'_pullbundle2extraprepare', pullbundle2extraprepare_widen) '_pullbundle2extraprepare', pullbundle2extraprepare_widen)
# define a function that narrowbundle2 can call after creating the # define a function that narrowbundle2 can call after creating the
# backup bundle, but before applying the bundle from the server # backup bundle, but before applying the bundle from the server
def setnewnarrowpats(): def setnewnarrowpats():
repo.setnarrowpats(newincludes, newexcludes) repo.setnarrowpats(newincludes, newexcludes)
repo.setnewnarrowpats = setnewnarrowpats repo.setnewnarrowpats = setnewnarrowpats
# silence the devel-warning of applying an empty changegroup # silence the devel-warning of applying an empty changegroup
overrides = {('devel', 'all-warnings'): False} overrides = {('devel', 'all-warnings'): False}
with ui.uninterruptable(): with ui.uninterruptable():
common = commoninc[0]
martinvonzUnsubmitted
Not Done

I think I'd prefer to not silence developer warnings more than necessary even if that means duplicating the repo.ui.configoverride(overrides, 'widen') (i.e. I'd prefer to keep that together with the wrappedextraprepare block).

martinvonz: I think I'd prefer to not silence developer warnings more than necessary even if that means…
if ellipsesremote:
ds = repo.dirstate ds = repo.dirstate
p1, p2 = ds.p1(), ds.p2() p1, p2 = ds.p1(), ds.p2()
with ds.parentchange(): with ds.parentchange():
ds.setparents(node.nullid, node.nullid) ds.setparents(node.nullid, node.nullid)
common = commoninc[0] with wrappedextraprepare,\
with wrappedextraprepare, repo.ui.configoverride(overrides, 'widen'): repo.ui.configoverride(overrides, 'widen'):
martinvonzUnsubmitted
Not Done

No need to change this, but FYI, I think the config override is one of the context managers that do their setup in __enter__ (not __init__), so it should be fine to assign repo.ui.configoverride(overrides, 'widen') to a variable if wanted to fit this on one line

martinvonz: No need to change this, but FYI, I think the config override is one of the context managers…
exchange.pull(repo, remote, heads=common) exchange.pull(repo, remote, heads=common)
with ds.parentchange(): with ds.parentchange():
ds.setparents(p1, p2) ds.setparents(p1, p2)
else:
with wrappedextraprepare,\
repo.ui.configoverride(overrides, 'widen'):
exchange.pull(repo, remote, heads=common)
repo.setnewnarrowpats() repo.setnewnarrowpats()
actions = {k: [] for k in 'a am f g cd dc r dm dg m e k p pr'.split()} actions = {k: [] for k in 'a am f g cd dc r dm dg m e k p pr'.split()}
addgaction = actions['g'].append addgaction = actions['g'].append
mf = repo['.'].manifest().matches(newmatch) mf = repo['.'].manifest().matches(newmatch)
for f, fn in mf.iteritems(): for f, fn in mf.iteritems():
if f not in repo.dirstate: if f not in repo.dirstate:

tests/test-narrow-widen-no-ellipsis.t

0: add inside 0: add inside
$ cd .. $ cd ..
Widen the narrow spec to see the widest file. This should not get the newly Widen the narrow spec to see the widest file. This should not get the newly
added upstream revisions. added upstream revisions.
$ cd narrow $ cd narrow
$ hg id -n
2
$ hg tracked --addinclude widest/f --debug $ hg tracked --addinclude widest/f --debug
comparing with ssh://user@dummy/master comparing with ssh://user@dummy/master
running python "*dummyssh" *user@dummy* *hg -R master serve --stdio* (glob) running python "*dummyssh" *user@dummy* *hg -R master serve --stdio* (glob)
sending hello command sending hello command
sending between command sending between command
remote: * (glob) remote: * (glob)
remote: capabilities: * (glob) remote: capabilities: * (glob)
remote: 1 remote: 1
getting widest/f getting widest/f
$ hg tracked $ hg tracked
I path:inside I path:inside
I path:widest/f I path:widest/f
$ cat widest/f $ cat widest/f
widest widest
$ hg id -n
2
Pull down the newly added upstream revision. Pull down the newly added upstream revision.
$ hg pull $ hg pull
pulling from ssh://user@dummy/master pulling from ssh://user@dummy/master
searching for changes searching for changes
adding changesets adding changesets
adding manifests adding manifests
adding file changes adding file changes